Skip to content

shards: avoid repeated branch repository scans - #1127

Merged
keegancsmith merged 2 commits into
sourcegraph:mainfrom
perfloop:perfloop-pr-open-2sb26ne0bq
Aug 14, 2026
Merged

shards: avoid repeated branch repository scans#1127
keegancsmith merged 2 commits into
sourcegraph:mainfrom
perfloop:perfloop-pr-open-2sb26ne0bq

Conversation

@perfloop-agent

@perfloop-agent perfloop-agent commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Rebase the branch-repository shard selector on current main and use the v2 Roaring API.

Replace the adaptive Roaring union with a fixed-size Bloom rejection filter. It builds only after enough direct misses and known remaining repository shards make setup worthwhile; exact bitmap checks remain authoritative. This avoids walking large bitmap containers just to estimate a union, while keeping direct filtering for short scans, saturated filters, unknown repository lists, and uncertain compound shards.

Add deterministic and randomized selection coverage. The deterministic case verifies the filter after the miss threshold, preserves exact bitmap contents and query identity, and checks matching shards after setup.

go test ./search, go test -race ./search, and go vet ./search pass. The local go test ./... run is blocked by the network-dependent grpc/chunk e2e test.

Workload: branch repository filtering across 10,000 four-repository shards with 128 branch bitmaps and 90% excluded shards

Metric Before After Change
ns/op 44867662 1838278 95.9% lower

Workload: branch repository filtering where 10,000 four-repository shards match only the final of 128 branch bitmaps

Metric Before After Change
ns/op 21821016 542041 97.5% lower

Workload: four final-branch matches followed by 9,996 excluded shards across 128 branch bitmaps

Metric Before After Change
ns/op 22485618 904355 96% lower

Workload: one four-repository shard matching only the final non-empty branch bitmap

Metric Before After Change
ns/op 2724 707.2 74% lower

Workload: 2,000 final-branch shards followed by 8,000 first-branch shards across 128 branch bitmaps

Metric Before After Change
ns/op 6094796 668735 89% lower

Workload: 1,400 miss shards followed by 8,600 fifth-branch shards across 128 branch bitmaps

Metric Before After Change
ns/op 5009322 508639 89.8% lower

Workload: 1,003 selected shards with sparse first-repository-only interior matches across 128 branch bitmaps

Metric Before After Change
ns/op 23635358 954396 96% lower
Checks: 3 passed.

Generated by Perfloop. Human sponsor: Tomas Senart. Measurements and checks.

@perfloop-agent
perfloop-agent force-pushed the perfloop-pr-open-2sb26ne0bq branch from 672d02f to c17c090 Compare August 8, 2026 03:50
@perfloop-agent perfloop-agent changed the title Adapt branch repository bitmap aggregation Fold branch repository bitmaps during shard selection Aug 8, 2026
@perfloop-agent

Copy link
Copy Markdown
Contributor Author

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, -benchmem, darwin/arm64 M3 Max; the declared claim gates were independently verified on linux/amd64):

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:

shape B/op allocs/op ns/op
BranchesRepos (10k shards) 32.1Ki → 55.1Ki (+23.5Ki) 7 → 18 −97.3%
LatePrefixThenFirst 80.1Ki → 88.3Ki (+8.1Ki) 7 → 13 −90.1%
SampledFirstRepoOnly 32.1Ki → 48.2Ki (+16.1Ki) 5 → 12 −94.1%

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 keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :botsnacks: Approving, but some inline feedback.

Comment thread search/branchesrepos_test.go Outdated
"math/rand/v2"
"testing"

"github.com/RoaringBitmap/roaring"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread search/shards.go Outdated
var containers uint64
for _, branch := range s.branches {
if !branch.Repos.IsEmpty() {
containers += branch.Repos.Stats().Containers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread search/shards.go
Comment on lines +374 to +385
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
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@perfloop-agent
perfloop-agent force-pushed the perfloop-pr-open-2sb26ne0bq branch from c17c090 to 346c922 Compare August 13, 2026 16:15
@keegancsmith keegancsmith changed the title Fold branch repository bitmaps during shard selection shards: avoid repeated branch repository scans Aug 14, 2026
@keegancsmith

Copy link
Copy Markdown
Member

I think there is opportunity to simplify this code a bit for very minor slowdown. I will follow-up with another PR and land this as is :)

@keegancsmith
keegancsmith merged commit 04ac08b into sourcegraph:main Aug 14, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants