Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve map gen construction and usage performance #949

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Improve map gen construction and usage performance #949

merged 2 commits into from
Sep 7, 2023

Conversation

bsless
Copy link
Contributor

@bsless bsless commented Sep 6, 2023 •
edited
Loading

Round 2 :)
See detailed commit message
Benchmark results:

(require '[malli.generator :as mg])

(dotimes [_ 10]
  (time
   (dotimes [_ 1e5]
     (mg/generator [:map [:a :int] [:b :int]]))))

"Elapsed time: 377.558951 msecs" ;; before
"Elapsed time: 359.188257 msecs" ;; after

(def g (mg/generator [:map [:a :int] [:b :int]]))

(dotimes [_ 10]
  (time
   (dotimes [_ 1e5]
     (mg/generate g))))

"Elapsed time: 511.243682 msecs" ;; before
"Elapsed time: 370.793034 msecs" ;; after

@bsless
Speed up map-gen construction and usage
ad219a7 
Unify the req and opt gens sequences to a single sequence,
this fuses a single level of fmap of tuples and simplifies reduction
over the entries when building the map

Extract and fully expand value-gen to avoid allocating a closure

Early exit loop in case of unreachable required key - also saves an
extra iteration at the end of the loop

Discussion:
Do we want to keep value gen, or accumulate keys and value gens then
fmap with zipmap?
@bsless
Fix map gen tests for new generators tree structure
d625dde
@ikitommi ikitommi merged commit 0079aea into metosin:master Sep 7, 2023
6 checks passed
@ikitommi
Copy link
Member

ikitommi commented Sep 7, 2023

code looks much cleaner, thanks!

@bsless
Copy link
Contributor Author

bsless commented Sep 7, 2023

Cheers
I still wonder if generation could be faster
Three options:

  • tuple of entries into map
  • fmap zipmap ks to tuple gen
  • tower of gen/bind assoc to transient map

But I'm pretty happy with how much I've been able to trim this down, too

@bsless
Copy link
Contributor Author

bsless commented Sep 7, 2023

For the record, this brings the malli example to (only?) 2x slower than the pure gen composition
https://github.com/bsless/fsm-test-check/blob/master/examples/add_delete_tx_malli.clj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bsless @ikitommi