datastore: insert via BSATN instead of via PV #2069

Centril · 2024-12-18T17:19:17Z

Description of Changes

This PR changes the structure of the datastore to insert via BSATN rather than a PV. But this isn't entirely true, we just use the fast-path for BSATN -> BFLATN but in the general case we still do BSATN -> PV -> BFLATN. Later, I'll fully remove the temporary step. One of the other required changes here is that we write and generate sequence values directly in BFLATN.

Fixes #2017.

Perf numbers on master

2025-01-03T09:30:37.776998Z  INFO: : Timing span "update_positions_by_id": 1.969988054s
2025-01-03T09:30:43.829835Z  INFO: : Timing span "update_positions_by_collect": 1.518314839s

with this PR:

2025-01-03T09:35:54.415502Z  INFO: : Timing span "update_positions_by_id": 1.64947372s
2025-01-03T09:36:00.880068Z  INFO: : Timing span "update_positions_by_collect": 1.26073871s

That is, this removes ~0.4516s and 0.3887s from the benchmarks respectively.

Flamegraph of InstanceEnv::insert:

Full flamegraph: https://flamegraph.com/share/14b571da-c9ba-11ef-9832-26c3e5347170

The next step after this PR is to avoid the pointer map in case there is a unique index.
After that, the next step is to add an update ABI.

API and ABI breaking changes

None

Expected complexity level and risk

4 -- limited scope, but lots of unsafe code and complicated logic.

Testing

The internal spacetimedb_table tests still use the old table.insert, so the new path. To compensate for this, without duplicating tests, a new proptest insert_bsatn_same_as_pv is added asserting that the result and side-effects of inserting via PV and BSATN are the same. Moreover, both insert paths try to share as much code as possible to improve test coverage. The higher level tests, starting with MutTxId now use the new path. Over time, we can replace the remaining old paths with the new and then move all tests to the new as well.

Reviewer notes

I would recommend reviewing in this order:

Start with spacetimedb_table
Move on to locking_tx_datastore
Move on to InstanceEnv
Review all the other boring changes to files like estimation.rs that were unfortunately necessary.

Review notes for Tyler

In traits.rs the following changes:

get_next_sequence_value_mut_tx is removed. It was unused.

insert_mut_tx changes from:

fn insert_mut_tx<'a>(..., row: ProductValue) -> Result<(AlgebraicValue, RowRef<'a>)>;

to:

fn insert_mut_tx<'a>(..., row: &[u8]) -> Result<(ColList, RowRef<'a>)>;

crates/core/src/host/instance_env.rs

which asserts that `table.insert(..)` does the same as using the bsatn path. Sprinkles various `Debug, PartialEq, Eq` derives to achieve this. Also uses `confirm_insertion` more to get that more under test. 2. Review and justify some unsafes.

Centril · 2025-01-03T11:53:34Z

crates/core/src/db/datastore/locking_tx_datastore/mod.rs

@@ -1,4 +1,4 @@
-#![forbid(unsafe_op_in_unsafe_fn)]
+#![deny(unsafe_op_in_unsafe_fn)]


Sadly caused by a rustc bug interacting with thread_local!.

TIL that deny and forbid are different lint levels. I have no clue which is worse.

gefjon

I have only reviewed the table and locking_tx_datastore changes with any particular care; everything else I glanced at, but didn't worry too much about, on the grounds that it's all safe code, it's barely changing and it's tested. The table and datastore parts I looked at quite carefully. I agree with your safety reasoning, and your benchmark results speak for themselves. I've left a few minor comment-related requests, like I always do. Great work on this! And thanks again for writing such a detailed PR description; it made review much smoother.

gefjon · 2025-01-03T15:58:04Z

crates/table/src/table.rs

    /// A [`StaticLayout`] for fast BFLATN <-> BSATN conversion,
    /// if the [`RowTypeLayout`] has a static BSATN length and layout.


Could you update this comment to also mention the StaticBsatnValidator?

gefjon · 2025-01-03T16:06:45Z

crates/table/src/table.rs

+    /// This does not check for set semantic or unique constraints.
+    ///
+    /// This is also useful when we need to insert a row temporarily to get back a `RowPointer`.
+    /// In this case, A call to this method should be followed by a call to [`delete_internal_skip_pointer_map`].


Can you expand this comment with a short paragraph about what will happen if this is called with a row that is not valid BSATN at the table's row type? Doesn't have to be super in-depth, but just to set our expectations between "Precisely detects all type errors" and "corrupts the table and burns down your house."

gefjon · 2025-01-03T16:10:28Z

crates/table/src/table.rs

+                // as `row_ty` was derived from the same schema as `seq` is part of.
+                let elem_ty = unsafe { &row_ty.elements.get_unchecked(seq.col_pos.idx()) };
+                // SAFETY:
+                // - `elem_ty` appears as a column in th row type.


Suggested change

// - `elem_ty` appears as a column in th row type.

// - `elem_ty` appears as a column in the row type.

gefjon · 2025-01-03T16:11:40Z

crates/table/src/table.rs

+    /// # Safety
+    ///
+    /// - `self.is_row_present(row)` must hold.
+    /// - `col_id` must be a valid column, with a primiive type, of the row type.


Suggested change

/// - `col_id` must be a valid column, with a primiive type, of the row type.

/// - `col_id` must be a valid column, with a primitive integer type, of the row type.

Spelling on "primitive." Being an integer is not a safety requirement as written, but I think it could be (see other comment). Whether or not we actually invoke UB if the column is non-integer, I think it's simplest in terms of docs to just say that we don't define behavior in that case, i.e. to make it a safety requirement as I'm doing here.

gefjon · 2025-01-03T16:15:00Z

crates/table/src/table.rs

+            PrimitiveType::Bool | PrimitiveType::F32 | PrimitiveType::F64 => {
+                panic!("`{:?}` is not a sequence integer type", &elem_ty.ty)
+            }


It feels mildly odd that we panic here, but unreachable_unchecked above. I don't feel strongly, but I might replace this with unreachable_unchecked just for consistency's sake.

gefjon · 2025-01-03T16:17:09Z

crates/table/src/table.rs

+    }
+
+    /// Performs all the checks necessary after having fully decided on a rows contents.
+    ///


Suggested change

///

///

/// This includes inserting the row into any applicable indices and/or the pointer map.

///

gefjon · 2025-01-03T16:24:07Z

crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs

+        })
+    }
+
+    /// Insert a row, encoded in BSATN, into a table.
    ///
    /// Requires:
    /// - `TableId` must refer to a valid table for the database at `database_address`.
    /// - `row` must be a valid row for the table at `table_id`.
    ///


Suggested change

///

///

/// Zero placeholders in auto-inc columns in the new row will be replaced with generated values

/// if and only if `GENERATE` is true.

/// This method is called with `GENERATE` false when updating the `st_sequence` system table.

///

gefjon · 2025-01-03T16:25:35Z

crates/core/src/db/datastore/locking_tx_datastore/mod.rs

@@ -1,4 +1,4 @@
-#![forbid(unsafe_op_in_unsafe_fn)]
+#![deny(unsafe_op_in_unsafe_fn)]


TIL that deny and forbid are different lint levels. I have no clue which is worse.

Centril requested a review from cloutiertyler as a code owner December 18, 2024 17:19

Centril marked this pull request as draft December 18, 2024 17:19

Centril force-pushed the centril/insert-bsatn branch 3 times, most recently from 5df3305 to 5dfebdd Compare December 18, 2024 17:45

Centril commented Dec 18, 2024

View reviewed changes

crates/core/src/host/instance_env.rs Outdated Show resolved Hide resolved

Centril force-pushed the centril/insert-bsatn branch from 374c094 to 5df4fc2 Compare January 2, 2025 13:37

Centril added 6 commits January 3, 2025 10:32

add temporary write_row_to_pages_bsatn that does BSATN -> PV -> BFLATN

ee30834

extract Table::insert_into_pointer_map

22c75fc

datastore: insert via bsatn instead of PV

886d904

InstanceEnv::insert: fix gen col write-back

5802273

insert_via_serialize_bsatn: fix nested mut borrow bug

55ac51e

Table::confirm_insertion: fix underflow bug

4af0705

Centril force-pushed the centril/insert-bsatn branch from 8275279 to 4af0705 Compare January 3, 2025 09:32

Centril commented Jan 3, 2025

View reviewed changes

Centril marked this pull request as ready for review January 3, 2025 12:15

Centril requested a review from gefjon January 3, 2025 12:15

gefjon approved these changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datastore: insert via BSATN instead of via PV #2069

datastore: insert via BSATN instead of via PV #2069

Centril commented Dec 18, 2024 •

edited

Loading

Centril Jan 3, 2025

gefjon Jan 3, 2025

gefjon left a comment

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

gefjon Jan 3, 2025

		@@ -1,4 +1,4 @@
		#![forbid(unsafe_op_in_unsafe_fn)]
		#![deny(unsafe_op_in_unsafe_fn)]

		/// A [`StaticLayout`] for fast BFLATN <-> BSATN conversion,
		/// if the [`RowTypeLayout`] has a static BSATN length and layout.

	// - `elem_ty` appears as a column in th row type.
	// - `elem_ty` appears as a column in the row type.

	/// - `col_id` must be a valid column, with a primiive type, of the row type.
	/// - `col_id` must be a valid column, with a primitive integer type, of the row type.

-    ///
+    ///
+    /// This includes inserting the row into any applicable indices and/or the pointer map.
+    ///

-    ///
+    ///
+    /// Zero placeholders in auto-inc columns in the new row will be replaced with generated values
+    /// if and only if `GENERATE` is true.
+    /// This method is called with `GENERATE` false when updating the `st_sequence` system table.
+    ///

datastore: insert via BSATN instead of via PV #2069

Are you sure you want to change the base?

datastore: insert via BSATN instead of via PV #2069

Conversation

Centril commented Dec 18, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

Reviewer notes

Review notes for Tyler

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gefjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril commented Dec 18, 2024 •

edited

Loading