Skip to content

Replace bits-and-blooms/bitset with roaring bitmaps#2386

Open
ValentaTomas wants to merge 1 commit intomainfrom
replace-bitset-with-roaring
Open

Replace bits-and-blooms/bitset with roaring bitmaps#2386
ValentaTomas wants to merge 1 commit intomainfrom
replace-bitset-with-roaring

Conversation

@ValentaTomas
Copy link
Copy Markdown
Member

Replace all direct usage of bits-and-blooms/bitset with roaring/v2 (v2.17.0). Uses the new Ranges() iterator (#522) for range iteration in CreateMapping and BitmapRanges.

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 14, 2026

PR Summary

Medium Risk
Touches snapshot diff metadata and dirty/empty block tracking used for exporting rootfs/memory diffs; a subtle bitmap semantics or concurrency mismatch could lead to incorrect diff contents or missing blocks. Risk is mitigated by updated tests but the change affects core data paths.

Overview
Replaces bits-and-blooms/bitset usage in snapshot dirty/empty tracking with roaring/v2 bitmaps, introducing shared/pkg/syncroaring for concurrency-safe mutation and read-only views used across orchestrator cache export, memory diff handling, and header diff mapping generation. Range iteration for dirty blocks/mappings is rewritten to use roaring’s Ranges() iterator, diff metadata/builders now emit syncroaring.ReadOnly bitmaps, and dependencies are updated accordingly (including removing the old atomicbitset wrapper).

Reviewed by Cursor Bugbot for commit 4b07195. Bugbot is set up for automated code reviews on this repo. Configure here.

@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch 6 times, most recently from fc7f1b4 to 0205899 Compare April 14, 2026 00:38
@ValentaTomas ValentaTomas marked this pull request as ready for review April 14, 2026 00:40
@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch from 6ec4c90 to b81ec2a Compare April 14, 2026 00:49
@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch 2 times, most recently from b6fa8a5 to b0717ba Compare April 14, 2026 00:56
@ValentaTomas ValentaTomas marked this pull request as draft April 14, 2026 06:10
@ValentaTomas ValentaTomas marked this pull request as ready for review April 14, 2026 07:13
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa35957fa3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

a few nits, otherwise LGTM

@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch 8 times, most recently from 7424c06 to 60f17c9 Compare April 14, 2026 19:32
@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch from 60f17c9 to a55db6c Compare April 14, 2026 19:36
Copy link
Copy Markdown
Member

@jakubno jakubno left a comment

Choose a reason for hiding this comment

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

Nice! 🚀 1 suggestion left in comments

@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch 2 times, most recently from 2057d3d to d975833 Compare April 14, 2026 19:48
Use roaring v2.17.0 with the new Ranges() iterator throughout.
Remove bitset as a direct dependency.
@ValentaTomas ValentaTomas force-pushed the replace-bitset-with-roaring branch from 9e25080 to 4b07195 Compare April 14, 2026 19:58
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4b07195. Configure here.

// The caller must ensure no concurrent SetRange calls while using the result.
func (b *Bitset) ReadOnly() *ReadOnly {
return &ReadOnly{bm: b.bm}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ReadOnly view shares live bitmap without copying

Medium Severity

Bitset.ReadOnly() returns a view sharing the same underlying *roaring.Bitmap without copying. The old atomicbitset.BitSet() called b.bm.ToBitSet(), which produced a snapshot copy. In ExportToDiff, the returned DiffMetadata.Dirty now aliases c.dirty.bm; after the lock is released, any concurrent WriteAtWithoutLocksetIsCachedc.dirty.SetRange mutates the bitmap that downstream consumers (e.g. ToDiffHeader, ExportMemory) are reading, creating a data race.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4b07195. Configure here.

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.

3 participants