Skip to content

execution/stagedsync: hand the trace sets to the apply result instead of copying - #23092

Merged
AskAlexSharov merged 3 commits into
mainfrom
alex/traces_no_copy_37
Aug 9, 2026
Merged

execution/stagedsync: hand the trace sets to the apply result instead of copying#23092
AskAlexSharov merged 3 commits into
mainfrom
alex/traces_no_copy_37

Conversation

@AskAlexSharov

Copy link
Copy Markdown
Collaborator

The publish path in nextResult allocated two empty maps into the apply result, then copied every entry of the transaction's trace sets into them. The result already owns those maps.

  • The tracer is a local in Worker.RunTxTaskNoLock, built per transaction. The parallel Worker has no CallTracer field — it keeps evm and ibs across transactions, not the tracer.
  • CallTracer.OnEnter lazily allocates froms/tos inside that instance and Froms()/Tos() hand them straight to result.TraceFroms / TraceTos. TxResult is not pooled.
  • applyLogsAndTraces4 only ranges over the maps — no writes, no deletes.
  • Nothing reads result.TraceFroms after the publish block. finalizedResults[tx-1] is read again, but only for .Receipt, and takeNextPending + markComplete publish each transaction once.

The block-end result in the same function already assigns them directly:

traceFroms:            lastResult.TraceFroms,
traceTos:              lastResult.TraceTos,

This makes the per-transaction path match. Costs two map allocations and one insert per distinct from/to address per transaction, on the serial apply loop.

Nil is safe: range over a nil map is zero iterations, and the block-end path already passes possibly-nil maps through.

If the tracer is ever made worker-held to drop its per-transaction allocation, this becomes wrong — but so would the block-end path, and the fix then is Reset semantics at the worker rather than a defensive copy on the apply loop.

No new tests: pure refactor, no behaviour change. Covered by the existing trace-index tests.

Green: execution/stagedsync, execution/tests, rpc/jsonrpc.

… of copying

The publish path allocated two empty maps and copied every entry of the tx's
trace sets into them. The tracer is built per tx inside Worker.RunTxTaskNoLock
and nothing else keeps a reference, so the result already owns those maps, and
the consumer only ranges over them. The block-end result four hundred lines
below already assigns them directly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes the parallel execution publish path in execution/stagedsync by removing per-transaction copying of trace address sets (from/to) into freshly-allocated maps when building the apply result. Instead, it directly hands the existing maps from the finalized execution result into the txResult sent through the apply/commit fan-out, aligning the per-transaction path with the existing block-end path and reducing allocations and per-entry inserts on the serial apply loop.

Changes:

  • Remove maps.Copy-based cloning of TraceFroms/TraceTos during nextResult publishing.
  • Assign result.TraceFroms/result.TraceTos directly into the per-tx txResult (traceFroms/traceTos), eliminating two map allocations per published tx.
  • Drop the now-unused maps import.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM — verified the ownership and read-only claims against the code, and ran execution/stagedsync under -race plus the rpc/jsonrpc trace tests locally on the branch.

Nit, pre-existing and fine as a follow-up: the block-finalize txResult carries traceFroms/traceTos that no consumer ever reads — isFinalize has been write-only since #20805, and no caller passes ApplyTxIndexes' skipReceiptCache although its docstring cites exactly that finalize result. So the block-end precedent cited here is safe but vacuous: the coinbase TracesToIdx entry actually comes from the block-end task's own per-tx publish. Dropping those dead fields and the stale docstring would tidy this up.

@AskAlexSharov
AskAlexSharov added this pull request to the merge queue Aug 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Aug 7, 2026
@AskAlexSharov
AskAlexSharov enabled auto-merge August 9, 2026 15:53
@AskAlexSharov
AskAlexSharov added this pull request to the merge queue Aug 9, 2026
Merged via the queue into main with commit af146ef Aug 9, 2026
133 checks passed
@AskAlexSharov
AskAlexSharov deleted the alex/traces_no_copy_37 branch August 9, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants