Lift the restriction of minimum 8-byte keys for the compact index #746
Conversation
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
jorisdral
force-pushed
the
jdral/lift-8-bytes-restriction
branch
2 times, most recently
from
3e8d2c3 to
1a2f622
Compare
jorisdral
requested review from
dcoutts,
mheinzel,
recursion-ninja and
wenkokke
as code owners
jorisdral
force-pushed
the
jdral/lift-8-bytes-restriction
branch
from
1a2f622 to
a41429e
Compare
dcoutts
approved these changes
Jul 3, 2025
jorisdral
force-pushed
the
jdral/lift-8-bytes-restriction
branch
from
a41429e to
99ed300
Compare
When the size of the raw bytes is less than 8, then a fallback method is used that is likely slower. When the size of the raw bytes is 8 or higher, then an additional integer comparison is made, but this has very little impact on performance, which I verified using micro-benchmarks. The upside to making the function safe for any input raw bytes is that the API and test generators become simpler, because there is one fewer constraint to satisfy: the minimum size of 8 bytes for serialised keys.
Now that `topBits64` is fully safe, remove the 8-byte key constraint when using the compact index. Instead, the config option for the index type includes a hint not to use the compact index if their keys are too small, because that will lead to bad performance.
A small bug related to the merge schedule and union levels was unearted after
recent changes to key generators for the state machine tests.
```
❯ cabal run lsm-tree-test -- --quickcheck-replay="(SMGen 13447116701882578385 16048829213438376903,35)" -p '$NF=="propLockstep_RealImpl_MockFS_IO"'
lsm-tree
Test.Database.LSMTree.StateMachine
propLockstep_RealImpl_MockFS_IO: FAIL (28.54s)
*** Failed! Assertion failed (after 1 test and 196 shrinks):
...
1 out of 1 tests failed (28.54s)
```
The bug was that the last level was always removing `Delete` entries even if
there was union level. This is now fixed.
jorisdral
force-pushed
the
jdral/lift-8-bytes-restriction
branch
from
99ed300 to
58119ff
Compare
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.
We make
topBits64safe for raw bytes of size less than 8. When the size of the raw bytes is less than 8, then a fallback method is used that is likely slower. When the size of the raw bytes is 8 or higher, then anadditional integer comparison is made, but this has very little impact on
performance, which I verified using micro-benchmarks. The upside to making the
function safe for any input raw bytes is that the API and test generators become
simpler, because there is one fewer constraint to satisfy: the minimum size of 8
bytes for serialised keys.
Now that
topBits64is fully safe, remove the 8-byte key constraint when usingthe compact index. Instead, the config option for the index type includes a hint
not to use the compact index if keys are too small, because that will lead
to bad performance.