Skip to content

feat: benchmark avm simulator#12985

Merged
dbanks12 merged 50 commits intomasterfrom
db/bench
Mar 27, 2025
Merged

feat: benchmark avm simulator#12985
dbanks12 merged 50 commits intomasterfrom
db/bench

Conversation

@dbanks12
Copy link
Contributor

@dbanks12 dbanks12 commented Mar 24, 2025

This PR does not integrate benchmarks into CI. It updates the simulator tests to support benchmarking, adds a bench test, and pretty-prints simulator benchmarks.

AvmSimulator

  • instrCounter tracked in machine state. When a nested call returns, its parent absorbs its instrCounter. This might seem weird, but it's the metric we want. If it feels too wrong, i'm fine having both an instrCounter and a totalInstrCounter. Or we can rename this one totalInstrCounter for clarity.

PublicTxSimulationTester, SimpleContractDataSource

  • SimpleContractDataSource now tracks contract & function names so that getDebugFunctionName() works properly in simulator tests
  • Tester only creates a single PublicTxSimulator that is used for all simulations instead of one per simulation
  • Test can create a TestExecutorMetrics and pass it into PublicTxSimulationTester constructor so that many test cases can aggregate metrics into the same class.

Metrics / Benchmarking

I opted not to use the telemetry based benchmarking used by e2e_block_building.test.ts. Instead, I created a custom TestExecutorMetrics for benchmarking the simulator in exactly the way that works for us. We can easily add toGithubActionsBenchmark() adapter function if it is valuable.

Running the tests with BENCH_OUTPUT_MD set will output the results to the specified markdown file. Running them without that env var set will log.info them.

New AMM test isolated to public simulation for measurements

This is brittle. It gives us measurements, but will break if any changes are made to AMM.
image
image
image

@dbanks12 dbanks12 requested a review from charlielye as a code owner March 24, 2025 17:40
@dbanks12 dbanks12 changed the title [WIP] feat: benchmark avm simulator feat: benchmark avm simulator Mar 26, 2025
@dbanks12 dbanks12 requested a review from spalladino March 26, 2025 01:55
@fcarreiro fcarreiro self-requested a review March 27, 2025 16:32
Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

Looks good in general

@dbanks12
Copy link
Contributor Author

Brittle AMM test's measurements
image

Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

Approving cause I requested changes but will let @spalladino approve

@dbanks12 dbanks12 requested a review from nventuro March 27, 2025 19:06
Comment on lines +13 to +16
/**
* A public tx simulator that tracks runtime metrics in production.
*/
export class TelemetryPublicTxSimulator extends MeasuredPublicTxSimulator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: why is this a different class, instead of just adding the trackSpans to the MeasuredPublicTxSimulator? I understand they shouldn't hurt during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because then the MeasuredPublicTxSimulator needs telemetry and needs a tracer so that the trackspans work! I don't feel strongly about keeping it this way though. I was just experimenting...

}

startRecordingTxSimulation(txLabel: string) {
assert(!this.currentTxLabel, 'Cannot start recording tx simulation when another is live');
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand we should never be simulating more than 1 tx at the same time, right? Otherwise we can use threadlocal storage for this flag, but it doesn't seem to be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this assertion is just to prevent footguns accidentally calling this twice without calling stop

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

This is indeed brittle, but a) it's well documented, so it should be very easy to update if needed, and b) I don't think we'll be performing any updates to this flow. So it makes sense to me to do thigs this way if the rapid execution speed is useful to you.

@dbanks12 dbanks12 enabled auto-merge (squash) March 27, 2025 21:06
@dbanks12 dbanks12 merged commit 00fae1b into master Mar 27, 2025
7 checks passed
@dbanks12 dbanks12 deleted the db/bench branch March 27, 2025 21:53
PhilWindle pushed a commit that referenced this pull request Mar 27, 2025
🤖 I have created a new Aztec Packages release
---


##
[0.82.3](v0.82.2...v0.82.3)
(2025-03-27)


### Features

* `msgpack` encoding for `Program` and `WitnessStack`
([#12841](#12841))
([1e58eb1](1e58eb1))
* 64 bit log type id, 64 bit log metadata
([#12956](#12956))
([20d734a](20d734a))
* AVM parsing tag validation
([#12936](#12936))
([56b1f0d](56b1f0d))
* **avm:** add calldata & returndata to context
([#13008](#13008))
([f03b2e5](f03b2e5))
* **avm:** merkle db hints (part 1)
([#12922](#12922))
([34ec9e8](34ec9e8))
* **avm:** merkle hints (part 2)
([#13077](#13077))
([fbbc6c7](fbbc6c7))
* **avm:** vm2 initial context
([#12972](#12972))
([e2b1361](e2b1361))
* benchmark avm simulator
([#12985](#12985))
([00fae1b](00fae1b))
* client flows benchmarks
([#13007](#13007))
([9bf7568](9bf7568))
* gas benchmark for "normal usage"
([#13073](#13073))
([4eb1156](4eb1156))
* Implement merkle writes in the merkle check gadget
([#13050](#13050))
([c94fe50](c94fe50))
* LogEncryption trait
([#12942](#12942))
([0b7e564](0b7e564))
* Node snapshot sync
([#12927](#12927))
([afde851](afde851)),
closes
[#12926](#12926)
* **p2p:** capture all gossipsub metrics
([#12930](#12930))
([cc940cb](cc940cb))
* Prover node snapshot sync
([#13097](#13097))
([1e77efb](1e77efb))
* staking asset handler
([#12968](#12968))
([af48184](af48184)),
closes
[#12932](#12932)
* stream crs data to disk
([#12996](#12996))
([d016e4d](d016e4d)),
closes
[#12948](#12948)
* track failed tests. add flake.
([f4936d7](f4936d7))
* Track test history.
([#13037](#13037))
([036bb32](036bb32))
* track total tx fee
([#12601](#12601))
([9612a4e](9612a4e))
* Validators sentinel
([#12818](#12818))
([770695c](770695c))


### Bug Fixes

* added #[derive(Eq)] to EcdsaPublicKeyNote
([#12966](#12966))
([0c21c74](0c21c74))
* Allow use of local blob sink client
([#13025](#13025))
([ba8d654](ba8d654))
* **avm:** semicolons are hard
([#12999](#12999))
([8871c83](8871c83))
* bootstrap network and sponsored fpc devnet
([#13044](#13044))
([8a47d8b](8a47d8b))
* Bump tsc target
([#13052](#13052))
([985e83b](985e83b))
* cycle_group fuzzer
([#12921](#12921))
([69f426e](69f426e))
* **docs:** Fix import errors in aztec.js tutorial
([#12969](#12969))
([856208a](856208a))
* **docs:** Load token artifact from the compiled source in the sample
dapp tutorial
([#12802](#12802))
([0838084](0838084)),
closes
[#12810](#12810)
* **docs:** Update sponsored fpc docs to use 82.2 syntax
([#13054](#13054))
([e5d425b](e5d425b))
* **e2e:** p2p
([#13002](#13002))
([1ece539](1ece539))
* extend e2e 2 pxes timeout. strip color codes for error_regex.
([73820e4](73820e4))
* flake
([6cc9e81](6cc9e81))
* fuzzer on staking asset handler constructor test
([#13101](#13101))
([d936285](d936285))
* invalid getCommittee function
([#13072](#13072))
([327341f](327341f))
* mac publish should use clang 18 like x-compiler, and use it
([#12983](#12983))
([7b83c45](7b83c45))
* make circuit parsing deterministic
([#11772](#11772))
([76ef873](76ef873))
* parse away trailing slash from consensus host
([#12577](#12577))
([6701806](6701806))
* prerelease versions should be pushed to install.aztec.network
([#13086](#13086))
([c4e6039](c4e6039))
* smoke
([#13060](#13060))
([7756b15](7756b15))
* some flake additions
([58638f1](58638f1))
* sponsored fpc arg parsed correctly
([#12976](#12976))
([#12977](#12977))
([a85f530](a85f530))
* starting the sandbox with no pxe should still deploy initial test
accounts
([#13047](#13047))
([d92d895](d92d895))
* Syntax error when running tests via jest after tsc build
([#13051](#13051))
([f972db9](f972db9))
* Use the correct image in aztec start
([#13058](#13058))
([06285cd](06285cd))
* yolo fix
([91e2f4b](91e2f4b))
* yolo fix nightly
([b3b3259](b3b3259))
* yolo fix obvious thing to track fails.
([2fee630](2fee630))
* yolo flakes
([e3b030a](e3b030a))
* yolo set -x
([bfd3205](bfd3205))
* yolo we suspect the halt is making tests fail that would have passed
([04e3fa2](04e3fa2))


### Miscellaneous

* `getIndexedTaggingSecretAsSender` oracle cleanup
([#13015](#13015))
([8e71e55](8e71e55))
* Add a script to generate cpp files for AVM2
([#13091](#13091))
([7bb43a9](7bb43a9))
* add default native proving for cli wallet
([#12855](#12855))
([c0f773c](c0f773c))
* add default native proving for cli wallet retry
([#13028](#13028))
([b2f4785](b2f4785))
* Alpha testnet into master
([#13033](#13033))
([d98fdbd](d98fdbd))
* AVM TS - move tag validation outside of instruction constructors
([#13038](#13038))
([45548ab](45548ab)),
closes
[#12934](#12934)
* **avm:** final codegen nuking
([#13089](#13089))
([9c82f3f](9c82f3f))
* **avm:** remove codegen (all but flavor)
([#13079](#13079))
([e1f2bdd](e1f2bdd))
* **bb:** minor acir buf C++ improvements
([#13042](#13042))
([1ebd044](1ebd044))
* boxes dep cleanup
([#12979](#12979))
([6540b7c](6540b7c))
* **ci:** less catch all e2e_p2p flakes
([#12737](#12737))
([2134634](2134634))
* comprehensive cleanup of translator flavor and use inheritance
properly in flavors
([#13041](#13041))
([dc5f78f](dc5f78f))
* compress storage footprint
([#12871](#12871))
([58c110f](58c110f))
* display warning when installing bb versions < 0.82.0
([#13027](#13027))
([7247fe7](7247fe7))
* **docs:** Update docs on fees and various other updates
([#12929](#12929))
([1dec907](1dec907))
* dump dmesg/net/cpu/mem usage at end of ci run
([#12967](#12967))
([8877792](8877792))
* fix governance util issue
([#13043](#13043))
([d768d26](d768d26))
* redundant if in affine from projective constructor
([#13045](#13045))
([3a7ba2d](3a7ba2d))
* remove addition of dummy ops in mock circuit producer
([#13003](#13003))
([a64d1dc](a64d1dc))
* remove dummy ops in decider pk
([#13049](#13049))
([da6d021](da6d021))
* replace relative paths to noir-protocol-circuits
([e1b88f6](e1b88f6))
* replace relative paths to noir-protocol-circuits
([849b4b0](849b4b0))
* replace relative paths to noir-protocol-circuits
([18a02d6](18a02d6))
* Revert "chore: add default native proving for cli wallet
([#12855](#12855))"
([#13013](#13013))
([98e2576](98e2576))
* Speed up and deflake sentinel test
([#13078](#13078))
([27f1eca](27f1eca))
* **testnet:** making consensus host mandatory input
([#12716](#12716))
([d47c74a](d47c74a))
* towards no more mock op_queues
([#12984](#12984))
([fefffa7](fefffa7))
* update bb version for noir 1.0.0-beta.0+
([#13026](#13026))
([dd68074](dd68074))
* update CODEOWNERS to reflect new sync method
([#12998](#12998))
([a3d1915](a3d1915))


### Documentation

* Add fees to cli reference
([#12884](#12884))
([4a0fd58](4a0fd58))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

4 participants