perf(storer): filter reserve chunks by proximity before unmarshaling in sampler - #5616
gacevicljubisa wants to merge 4 commits into
Conversation
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
|
nice optimization, but: this is also going to silently break without absolutely anyone knowing if the index structure changes in a future migration, breaking the whole sampling process. is there any way to guarantee the contract is maintained? |
@gacevicljubisa Maybe we can add a method on chunkBinItem itself to do this instead of defining a separate struct. I agree this might be an issue if we change the chunkBinItem. Something like UnmarshalAddress which only unmarshals the part we are interested in. |
|
@acud @aloknerurkar those are valid concerens and I have added 2 new commits trying to address those concerns and proposals. Instead of having method on chunkBinItem, i have cretead constatnts for each offset to share the layout. Also, I added unit test TestChunkBinItemLayout that pins ChunkBinItem, and any change happens in the future, this test will fail. Let me know what do you think about this. |
|
consider a slightly simpler and potentially cleaner approach: you don't need to spread the offset into another helper function (nor the consts indices) and can just reuse the existing unmarshal functionality without having to |
Checklist
Description
This PR introduces an early proximity filter for the reserve chunk iterator during
ReserveSample:ChunkBinItemserializes the chunk address at a fixed offset (byte 9, right after the 1-byte bin and 8-byte bin ID),reserve.ProximityFilterextracts the 32-byte chunk address directly from the raw LevelDB value buffer.< committedDepth, the iterator skips it immediately. This completely avoids allocating a value copy, instantiating&reserve.ChunkBinItem{}, and unmarshaling all fields (Address,BatchID,StampHash, etc.) for non-matching chunks.committedDepth > storageRadius), the reserve contains chunks in binscommittedDepthfor the sample round. For these nodes, this eliminates large amounts of heap allocations and unmarshaling work during sampling.Open API Spec Version Changes (if applicable)
N/A
Motivation and Context (Optional)
Part of reserve sampling performance optimizations.
Related Issue (Optional)
#5174
Screenshots (if appropriate):
AI Disclosure