node48 children can have holes #565
Merged
Conversation
This file contains 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
This matches the [java implementation][1] in Roaring.
When removing from a node48, the child is simply set to NULL. For example, if
the state is (ignoring that it actually needs at least 17 items to not be a
node16):
```
keys: {[0]=0, [1]=1}
children: {[0]=child1, [1]=child2}
count: 2
```
After removing key 0, the state will be:
```
keys: {[0]=48, [1]=1}
children: {[0]=NULL, [1]=child2}
count: 1
```
If we then try to insert at key 3, the item at `children[count]` is already in
use: we have to find and reuse the 0th child.
This also has some fixes for `art_printf`:
* When printing keys, don't cast to char, if char is signed, when integer
promoted, a 0xFF char will be printed as `FFFFFFFF`
* Correctly output keys for node48
* Include child index for node48
* Correct child output for node48
[1]: https://github.com/RoaringBitmap/RoaringBitmap/blob/1ed0242d1f298cf93d3666a754eb4bb4c5180c4c/RoaringBitmap/src/main/java/org/roaringbitmap/art/Node48.java#L181-L187
SLieve
reviewed
Jan 20, 2024
Rather than searching for an unset child, we can just use the first set bit in a bitset of available children. Also, when freeing the node, rather than iterating through all 256 possible keys, just iterate through all set children directly. This slightly increases the size of the node48 struct, but it means: * We don't have to initialize all children to null at init time * We don't have to loop looking for a child (sometimes) on insert
Dr-Emann
force-pushed
the
r64_node48_holes
branch
from
a8de34c to
3779cd1
Compare
Dr-Emann
commented
Jan 20, 2024
SLieve
approved these changes
Jan 20, 2024
Dr-Emann
force-pushed
the
r64_node48_holes
branch
from
3779cd1 to
fe2d01c
Compare
|
Forgot to free at the end of the test |
|
As a bonus, with this and the other PRs merged so far, I've now been fuzzing for a few hours with no issues 🎉 |
|
Nice! |
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.
When removing from a node48, the child is simply set to NULL. For example, if the state is (ignoring that it actually needs at least 17 items to not be a node16):
After removing key 0, the state will be:
If we then try to insert at key 3, the item at
children[count]is already in use: we have to find and reuse the 0th child.Rather than matching the java implementation (though see the first commit 7dbad58), keep a bitset of available children slots. This slightly increases the size of the node48 struct, but it means:
This also has some fixes for
art_printf:FFFFFFFF