Skip to content

perf(core): decode execution status via partial-CBOR projection - #3890

Open
EgeCaner wants to merge 4 commits into
mainfrom
perf/core/reduce-allocs-on-execution-status-read
Open

perf(core): decode execution status via partial-CBOR projection#3890
EgeCaner wants to merge 4 commits into
mainfrom
perf/core/reduce-allocs-on-execution-status-read

Conversation

@EgeCaner

Copy link
Copy Markdown
Contributor

Same CBOR discard trick applied to ExecutionStatus reads. Saves 2 alloc per skipped key, for status 7 of them are skipped thus saves 14 alloc on execution status read.

Block 0 — genesis-era (all 0-event receipts)

tx events time old→new Δ time mem old→new Δ mem allocs old→new Δ allocs
00 0 3.27 → 3.29 µs +0.7% 880 → 676 B −23.2% 32 → 18 −43.8%
01 0 3.25 → 3.34 µs +2.9% 880 → 676 B −23.2% 32 → 18 −43.8%
02 0 3.42 → 3.62 µs +5.8% 880 → 676 B −23.2% 32 → 18 −43.8%
03 0 3.26 → 3.34 µs +2.6% 880 → 676 B −23.2% 32 → 18 −43.8%
04 0 3.24 → 3.40 µs +4.7% 880 → 676 B −23.2% 32 → 18 −43.8%
05 0 3.28 → 3.41 µs +3.9% 880 → 676 B −23.2% 32 → 18 −43.8%
06 0 3.47 → 3.58 µs +3.4% 880 → 676 B −23.2% 32 → 18 −43.8%
07 0 3.28 → 3.36 µs +2.3% 880 → 676 B −23.2% 32 → 18 −43.8%
08 0 3.28 → 3.35 µs +2.1% 880 → 676 B −23.2% 32 → 18 −43.8%
09 0 3.27 → 3.36 µs +2.6% 880 → 676 B −23.2% 32 → 18 −43.8%
10 0 3.27 → 3.32 µs +1.4% 880 → 676 B −23.2% 32 → 18 −43.8%
11 0 3.34 → 3.31 µs −0.7% 880 → 676 B −23.2% 32 → 18 −43.8%
12 0 3.53 → 3.55 µs +0.4% 880 → 676 B −23.2% 32 → 18 −43.8%
13 0 3.33 → 3.35 µs +0.4% 880 → 676 B −23.2% 32 → 18 −43.8%
14 0 3.27 → 3.30 µs +1.1% 880 → 676 B −23.2% 32 → 18 −43.8%
15 0 3.29 → 3.27 µs −0.4% 880 → 676 B −23.2% 32 → 18 −43.8%
16 0 3.28 → 3.30 µs +0.4% 880 → 676 B −23.2% 32 → 18 −43.8%
17 0 3.26 → 3.28 µs +0.5% 880 → 676 B −23.2% 32 → 18 −43.8%

Block 763497 — mainnet (sorted by event count)

tx events time old→new Δ time mem old→new Δ mem allocs old→new Δ allocs
16 2 4.01 → 4.10 µs +2.4% 896 → 696 B −22.3% 33 → 19 −42.4%
01 3 4.27 → 4.27 µs −0.1% 896 → 696 B −22.3% 33 → 19 −42.4%
08 3 4.72 → 4.81 µs +2.0% 896 → 696 B −22.3% 33 → 19 −42.4%
13 3 4.37 → 4.36 µs −0.1% 896 → 696 B −22.3% 33 → 19 −42.4%
14 7 22.40 → 22.38 µs −0.1% 896 → 696 B −22.3% 33 → 19 −42.4%
10 8 9.20 → 9.12 µs −0.9% 896 → 696 B −22.3% 33 → 19 −42.4%
06 9 10.99 → 10.83 µs −1.4% 896 → 696 B −22.3% 33 → 19 −42.4%
11 11 8.99 → 8.85 µs −1.6% 896 → 696 B −22.3% 33 → 19 −42.4%
12 14 16.92 → 16.86 µs −0.4% 896 → 696 B −22.3% 33 → 19 −42.4%
09 15 8.44 → 8.42 µs −0.2% 896 → 696 B −22.3% 33 → 19 −42.4%
05 23 13.76 → 13.65 µs −0.8% 896 → 696 B −22.3% 33 → 19 −42.4%
04 26 14.14 → 13.98 µs −1.2% 896 → 696 B −22.3% 33 → 19 −42.4%
15 32 14.56 → 14.77 µs +1.4% 896 → 696 B −22.3% 33 → 19 −42.4%
00 42 20.99 → 20.76 µs −1.1% 896 → 696 B −22.3% 33 → 19 −42.4%
02 42 20.62 → 20.59 µs −0.1% 896 → 696 B −22.3% 33 → 19 −42.4%
07 45 16.36 → 16.23 µs −0.8% 896 → 696 B −22.3% 33 → 19 −42.4%
03 52 36.69 → 36.78 µs +0.2% 896 → 696 B −22.3% 33 → 19 −42.4%

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @EgeCaner's task in 5m 2s —— View job


Review: perf(core): decode execution status via partial-CBOR projection

  • Gather context (diff vs perf/core/avoid-per-skipped-key-alloc-headers, CLAUDE.md)
  • Review core/partial_cbor.go projection logic
  • Review call sites in block_transaction*.go
  • Review test coverage
  • Post review (5 inline comments)

The mechanism is right and the change is well-scoped: discardedReceiptSkeleton names all 9 TransactionReceipt fields, receiptExecutionStatus shadows the 2 it wants at depth 0 (shallowest wins), so all 7 unwanted keys skip the allocating unmatched-key path — 7 × 2 = 14 allocs, matching the 32→18 / 33→19 in your tables. Un-exporting ExecutionStatuses() is a bonus: no caller outside core used it. Behaviour is unchanged for the read path — accessors_test.go:214 still compares the partial decode against the full receipt, including the reverted-with-heavy-fields case, so the semantics are pinned end-to-end.

No blockers. No correctness, concurrency, or resource issues found. Everything below is about keeping the guard rails as tight as the header work set them.

Important

  • block_transaction_serializer.go:52 — unguarded drift on the ExecutionStatus side. The package exhaustively guards projection ↔ TransactionReceipt, but nothing guards projection ↔ ExecutionStatus. Add a field to ExecutionStatus and this hand-written copy silently returns its zero value on every read, with no test failing. A reflection assertion that the projection shadows every ExecutionStatus key with a typed (non-discardedCBOR) field closes it, reusing the existing cborKeys.
  • core/receipt_test.go:14 is now stale. TestExecutionStatusDecodesFromReceipt asserts a bare ExecutionStatus decodes from an encoded receipt — the exact naive shape this PR replaced. Nothing in production does that anymore, so it guards no live path. Retarget at receiptExecutionStatus or delete as superseded. Same for the doc on ExecutionStatus (core/receipt.go:29-31): "decoded on its own to skip the heavier fields" now describes receiptExecutionStatus.

Nits

  • partial_cbor.go:102receiptExecutionStatus breaks the <source><field>Projection convention its own accessor (executionStatusProjections) follows. Also worth a comment that the fields are restated rather than embedded from ExecutionStatus: embedding would put Reverted at equal depth from two embeds, which encoding/json visibility rules (which fxamacker adopts) drop entirely — silently reporting every reverted tx as succeeded, with the key-set guards still green.
  • partial_cbor_test.go:62 — the extraction dropped "the strict DecMode lacks the encoder's tag set; valid only because Header has no tagged types". It matters more now: receipts carry []*Event, *ExecutionResources, *L1ToL2Message, none currently in encoder/registry, but registering one later turns this guard into a misleading unregistered-tag failure. The deleted vacuity assertion is still covered by TestStrictGuardCatchesKeyAsIntDrift:170 — a cross-reference would keep that findable.
  • partial_cbor_test.go:407 — headers have the CI-enforced alloc canary (TestDiscardReducesReadAllocations); receipts get only a benchmark, which CI doesn't assert. headerProjectionCase already models the pair — generalising it would cover both and drop this hand-rolled duplicate.

⚠️ I could not run go test ./core/... in this environment (command not permitted), so all of the above is from static reading; the alloc claims are taken from your benchmark tables, not reproduced.
• branch perf/core/reduce-allocs-on-execution-status-read

Comment thread core/block_transaction_serializer.go Outdated
Comment thread core/partial_cbor.go Outdated
Comment thread core/partial_cbor_test.go
Comment thread core/partial_cbor_test.go
Comment thread core/partial_cbor_test.go
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.39%. Comparing base (33a422d) to head (94dbacb).

Files with missing lines Patch % Lines
blockchain/blockchain.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3890      +/-   ##
==========================================
+ Coverage   75.26%   75.39%   +0.13%     
==========================================
  Files         443      443              
  Lines       40058    39940     -118     
==========================================
- Hits        30150    30114      -36     
+ Misses       7806     7733      -73     
+ Partials     2102     2093       -9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from perf/core/avoid-per-skipped-key-alloc-headers to main July 30, 2026 16:20
@EgeCaner
EgeCaner force-pushed the perf/core/reduce-allocs-on-execution-status-read branch from e35cb36 to 94dbacb Compare July 30, 2026 23:44
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @EgeCaner's task in 5m 5s —— View job


Re-review: perf(core): decode execution status via partial-CBOR projection

  • Gather context (3 new commits since the last review)
  • Review the ExecutionStatusTransactionExecutionStatus rename fan-out
  • Verify the dropped receipt_test.go test is genuinely superseded
  • Build + typecheck
  • Post review

No blockers, no new findings. The three follow-up commits are a clean resolution of the previous round.

What I verified on the new commits

Rename fan-out is complete and consistent. grep across core/, blockchain/, sync/, node/, rpc/v9, rpc/v10 and mocks/ shows zero stragglers — no core.ExecutionStatus, no ExecutionStatuses(), no GetExecutionStatusByBlockAndIndex remain. TransactionExecutionStatusByBlockNumberAndIndexGetTransactionExecutionStatusByBlockAndIndexTransactionExecutionStatus now reads as one consistent chain instead of three different names for the same concept (blockchain/blockchain.go:337-341).

The projection rename landed correctly. receiptExecutionStatusProjection (core/partial_cbor.go:102) matches the <source><field>Projection convention, and executionStatusProjectionSlice (core/block_transaction.go:96) keeps the unexported generic instantiation readable at the call site. extractExecutionStatus.extract (core/block_transaction_serializer.go:47-58) correctly returns a zero TransactionExecutionStatus on the error path rather than a partially-populated one.

Dropping TestExecutionStatusDecodesFromReceipt loses no coverage. I checked the specific worry from the last round — that sampleReceipt() leaves Events/ExecutionResources/L1ToL2Message/L2ToL1Message nil, so the unit-level discard path only ever skips null. It's covered elsewhere for real: accessors_test.go:254 ("reverted receipt with heavy fields") seeds a reverted receipt with Events, L2ToL1Message and ExecutionResources taken from a real Sepolia block (4072139, via setupForTxsAndReceiptsTests), writes it through WriteTransactionsAndReceipts, and asserts the partial decode matches the full receipt. So the discard path does skip genuinely populated nested values, through the real DB serializer — not just in a hand-built fixture. Deleting the old test was the right call; it was pinning the naive shape this PR replaced.

Test-helper extraction is behaviour-preserving. assertSkeletonNamesAllFields / assertProjectionsCoverSource / assertProjectionsCoverEveryWireKey are faithful factorings of the inlined header assertions, and the Non-vacuous: see TestStrictGuardCatchesKeyAsIntDrift cross-reference keeps the deleted vacuity block discoverable. TestExecutionStatusProjectionDecodesShadowedFields:378 pins the load-bearing property — that the depth-0 Reverted/RevertReason win the key over the embedded discardedCBOR fields — through the real encoder, which is exactly the guard the equal-depth-embed hazard needed.

Nit (optional, non-blocking)

  • TestProjectionsAreDecodeOnly:325 still only exercises headerHashProjection. Adding &receiptExecutionStatusProjection{} is a one-liner, though the marshal refusal lives on the shared discardedCBOR so the mechanism is already proven. Purely symmetry — same category as the alloc-canary point you already declined, and I'd apply the same reasoning.

Your earlier replies on the drift guard ("developer's responsibility") and the receipt alloc canary are reasonable calls given the header canary covers the shared mechanism and the key-coverage guards catch receipt-side field drift; I'm not re-litigating either.

Verification caveat

go build ./... succeeds for every package touched here (the only failure is cmd/juno failing to link against pre-built Rust artifacts — -ljuno_starknet_rs, -ljuno_starknet_compiler_rs, -lbz2 — unrelated to this PR and absent in this environment). go vet ./core/... ./blockchain/... ./rpc/v9/... ./rpc/v10/... ./mocks/... ./sync/... ./node/... passes clean, which typechecks the test files too. I was not permitted to run go test in this environment, so the assertions above about test content are from reading, not from a green run — CI's result stands as the authority there. The benchmark numbers in the description are likewise taken as given, not reproduced.
• branch perf/core/reduce-allocs-on-execution-status-read

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.

1 participant