Repository navigation
perf(storer): filter excluded batches and rogue chunks early in reserve sample - #5634
gacevicljubisa wants to merge 7 commits into
Conversation
| isExcludedBatch, err := db.batchExclusionFilter(minBatchBalance) | ||
| if err != nil { | ||
| db.logger.Error(err, "get batches below value") | ||
| db.logger.Error(err, "get batch exclusion filter") |
There was a problem hiding this comment.
Not introduced here (master also logs and continues, with a partial map), but since this is being touched: if this fails, the sample includes chunks from below-balance batches and the reveal will disagree with the neighborhood, which gets the node frozen. Returning the error would make the node skip the round instead:
if err != nil {
return Sample{}, fmt.Errorf("batch exclusion filter: %w", err)
}
There was a problem hiding this comment.
It makes sense. Great for noticing this.
2b1a4ee to
2eadeb8
Compare
| isExcludedBatch, err := db.batchExclusionFilter(minBatchBalance) | ||
| if err != nil { | ||
| db.logger.Error(err, "get batches below value") | ||
| return Sample{}, fmt.Errorf("batch exclusion filter: %w", err) |
There was a problem hiding this comment.
This return happens before any g.Go, so the defer's g.Wait() is nil and recordReserveSampleMetrics labels the run success, while the caller still skips the round, so dashboard would look more optimistic, then it actually is.
|
|
||
| select { | ||
| case chunkC <- ch: | ||
| stats.TotalIterated++ |
There was a problem hiding this comment.
Broken Invariant (TotalIterated < BelowBalanceIgnored):
Semantically and historically on master, TotalIterated counts all candidate reserve chunks iterated within the committed depth. BelowBalanceIgnored and RogueChunk represent subsets of those iterated chunks that were dropped.
Because the author returns early before stats.TotalIterated++, filtered chunks are never counted in TotalIterated.
If a neighborhood has 100 chunks and 80 are from expired batches:
- On master: TotalIterated = 100, BelowBalanceIgnored = 80.
- On this branch: TotalIterated = 20, BelowBalanceIgnored = 80.
If all chunks in a neighborhood belong to expired batches: TotalIterated = 0, BelowBalanceIgnored = 100.
| defer func() { | ||
| duration := time.Since(t) | ||
| err := g.Wait() | ||
| _ = g.Wait() |
There was a problem hiding this comment.
This error is recorded in the function below. Instead of removing it, maybe we can move it below the error return below and keep it as is.
There was a problem hiding this comment.
the _ = g.Wait() error isn't lost. It's already handled at the end of the function.
if err := g.Wait(); err != nil {
db.logger.Info("reserve sampler finished with error", "err", err, "duration", time.Since(t), "storage_radius", committedDepth, "consensus_time_ns", consensusTime, "stats", fmt.Sprintf("%+v", allStats))
return Sample{}, fmt.Errorf("sampler: failed creating sample: %w", err)
}But you are right, having it here in defer is confusing, and it is redundant, so _ = g.Wait() is removed from defer completly.
5 participants
Checklist
Description
Optimize batch filtering in ReserveSample by replacing
map[string]struct{}with a stack-allocated[32]bytelookup, where string conversions and heap allocations are eliminated. Filtering is moved to Phase 1.Benchmark Results
BenchmarkReserveSample10kWith1000ExcludedBatches(10k chunks in reserve, 1000 excluded batches):master3,294,111 ns/op(~3.29 ms)1,412,442 ns/op(~1.41 ms)67,081 allocs/op66,079 allocs/op4,299,470 B/op4,318,118 B/opOpen API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure