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.

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

More efficient QC.shuffle in transaction generator #1929

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

uroboros
Copy link
Contributor

Resolves JIRA issue CAD-2062

The transaction generator was "QuickCheck shuffling" potentially large lists - these shuffles have been replaced by more efficient ruffles.

pure (applyDelta neededKeys neededScripts keySpace tx delta)

-- ======================================================

-- | Return up to /k/ random elements from /items/
-- (instead of the less efficient /take k <$> QC.shuffle items/)
ruffle :: Int -> [a] -> Gen [a]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great word 😄

Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@uroboros uroboros merged commit 4bcc2ed into master Oct 20, 2020
@iohk-bors iohk-bors bot deleted the uroboros/better_shuffle branch October 20, 2020 19:03
@mrBliss
Copy link
Contributor

mrBliss commented Oct 21, 2020

@uroboros Great! We've been wanting this in consensus for a while now. Do you have measurements of how much faster this is than the previous version?

@uroboros
Copy link
Contributor Author

@uroboros Great! We've been wanting this in consensus for a while now. Do you have measurements of how much faster this is than the previous version?

@mrBliss I was using Tim's genTx benchmark as reference, which looks very promising! (The change was noticeable in the property test times too, but much less pronounced)

BEFORE -------------------------------

benchmarking gen/genTx/1000
time 1.537 ms (1.522 ms .. 1.555 ms)
0.999 R² (0.999 R² .. 0.999 R²)
mean 1.541 ms (1.529 ms .. 1.554 ms)
std dev 44.23 μs (35.12 μs .. 58.08 μs)
variance introduced by outliers: 17% (moderately inflated)

AFTER -------------------------------

benchmarking gen/genTx/1000
time 837.4 μs (825.8 μs .. 846.9 μs)
0.999 R² (0.998 R² .. 0.999 R²)
mean 848.4 μs (839.9 μs .. 857.3 μs)
std dev 29.36 μs (24.47 μs .. 35.82 μs)

@uroboros
Copy link
Contributor Author

would be interesting to see how it plays out in consensus tests

@mrBliss
Copy link
Contributor

mrBliss commented Oct 21, 2020

So a 2x speed-up.

At the moment we have disabled the generator in the consensus because it was prohibitively slow, but also because it triggered a failing assertion in the network layer (that the network team isn't going to fix soon). The latter means that we can't turn it back on again 🙁.

However, we also use this generator (in)directly, when generating random blocks and transactions for our serialisation roundtrip tests, which are rather slow at the moment. If these get 2x faster, that would also be nice.

I'll report back when I have results (this will take a while, as I still have to propagate a bunch of changes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants