Fold branch repository bitmaps during shard selection - #1127
Fold branch repository bitmaps during shard selection#1127perfloop-agent wants to merge 2 commits into
Conversation
Track direct branch bitmap membership checks during shard filtering. Once the observed work and remaining shard count repay the roaring container cost, fold the non-empty bitmaps into one lookup bitmap. Retain a lone bitmap directly to avoid cloning it. Keep the branch query unchanged for later per-repository filtering. Add deterministic differential coverage for direct, folded, and empty-union paths.
672d02f to
c17c090
Compare
|
Supplementing the claim record with the full allocation profile of this change, since the recorded claims gate allocations on only four of the eleven benchmark shapes. Measured on the exact PR head vs its benchmark-scaffold parent (interleaved A/B, 10 pairs, Allocations are byte-identical to upstream on 8 of 11 shapes, including every small-shape guardrail (OneShard, Overlapping ×2, LargeBitmapsFewShards/ModerateShards, LateMatch, MatchingPrefix, MissPrefixThenFifth). The three shapes where the fold engages pay its one-time union build, and buy back the cost in the same query:
The only statistically significant time regressions anywhere are the two fully-overlapping micro-shapes at +55–57 ns/query on ~700–800 ns operations (within the recorded 500 ns allowances). Queries with ≤4 branches take a code path identical to upstream. |
keegancsmith
left a comment
There was a problem hiding this comment.
Thanks! :botsnacks: Approving, but some inline feedback.
| "math/rand/v2" | ||
| "testing" | ||
|
|
||
| "github.com/RoaringBitmap/roaring" |
There was a problem hiding this comment.
Can you rebase onto main, we have since switched to using github.com/RoaringBitmap/roaring/v2. I believe the API is the same so just need to update this string once on main.
| var containers uint64 | ||
| for _, branch := range s.branches { | ||
| if !branch.Repos.IsEmpty() { | ||
| containers += branch.Repos.Stats().Containers |
There was a problem hiding this comment.
Given we are all agent pilled, let me post the perf concern my agent raised:
P2 — Stats() introduces a second complete bitmap traversal
Before filtering, the implementation already calls GetCardinality() on every branch bitmap at shards.go:528-531.
Once 16,384 probes are reached, maybeFold calls Stats() on every non-empty bitmap at shards.go:425-429. Stats() walks every Roaring container; it is not an O(1) container-count lookup.
Therefore, large, widely distributed bitmaps can receive two full container traversals even when the heuristic subsequently decides not to fold. The benchmarks named “LargeBitmaps” only put about 100 nearby IDs into each bitmap, so they do not cover many-container bitmaps.
A small fix would be to calculate Stats() once during initial sizing and use both stats.Cardinality and stats.Containers, passing the container count to the selector.
I’d raise this as a performance concern rather than a correctness blocker, but it matters because this is specifically a performance PR.
| } | ||
| } | ||
|
|
||
| func TestSelectRepoSetBranchesReposManyBranches(t *testing.T) { |
There was a problem hiding this comment.
P2 — No deterministic test exercises the non-empty multi-bitmap fold
TestSelectRepoSetBranchesReposManyBranches looks intended to exercise folding, but it performs only 16,134 probes:
- first match: 1
- fifth-branch match: 5
- final-branch match: 128
- 125 misses: 125 × 128
- total: 16,134
The threshold is 16,384, so that test never folds.
The empty-union test reaches the folding code but does not exercise the important clone-and-OR sequence. The randomized test may enter different paths, but it neither instruments nor asserts that a non-empty fold occurred.
There is also a weakness in the mutation assertion at branchesrepos_test.go:92-105: q.String() prints only bitmap cardinality when a bitmap has multiple entries. A mutation that changes IDs without changing cardinality would go undetected.
I’d ask for a deterministic test with:
- enough miss shards to cross 16,384 probes;
- at least two non-empty branch bitmaps;
- matching shards after the fold;
- exact bitmap-content comparison before and after;
- confirmation that the returned multi-branch query is still the original query.
| const ( | ||
| // Wait until direct membership checks have paid for inspecting the branch | ||
| // bitmaps before considering an aggregate. | ||
| branchesReposMinimumProbes uint64 = 16 << 10 | ||
|
|
||
| // A fold near the end of shard selection cannot repay its setup cost. | ||
| branchesReposMinimumRemainingShards = 16 | ||
|
|
||
| // branchesReposUnionContainerCost conservatively prices copying one roaring | ||
| // container as 128 direct bitmap membership checks. | ||
| branchesReposUnionContainerCost uint64 = 128 | ||
| ) |
There was a problem hiding this comment.
I'd be interested to know how these values were tuned/found? Couldn't really see it from the perfloop case. They do seem reasonable though.
They also seem mostly tuned for single repo shards which I think is likely the vast majority of shards in most places. There is the risk you have a few compound shards you search last => the estimates get way off. But yeah, I think this is good.
Summary
selectRepoSetfiltersBranchesReposqueries.Tradeoffs
Small scans and folds near the end of shard selection remain on the direct lookup path. A justified multi-bitmap fold can allocate once to avoid repeated branch scans over the remaining shards.
Testing
go test ./searchgo test -race ./searchgo vet ./searchgo test ./...was attempted, but unrelated gitindex and network-dependent grpc/chunk failures prevented completion; the changedsearchpackage passed independently.On workload
selectRepoSet with 128 branch bitmaps across 10,000 four-repository shards, 90% of which are excluded, medianns/opmoved from 40519219 to 1392258 (96.6% lower).On workload
selectRepoSet with 128 branch bitmaps across 10,000 four-repository shards whose IDs match only the final branch bitmap, medianns/opmoved from 22663531 to 507947 (97.8% lower).On workload
selectRepoSet with four final-branch matches followed by 9,996 miss shards across 128 branch bitmaps, medianns/opmoved from 22860989 to 849872 (96.3% lower).On workload
selectRepoSet with 1,003 selected shards where the three interior sample shards match only through their first repository, medianns/opmoved from 21446219 to 1295738 (94% lower).On workload
selectRepoSet with 1,400 miss shards followed by 8,600 fifth-branch shards across 128 branch bitmaps, medianns/opmoved from 4303116 to 529135 (87.7% lower).On workload
selectRepoSet with 2,000 final-branch shards followed by 8,000 first-branch shards across 128 branch bitmaps, medianns/opmoved from 4805661 to 480780 (90% lower).All 3 declared correctness checks passed.
Authored and verified by Perfloop: every claim above was co-measured on both trees and independently re-verified before submission — the full record is public: case_erzmq5mptg. Replies from this account are human-approved, and a human operator is accountable for this contribution.