perf(rpc): add progressive block trace caching - #4021
danielntmd wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
==========================================
+ Coverage 79.19% 79.27% +0.07%
==========================================
Files 464 468 +4
Lines 35741 35961 +220
==========================================
+ Hits 28305 28507 +202
- Misses 7427 7445 +18
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
58d79fc to
6ab7d60
Compare
GitHub Actions
Inactive
|
Claude finished @danielntmd's task in 6m 39s —— View job Review complete
I focused on concurrency correctness in the new Findings: no blockers or bugs. The single-flight-per-block design, panic-safe waiter release ( Two non-blocking nits posted inline:
One thing worth double-checking with the author rather than a code issue: the VM is now always invoked with Note: I wasn't able to run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Claude finished @danielntmd's task in 4m 16s —— View job Review complete
This PR was synchronized with a new commit ( New commit review: correct, no issues.
Severity: none (nit/important/blocker) — no new issues found. The two nits from the earlier review round (feeder-gateway path lacking single-flight coordination, and Note: |
GitHub Actions
Inactive
Collect initial reads on demand, replaying the block when cached traces lack requested reads. Shape responses at the RPC boundary and share finalised trace routing. Publish immutable cache records with per-block flight coordination. Trim test fixtures and strengthen checkpoint and prefix preservation coverage.
GitHub Actions
Inactive
Review in progress
|
| func emptyInitialReads() *InitialReads { | ||
| return &InitialReads{ | ||
| Storage: []StorageEntry{}, | ||
| Nonces: []NonceEntry{}, | ||
| ClassHashes: []ClassHashEntry{}, | ||
| DeclaredContracts: []DeclaredContractEntry{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not return by value? (reduce the extra alloc)
There was a problem hiding this comment.
Also, being used only twice, you could consider inlining it. Up to you!
There was a problem hiding this comment.
Why not return by value? (reduce the extra alloc)
rpc/v10/simulation.go:121
type TraceBlockTransactionsResponse struct {
Traces []TracedBlockTransaction `json:"traces"`
InitialReads *InitialReads `json:"initial_reads"`
}
InitialReads is a pointer, we could change the field to a value, but then we would need to track absence separately since we currently use nil.
There was a problem hiding this comment.
You can return by value and still treated as a pointer via the use of new(...).
Wherever you call emptyInitialReads() will become new(emptyInitialReads())
There was a problem hiding this comment.
Here are the suggested code shapes and their respective escaped diagnostics and benchmarks.
// Current:
func emptyInitialReads() *InitialReads {
return &InitialReads{ /* four empty slices */ }
}
response.InitialReads = emptyInitialReads()
// Proposed value-returning helper:
func emptyInitialReads() InitialReads {
return InitialReads{ /* same four empty slices */ }
}
// Explicit address:
reads := emptyInitialReads()
response.InitialReads = &reads
// Or the proposed new(...) construction:
response.InitialReads = new(emptyInitialReads())
| Shape | B/op | allocs/op |
|---|---|---|
| Current pointer return | 96 | 1 |
Value return, then &reads |
96 | 1 |
Value return, then new(...) |
96 | 1 |
The problem is that both constructions escape through the response returned by traceFinalisedBlock, so returning by value and wrapping it in new retains allocation. We can avoid this allocation for the empty block path when no reads are requested by leaving InitialReads as nil, as such:
response := TraceBlockTransactionsResponse{
Traces: []TracedBlockTransaction{},
}
if returnInitialReads {
response.InitialReads = emptyInitialReads()
}
return response, defaultExecutionHeader(), nil
This removes the allocation for unflagged empty-block requests while preserving the external response format. The requested-reads path still allocates, as it did before this PR.
| // storeComplete takes ownership of the response containers. InitialReads must be non-nil. | ||
| func (c *blockTraceCache) storeComplete( | ||
| blockHash felt.Felt, | ||
| response TraceBlockTransactionsResponse, | ||
| ) { | ||
| record := &blockTraceRecord{ | ||
| traces: response.Traces, | ||
| initialReads: response.InitialReads, | ||
| complete: true, | ||
| } | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| c.records.Add(blockHash, record) | ||
| } | ||
|
|
||
| func (c *blockTraceCache) record(blockHash felt.Felt) (*blockTraceRecord, bool) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| return c.records.Get(blockHash) | ||
| } | ||
|
|
||
| func (c *blockTraceCache) finishLocked(blockHash felt.Felt, flight chan struct{}) { | ||
| delete(c.flights, blockHash) | ||
| close(flight) | ||
| } |
There was a problem hiding this comment.
small perf: blockHash should be passed by reference, @infrmtcs please explain why
There was a problem hiding this comment.
Applied. @infrmtcs no need. Passing by value can introduce copies between function calls.
Measured over 10 × 1-second samples per revision using direct Go handler calls with fixed fixtures.
| Cache-hit handler | Before | After | Time saved per request |
|---|---|---|---|
| TraceTransaction | 311.90 ns/op | 310.30 ns/op | 1.60 ns (0.51%) |
| TraceBlockTransactions | 174.40 ns/op | 171.90 ns/op | 2.50 ns (1.43%) |
There was a problem hiding this comment.
Yes, really small. Just to have the style set to pass by reference value types bigger than one word and they are meant to be read only.
You can pass by value if mutating
Review in progress
|
GitHub Actions
Inactive
Review in progress
|
GitHub Actions
Inactive
|
Measured over 10 × 1-second samples per revision using direct Go handler calls with fixed fixtures.
|
|
Claude finished @danielntmd's task in 18m 15s —— View job Review complete
This round covers four commits since the last completed review: No blockers or new bugs found. The
Verified The two nits raised in earlier rounds (feeder-gateway path lacking single-flight coordination, |
GitHub Actions
Inactive
|
Closing for stacked PR #4065 |
2 participants
User description
Summary
TraceTransaction currently executes every transaction in a finalized block on a cache miss, even when the requested transaction appears near the beginning.
This PR caches successful tracing progress from the beginning of each block (a prefix). Later requests reconstruct the state at the cached point and execute only the remaining transactions through the requested target (the missing suffix). Only one request extends a block at a time, while different blocks can be traced concurrently.
Successful execution publishes a new cache record while existing cached traces remain available to readers.
Workflow
Cache Invariants
Tracing Behavior
Benchmarks
Cold
starknet_traceTransactionat 100 VUsBroad corpus sample size: 1,000
Everything else: 200
Interleaved
traceTransaction->traceBlockTransactionsSample size: 200
PR Type
Enhancement, Tests
Description
Implement progressive block trace caching.
Add per-block trace flight coordination.
Optimize memory usage in trace adaptations.
Refactor RPC handlers for targeted tracing.
File Walkthrough
5 files
Optimize VM initial reads adaptation by reducing copiesAdd helper function for empty initial readsRefactor tracing to support progressive execution targetsImplement custom trace cache with flight coordinationImplement state reconstruction and progressive tracing logic1 files
Integrate custom progressive block trace cache3 files
Add unit tests for progressive trace cacheAdd unit tests for progressive block tracingUpdate trace tests for progressive caching behavior