perf(storer): reuse buffers and eliminate per-chunk allocations in reserve sample by gacevicljubisa · Pull Request #5612 · ethersphere/bee

gacevicljubisa · 2026-09-14T12:06:49Z

Checklist

I have read the coding guide.
My change requires a documentation update, and I have done it.
I have added tests to cover my changes.
I have filled out the description and linked the related issues.

Description

This PR addresses per-chunk allocations and buffer overhead in ReserveSample (issue #5174):

GetterInto interface: Adds storage.GetterInto to allow reading chunk data directly into a caller-provided reusable buffer, avoiding per-call buffer allocations in storage readers.
Hoist ChunkStore handle: Instantiates ChunkStore() once per worker instead of per chunk, eliminating 3 allocations per chunk.
Per-worker reusable buffer: Each sampler worker reuses a swarm.SocMaxChunkSize buffer for chunk reading and feeds raw byte slices directly into transformedAddress, eliminating swarm.NewChunk heap
escapes.
Lazy chunk loading for candidates: Full chunk payloads are no longer pushed through sampleItemChan for all millions of chunks. Full chunk data is loaded in Phase 3 only for the ~200 items that
actually qualify for the sample candidate set.
Raw batch ID transfer: Avoids instantiating postage.NewStamp for every iterated chunk in workers by passing raw batchID through an internal candidate struct.
Safety guards & benchmarks: Adds assertSampleDataIntact and assertSampleDataNotShared to prevent buffer aliasing/corruption regressions, adds BenchmarkChunkStoreGet micro-benchmark, and adds
BenchmarkReserveSample10k.

A/B on bee-light-testnet, two nodes with identical config (no SIMD, verbosity 3, 1500m CPU, storage radius 2, isWarmingUp: false). Sample triggered via GET /rchash/2/<own-overlay>/<anchor2>; allocations attributed with pprof -diff_base focused on the ReserveSample subtree.

Per chunk (ReserveSample subtree)	master `b6537ea2`	this branch	Δ
Allocations	132.7	104.0	−21.7%
↳ chunk retrieval	60.5	36.4	−39.9%
Bytes	11,007 B	4,489 B	−59.2%
↳ chunk retrieval	8,774 B	2,615 B	−70.2%
↳ `chunkstore.readChunk` buffer	4,876 B	0 B	−100%
`transformedAddress` (control)	68.3	67.6	−1.1%
`bmt.Write` (control)	63.0	62.6	−0.7%

Total allocation churn per sample round: 24.65 GB → 10.30 GB over 2.405M chunks.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

This PR contains code that has been generated by an LLM.
I have reviewed the AI generated code thoroughly.
I possess the technical expertise to responsibly review the code generated in this PR.

Sampling is about to start reading chunks into a buffer that each worker reuses. Nothing today checks that the bytes handed back in a SampleItem are still the bytes of that chunk, so a reused buffer would silently hand the redistribution proof the contents of some later chunk. assertValidSample now checks two things for every item: that ChunkData still reproduces ChunkAddress, and that no two items share a backing array. Every existing sample test picks both up. Also add the rulers for the work that follows. BenchmarkReserveSample1k keeps its name and behaviour so the recorded baseline stays comparable; its body moves to a helper that BenchmarkReserveSample10k reuses over a ten times larger reserve. BenchmarkChunkStoreGet measures a single chunk read, split into a variant that builds the ChunkStore handle per call as the sampler does today and one that hoists it, so the cost of the handle alone is visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFUseFQ8rhp6N9X6YS7pbq

db.ChunkStore() builds three objects every time it is called, and the sampler called it once per chunk. On a testnet node that is 2.3 million handles per round, thrown away immediately. Hoist it to one per worker. The handle is deliberately not shared across workers: the read-only chunk store makes no thread-safety promise, and three allocations per worker is already nothing next to three per chunk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFUseFQ8rhp6N9X6YS7pbq

…lazily

…locations

Both ReserveSample benchmarks fill their reserve with chunk.GenerateValidRandomChunkAt, which produces content-addressed chunks only. The SOC branch of transformedAddress is therefore never measured by them, and any work that branch does beyond hashing the wrapped CAC is invisible. Benchmark the function directly, one case per chunk type, through a shim in export_test.go. The shim takes a swarm.Chunk and is held to that signature on purpose so the same benchmark source can be run against branches whose internal transformedAddress is shaped differently; only the shim changes between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFUseFQ8rhp6N9X6YS7pbq

aloknerurkar · 2026-09-15T18:53:34Z


 type ChunkStore interface {
 	Getter
+	GetterInto


Not sure if we need to add it to the ChunkStore interface. Since we only use ReadOnlyChunkStore for sampling, we can only add it to the interface below. I think we can even avoid adding it to transaction.ChunkStore also.

Dropping it makes ReadOnlyChunkStore no longer a subset of ChunkStore, which breaks the test storages and the mock storer that return the same value as both. Then even more changes are needed...

aloknerurkar · 2026-09-15T18:54:04Z

 	return ch, err
 }

+func (c *chunkStoreTrx) GetInto(ctx context.Context, addr swarm.Address, buf []byte) (n int, err error) {


Check comment above. Since we only need ReadOnlyChunkStore to provide this function, we can avoid adding it here.

martinconic · 2026-09-18T12:33:24Z

+				continue
+			}
+
+			ch, err := phase3ChunkStore.Get(ctx, item.chunkAddress)


instead ctx shouldn't we also use in this Phase 3 gCtx ?

gCtx is part of the worker group, and errgroup cancels it as soon as Wait returns, so phase3 needs to live longer than this.

martinconic · 2026-09-18T13:30:18Z

+				continue
+			}
+
+			ch, err := phase3ChunkStore.Get(ctx, item.chunkAddress)


phase3ChunkStore.Get runs before the consensusTime check: stamp.Timestamp() is already in memory from Step 1. Loading the 4096-byte chunk from Sharky before verifying stamp.Timestamp() <= consensusTime causes completely wasted disk reads for every chunk stamped past consensusTime.

Also, phase3ChunkStore.Get runs before db.validStamp: In Swarm, ValidStamp does not inspect chunk data—it only validates chunk.Address() and chunk.Stamp(). Performing a disk read before verifying whether the stamp is even valid burns I/O for expired or invalid stamps.

Here is another comment that would cover this as well, but as said, benefit is small, becuase on 4M chunks we are execting beetween 200-250 in the last phase.

martinconic · 2026-09-18T14:11:05Z

@@ -21,6 +21,14 @@ type Getter interface {
 	Get(context.Context, swarm.Address) (swarm.Chunk, error)
 }



implementations return a bare fmt.Errorf("chunk store: buffer too small: %d < %d", ...). A caller can't distinguish it from ErrNotFound.
Maybe add:

// ErrBufferTooSmall is returned by GetInto when len(buf) is smaller than the // stored chunk. Callers may inspect it with errors.Is to retry with a larger buffer. var ErrBufferTooSmall = errors.New("storage: buffer too small")

No stored chunk can exceed swarm.SocMaxChunkSize. Sharky rejects larger writes with ErrTooLong. So a caller sized to that constant can never hit this. Nothing to match until some caller really needs it.

aloknerurkar · 2026-09-22T15:15:59Z

+
+			ch, err := phase3ChunkStore.Get(ctx, item.chunkAddress)
+			if err != nil {
+				stats.ChunkLoadFailed++


We should have a separate metric for ChunkLoadFailed for phase2 and phase3. We could count chunks which were previously available but are no longer available due to eviction here.

aloknerurkar · 2026-09-22T15:18:58Z

+		return swarm.ZeroAddress, errors.New("chunk data too short for soc")
 	}
-	taddrCac, err := transformedAddressCAC(hasher, cacChunk)
+	cursor := swarm.HashSize + swarm.SocSignatureSize


It is unreachable but a cheap len(data) > swarm.SocMaxChunkSize guard alongside the existing SocMinChunkSize check would restore the rejection without giving back any of the allocation win. Especially when this fuzzing PR caught these type of cases. If someone changes some code later without changing this, better safe than sorry.

aloknerurkar · 2026-09-22T15:23:13Z


+// GetterInto is like Getter but reads chunk data into a caller-provided buffer,
+// avoiding per-call allocations. len(buf) must be at least the chunk size;
+// GetInto never writes past len(buf). Returns the number of bytes read into


nit: "Must" invites someone to size a buffer from a constant and skip checking the error, which is exactly the case that silently returns n=0 with a non-nil error today.

Suggest rewording to describe what actually happens — something like: "If len(buf) is smaller than the stored chunk, GetInto returns an error and does not write to buf. Returns the number of bytes read; callers use buf[:n]."

This is adjacent to the ErrBufferTooSmall thread but distinct: that one is about giving the error a type callers can match on, this one is about the prose describing the opposite contract from the code either way

acud

good stuff. thanks!

gacevicljubisa and others added 2 commits September 14, 2026 11:57

github-actions Bot assigned gacevicljubisa Sep 14, 2026

gacevicljubisa added the testing label Sep 14, 2026

gacevicljubisa added 2 commits September 14, 2026 16:02

perf(storer): read into reusable worker buffer and load sample items …

1682441

…lazily

perf(storer): pass batch id through sampler channel to avoid stamp al…

841bbaf

…locations

gacevicljubisa force-pushed the perf/sample-01-hoist-chunkstore branch from 444d89d to 841bbaf Compare September 14, 2026 14:11

gacevicljubisa requested review from acud and janos September 15, 2026 14:16

gacevicljubisa removed the testing label Sep 15, 2026

acud reviewed Sep 15, 2026

View reviewed changes

Comment thread pkg/file/joiner/joiner_test.go

aloknerurkar reviewed Sep 15, 2026

View reviewed changes

fix(storage): check buffer capacity in GetInto

207b420

gacevicljubisa requested a review from acud September 15, 2026 21:00

acud reviewed Sep 15, 2026

View reviewed changes

Comment thread pkg/storer/sample.go Outdated

gacevicljubisa requested review from akrem-chabchoub, martinconic and sbackend123 September 16, 2026 14:21

fix(storage): check buffer length in GetInto, not capacity

b9bf777

martinconic reviewed Sep 18, 2026

View reviewed changes

fix: address comments

ad8b3e6

aloknerurkar reviewed Sep 22, 2026

View reviewed changes

fix(storer): track assembly chunk load failures and guard chunk sizes

84d70a5

gacevicljubisa requested a review from aloknerurkar September 23, 2026 09:10

aloknerurkar approved these changes Sep 23, 2026

View reviewed changes

acud approved these changes Sep 23, 2026

View reviewed changes

gacevicljubisa merged commit f80f16c into master Sep 24, 2026
15 checks passed

gacevicljubisa deleted the perf/sample-01-hoist-chunkstore branch September 24, 2026 12:52

		@@ -21,6 +21,14 @@ type Getter interface {
		Get(context.Context, swarm.Address) (swarm.Chunk, error)
		}

Conversation

gacevicljubisa commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gacevicljubisa commented Sep 14, 2026 •

edited

Loading