Allow integer packing and unpacking of partial chunks #3681
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
royi-luo
commented
Jun 20, 2024
royi-luo
commented
Jun 20, 2024
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
3 times, most recently
from
3ea6f03
to
b8fb1c1
Compare
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
f532892
to
0fff194
Compare
royi-luo
commented
Jun 24, 2024
royi-luo
commented
Jun 24, 2024
|
Some database sizes before/after the change (sizes in KB unless stated otherwise):
|
For the ldbc-sf01 dataset, I noticed that the database size is non-deterministic (it fluctuates between runs), @benjaminwinger @ray6080 could one of you explain why this is the case? I do notice that the number of pages goes down though (below is the Comment table as an example) BeforeAfter |
Benchmark ResultMaster commit hash:
|
Looks like the non-determinism was due to parallel copies changing how the data is arranged, the numbers have been updated with single-threaded copying. |
benjaminwinger
approved these changes
Jun 24, 2024
| // TODO(bmwinger): Now I'm a bit confused here. Why do we always need to unpack the chunk | ||
| // first? | ||
| // Can't we just unpack the chunk if we need to modify only partial of it? | ||
| fastunpack(chunkStart, chunk, header.bitWidth); |
There was a problem hiding this comment.
This change should be able to address the comment here. You can do an unaligned pack first (like the one at the end) and then this loop can be modified to remove the unpack and just pack the 32-value chunks (which should be able to be simplified; the looping is a little complicated because it starts and ends at arbitrary points in the chunk, but if its only being used for full chunks the values can just be copied from the srcBuffer into the temporary chunk minus the offset, and then packed, before moving to the next 32-value chunk).
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
3a5527b
to
ca96787
Compare
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
ca96787
to
80cca78
Compare
ray6080
approved these changes
Jun 24, 2024
| #include "common/utils.h" | ||
|
|
||
| namespace kuzu::storage { | ||
| namespace { |
There was a problem hiding this comment.
Do we need L9 here mespace { ? Also L111.
There was a problem hiding this comment.
I made an anonymous namespace for the internal functions, I can remove if it's not part of our coding standards
There was a problem hiding this comment.
hmm I don't think we have that convention, but not against adding that either. @benjaminwinger what do you think?
There was a problem hiding this comment.
I'm fine with it. It's similar to making functions static, but somewhat more capable since you can also use it for classes/structs which are local to the translation unit.
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
80cca78
to
c805e5a
Compare
Benchmark ResultMaster commit hash:
|
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
7cb1820
to
44f0d81
Compare
Quick performance benchmark on a dataset with 100M nodes:
|
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
44f0d81
to
4245526
Compare
Benchmark ResultMaster commit hash:
|
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
c2ceb5f
to
c45cc9a
Compare
Benchmark ResultMaster commit hash:
|
royi-luo
force-pushed
the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
from
c45cc9a
to
811a815
Compare
Benchmark ResultMaster commit hash:
|
royi-luo
deleted the
royi/compression-allow-pack-and-unpack-partial-chunk
branch
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.
Description
#2083
Reworked single-value bitpacking/unpacking for int128 to work for all integral types.
Used the above to allow packing of a number of values per page that isn't a multiple of 32 (by packing individual values at the end of each page).
Contributor agreement