Refactor write buffer in lookupsIO #397
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
dcoutts
requested review from
jorisdral,
mheinzel and
recursion-ninja
as code owners
jorisdral
requested changes
Sep 24, 2024
dcoutts
force-pushed
the
dcoutts/lookupsIO-writebuffer
branch
from
b90b79a
to
e1b27e8
Compare
Previously lookupsIO dealt only with lookups in runs, and combining results from the write buffer was done in D.L.Internal. Originally that was ok because combining the write buffer results was simple, but it has grown more complex with the blobs feature being properly enabled. So now we pass it down into lookupsIO, specifically into intraPageLookups where all the results are combined. It works nicely here, since per-key results are accumulated into a mutable vector, so the only change is to initialise that array with the write buffer lookups. All the lookup results from runs are then accumulated on top. This should also mildly reduce allocations. In addition, instead of D.L.Internal.lookups getting passed in a function for converting into the final result, just return the intermediate representation and have the final conversion done in the API wrappers. This approach is more friendly for inlining and specialisation (so the deserialisation can be specialised at the call site). The benchmarks are updated but use an empty write buffer. Plausibly we might want some micro benchmarks for lookupIO et al that do make use of the write buffer too.
dcoutts
force-pushed
the
dcoutts/lookupsIO-writebuffer
branch
from
e1b27e8
to
3e22f31
Compare
jorisdral
approved these changes
Sep 30, 2024
dcoutts
force-pushed
the
dcoutts/lookupsIO-writebuffer
branch
from
3e22f31
to
af0beab
Compare
dcoutts
changed the title
|
I've removed the patch for enabling blobs in various tests since it doesn't need to be in this PR at all, and some of them need fixing. I'll open a separate PR for those. |
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.
See the individual patches for more detailed descriptions.