Skip to content

[Storage] PR 3627 feedback#3635

Merged
roberto-bayardo merged 6 commits into
locked-bitmapfrom
danlaine/3627-nits
Apr 21, 2026
Merged

[Storage] PR 3627 feedback#3635
roberto-bayardo merged 6 commits into
locked-bitmapfrom
danlaine/3627-nits

Conversation

@danlaine
Copy link
Copy Markdown
Collaborator

@danlaine danlaine commented Apr 20, 2026

Follow-up on review feedback for PR #3627, which replaces the Current QMDB's layered bitmap with Arc<SharedBitmap<RwLock<BitMap>>>. This PR addresses two small inefficiencies and a coverage gap surfaced in review, without changing the PR's design.

Changes

1. Cache terminal Arc<SharedBitmap> on BitmapBatchLayer (batch.rs)

Add shared: Arc<SharedBitmap<N>> to BitmapBatchLayer. Collapses:

  • BitmapBatch::shared() from an O(chain-depth) walk to a 2-arm match.
  • BitmapBatch::pruned_chunks() from an O(chain-depth) walk to self.shared().pruned_chunks().

Cost: 8 bytes per layer + one Arc::clone at construction. The invariant (cached shared equals the chain terminal) is trivial by construction at both sites (compute_current_layer, trim_committed) — each sets the field via Arc::clone(parent.shared()), and BitmapBatch::Base is already the terminal.

Both callers (trim_committed on every new_batch; pruned_chunks once per build_chunk_overlay) are on the per-batch hot path. The walk was redundant after PR #3627's restructuring and is now gone.

2. Add test_current_live_batch_child_after_prune (mod.rs)

Covers the specific scenario: a live parent batch whose new_batch() is called after the DB has advanced its pruning boundary. The internal trim_committed must observe the advanced boundary on the shared bitmap, and the resulting child must merkleize and apply correctly.

Not covered by the existing test_current_live_batch_safe_across_prune, which tests applying a pre-prune child rather than extending a live parent post-prune.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
commonware-mcp f6947ad Apr 21 2026, 05:29 PM

@roberto-bayardo
Copy link
Copy Markdown
Collaborator

re: benchmark, I was going to remove it from this PR in favor of the standalone #3633

...just in case we come up with a non-locking based solution to this issue.

So could you move the benchmark suggestions there?

@danlaine danlaine force-pushed the danlaine/3627-nits branch from 9737a09 to 9b5d072 Compare April 20, 2026 19:48
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 20, 2026

Deploying monorepo with  Cloudflare Pages  Cloudflare Pages

Latest commit: f6947ad
Status: ✅  Deploy successful!
Preview URL: https://46e92e30.monorepo-eu0.pages.dev
Branch Preview URL: https://danlaine-3627-nits.monorepo-eu0.pages.dev

View logs

@danlaine
Copy link
Copy Markdown
Collaborator Author

re: benchmark, I was going to remove it from this PR in favor of the standalone #3633

...just in case we come up with a non-locking based solution to this issue.

So could you move the benchmark suggestions there?

Removed benchmark changes from here

@danlaine danlaine moved this to Ready for Review in Tracker Apr 20, 2026
@roberto-bayardo
Copy link
Copy Markdown
Collaborator

apparently I neglected to merge this when I approved it and now there are conflicts :-(

@roberto-bayardo roberto-bayardo merged commit 26982f6 into locked-bitmap Apr 21, 2026
163 of 168 checks passed
@roberto-bayardo roberto-bayardo deleted the danlaine/3627-nits branch April 21, 2026 17:45
@github-project-automation github-project-automation Bot moved this from Ready for Review to Done in Tracker Apr 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 98.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.87%. Comparing base (0fc4391) to head (f6947ad).
⚠️ Report is 14 commits behind head on locked-bitmap.

Files with missing lines Patch % Lines
storage/src/qmdb/current/batch.rs 96.87% 2 Missing ⚠️
@@              Coverage Diff               @@
##           locked-bitmap    #3635   +/-   ##
==============================================
  Coverage          95.87%   95.87%           
==============================================
  Files                440      440           
  Lines             172039   172131   +92     
  Branches            3998     3999    +1     
==============================================
+ Hits              164941   165030   +89     
- Misses              5831     5835    +4     
+ Partials            1267     1266    -1     
Files with missing lines Coverage Δ
storage/src/qmdb/current/mod.rs 99.08% <100.00%> (+0.01%) ⬆️
storage/src/qmdb/current/batch.rs 91.31% <96.87%> (+0.42%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fc4391...f6947ad. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants