Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compact in-memory representation of RecordRef #2053

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Add compact in-memory representation of RecordRef #2053

merged 3 commits into from
Sep 20, 2023

Conversation

Jesse-Bakker
Copy link
Contributor

@Jesse-Bakker Jesse-Bakker commented Sep 19, 2023

This adds a compact RecordRef representation, significantly reducing the
per-field overhead for records. There are some more improvements that can be
made here, namely not storing the schema per-record and removing the extra pointer
indirection for the field data.

Overhead

version Overhead / record Overhead / field
main 1 fat pointer (16 bytes) 8-24 bytes
compact-record 1 fat pointer + 1 thin pointer (24 bytes) 1 byte

Memory measurements

5M records, 4 integers, 1 float (near-ideal case)

version RSS(MB) improvement
main 1390 0%
compact-record 1066 23.3%

5M records, 1 integers, 2 strings

version RSS(MB) improvement
main 1508 0%
compact-record 1391 7.7%

@Jesse-Bakker Jesse-Bakker requested a review from chubei September 19, 2023 12:33
Copy link
Contributor

@chubei chubei left a comment

Choose a reason for hiding this comment

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

Clever use of the fact that Field alignment is less than Field size!

Unfortunately there are some changes we need to revert.

I'm far from proficient in such memory tricks so also left some questions here, mostly to help me understand the implementation.

dozer-types/src/types/field.rs Show resolved Hide resolved
dozer-recordstore/src/store.rs Outdated Show resolved Hide resolved
dozer-recordstore/src/store.rs Show resolved Hide resolved
dozer-recordstore/src/store.rs Outdated Show resolved Hide resolved
dozer-recordstore/src/lib.rs Show resolved Hide resolved
dozer-recordstore/src/lib.rs Outdated Show resolved Hide resolved
dozer-recordstore/src/lib.rs Outdated Show resolved Hide resolved
@Jesse-Bakker Jesse-Bakker requested a review from chubei September 20, 2023 08:08
Copy link
Contributor

@chubei chubei left a comment

Choose a reason for hiding this comment

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

Wonderful!

Cargo.lock Outdated Show resolved Hide resolved
dozer-sql/src/pipeline/expression/tests/test_common.rs Outdated Show resolved Hide resolved
@Jesse-Bakker Jesse-Bakker requested a review from chubei September 20, 2023 09:42
Because alignment of `i128` and `u128` is 8 on linux x86_64 and 16 on M1
@chubei chubei enabled auto-merge September 20, 2023 13:41
@chubei chubei added this pull request to the merge queue Sep 20, 2023
Merged via the queue into getdozer:main with commit 70ef6ae Sep 20, 2023
4 checks passed
@chubei chubei deleted the compact-record branch September 20, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants