chore(avm): report total instructions executed from the public tx simulator#24968
Open
vezenovm wants to merge 8 commits into
Open
chore(avm): report total instructions executed from the public tx simulator#24968vezenovm wants to merge 8 commits into
vezenovm wants to merge 8 commits into
Conversation
…lator The simulator contract benchmarks lost their "instructions executed" metric when the TS AVM simulator was removed: recordEnqueuedCallSimulation (which carried the per-call instruction count) has no callers under the C++ simulator, so TestExecutorMetrics summed an always-empty list and reported a flat 0. manaUsed had the same problem but was rescued by reading tx-level gasUsed; the instruction count was not, because the C++ result carried no such field. Expose it from the C++ simulator via a new TxSimulationResult field, total_instructions_executed, set to the AVM execution-id delta: the execution loop advances the execution id once per instruction it processes, across all enqueued and nested calls, so the delta is the number of executed instructions (equivalently, execution trace rows). Thread it through PublicTxResult and record it as the tx-level TestExecutorMetrics totalInstructionsExecuted, which the benchmark JSON already reports.
…serialization fallback Addresses review feedback: - avm_test.test.ts: run the real simulator through an explicit TestExecutorMetrics and assert the emitted totalInstructionsExecuted matches the value on the PublicTxResult, so a miswired metrics call site (which the prior tests would not have caught) fails the test. - avm.test.ts: exercise PublicTxResult.fromPlainObject (the production C++ deserialization path, not just the schema) for present, missing, and null totalInstructionsExecuted, using a real serialized tx_result as the base so sub-fields keep their C++ shapes.
…in object The committed tx_result testdata fixture is obsolete (it has no publicTxEffect key), so PublicTxResult.fromPlainObject throws in PublicTxEffect.fromPlainObject before reaching the totalInstructionsExecuted fallback. Replace it with a minimal, well-formed plain object built in the test (Fr/RevertCode plain values are bare numbers; the optional proving fields are omitted) so the present/missing/null cases actually exercise `obj.totalInstructionsExecuted ?? 0`.
Drop the isolated PublicTxResult schema/fromPlainObject unit tests and the standalone TestExecutorMetrics unit test. The avm_test bulk integration test already exercises totalInstructionsExecuted through the real simulator and into the benchmark JSON, so those add nothing. Keep the C++ fast-vs-witgen count check, drop the redundant halting-instruction test, shorten the TxSimulationResult field comment to one line, and drop the now-pointless "(C++)"/"hint-collecting" qualifiers in comments (there is only one simulator).
vezenovm
changed the title
The bulk test keeps the direct assertion that the tx result carries a non-zero instruction count (exercising the C++ result deserialization); the metrics round-trip is validated by the actual benchmark CI job, so the in-test benchmark-JSON matching and its metrics plumbing are removed.
The tx-level total_instructions_executed replaced a TS-side sum over per-enqueued-call counts, so pin that it still spans every enqueued call and every nested call underneath them.
vezenovm
marked this pull request as ready for review
July 24, 2026 17:31
vezenovm
requested review from
Thunkar,
dbanks12 and
fcarreiro
and removed request for
MirandaWood
July 24, 2026 17:31
vezenovm
commented
Jul 24, 2026
Collaborator
Flakey Tests🤖 says: This CI run detected 3 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
fcarreiro
reviewed
Jul 25, 2026
| GasUsed gas_used; | ||
| RevertCode revert_code; | ||
| // Total AVM instructions executed by the tx, across all enqueued calls and their nested calls. | ||
| uint32_t total_instructions_executed = 0; |
Contributor
There was a problem hiding this comment.
This is a bit ad-hoc, and ad-hoc additions grow organically. There was already a plan to support statistics such as this one, but we never got to implement it: http://linear.app/aztec-labs/issue/AVM-194/implement-stats-collection-for-c-simulator
The idea is to add a map string->string here.
Then, use collect_statistics as a filter (because some stats might be expensive): https://github.com/AztecProtocol/aztec-packages/blob/next/barretenberg/cpp/src/barretenberg/vm2/common/avm_io.hpp#L453C10-L453C28
The original plan, as the Linear issue mentions, was to use a kind of statistics collector class, which could then be either a noop, or a real one. I don't expect anyone will be adding more stats than this, so maybe THAT is overkill and you can avoid it.
I saw you used the execution_id to count instructions. That is smart, since you don't need to modify the simulator loop itself to add an instruction counter. However, it uses/exposes internal information that might not always work like that. I'm not completely against it, especially since you avoid modifying the Execution loop. Strictly speaking it could bring problems down the line, but the AVM is in maintenance mode so I don't expect that would happen.
Of course, I'm not in the team anymore, so @IlyasRidhuan to decide on the above ;)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The public-tx simulator contract benchmarks (dashboard:
yarn-project/simulator/<contract tests>)lost their "instructions executed" metric when the TS AVM simulator was removed only total duration
and mana remained. This is a regression in our ability to track opcode-level work (e.g. the public
dispatch/macro optimizations thatpublic_fns_with_emit_repro_contracttracked in the benchmark).Root cause
recordEnqueuedCallSimulation(which carried the per-call instruction count from the old TSinterpreter) has no callers once simulation routes through the C++ simulator, so
TestExecutorMetrics.stopRecordingTxSimulationsummed an always-emptyenqueuedCallslist andreported a flat
0.manaUsedhit the same wall but was rescued earlier by reading the tx-levelgasUsed; the instruction count was not, because the C++ result carried no such field.(For clarity: the public-bytecode-size metric was not lost — it moved to a dedicated noir-contracts
bench and lives on the dashboard under
noir-projects/noir-contracts/artifact-size/<contract>/.)Change
total_instructions_executedtoTxSimulationResult, populated in both result builders(
simulate_fast_internalandsimulate_for_witgen_internal) from the AVM execution-id delta. Theexecution loop advances the execution id exactly once per instruction it processes — across all
enqueued and nested calls — so the delta is the number of instructions executed, i.e. the number of
AVM execution trace rows. This is a simulation-output field only: no proving / VK impact.
PublicTxResultand record it as the tx-levelTestExecutorMetrics.totalInstructionsExecuted, which the benchmark JSON already emits — so thedashboard series is repopulated with no dashboard-side change.
diverge on it.
The metric counts AVM execution steps: one per instruction the VM processes, including a terminal
halting/failing instruction. For successful executions — which is all of the benchmarked contracts —
this equals the number of instructions executed.