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

Ensure that transaction tokens are unique in the mempool benchmarks #1095

Merged
merged 2 commits into from
May 14, 2024

Conversation

jorisdral
Copy link
Contributor

@jorisdral jorisdral commented May 9, 2024

No description provided.

@jorisdral jorisdral force-pushed the jdral/mempool-bench-investigations branch 2 times, most recently from a3ee2d9 to 67c9c4c Compare May 9, 2024 10:59
@jorisdral jorisdral changed the title Fix mempool benchmark setup Ensure that transaction tokens are unique in the mempool benchmarks May 9, 2024
@jorisdral jorisdral force-pushed the jdral/mempool-bench-investigations branch from 67c9c4c to 01e0654 Compare May 9, 2024 11:00
@jorisdral
Copy link
Contributor Author

jorisdral commented May 9, 2024

We might want to switch to criterion instead of tasty-bench (eventhough I had initially advocated for using tasty-bench in this benchmark 😛). criterion has perRunEnv, which is arguably more suitable than withResource, and then mental/manual subtraction of measurements won't be required. There is the question whether criterion is compatible with github-action-benchmark. I found this PR, but it hasn't been updated in a while, but it leads me to believe that the CSV/JSON output of criterion can be mapped to the format that github-action-benchmark requires. It would mean we'd have to write the format conversion stuff that we currently have for tasty-bench for criterion as well

@jorisdral jorisdral self-assigned this May 9, 2024
@jorisdral jorisdral marked this pull request as ready for review May 9, 2024 11:23
@jorisdral jorisdral requested a review from a team as a code owner May 9, 2024 11:23
jorisdral added 2 commits May 14, 2024 11:08
Data of type `Token` is what a mempool `TestBlock` transaction (`Tx`) consumes
and produces. Previously, this token was a newtype around a `Word8`. There can
only be around 256 unque `Word8` tokens, which means that its likely for this
value to wrap around. In the current benchmark scenario, where we add linearly
dependent transactions with only one input and output to a mempool, this wrap
around was occurring and therefore the list of transaction that was being
generated had many duplicates. The type of tokens is therefore changed to be a
newtype around an `Int`, which won't wrap around (in a realistic benchmark).

Abovementioned change exposed subtle performance effects on the mempool. Most of
this can be attributed to the `Tx` type doubling as a `GenTxId`. The mempool
keeps track of these identifiers in a set. Since there were only around 256
unique transactions, this set was rather small. Changing tokens to be `Int`s
ensured that this set could grow much larger before becoming full, and that by
itself has subtle effects on the performance of the mempool.

Apart from changing the type of tokens, the mempool benchmark setup is reworked:
* Transactions that are used as benchmark inputs are pre-generated and fully
  evaluated so that the generation work is not measured in the benchmarked
  function itself.
* Instead of opening a mempool once and removing transactions after each
  benchmark run, a new mempool is opened in each benchmark run. Removing
  transactions proved to be prohibitively costly compared to the cost of opening
  a new mempool.
* A new benchmark is added that measures just the mempool setup time. To obtain
  the cost of just adding the transactions without cost of opening the mempool,
  the time of this new benchmark can be manually subtracted from the full
  benchmark time.
@jorisdral jorisdral force-pushed the jdral/mempool-bench-investigations branch from 01e0654 to 9f7a6ef Compare May 14, 2024 09:09
@jorisdral jorisdral added this pull request to the merge queue May 14, 2024
Merged via the queue into main with commit fbf86c7 May 14, 2024
14 checks passed
@jorisdral jorisdral deleted the jdral/mempool-bench-investigations branch May 14, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants