Add a bechmark for adding transactions to the mempool #4400

dnadales · 2023-02-24T11:07:44Z

Part of #4382

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- The documentation has been properly updated
- New tests are added if needed and existing tests are updated
- Any changes affecting Consensus packages must have an entry in the appropriate changelog.d directory created using scriv. If in doubt, see the Consensus release process.
- If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

a convenience function to create an override for the mempool capacity using the provided number bytes.

abailly-iohk

LGTM! Ideally we would like to dump/publish those results somewhere and enable comparisons across changes, otherwise I am afraid we won't reap the benefits of this approach.

jorisdral

Added some remarks!

EDIT: I've noticed that CI does not build yet build benchmark suites, so we should probably enable that

jorisdral · 2023-02-24T13:17:15Z

ouroboros-consensus-test/ouroboros-consensus-test.cabal

@@ -326,3 +326,45 @@ test-suite test-infra
                       -Wredundant-constraints
                       -Wmissing-export-lists
                       -fno-ignore-asserts
+
+benchmark bench-mempool


If we plan on writing more benchmarks for other consensus components, would we want to have a single benchmark executable or one for each component that we target?

What about one per component? Why? To keep them isolated. But I don't know if the other approach as more advantages. Thoughts?

I'm not opposed to having one per component. An advantage to one executable for all is that we can reuse code, but I don't know how often we will actually be in a situation where we can reuse code. I think one advantage of having multiple executables is that benchmark running times tend to be on the lengthy side, so not having one monolith executable that takes ages to run is probably a good thing.

jorisdral · 2023-02-24T13:19:22Z

ouroboros-consensus-test/ouroboros-consensus-test.cabal

+                       -Wredundant-constraints
+                       -Wmissing-export-lists
+                       -Wno-unticked-promoted-constructors
+                       -fno-ignore-asserts


If the code under benchmark contains relatively costly assertions, not ignoring assertions might inflate the results. This is just something to take into consideration, it is not something that should change now

True! I missed that. Thanks!

jorisdral · 2023-02-24T13:21:27Z

...oros-consensus/changelog.d/20230224_113043_damian.only_4382_add_getSnapshotFor_benchmarks.md

+- Added `mkCapacityBytesOverride`, a convenience function to create an override
+  for the mempool capacity using the provided number bytes.


Did we decide on present or past tense? I'm mainly using present tense

Good point. We have no consistency at the moment. I'll raise this with the team.

jorisdral · 2023-02-24T13:22:30Z

ouroboros-consensus-test/src/Test/Util/TestBlock.hs

+-- | Apply the payload to a ticked state directly to the payload dependent state
+-- portion of it, leaving the rest of the input ticked state unaltered.


Suggested change

-- | Apply the payload to a ticked state directly to the payload dependent state

-- portion of it, leaving the rest of the input ticked state unaltered.

-- | Apply the payload directly to the payload dependent state

-- portion of a ticked state , leaving the rest of the input ticked state unaltered.

This is much simpler ❤️ Thanks!

jorisdral · 2023-02-24T13:22:39Z

ouroboros-consensus-test/src/Test/Util/TestBlock.hs

+            (Ticked (LedgerState (TestBlockWith ptype)))
+applyDirectlyToPayloadDependentState (TickedTestLedger st) tx = do
+    payloadDepSt' <- applyPayload (payloadDependentState st) tx
+    pure $ TickedTestLedger $ st { payloadDependentState = payloadDepSt' }


Suggested change

pure $ TickedTestLedger $ st { payloadDependentState = payloadDepSt' }

pure $ TickedTestLedger $ st { payloadDependentState = payloadDepSt' }

jorisdral · 2023-02-24T13:33:31Z

ouroboros-consensus-test/bench-mempool/Main.hs

+                 length mempoolTxs @?= n
+             ]
+      where
+        benchAddNTxs n = bench (show n) $ nfIO $ addNTxs' $ mkNTryAddTxs n


If the cost of adding the txs is what you want to benchmark, and not opening the mocked mempool or creating the mocked transactions you want to add, I would suggest using env/envWithCleanup to do that work before running the benchmark, and then only calling the function-under-bench with those inputs. The inputs will by then already have been forced to normal form by env, such that the benchmark only measures the time spent adding the txs. Using env might not be strictly useful if the setup is much cheaper than adding the txs, but I don't think it hurts to use env regardless

It would look something like:

Suggested change

benchAddNTxs n = bench (show n) $ nfIO $ addNTxs' $ mkNTryAddTxs n

benchAddNTxs n = env (doSetup n) $ \(mempool, txs) ->

bench (show n) $ nfIO $ addNTxs' mempool

Absolutely! I overlooked this. Thank you 🙏

Hmm, env's documentation recommends using withResource, which does not seem to perform any normal form evaluation.

I hope I never forget to use a function like env when benchmarking.

Before env:

With env:

Generating transactions looked relatively cheap, so maybe opening the mempool was the costly part

jorisdral · 2023-02-24T13:36:46Z

ouroboros-consensus-test/bench-mempool/Main.hs

+             [ testCase "Transactions are added" $ do
+                 let
+                   n   = 1000
+                   txs = mkNTryAddTxs 1000
+                 mempool    <- addNTxs txs
+                 mempoolTxs <- getTxs mempool
+                 mempoolTxs @?= txsAddedInCmds txs
+                 length mempoolTxs @?= n
+             ]


To be sure, I would run this test for each of the n you use in the benchmarks above here, e.g., by moving the test into benchAddNTxs. Each benchmark then has a sanity check

Another addition would be to add a regular quickcheck test that runs the test for arbitrary values of n, such that we're also leveraging property testing in addition to sanity checks for the benchmarks

Good point. In this case, I wanted to avoid the cost of having to test each of the values of n. Eventually, we should test the interpretation of commands by means of the mempool property tests, but I think it makes sense to test each case.

jorisdral · 2023-02-24T13:42:35Z

ouroboros-consensus-test/bench-mempool/Main.hs

+-------------------------------------------------------------------------------}
+
+addNTxs' :: [MempoolCmd TestBlock] -> IO ()
+addNTxs' = void . addNTxs


I can't check now because the module containing MemppolWithMockedLedgerItf is not available on the branch, but could the void here be preventing some evaluation inside the mempool that we would want to force?

I removed this function in favour of the new approach using env.

ouroboros-consensus-test/bench-mempool/Bench/Consensus/MempoolWithMockedLedgerItf.hs

jorisdral · 2023-02-24T14:33:23Z

ouroboros-consensus-test/bench-mempool/Bench/Consensus/MempoolWithMockedLedgerItf.hs

+
+import           Ouroboros.Consensus.Mempool (Mempool)
+import qualified Ouroboros.Consensus.Mempool as Mempool
+


Suggested change

dnadales · 2023-02-27T11:25:06Z

LGTM! Ideally we would like to dump/publish those results somewhere and enable comparisons across changes, otherwise I am afraid we won't reap the benefits of this approach.

Makes sense. Do you have any references I could look into to see how this is done? We can do this in a following step.

abailly-iohk · 2023-02-27T13:11:49Z

We can definitely do this in a following step. The way I do it is usually quite simple: generate some markdown file containing your data, publish it as an artifact as part of the CI, then download it somewhere else (eg. in the docs) for publication.

dnadales · 2023-02-28T11:50:45Z

bors r+

iohk-bors · 2023-02-28T13:47:00Z

Build succeeded:

dnadales requested a review from nfrisby as a code owner February 24, 2023 11:07

dnadales requested review from abailly-iohk, bartfrenk and jorisdral and removed request for nfrisby February 24, 2023 11:08

dnadales added 2 commits February 24, 2023 12:54

Add mkCapacityBytesOverride

3942477

a convenience function to create an override for the mempool capacity using the provided number bytes.

Create an empty mempool benchmark

ddd446e

dnadales force-pushed the dnadales/4382-add-getSnapshotFor-benchmarks branch from 1ce84b2 to ead040a Compare February 24, 2023 11:55

abailly-iohk approved these changes Feb 24, 2023

View reviewed changes

jorisdral reviewed Feb 24, 2023

View reviewed changes

Add a benchmark for adding transactions

3ecdba7

dnadales force-pushed the dnadales/4382-add-getSnapshotFor-benchmarks branch from ead040a to 3ecdba7 Compare February 24, 2023 14:09

jorisdral reviewed Feb 24, 2023

View reviewed changes

dnadales added 2 commits February 27, 2023 16:13

Address minor remarks.

4f1ac56

Use env

7747e04

jorisdral approved these changes Feb 27, 2023

View reviewed changes

iohk-bors bot merged commit 2aa7ee9 into master Feb 28, 2023

iohk-bors bot deleted the dnadales/4382-add-getSnapshotFor-benchmarks branch February 28, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a bechmark for adding transactions to the mempool #4400

Add a bechmark for adding transactions to the mempool #4400

dnadales commented Feb 24, 2023 •

edited

Loading

abailly-iohk left a comment

jorisdral left a comment •

edited

Loading

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

jorisdral Feb 27, 2023

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

jorisdral Feb 24, 2023

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

dnadales Feb 27, 2023

dnadales Feb 27, 2023 •

edited

Loading

jorisdral Feb 27, 2023

jorisdral Feb 24, 2023 •

edited

Loading

dnadales Feb 27, 2023

jorisdral Feb 24, 2023

dnadales Feb 27, 2023

jorisdral Feb 24, 2023

dnadales commented Feb 27, 2023

abailly-iohk commented Feb 27, 2023

dnadales commented Feb 28, 2023

iohk-bors bot commented Feb 28, 2023

		- Added `mkCapacityBytesOverride`, a convenience function to create an override
		for the mempool capacity using the provided number bytes.

		-- \| Apply the payload to a ticked state directly to the payload dependent state
		-- portion of it, leaving the rest of the input ticked state unaltered.

	pure $ TickedTestLedger $ st { payloadDependentState = payloadDepSt' }
	pure $ TickedTestLedger $ st { payloadDependentState = payloadDepSt' }

	benchAddNTxs n = bench (show n) $ nfIO $ addNTxs' $ mkNTryAddTxs n
	benchAddNTxs n = env (doSetup n) $ \(mempool, txs) ->
	bench (show n) $ nfIO $ addNTxs' mempool


		import Ouroboros.Consensus.Mempool (Mempool)
		import qualified Ouroboros.Consensus.Mempool as Mempool

Add a bechmark for adding transactions to the mempool #4400

Add a bechmark for adding transactions to the mempool #4400

Conversation

dnadales commented Feb 24, 2023 • edited Loading

Checklist

abailly-iohk left a comment

Choose a reason for hiding this comment

jorisdral left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadales Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadales commented Feb 27, 2023

abailly-iohk commented Feb 27, 2023

dnadales commented Feb 28, 2023

iohk-bors bot commented Feb 28, 2023

dnadales commented Feb 24, 2023 •

edited

Loading

jorisdral left a comment •

edited

Loading

dnadales Feb 27, 2023 •

edited

Loading

jorisdral Feb 24, 2023 •

edited

Loading