perf: use autoresearch to reduce allocations#3225
Conversation
Result: {"status":"keep","allocs_per_op":81,"bytes_per_op":25934,"ns_per_op":34001}
…allocations per block
Result: {"status":"keep","allocs_per_op":79,"bytes_per_op":25697,"ns_per_op":34147}
… make([][]byte, n) allocation
Result: {"status":"keep","allocs_per_op":78,"bytes_per_op":22996,"ns_per_op":33091}
…pper and txsToByteSlices allocations
Result: {"status":"keep","allocs_per_op":77,"bytes_per_op":20276,"ns_per_op":32480}
…allocation
Result: {"status":"keep","allocs_per_op":74,"bytes_per_op":12192,"ns_per_op":31624}
…10 allocs per block Replace per-call allocation of pb.Header/pb.Version/pb.Data/pb.Metadata with sync.Pool reuse in the hot MarshalBinary path. ToProto() API is unchanged — only MarshalBinary is affected since it consumes the result immediately. Metrics (100_txs benchmark): - 74 → 64 allocs/op (-13.5%) - ~12.1 → ~11.1 KB (-8.3%) - ~31ns flat
…Signer/pb.Version Eliminates 4 struct allocations per signed header marshal: pb.SignedHeader, pb.Header, pb.Version, pb.Signer. These are now borrowed from sync.Pool and returned after proto.Marshal completes. Metrics (100_txs benchmark): - 64 → 56 allocs/op - ~11KB → ~10.2KB
…er block State.ToProto allocated pb.State + pb.Version + timestamppb.Timestamp per block. MarshalBinary now pools those structs and returns the marshaled bytes directly. pkg/store/batch.UpdateState switched from ToProto+proto.Marshal to MarshalBinary.
📝 WalkthroughWalkthroughThis PR optimizes memory allocation and serialization performance by introducing object pooling for protobuf messages and SHA256 hashers, replacing protobuf-based state conversions with binary marshalling, and leveraging unsafe slice casting for efficient type reinterpretation across multiple components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 44s —— View job Code Review
Good overall work — the allocation reductions are meaningful and the approach is sound. There are a few correctness issues worth addressing before merge. 🔴 Bugs1.
|
|
tac0turtle
force-pushed
the
autoresearch/block-perf-2026-04-04
branch
from
9148ccc to
2438dc0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3225 +/- ##
==========================================
+ Coverage 61.79% 62.20% +0.40%
==========================================
Files 120 120
Lines 12670 12799 +129
==========================================
+ Hits 7830 7962 +132
+ Misses 3962 3961 -1
+ Partials 878 876 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
types/hash_memo_bench_test.go (1)
35-42: Consider invalidating cache between iterations for consistent baseline.In
BenchmarkHeaderHash_Single, the header is created once outside the loop. WhileHash()doesn't setcachedHash, if future refactoring changes this behavior, the benchmark would measure cached hits after the first iteration. Consider addingh.InvalidateHash()inside the loop for robustness:♻️ Suggested improvement for robustness
func BenchmarkHeaderHash_Single(b *testing.B) { h := GetRandomHeader("bench-chain", GetRandomBytes(32)) b.ReportAllocs() b.ResetTimer() for b.Loop() { + h.InvalidateHash() _ = h.Hash() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/hash_memo_bench_test.go` around lines 35 - 42, BenchmarkHeaderHash_Single currently constructs h once and calls h.Hash() repeatedly; to ensure each iteration measures an uncached baseline, call h.InvalidateHash() before each h.Hash() invocation (or recreate h inside the loop using GetRandomHeader/GetRandomBytes) so any cachedHash is cleared; update the loop in BenchmarkHeaderHash_Single to invoke h.InvalidateHash() (or reinitialize h) on every iteration before calling h.Hash().types/hashing.go (1)
128-135: Ignored marshal error could silently produce incorrect commitment.Line 131 ignores the error from
proto.Marshal. While this is consistent with the existing pattern inHash()(line 120-121), a marshal failure would produce a hash of nil/empty bytes, potentially causing consensus issues. Consider at least logging failures in debug builds or returning an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/hashing.go` around lines 128 - 135, DACommitment currently ignores the error from proto.Marshal which can yield an incorrect commitment; change DACommitment to surface marshal failures instead of silently hashing nil by returning (Hash, error) and propagate this change to callers (mirror the new signature across any places invoking DACommitment), or if changing signature is impractical, check the error and log it via the package logger in a debug build and return a zero-value Hash; locate DACommitment and the related Hash() usage to implement proper error handling and ensure any callers handle the returned error or the logged failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@types/hash_memo_bench_test.go`:
- Around line 35-42: BenchmarkHeaderHash_Single currently constructs h once and
calls h.Hash() repeatedly; to ensure each iteration measures an uncached
baseline, call h.InvalidateHash() before each h.Hash() invocation (or recreate h
inside the loop using GetRandomHeader/GetRandomBytes) so any cachedHash is
cleared; update the loop in BenchmarkHeaderHash_Single to invoke
h.InvalidateHash() (or reinitialize h) on every iteration before calling
h.Hash().
In `@types/hashing.go`:
- Around line 128-135: DACommitment currently ignores the error from
proto.Marshal which can yield an incorrect commitment; change DACommitment to
surface marshal failures instead of silently hashing nil by returning (Hash,
error) and propagate this change to callers (mirror the new signature across any
places invoking DACommitment), or if changing signature is impractical, check
the error and log it via the package logger in a debug build and return a
zero-value Hash; locate DACommitment and the related Hash() usage to implement
proper error handling and ensure any callers handle the returned error or the
logged failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12528a06-2158-4aad-8475-8990e96b4bcb
📒 Files selected for processing (7)
block/internal/executing/executor.gopkg/store/batch.gotypes/hash_memo_bench_test.gotypes/hashing.gotypes/serialization.gotypes/serialization_test.gotypes/tx.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sync/syncer_status_test.go (1)
28-37:⚠️ Potential issue | 🟠 MajorMove
require.NoErrorout of worker goroutines.
require.NoError(t, err)insidewg.Gois unsafe becauserequirecallst.FailNow(), which must only be called from the test goroutine—calling it from a spawned goroutine will only exit that goroutine, not the test. Collect errors in a channel and assert afterwg.Wait()on the main test goroutine.Suggested fix
started := make(chan struct{}) release := make(chan struct{}) var wg sync.WaitGroup + errs := make(chan error, 8) for range 8 { wg.Go(func() { _, err := status.startOnce(func() error { if calls.Add(1) == 1 { close(started) } <-release return nil }) - require.NoError(t, err) + errs <- err }) } <-started close(release) wg.Wait() + close(errs) + + for err := range errs { + require.NoError(t, err) + } require.Equal(t, int32(1), calls.Load()) require.True(t, status.isStarted())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/syncer_status_test.go` around lines 28 - 37, Move the test assertion out of the worker goroutine: replace the inline require.NoError(t, err) inside the wg.Go anonymous function (the block that calls status.startOnce with the calls/started/release coordination) by sending err to a buffered errors channel (or storing it in a slice protected by a mutex), and after wg.Wait() (on the main test goroutine) iterate the collected errors and call require.NoError for each; keep the existing status.startOnce, calls, started, release and wg.Go logic but ensure no t.FailNow()-style assertions run inside the spawned goroutine.
🧹 Nitpick comments (2)
types/serialization.go (2)
252-291: Consider defer-based cleanup for maintainability.The three exit paths each duplicate cleanup logic for four pooled objects. While the current implementation is correct and avoids defer overhead in this hot path, the verbosity increases maintenance risk if the function is modified later.
If future changes require additional exit paths, consider a helper pattern:
♻️ Optional refactor using defer
func (sh *SignedHeader) MarshalBinary() ([]byte, error) { psh := pbSignedHeaderPool.Get().(*pb.SignedHeader) ph := pbHeaderPool.Get().(*pb.Header) pv := pbVersionPool.Get().(*pb.Version) psi := pbSignerPool.Get().(*pb.Signer) defer func() { pv.Reset(); pbVersionPool.Put(pv) ph.Reset(); pbHeaderPool.Put(ph) psi.Reset(); pbSignerPool.Put(psi) psh.Reset(); pbSignedHeaderPool.Put(psh) }() // ... rest of logic with simple returns ... }Given this is a hot path where the defer overhead may matter, the current explicit approach is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/serialization.go` around lines 252 - 291, The cleanup for pooled protobuf objects (ph, pv, psi, psh from pbHeaderPool, pbVersionPool, pbSignerPool, pbSignedHeaderPool) is duplicated across multiple return paths in SignedHeader.MarshalBinary; replace the repeated reset/put blocks with a single deferred cleanup closure at the top of the function that calls ph.Reset(); pbHeaderPool.Put(ph), pv.Reset(); pbVersionPool.Put(pv), psi.Reset(); pbSignerPool.Put(psi), and psh.Reset(); pbSignedHeaderPool.Put(psh) so all return paths can simply return bz, err without repeating cleanup logic.
142-144: Inconsistency withDACommitmentnil handling.This code defensively checks
d.Txs != nilbefore theunsafe.Slicereinterpretation, which is good practice. However,DACommitment()intypes/hashing.go:128-137uses the same pattern without a nil check:pbData := pb.Data{Txs: unsafe.Slice((*[]byte)(unsafe.SliceData(d.Txs)), len(d.Txs))}While both are technically safe (when
d.Txsis nil,unsafe.Slice(nil, 0)returns nil), the inconsistency could cause confusion during maintenance. Consider aligning the nil-check approach across both usages for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/serialization.go` around lines 142 - 144, DACommitment in types/hashing.go currently builds pb.Data using unsafe.Slice without a nil check for d.Txs; make it consistent with the pattern in types/serialization.go by checking if d.Txs != nil before calling unsafe.Slice, and only set pbData.Txs when non-nil (use the same conditional assignment used around pd.Txs in the serialization code) so both places handle nil slices the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/sync/syncer_status_test.go`:
- Around line 28-37: Move the test assertion out of the worker goroutine:
replace the inline require.NoError(t, err) inside the wg.Go anonymous function
(the block that calls status.startOnce with the calls/started/release
coordination) by sending err to a buffered errors channel (or storing it in a
slice protected by a mutex), and after wg.Wait() (on the main test goroutine)
iterate the collected errors and call require.NoError for each; keep the
existing status.startOnce, calls, started, release and wg.Go logic but ensure no
t.FailNow()-style assertions run inside the spawned goroutine.
---
Nitpick comments:
In `@types/serialization.go`:
- Around line 252-291: The cleanup for pooled protobuf objects (ph, pv, psi, psh
from pbHeaderPool, pbVersionPool, pbSignerPool, pbSignedHeaderPool) is
duplicated across multiple return paths in SignedHeader.MarshalBinary; replace
the repeated reset/put blocks with a single deferred cleanup closure at the top
of the function that calls ph.Reset(); pbHeaderPool.Put(ph), pv.Reset();
pbVersionPool.Put(pv), psi.Reset(); pbSignerPool.Put(psi), and psh.Reset();
pbSignedHeaderPool.Put(psh) so all return paths can simply return bz, err
without repeating cleanup logic.
- Around line 142-144: DACommitment in types/hashing.go currently builds pb.Data
using unsafe.Slice without a nil check for d.Txs; make it consistent with the
pattern in types/serialization.go by checking if d.Txs != nil before calling
unsafe.Slice, and only set pbData.Txs when non-nil (use the same conditional
assignment used around pd.Txs in the serialization code) so both places handle
nil slices the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31a32c62-ce85-43f4-9ada-8db1607e112f
📒 Files selected for processing (3)
pkg/sync/syncer_status_test.gotypes/hashing.gotypes/serialization.go
🚧 Files skipped from review as they are similar to previous changes (1)
- types/hashing.go
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
i think Claude's comment make sense.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
types/serialization.go (1)
74-102: Add direct wire-compatibility tests for the pooled marshalers.These paths now hand-fill pooled protobuf structs and use
unsafeslice views instead of the olderToProto()helpers. The provided context only shows new coverage forState.MarshalBinaryround-tripping andData.ToProto()copy semantics, so I’d add byte-for-byte equivalence tests againstproto.Marshal(x.ToProto())forHeader,Data, andSignedHeader, including nil vs emptyTxs.Also applies to: 124-149, 223-275, 513-544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/serialization.go` around lines 74 - 102, Add byte-for-byte wire-compatibility tests that compare the pooled marshalers against the original ToProto+proto.Marshal path: for Header (Header.MarshalBinary), Data (Data.ToProto), and SignedHeader ensure proto.Marshal(x.ToProto()) equals x.MarshalBinary() output, and include cases for nil vs empty Txs and for legacy unknown fields (encodeLegacyUnknownFields). Implement tests that construct identical inputs, call both proto.Marshal(x.ToProto()) and the pooled marshal (e.g. Header.MarshalBinary / SignedHeader.MarshalBinary / Data.MarshalBinary), and assert equality of returned bytes and errors; reuse the existing test helpers used for State.MarshalBinary round-tripping to minimize duplication.types/hashing.go (1)
128-137: Lock downDACommitment()equivalence with edge-case tests.This path now bypasses
Data.MarshalBinary()and hashes a manually assembledpb.Data. Since the commitment is consensus-sensitive, I’d add regression cases that compare the new hash against the previous marshal-and-hash path for nilTxs, empty-but-non-nilTxs, and normal multi-tx batches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/hashing.go` around lines 128 - 137, The DACommitment() change bypasses Data.MarshalBinary() and must be regression-tested: add unit tests that compute the commitment using Data.DACommitment() and also via the previous marshal-and-hash path (call Data.MarshalBinary() then leafHashOpt with same sha256Pool/hasher) for three cases—nil Txs, empty-but-non-nil Txs (make a zero-length slice), and a normal multi-tx Data—asserting equality for each; reference DACommitment(), Data.MarshalBinary(), pb.Data and leafHashOpt to locate code paths and ensure the tests reproduce the exact hasher/sha256Pool usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@types/hashing.go`:
- Around line 128-137: The DACommitment() change bypasses Data.MarshalBinary()
and must be regression-tested: add unit tests that compute the commitment using
Data.DACommitment() and also via the previous marshal-and-hash path (call
Data.MarshalBinary() then leafHashOpt with same sha256Pool/hasher) for three
cases—nil Txs, empty-but-non-nil Txs (make a zero-length slice), and a normal
multi-tx Data—asserting equality for each; reference DACommitment(),
Data.MarshalBinary(), pb.Data and leafHashOpt to locate code paths and ensure
the tests reproduce the exact hasher/sha256Pool usage.
In `@types/serialization.go`:
- Around line 74-102: Add byte-for-byte wire-compatibility tests that compare
the pooled marshalers against the original ToProto+proto.Marshal path: for
Header (Header.MarshalBinary), Data (Data.ToProto), and SignedHeader ensure
proto.Marshal(x.ToProto()) equals x.MarshalBinary() output, and include cases
for nil vs empty Txs and for legacy unknown fields (encodeLegacyUnknownFields).
Implement tests that construct identical inputs, call both
proto.Marshal(x.ToProto()) and the pooled marshal (e.g. Header.MarshalBinary /
SignedHeader.MarshalBinary / Data.MarshalBinary), and assert equality of
returned bytes and errors; reuse the existing test helpers used for
State.MarshalBinary round-tripping to minimize duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 852b7f26-794c-42d9-a7b8-3ffa9b36de8c
📒 Files selected for processing (3)
block/internal/executing/executor.gotypes/hashing.gotypes/serialization.go
2 participants
Overview
This pr used autoresearch to run a loop on reducing allocations
3 a8b6352 78allocs 22,996 33,091 keep Unsafe reinterpret cast of Txs to [][]byte in ApplyBlock — eliminates make([][]byte, n) allocation
4 823aa62 77allocs 20,276 32,480 keep Direct pb.Data serialization in DACommitment — avoids pruned Data wrapper and txsToByteSlices allocations
5 0720b44 74allocs 12,192 31,624 keep unsafe.Slice in Data.ToProto() — eliminates txsToByteSlices [][]byte allocation
6 — 74allocs 12,187 31,217 discard Reverted hand-written HashSlim wire encoder — produced different hashes than MarshalBinary
7 ccbc2e4 64allocs 11,130 31,570 keep sync.Pool for protobuf message structs in MarshalBinary — eliminates 10 allocs per block
8 805672e 56allocs 10,217 31,037 keep Pooled SignedHeader.MarshalBinary — reuse pb.SignedHeader/pb.Header/pb.Signer/pb.Version structs
Summary by CodeRabbit
Tests
Chores