Make Page always fully init (#1193)

* Make `Page` always fully init

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.

* Update blake3

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.

* Address Mazdak's review.

Still pending his agreeing with me that `poison` is a better name than `uninit`.

* "Poison" -> "uninit"

Against my best wishes, for consistency with the broader Rust community's poor choices.

* Remove unnecessary `unsafe` blocks

* More unnecessary `unsafe`; remove forgotten SAFETY comments

Cargo.toml

@@ -109,7 +109,7 @@ axum-extra = { version = "0.9", features = ["typed-header"] }
 backtrace = "0.3.66"
 base64 = "0.21.2"
 bitflags = "2.3.3"
-blake3 = "1.5"
+blake3 = "1.5.1"
 brotli = "3.5"
 byte-unit = "4.0.18"
 bytes = "1.2.1"

crates/table/benches/page.rs

@@ -1,5 +1,5 @@
 use core::hash::BuildHasher;
-use core::mem::{self, MaybeUninit};
+use core::mem;
 use core::time::Duration;
 use criterion::measurement::{Measurement, WallTime};
 use criterion::{black_box, criterion_group, criterion_main, Bencher, BenchmarkId, Criterion, Throughput};
@@ -12,12 +12,11 @@ use spacetimedb_table::bflatn_from::serialize_row_from_page;
 use spacetimedb_table::bflatn_to::write_row_to_page;
 use spacetimedb_table::blob_store::NullBlobStore;
 use spacetimedb_table::eq::eq_row_in_page;
-use spacetimedb_table::indexes::{PageOffset, RowHash};
+use spacetimedb_table::indexes::{Byte, Bytes, PageOffset, RowHash};
 use spacetimedb_table::layout::{row_size_for_type, RowTypeLayout};
 use spacetimedb_table::page::Page;
 use spacetimedb_table::row_hash::hash_row_in_page;
 use spacetimedb_table::row_type_visitor::{row_type_visitor, VarLenVisitorProgram};
-use spacetimedb_table::util;
 use spacetimedb_table::var_len::{AlignedVarLenOffsets, NullVarLenVisitor, VarLenGranule, VarLenMembers, VarLenRef};
 
 fn time<R>(acc: &mut Duration, body: impl FnOnce() -> R) -> R {
@@ -51,8 +50,11 @@ fn clear_zero(page: &mut Page) {
     unsafe { page.zero_data() };
 }
 
-fn as_bytes<T>(t: &T) -> &[MaybeUninit<u8>] {
-    let ptr = (t as *const T).cast::<MaybeUninit<u8>>();
+// Strictly this would be unsafe,
+// since it causes UB when applied to types that contain padding/`poison`,
+// but it's a benchmark so who cares.
+fn as_bytes<T>(t: &T) -> &Bytes {
+    let ptr = (t as *const T).cast::<Byte>();
     unsafe { std::slice::from_raw_parts(ptr, mem::size_of::<T>()) }
 }
 
@@ -66,12 +68,15 @@ unsafe trait Row {
 }
 
 #[allow(clippy::missing_safety_doc)] // It's a benchmark, clippy. Who cares.
+/// Apply only to types which:
+/// - Contain no padding bytes.
+/// - Contain no members which are stored BFLATN as var-len.
 unsafe trait FixedLenRow: Row + Sized {
-    fn as_bytes(&self) -> &[MaybeUninit<u8>] {
+    fn as_bytes(&self) -> &Bytes {
         as_bytes(self)
     }
 
-    unsafe fn from_bytes(bytes: &[MaybeUninit<u8>]) -> &Self {
+    unsafe fn from_bytes(bytes: &Bytes) -> &Self {
         let ptr = bytes.as_ptr();
         debug_assert_eq!(ptr as usize % mem::align_of::<Self>(), 0);
         debug_assert_eq!(bytes.len(), mem::size_of::<Self>());
@@ -241,6 +246,7 @@ fn insert_var_len_clean_page(c: &mut Criterion, visitor: &impl VarLenMembers, vi
             |b, &len_in_bytes| {
                 let mut page = Page::new(row_size_for_type::<VarLenRef>());
                 unsafe { page.zero_data() };
+                // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
                 let data = [0xa5u8].repeat(len_in_bytes);
                 iter_time_with_page(b, &mut page, clear_zero, |_, _, page| {
                     fill_with_var_len(page, &data, visitor)
@@ -263,6 +269,7 @@ fn insert_var_len_dirty_page(c: &mut Criterion, visitor: &impl VarLenMembers, vi
             &len_in_bytes,
             |b, &len_in_bytes| {
                 let mut page = Page::new(row_size_for_type::<VarLenRef>());
+                // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
                 let data = [0xa5u8].repeat(len_in_bytes);
                 fill_with_var_len(&mut page, &data, visitor);
 
@@ -294,11 +301,13 @@ fn insert_opt_str(c: &mut Criterion) {
     assert!(fixed_row_size.len() == 6);
     let mut clean_page_group = c.benchmark_group("insert_optional_str/clean_page");
 
-    let mut variant_none = util::uninit_array::<u8, 6>();
-    variant_none[4].write(1);
+    // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
+    let mut variant_none = [0xa5u8; 6];
+    variant_none[0] = 1;
 
-    let mut variant_some = util::uninit_array::<u8, 6>();
-    variant_some[4].write(0);
+    // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
+    let mut variant_some = [0xa5u8; 6];
+    variant_some[0] = 0;
 
     for some_ratio in [0.0, 0.1, 0.25, 0.5, 0.75, 0.9, 1.0] {
         for &data_length_in_bytes in if some_ratio == 0.0 { &[0][..] } else { &VL_SIZES } {
@@ -311,6 +320,7 @@ fn insert_opt_str(c: &mut Criterion) {
 
             clean_page_group.throughput(Throughput::Bytes((rows_per_page * avg_row_useful_size) as u64));
 
+            // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
             let var_len_data = [0xa5].repeat(data_length_in_bytes);
             clean_page_group.bench_with_input(
                 BenchmarkId::new(
@@ -391,6 +401,7 @@ fn iter_read_fixed_len<Row: FixedLenRow>(c: &mut Criterion) {
         // Construct a page which is approximately `fullness_ratio` full,
         // i.e. contains approximately `fullness_ratio * U64S_PER_PAGE` rows.
         let mut partial_page = Page::new(row_size_for_type::<Row>());
+        // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
         fill_with_fixed_len::<Row>(&mut partial_page, Row::from_u64(0xa5a5a5a5_a5a5a5a5), &visitor);
         // `delete_u64s_to_approx_fullness_ratio` uses a seeded `StdRng`,
         // so this should be consistent-ish.
@@ -414,6 +425,7 @@ fn iter_read_fixed_len<Row: FixedLenRow>(c: &mut Criterion) {
     }
 
     let mut full_page = Page::new(row_size_for_type::<Row>());
+    // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
     fill_with_fixed_len(&mut full_page, Row::from_u64(0xa5a5a5a5_a5a5a5a5), &visitor);
     group.throughput(Throughput::Bytes((full_page.num_rows() * mem::size_of::<Row>()) as u64));
     group.bench_with_input(
@@ -436,6 +448,7 @@ fn copy_filter_into_fixed_len_keep_ratio<Row: FixedLenRow>(b: &mut Bencher, keep
     let mut target_page = Page::new(row_size_for_type::<Row>());
 
     let mut src_page = Page::new(row_size_for_type::<Row>());
+    // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
     fill_with_fixed_len::<Row>(&mut src_page, Row::from_u64(0xa5a5a5a5_a5a5a5a5), &visitor);
 
     let mut rng = StdRng::seed_from_u64(0xa5a5a5a5_a5a5a5a5);
@@ -525,6 +538,7 @@ fn product_value_test_cases() -> impl Iterator<
         (
             "U32",
             [AlgebraicType::U32].into(),
+            // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
             product![0xa5a5_a5a5u32],
             Some(NullVarLenVisitor),
             Some(AlignedVarLenOffsets::from_offsets(&[])),
@@ -539,6 +553,7 @@ fn product_value_test_cases() -> impl Iterator<
         (
             "Option<U32>/Some",
             [AlgebraicType::option(AlgebraicType::U32)].into(),
+            // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
             product![AlgebraicValue::OptionSome(AlgebraicValue::U32(0xa5a5_a5a5))],
             Some(NullVarLenVisitor),
             Some(AlignedVarLenOffsets::from_offsets(&[])),

crates/table/benches/page_manager.rs

@@ -1,5 +1,5 @@
 use core::iter;
-use core::mem::{self, MaybeUninit};
+use core::mem;
 use core::time::Duration;
 use criterion::measurement::{Measurement, WallTime};
 use criterion::{
@@ -12,7 +12,8 @@ use spacetimedb_sats::db::def::{TableDef, TableSchema};
 use spacetimedb_sats::{AlgebraicType, AlgebraicValue, ProductType, ProductValue};
 use spacetimedb_table::blob_store::NullBlobStore;
 use spacetimedb_table::btree_index::BTreeIndex;
-use spacetimedb_table::indexes::{PageOffset, RowPointer, Size, SquashedOffset, PAGE_DATA_SIZE};
+use spacetimedb_table::indexes::Byte;
+use spacetimedb_table::indexes::{Bytes, PageOffset, RowPointer, Size, SquashedOffset, PAGE_DATA_SIZE};
 use spacetimedb_table::layout::{row_size_for_bytes, row_size_for_type};
 use spacetimedb_table::pages::Pages;
 use spacetimedb_table::row_type_visitor::{row_type_visitor, VarLenVisitorProgram};
@@ -45,8 +46,11 @@ fn iter_time_with<P, B, X>(
     })
 }
 
-fn as_bytes<T>(t: &T) -> &[MaybeUninit<u8>] {
-    let ptr = (t as *const T).cast::<MaybeUninit<u8>>();
+// Strictly this would be unsafe,
+// since it causes UB when applied to types that contain padding/`poison`,
+// but it's a benchmark so who cares.
+fn as_bytes<T>(t: &T) -> &Bytes {
+    let ptr = (t as *const T).cast::<Byte>();
     unsafe { std::slice::from_raw_parts(ptr, mem::size_of::<T>()) }
 }
 
@@ -64,12 +68,15 @@ unsafe trait Row {
 }
 
 #[allow(clippy::missing_safety_doc)] // It's a benchmark, clippy. Who cares.
+/// Apply only to types which:
+/// - Contain no padding bytes.
+/// - Contain no members which are stored BFLATN as var-len.
 unsafe trait FixedLenRow: Row + Sized {
-    fn as_bytes(&self) -> &[MaybeUninit<u8>] {
+    fn as_bytes(&self) -> &Bytes {
         as_bytes(self)
     }
 
-    unsafe fn from_bytes(bytes: &[MaybeUninit<u8>]) -> &Self {
+    unsafe fn from_bytes(bytes: &Bytes) -> &Self {
         let ptr = bytes.as_ptr();
         debug_assert_eq!(ptr as usize % mem::align_of::<Self>(), 0);
         debug_assert_eq!(bytes.len(), mem::size_of::<Self>());
@@ -234,6 +241,7 @@ fn insert_one_page_fixed_len(c: &mut Criterion) {
         ));
         group.bench_function(name, |b| {
             let mut pages = Pages::default();
+            // `0xa5` is the alternating bit pattern, which makes incorrect accesses obvious.
             insert_one_page_worth_fixed_len(&mut pages, visitor, &R::from_u64(0xa5a5a5a5_a5a5a5a5));
             let pre = |_, pages: &mut Pages| pages.clear();
             iter_time_with(b, &mut pages, pre, |_, _, pages| {

crates/table/benches/var_len_visitor.rs

@@ -1,12 +1,12 @@
 use core::slice;
 use criterion::{black_box, criterion_group, criterion_main, Criterion};
 use spacetimedb_sats::{AlgebraicType, ProductType};
+use spacetimedb_table::indexes::{Byte, Bytes};
 use spacetimedb_table::row_type_visitor::{dump_visitor_program, row_type_visitor, VarLenVisitorProgram};
-use spacetimedb_table::util::uninit_array;
 use spacetimedb_table::var_len::{AlignedVarLenOffsets, NullVarLenVisitor, VarLenMembers, VarLenRef};
-use std::mem::{self, MaybeUninit};
+use std::mem;
 
-fn visit_count(row: &[MaybeUninit<u8>], visitor: &impl VarLenMembers) {
+fn visit_count(row: &Bytes, visitor: &impl VarLenMembers) {
     black_box(unsafe { visitor.visit_var_len(row) }.count());
 }
 
@@ -23,8 +23,8 @@ fn visitor_program(row_ty: impl Into<ProductType>) -> VarLenVisitorProgram {
 }
 
 fn visit_fixed_len(c: &mut C) {
-    let row = &uninit_array::<u32, 1>();
-    let row = row.as_ptr().cast::<MaybeUninit<u8>>();
+    let row = &[0xa5a5_a5a5u32; 1];
+    let row = row.as_ptr().cast::<Byte>();
     let row = unsafe { slice::from_raw_parts(row, mem::size_of::<u32>()) };
 
     let mut group = c.benchmark_group("visit_fixed_len/u64");
@@ -49,8 +49,8 @@ fn visit_fixed_len(c: &mut C) {
 }
 
 fn visit_var_len_product(c: &mut C) {
-    let row = &uninit_array::<VarLenRef, 1>();
-    let row = row.as_ptr().cast::<MaybeUninit<u8>>();
+    let row = &[VarLenRef::NULL; 1];
+    let row = row.as_ptr().cast::<Byte>();
     let row = unsafe { slice::from_raw_parts(row, mem::size_of::<VarLenRef>()) };
 
     let mut group = c.benchmark_group("visit_var_len_product/VarLenRef");
@@ -73,20 +73,20 @@ fn visit_var_len_sum(c: &mut C) {
 
     let visitor = &visitor_program([AlgebraicType::sum([AlgebraicType::String, AlgebraicType::unit()])]);
 
-    let row = &mut uninit_array::<u16, 3>();
-    let row = row.as_mut_ptr().cast::<MaybeUninit<u8>>();
+    let row = &mut [0xa5a5u16; 3];
+    let row = row.as_mut_ptr().cast::<Byte>();
     let row = unsafe { slice::from_raw_parts_mut(row, 6) };
 
     group.bench_function("none/VarLenVisitorProgram", |b| {
         // None
-        row[4].write(1);
+        row[0] = 1;
 
         b.iter(|| visit_count(row, visitor));
     });
 
     group.bench_function("some/VarLenVisitorProgram", |b| {
         // Some
-        row[4].write(0);
+        row[0] = 0;
 
         b.iter(|| visit_count(row, visitor));
     });

crates/table/src/bflatn_from.rs

@@ -110,8 +110,7 @@ unsafe fn serialize_sum<S: Serializer>(
     ty: &SumTypeLayout,
 ) -> Result<S::Ok, S::Error> {
     // Read the tag of the sum value.
-    // SAFETY: `bytes[curr_offset..]` hold a sum value at `ty`.
-    let (tag, data_ty) = unsafe { read_tag(bytes, ty, curr_offset.get()) };
+    let (tag, data_ty) = read_tag(bytes, ty, curr_offset.get());
 
     // Serialize the variant data value.
     let data_offset = &Cell::new(curr_offset.get() + ty.offset_of_variant_data(tag));
@@ -133,20 +132,9 @@ unsafe fn serialize_sum<S: Serializer>(
 }
 
 /// Reads the tag of the sum value and selects the data variant type.
-///
-/// # Safety
-///
-/// `bytes[curr_offset..]` has a sum value typed at `ty`.
-pub unsafe fn read_tag<'ty>(
-    bytes: &Bytes,
-    ty: &'ty SumTypeLayout,
-    curr_offset: usize,
-) -> (u8, &'ty AlgebraicTypeLayout) {
+pub fn read_tag<'ty>(bytes: &Bytes, ty: &'ty SumTypeLayout, curr_offset: usize) -> (u8, &'ty AlgebraicTypeLayout) {
     let tag_offset = ty.offset_of_tag();
     let tag = bytes[curr_offset + tag_offset];
-    // SAFETY: Caller promised that `bytes[curr_offset..]` has a sum value typed at `ty`.
-    // We can therefore assume that `curr_offset + tag_offset` refers to a valid `u8`.
-    let tag = unsafe { tag.assume_init() };
 
     // Extract the variant data type depending on the tag.
     let data_ty = &ty.variants[tag as usize].ty;

crates/table/src/bflatn_to.rs

@@ -11,8 +11,8 @@ use super::{
     },
     page::{GranuleOffsetIter, Page, VarView},
     pages::Pages,
-    util::{maybe_uninit_write_slice, range_move},
-    var_len::{visit_var_len_assume_init, VarLenGranule, VarLenMembers, VarLenRef},
+    util::range_move,
+    var_len::{VarLenGranule, VarLenMembers, VarLenRef},
 };
 use spacetimedb_sats::{bsatn::to_writer, buffer::BufWriter, AlgebraicType, AlgebraicValue, ProductValue, SumValue};
 use thiserror::Error;
@@ -151,12 +151,12 @@ impl BflatnSerializedRowBuffer<'_> {
         //    and `fixed_buf.len()` matches exactly the size of the row type.
         // - `fixed_buf`'s `VarLenRef`s are initialized up to `last_allocated_var_len_index`.
         // - `visitor` is proper for the row type.
-        let visitor_iter = unsafe { visit_var_len_assume_init(visitor, self.fixed_buf) };
+        let visitor_iter = unsafe { visitor.visit_var_len(self.fixed_buf) };
         for vlr in visitor_iter.take(self.last_allocated_var_len_index) {
             // SAFETY: The `vlr` came from the allocation in `write_var_len_obj`
             // which wrote it to the fixed part using `write_var_len_ref`.
             // Thus, it points to a valid `VarLenGranule`.
-            unsafe { self.var_view.free_object_ignore_blob(vlr) };
+            unsafe { self.var_view.free_object_ignore_blob(*vlr) };
         }
     }
 
@@ -165,7 +165,8 @@ impl BflatnSerializedRowBuffer<'_> {
         for (vlr, value) in self.large_blob_insertions {
             // SAFETY: `vlr` was given to us by `alloc_for_slice`
             // so it is properly aligned for a `VarLenGranule` and in bounds of the page.
-            // However, as it was added to `self.large_blob_insertion`, it is also uninit.
+            // However, as it was added to `self.large_blob_insertions`,
+            // we have not yet written the hash to that granule.
             unsafe {
                 self.var_view.write_large_blob_hash_to_granule(blob_store, &value, vlr);
             }
@@ -250,7 +251,8 @@ impl BflatnSerializedRowBuffer<'_> {
         // so we need to check that our `ProductValue` has the same number of elements
         // as our `ProductTypeLayout` to be sure it's typed correctly.
         // Otherwise, if the value is too long, we'll discard its fields (whatever),
-        // or if it's too long, we'll leave some fields in the page uninit (very bad).
+        // or if it's too long, we'll leave some fields in the page "uninit"
+        // (actually valid-unconstrained) (very bad).
         if ty.elements.len() != val.elements.len() {
             return Err(Error::WrongType(
                 ty.algebraic_type(),
@@ -302,7 +304,9 @@ impl BflatnSerializedRowBuffer<'_> {
         } else {
             // Write directly to the page.
             // SAFETY: `vlr.first_granule` points to a granule
-            // even though the granule's data is uninit as of yet.
+            // even though the granule's data is not initialized as of yet.
+            // Note that the granule stores valid-unconstrained bytes (i.e. they are not uninit),
+            // but they may be leftovers from a previous allocation.
             let iter = unsafe { self.var_view.granule_offset_iter(vlr.first_granule) };
             let mut writer = GranuleBufWriter { buf: None, iter };
             to_writer(&mut writer, val).unwrap();
@@ -347,7 +351,7 @@ impl BflatnSerializedRowBuffer<'_> {
 
                     // Write to the granule.
                     for (to, byte) in write_to.iter_mut().zip(extend_with) {
-                        to.write(*byte);
+                        *to = *byte;
                     }
 
                     slice = rest;
@@ -377,7 +381,7 @@ impl BflatnSerializedRowBuffer<'_> {
     /// Write `bytes: &[u8; N]` starting at the current offset
     /// and advance the offset by `N`.
     fn write_bytes<const N: usize>(&mut self, bytes: &[u8; N]) {
-        maybe_uninit_write_slice(&mut self.fixed_buf[range_move(0..N, self.curr_offset)], bytes);
+        self.fixed_buf[range_move(0..N, self.curr_offset)].copy_from_slice(bytes);
         self.curr_offset += N;
     }
 
@@ -458,7 +462,7 @@ pub mod test {
     use spacetimedb_sats::proptest::generate_typed_row;
 
     proptest! {
-        #![proptest_config(ProptestConfig::with_cases(2048))]
+        #![proptest_config(ProptestConfig::with_cases(if cfg!(miri) { 8 } else { 2048 }))]
         #[test]
         fn av_serde_round_trip_through_page((ty, val) in generate_typed_row()) {
             let ty: RowTypeLayout = ty.into();