-
Notifications
You must be signed in to change notification settings - Fork 110
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
Make Page
always fully init
#1193
Conversation
Per discussion on the snapshotting proposal, this PR changes the type of `Page.row_data` to `[u8; _]`, where previously it was `[MaybeUninit<u8>; _]`. This turns out to be shockingly easy, as our serialization codepaths never write padding bytes into a page. The only place pages ever became `poison` was the initial allocation; changing this to `alloc_zeroed` causes the `row_data` to always be valid at `[u8; _]`. The majority of this diff is replacing `MaybeUninit`-specific operators with their initialized equivalents, and updating comments and documentation to reflect the new requirements. This change also revealed a bug in the benchmarks introduced when we swapped the order of sum tags and payloads ( #1063 ), where benchmarks used a hardcoded offset for the tag which had not been updated.
benchmarks please |
Criterion benchmark resultsCriterion benchmark reportYOU SHOULD PROBABLY IGNORE THESE RESULTS. Criterion is a wall time based benchmarking system that is extremely noisy when run on CI. We collect these results for longitudinal analysis, but they are not reliable for comparing individual PRs. Go look at the callgrind report instead. empty
insert_1
insert_bulk
iterate
find_unique
filter
serialize
stdb_module_large_arguments
stdb_module_print_bulk
remaining
|
Callgrind benchmark resultsCallgrind Benchmark ReportThese benchmarks were run using callgrind, Measurement changes larger than five percent are in bold. In-memory benchmarkscallgrind: empty transaction
callgrind: filter
callgrind: insert bulk
callgrind: iterate
callgrind: serialize_product_value
callgrind: update bulk
On-disk benchmarkscallgrind: empty transaction
callgrind: filter
callgrind: insert bulk
callgrind: iterate
callgrind: serialize_product_value
callgrind: update bulk
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this, it makes safety arguments easier & gets rid of a potential source of nondeterminism if non-zeroed data ever did end up visible somehow. Alloc_zeroed ought to be a tiny perf hit anyway.
Hmm, I guess it's a bigger perf hit than I thought looking at the benches, but I think this is still manageable. Inserts would be where it shows up.
Blake3 only supports running under Miri as of 1.15.1, the latest version. Prior versions hard-depended on SIMD intrinsics which Miri doesn't support.
Still pending his agreeing with me that `poison` is a better name than `uninit`.
Against my best wishes, for consistency with the broader Rust community's poor choices.
Description of Changes
Per discussion on the snapshotting proposal, this PR changes the type of
Page.row_data
to[u8; _]
, where previously it was[MaybeUninit<u8>; _]
.This turns out to be shockingly easy, as our serialization codepaths never write padding bytes into a page. The only place pages ever became
poison
was the initial allocation; changing this toalloc_zeroed
causes therow_data
to always be valid at[u8; _]
.The majority of this diff is replacing
MaybeUninit
-specific operators with their initialized equivalents, and updating comments and documentation to reflect the new requirements.This change also revealed a bug in the benchmarks introduced when we swapped the order of sum tags and payloads (#1063 ), where benchmarks used a hardcoded offset for the tag which had not been updated.
API and ABI breaking changes
N/a.
Expected complexity level and risk
3
uninit
.Testing
cargo test --benches
in thetable
directory.MIRIFLAGS=-Zmiri-disable-isolation PROPTEST_CASES=8 cargo miri test
in thetable
directory.spacetimedb-table
tests?