Skip to content

Conversation

danlaine
Copy link
Collaborator

@danlaine danlaine commented Sep 11, 2025

Follow up PR to #1516 based on this comment.

This PR removes the invariant that the bitmap has at least 1 chunk.

This requires corresponding changes in adb::current::Current, which uses the BitMap.

@danlaine danlaine self-assigned this Sep 11, 2025
@danlaine danlaine added this to Tracker Sep 11, 2025
@danlaine danlaine marked this pull request as ready for review September 11, 2025 20:17
@danlaine danlaine moved this to In Progress in Tracker Sep 11, 2025
@roberto-bayardo
Copy link
Collaborator

Can you clarify why this is desirable? It seems to be adding complexity (albeit not much). The "this comment" link in the PR description doesn't work from me (perhaps because I'm using the "new" github UI?)

/// The position within the last chunk where the next bit is to be appended.
///
/// Invariant: This value is always in the range [0, N * 8).
/// Invariant: If chunks.is_empty(), then next_bit == 0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this increase in invariant complexity makes me a little sad

@danlaine
Copy link
Collaborator Author

Can you clarify why this is desirable? It seems to be adding complexity (albeit not much). The "this comment" link in the PR description doesn't work from me (perhaps because I'm using the "new" github UI?)

It's a link to @BrendanChou's comment that reads:

I personally found it easier to not do this and make the number of chunks equal to len().div_ceil(N * 8)... wonder if that would simplify the code or not

Sounds like there's disagreement as to which invariant is simpler. I'm not especially opinionated. I suppose I slightly favor this PR's approach because it makes serialization more straightforward. Namely, you don't need to worry about serializing an empty chunk at the end of the bitmap.

Copy link

codecov bot commented Sep 22, 2025

Codecov Report

❌ Patch coverage is 98.38710% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (danlaine/separate-bitmap@b1b4c49). Learn more about missing BASE report.

Files with missing lines Patch % Lines
storage/src/mmr/bitmap.rs 97.91% 1 Missing ⚠️
utils/src/bitmap/mod.rs 98.48% 1 Missing ⚠️
@@                     Coverage Diff                     @@
##             danlaine/separate-bitmap    #1589   +/-   ##
===========================================================
  Coverage                            ?   92.29%           
===========================================================
  Files                               ?      295           
  Lines                               ?    76250           
  Branches                            ?        0           
===========================================================
  Hits                                ?    70375           
  Misses                              ?     5875           
  Partials                            ?        0           
Files with missing lines Coverage Δ
storage/src/adb/current.rs 96.12% <100.00%> (ø)
utils/src/bitmap/prunable.rs 99.13% <100.00%> (ø)
storage/src/mmr/bitmap.rs 95.36% <97.91%> (ø)
utils/src/bitmap/mod.rs 97.03% <98.48%> (ø)

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 b1b4c49...6a47066. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants