Repository navigation
perf(bitmap): grow a large buffer by a fraction instead of doubling it - #60
Merged
Merged
Conversation
Callers outside the package can see only the part of a bitmap's buffer that LenInBytes counts, not the allocation behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both reallocation sites grew a buffer by its own capacity, so a bitmap of tens of megabytes carried a second copy of itself in slack it would never fill, for as long as the process ran. They now share grownCap, which keeps doubling while the buffer is small and adds a fraction of the grown size past a limit. Below the limit the arithmetic is unchanged, which is what the existing capacity assertions encode. It costs copy work: building a large bitmap copies several times the final size where doubling copied about one. That is a one-off at build time against slack that stays resident. BenchmarkGrownCap sweeps the limit and the fraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
antas-marcin
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both of sroar's reallocation sites grew a buffer by its own capacity, which is right for a small bitmap and wrong for a large one. This puts one shared
grownCapbehind both, keeps the old rule below a size limit and takes a fraction of the grown size above it. It also exportscapInBytesasCapInBytes.Motivation
A bitmap grown by repeated
Orof monotone doc IDs — weaviate's in-memory roaringset range representation, and any accumulator built that way — ends up carrying close to a second copy of itself in slack. That slack is resident for as long as the process runs, while the copying that avoids it is a one-off at build time.Approach
Below
growDoubleLimitthe arithmetic is byte-identical to the oldcap + max(cap, need), which is what lets the package's existing capacity assertions stand unchanged.The limit branches on the old capacity, not on the grown size, so a fresh bitmap absorbing one large
Orstill lands on an exact fit. That is deliberate:roaringset.Condenseis exactly that shape and its doc comment says the minimal size is the point. A bitmap that keeps accumulating instead pays one copy for it at the next merge.The slack has no floor. An earlier form floored it at the limit so the handoff from doubling stayed continuous, which made every bitmap just past the limit carry a limit-sized slack and turned the band above it into arithmetic growth. Dropping the floor costs monotonicity of
grownCapincp— a buffer just under the limit grows larger than one just over it — which is harmless for a single buffer, whose capacity only ever increases.Key areas for review
bitmap.gogrownCapWith— the two arms and the step down at the limitbitmap_opt.goexpandConditionally— check the new capacity still coversln+sizeKeys+sizeContainersbitmap_test.goTestGrownCap— the rows either side of the limit are what pin the discontinuityRisks
BenchmarkGrownCapsweeps both constants so the balance can be re-decided rather than re-derived.roaringset/binary_search_tree.godocuments that sroar grows by doubling, and its heap-ratio test bounds slack accordingly. Every shape that test builds stays below the limit, so the bounds hold; the prose wants fixing when weaviate bumps sroar.Testing
TestGrownCapcovers both arms, the boundary and the exact-fit case.TestGrowthKeepsSlackandTestGrowthKeepsSlackOnSetdrive the two growth sites end to end throughOrandSet; both fail against the old formula. The package's existing capacity assertions are unchanged.Unrelated, found while reviewing:
FillUpcomputesrequiredContainersCount * maxContainerSizeinint, which wraps on a 32-bit build. Recorded in the findings inbox, not fixed here.