feat: make Value Send by switching Text/Blob payloads to Arc (#688) - #690
Open
dpsiderius wants to merge 2 commits into
Open
feat: make Value Send by switching Text/Blob payloads to Arc (#688)#690dpsiderius wants to merge 2 commits into
dpsiderius wants to merge 2 commits into
Conversation
`Value::Text(Rc<str>)`/`Blob(Rc<[u8]>)` made `Value` itself `!Send`, and a result row is a `Vec<Value>`, 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<dyn PageSource>` and `Rc<RefCell<Pager>>` 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) <noreply@anthropic.com>
# Conflicts: # .openspec/adr/index.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Value::Text(Rc<str>)andValue::Blob(Rc<[u8]>)(
src/record/value.rs:15-17) madeValueitself!Send. A result row is aVec<Value>, so no query result could cross a thread boundary at all —which blocks spec 013/Req 4's
Send + Syncconnection handle outright.Switches both payloads to
Arc, and adds a compile-time assertion so theproperty cannot regress silently.
Why this does not contradict ADR-0013 or ADR-0017
It looks like it does, and that is the whole reason ADR-0039 exists.
Both of those ADRs rejected
Arcfor the pager, on the grounds that oneVmshares a page source across N cursors via cheapRcclones, so atomicswould tax the Tier 0 read path.
Value's payloads were never their subject.Rc<dyn PageSource>andRc<RefCell<Pager>>are untouched here, so bothADRs' actual subject matter is unaffected.
The natural reading of those two ADRs is "
Arcanywhere is settled against",and without ADR-0039 the next person would either re-litigate this or assume
it was an oversight. Note that spec 013 and ADR-0034 both attribute the
Sendproblem to the pager alone — neither mentions
Value, which is the part thatactually blocks Req 4.
Spike 014 (#682) measured this rather than arguing it
Value::Text(s.to_string().into()), and.into()is identical forRc<str>andArc<str>, so only the ~12 sites that name the type neededediting (plus 4 orphaned imports).
full_drain/batchis single-threaded withno boundary, so it isolates the atomics tax:
Rcruns spanned5.42–5.62 ms,
Arcruns 5.24–5.33 ms.ran 5.13 ms against 7.34 ms for the owned-copy alternative that
Rcwould have forced.One caveat, carried into the ADR rather than buried: the spike's noise
floor is a few percent, and one comparison showed
Arc5.6% faster — whichis not a credible result from adding atomics. The honest reading is that the
difference is inside run-to-run variance. The claim is "no measurable cost",
not "faster", and it should be re-measured on
tests/performance/engine.rsagainst the pinned oracle before anyone quotesit as settled.
The assertion is mutation-checked
Reverting either payload to
Rcfails the build with "cannot be sentbetween threads safely", rather than deferring the error to some future
consumer of the API.
Notes for a reviewer
ADR-0039 and feat: streaming Execution primitive — read a result row without materializing the rest #683 carries ADR-0040. (0038 went to feat: private crate registry via JFrog Artifactory (#12) #684.)
make assurancedoes not move — still 86/86 and 276/276. Spec 013 isthe subject of Feat/embedding api spec #678, still
Status: Proposedand unmerged, so Req 4 is notyet in the dashboard's denominator.
CHANGELOG.mdentry or version bump: folded separately per chore: fold #663 into 0.18.9, revert premature 0.18.10 bump #671.Test plan
cargo test --locked— 1562 passed / 0 failed, byte-identicalcount to the
origin/mainRcbaseline measured the same waycargo test --locked --test corpus— 380, unchangedValueisSend + Sync, proven at compile time and mutation-checkedRc<dyn PageSource>/Rc<RefCell<Pager>>unchanged — grep-verifiablein the diff, which touches only
src/record/{value,decode}.rsandsrc/vdbe/{cursor,hash_agg,result,sorter}.rsmake lint,cargo fmt --check,make check-mod-filescleanmake assurance— 86/86, 276/276, no dead links847cb19from a clean detached worktreeSpend: matched the
smallestimate. The care was in the ADR, not thecode — as the ticket predicted.
Refs: 013/Req-4, #682, #678, #683
Closes #688
🤖 Generated with Claude Code