From a77d265b51c31b09c1a9222c30721e4f46c6b3ab Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 4 Sep 2026 10:49:35 +0200 Subject: [PATCH] feat: make Value Send by switching Text/Blob payloads to Arc (#688) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Value::Text(Rc)`/`Blob(Rc<[u8]>)` made `Value` itself `!Send`, and a result row is a `Vec`, so no query result could cross a thread boundary at all. That blocks spec 013/Req 4's `Send + Sync` connection handle outright. ADR-0013 and ADR-0017 look like they forbid this, and they do not: both are about the *pager*, where one `Vm` shares a page source across N cursors via cheap `Rc` clones. `Value`'s payloads were never their subject. `Rc` and `Rc>` are untouched here. ADR-0039 records that distinction, because the natural reading of those two ADRs is "`Arc` anywhere is settled against" and the next person would otherwise re-litigate it or assume an oversight. Spike 014 (#682) measured rather than argued: +22/-17 across 6 files (construction sites use `.into()`, identical for both types, so only the ~12 that name the type changed), 1562 tests unchanged, and no measurable read-path cost — `Rc` runs spanned 5.42-5.62 ms against `Arc`'s 5.24-5.33 ms on a single-threaded full drain. It also removes the boundary copy permanently: 5.13 ms against 7.34 ms for the owned-copy alternative. A `const` assertion in `value.rs` now enforces `Send + Sync` at compile time. Mutation-checked: reverting either payload to `Rc` fails the build with "cannot be sent between threads safely" rather than deferring the error to some future consumer. One honest caveat carried into ADR-0039: the spike's noise floor is a few percent and one run showed `Arc` 5.6% *faster*, which is not a credible result from adding atomics. The claim is "no measurable cost", not "faster", and it should be re-measured on `tests/performance/engine.rs` against the pinned oracle before being quoted as settled. Verified: 1562 unit and 380 corpus tests pass, clippy/fmt clean. Refs: 013/Req-4, #688, #682, #678 Co-Authored-By: Claude Opus 5 (1M context) --- .../adr/0039-value-payloads-are-arc-not-rc.md | 84 +++++++++++++++++++ .openspec/adr/index.md | 1 + src/record/decode.rs | 14 ++-- src/record/value.rs | 18 +++- src/vdbe/cursor.rs | 11 +-- src/vdbe/hash_agg.rs | 4 +- src/vdbe/result.rs | 4 +- src/vdbe/sorter.rs | 2 +- 8 files changed, 118 insertions(+), 20 deletions(-) create mode 100644 .openspec/adr/0039-value-payloads-are-arc-not-rc.md diff --git a/.openspec/adr/0039-value-payloads-are-arc-not-rc.md b/.openspec/adr/0039-value-payloads-are-arc-not-rc.md new file mode 100644 index 00000000..8d1afd44 --- /dev/null +++ b/.openspec/adr/0039-value-payloads-are-arc-not-rc.md @@ -0,0 +1,84 @@ +# 0039: `Value`'s text and blob payloads are `Arc`, not `Rc` + +Date: 2026-09-04 + +## Context + +`Value::Text(Rc)` and `Value::Blob(Rc<[u8]>)` made `Value` itself +`!Send`. A result row is a `Vec`, so no query result could cross a +thread boundary at all. + +That blocks spec 013's embedding API outright. Its Requirement 4 asks for a +`Send + Sync` connection handle, and ADR-0034 (as filed in PR #678) proposes a +worker thread that "creates its `Rc` graph on a thread it owns and never lets +it leave". Rows are precisely the thing that must leave, so the proposed design +is not sufficient as written — a gap neither the spec nor that ADR records, +because both locate the `Send` problem in the pager alone. + +**The obvious objection is that ADR-0013 and ADR-0017 already rejected `Arc`. +They did not — not for this.** Both are about the *pager*: one `Vm` shares a +page source across N cursors via cheap `Rc` clones, and making that `Arc` would +tax the Tier 0 read path to serve a requirement that lives at the connection +boundary. `Value`'s payloads were never their subject. The natural reading of +those two ADRs is nevertheless "`Arc` anywhere is settled against", which is +why this needs writing down rather than being left as an apparent +contradiction. + +Spike 014 (#682) measured the change instead of arguing it: + +- **+22/−17 across 6 files.** Nearly every construction site writes + `Value::Text(s.to_string().into())`, and `.into()` is identical for `Rc` + and `Arc`, so only the ~12 sites that *name* the type needed editing. +- **1562 tests pass, 0 fail** — identical to the `Rc` baseline. +- **No measurable read-path cost.** `full_drain/batch` is single-threaded with + no thread boundary, so it isolates the tax: `Rc` runs spanned 5.42–5.62 ms, + `Arc` runs 5.24–5.33 ms. One comparison reported `Arc` 5.6% *faster*, which + is not a credible speedup from adding atomics — the honest reading is that + the difference sits inside run-to-run variance. +- It removes the boundary copy permanently: handing rows over untouched ran + 5.13 ms against 7.34 ms for the owned-copy alternative. + +## Decision + +`Value::Text` and `Value::Blob` hold `Arc` and `Arc<[u8]>`. `Value` is +therefore `Send + Sync`, enforced by a `const` assertion in +`src/record/value.rs` rather than by convention. + +`Rc` and `Rc>` are **unchanged**. ADR-0013 and +ADR-0017 remain in force on exactly the question they decided. + +## Alternatives rejected + +- **An owned copy at the API boundary** (`String`/`Vec`), leaving `Value` + as `Rc`. Works, and was the spike's first prototype — but it costs an + allocation and copy per text and blob value on every row, forever, and it + puts two value types in front of consumers: the engine's and the API's. + Measured 7.34 ms against 5.13 ms on a 50,000-row drain. Kept as the fallback + if the `Arc` change is ever judged too invasive on principle, since the + measured case against it is the only case against it. +- **Handing back encoded record bytes** and making the consumer decode. No + per-value copies, but it moves the record format into the public API and + pushes decoding onto every consumer. +- **`Arc` for the pager too**, unifying the story. Rejected: that is the change + ADR-0013 and ADR-0017 actually considered and refused, and nothing here + disturbs their reasoning. The read path shares a page source across cursors + on one thread; the connection boundary is a different problem with a + different answer. + +## Consequences + +- `Value` is `Send + Sync` and consumers may move rows between threads. The + embedding API's worker-thread design (spec 013/Req 4) becomes implementable + without a second value type. +- Cloning a `Value` now costs an atomic increment rather than a non-atomic one. + Measured as unobservable on the read path, but the spike's noise floor is a + few percent, so **this should be re-measured on `tests/performance/engine.rs` + against the pinned oracle before the figure is quoted anywhere as settled.** +- ADR-0034 (PR #678) needs amending or superseding on its Req 4 rationale: the + `Send` obstacle is not only the pager. That is #678's to fix, not this ADR's. +- Anything that reconstructs a `Value` from a shared buffer now needs `Arc` + semantics; `src/record/decode.rs` and the sorter/hash-aggregation paths were + the only such sites and are updated here. +- ADR numbering is contended across three unmerged branches: `feat/683` claims + 0038, this claims 0039, and PR #678's claims whatever is free at its merge. + Numbers are only settled by merge order, so the last to land renumbers. diff --git a/.openspec/adr/index.md b/.openspec/adr/index.md index cb18749d..283d0eff 100644 --- a/.openspec/adr/index.md +++ b/.openspec/adr/index.md @@ -41,3 +41,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way** | [0035](0035-wal-resume-hint-cache-supersedes-0026.md) | `Pager`-cached WAL resume hint supersedes ADR-0026's per-flush rescan | 2026-08-29 | | [0036](0036-pragma-synchronous-fsync-policy.md) | `PRAGMA synchronous` fsync-skip policy, and why `SynchronousMode` lives in `header.rs` | 2026-08-29 | | [0037](0037-macos-plain-fsync-not-fullfsync.md) | On macOS, `Vfs::sync` calls plain `fsync(2)`, not `std`'s `F_FULLFSYNC` | 2026-08-30 | +| [0039](0039-value-payloads-are-arc-not-rc.md) | `Value`'s text and blob payloads are `Arc`, not `Rc` | 2026-09-04 | diff --git a/src/record/decode.rs b/src/record/decode.rs index d29c3105..d462e533 100644 --- a/src/record/decode.rs +++ b/src/record/decode.rs @@ -1,6 +1,6 @@ // Copyright 2026 Schuberg Philis // SPDX-License-Identifier: Apache-2.0 -use std::rc::Rc; +use std::sync::Arc; use super::error::RecordError; use super::value::{TextEncoding, Value}; @@ -389,17 +389,17 @@ pub(crate) fn decode_serial_value( } } -/// Decodes text bytes straight into `Rc`. The UTF-8 case (by far the -/// common one) builds the `Rc` directly from the validated byte slice +/// Decodes text bytes straight into `Arc`. The UTF-8 case (by far the +/// common one) builds the `Arc` directly from the validated byte slice /// instead of routing through an intermediate `String`, avoiding a second /// allocation and copy per text column. -fn decode_text(bytes: &[u8], encoding: TextEncoding) -> Result, RecordError> { +fn decode_text(bytes: &[u8], encoding: TextEncoding) -> Result, RecordError> { match encoding { TextEncoding::Utf8 => std::str::from_utf8(bytes) - .map(Rc::from) + .map(Arc::from) .map_err(|_| RecordError::InvalidUtf8), - TextEncoding::Utf16Le => decode_utf16(bytes, u16::from_le_bytes).map(Rc::from), - TextEncoding::Utf16Be => decode_utf16(bytes, u16::from_be_bytes).map(Rc::from), + TextEncoding::Utf16Le => decode_utf16(bytes, u16::from_le_bytes).map(Arc::from), + TextEncoding::Utf16Be => decode_utf16(bytes, u16::from_be_bytes).map(Arc::from), } } diff --git a/src/record/value.rs b/src/record/value.rs index 3d1c7778..15068acd 100644 --- a/src/record/value.rs +++ b/src/record/value.rs @@ -1,6 +1,6 @@ // Copyright 2026 Schuberg Philis // SPDX-License-Identifier: Apache-2.0 -use std::rc::Rc; +use std::sync::Arc; /// A single decoded column value, per SQLite's dynamic type system. #[derive(Debug, Clone, PartialEq)] @@ -12,11 +12,23 @@ pub enum Value { /// An 8-byte IEEE 754 floating-point value. Real(f64), /// A text value, decoded according to the database's `TextEncoding`. - Text(Rc), + Text(Arc), /// An uninterpreted byte sequence. - Blob(Rc<[u8]>), + Blob(Arc<[u8]>), } +/// `Value` must stay `Send + Sync` (#688): the embedding API's +/// connection handle hands result rows to another thread, and a row is +/// a `Vec`. `Rc` payloads made that impossible, and nothing but a +/// compile-time check keeps it from silently regressing — swapping +/// either payload back to `Rc` would otherwise only fail much later, in +/// whichever consumer tried to cross a thread. +/// +/// This is deliberately *not* a claim about `Pager`/`PageSource`, which +/// stay `Rc` per ADR-0013 and ADR-0017. See ADR-0039. +const fn assert_value_send_sync() {} +const _: () = assert_value_send_sync::(); + /// The database's text encoding, from database header byte 56. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum TextEncoding { diff --git a/src/vdbe/cursor.rs b/src/vdbe/cursor.rs index d66e8194..beeb1233 100644 --- a/src/vdbe/cursor.rs +++ b/src/vdbe/cursor.rs @@ -51,6 +51,7 @@ use std::cell::RefCell; use std::collections::{BTreeMap, HashMap}; use std::rc::Rc; +use std::sync::Arc; use crate::btree::{self, IndexCursor, TableCursor}; use crate::record::{ @@ -115,7 +116,7 @@ pub(crate) enum CursorSlot { /// for table cursors. Valid only while `cached_blob` still /// points at the same allocation as `register`'s current value. header_cache: RowHeaderCache, - cached_blob: Option>, + cached_blob: Option>, }, Sorter(crate::vdbe::sorter::SorterState), /// A hash-aggregation table (#570) — opened by `HashAggOpen`, the @@ -586,9 +587,9 @@ fn normalize_key_values(values: &[Value], collations: &[Collation]) -> Vec { - Value::Text(Rc::from(s.to_ascii_lowercase().as_str())) + Value::Text(Arc::from(s.to_ascii_lowercase().as_str())) } - (Value::Text(s), Collation::RTrim) => Value::Text(Rc::from(s.trim_end_matches(' '))), + (Value::Text(s), Collation::RTrim) => Value::Text(Arc::from(s.trim_end_matches(' '))), _ => v.clone(), }) .collect() @@ -960,7 +961,7 @@ fn read_row_column( cached_blob, .. } => { - if !cached_blob.as_ref().is_some_and(|b| Rc::ptr_eq(b, &bytes)) { + if !cached_blob.as_ref().is_some_and(|b| Arc::ptr_eq(b, &bytes)) { header_cache.invalidate(); *cached_blob = Some(bytes.clone()); } @@ -4139,7 +4140,7 @@ mod tests { // ever produce on its own. state.set_current(Some(btree::IndexRow { payload: btree::Payload::Owned(encode_record( - &[Value::Text(Rc::from("not-a-rowid"))], + &[Value::Text(Arc::from("not-a-rowid"))], TextEncoding::Utf8, )), })); diff --git a/src/vdbe/hash_agg.rs b/src/vdbe/hash_agg.rs index 5a509b1a..792440aa 100644 --- a/src/vdbe/hash_agg.rs +++ b/src/vdbe/hash_agg.rs @@ -57,7 +57,7 @@ //! insert), so no query can reach it. use std::collections::HashMap; -use std::rc::Rc; +use std::sync::Arc; use crate::record::{Collation, Value}; use crate::vdbe::affinity::{apply_affinity, Affinity}; @@ -80,7 +80,7 @@ struct GroupSlot { /// observably pick the group's first row, and the sort strategy — /// whose sort is stable, so a group's ties stay in scan order — /// picks the same one. - row: Rc<[u8]>, + row: Arc<[u8]>, /// This group's key values in key order (not record order), /// post-affinity, used only to order the groups at `HashAggRewind`. key_values: Vec, diff --git a/src/vdbe/result.rs b/src/vdbe/result.rs index f3217625..c29797b8 100644 --- a/src/vdbe/result.rs +++ b/src/vdbe/result.rs @@ -5,7 +5,7 @@ //! record serialization (`MakeRecord`, reusing spec 003's on-disk record //! encoding byte-for-byte), and row emission (`ResultRow`). -use std::rc::Rc; +use std::sync::Arc; use crate::record::{encode_record_into, TextEncoding, Value}; use crate::vdbe::affinity::{apply_affinity, Affinity}; @@ -166,7 +166,7 @@ pub fn make_record(vm: &mut Vm, instr: &Instruction) -> Result &mut scratch, &mut encode_scratch, ); - let payload: Rc<[u8]> = Rc::from(scratch.as_slice()); + let payload: Arc<[u8]> = Arc::from(scratch.as_slice()); *vm.record_scratch() = scratch; *vm.encode_scratch() = encode_scratch; *vm.make_record_values_scratch() = values; diff --git a/src/vdbe/sorter.rs b/src/vdbe/sorter.rs index b5d9ab41..b0b5e554 100644 --- a/src/vdbe/sorter.rs +++ b/src/vdbe/sorter.rs @@ -286,7 +286,7 @@ pub fn sorter_insert(vm: &mut Vm, instr: &Instruction) -> Result, Vec); +type SorterRow = (std::sync::Arc<[u8]>, Vec); /// Restores the max-heap property (root = worst row, per `compare_rows`) /// after appending a new element at `buf`'s end — bubbles it up while