Do not take a global mutex to learn the free list is empty - #89
Closed
pathscale wants to merge 19 commits into
Closed
Do not take a global mutex to learn the free list is empty#89pathscale wants to merge 19 commits into
pathscale wants to merge 19 commits into
Conversation
added 19 commits
September 2, 2026 14:07
A schema is written down once, in a `worktable!` invocation, and the parser that understands it lived in `worktable_codegen`, which is `proc-macro = true`. A proc-macro crate can export nothing but macros, so every type describing a schema — the columns, the primary key, the indexes, the queries — was unreachable from any other crate however public it was declared. `mod common` was not public at that crate's root either. So anything wanting to *read* a declaration had two options: re-implement the grammar and drift from it, or do without. A diagram, a migration tool, a documentation generator and an editor all want to read one. `lib.rs` has carried `// TODO: Refactor this codegen stuff because it's now too strange.` `model` and `parser` move to `worktable_dsl`, a plain library. Nothing in them changed; the dependencies are the five they already used, none added, none dropped. `worktable_codegen` now depends on it, so there is one grammar rather than a copy that can disagree with the compiler about what a schema means. `name_generator` stays in codegen. It invents Rust identifiers for generated code, which is not the schema language, and generators here define inherent `impl`s on `WorktableNameGenerator` — the orphan rule allows that only in the crate owning the type. I had it in the extracted crate first and the compiler made the same argument the design does. `crate::common::` still resolves, through a thin module that re-exports the new crate, so the 127 paths across 67 files are untouched and the diff stays a move rather than a sweep. An integration test reads a declaration from outside, which is the claim worth pinning: it compiles as its own crate, so it stops building if this ever becomes a proc-macro crate again. It also records a property no caller existed to depend on before. `Columns::columns_map` is a `std::collections::HashMap`, whose iteration order Rust randomises per process; two runs of the same input gave `["answered", "project_id", "id"]` and `["project_id", "answered", "id"]`. The macro never cared, and the parser's own tests collect it into another `HashMap` and assert membership, so nothing noticed. A consumer rendering columns in that order draws a different table every run. `field_positions` already carries the declaration order and is the field to sort by; the test asserts that, and says so, so the next consumer learns it here rather than by shipping the bug.
`pub use worktable_dsl::{Parser, *};` names `Parser` and then re-exports it
again through the glob, and rustc reports the glob as unused: nothing reaches
the shim that way, because every caller goes through `crate::common::model::`
or `crate::common::parser::`.
That is only a warning locally, which is why it survived the extraction. CI
runs `cargo clippy --workspace --all-targets -- -D warnings`, where it is a
build failure.
`worktable_dsl` can parse a declaration, which is half of what a designer needs. The other half is holding one, comparing it, storing it, and writing it back out as text the compiler accepts. `crate::model` cannot do any of that: it is built out of `Ident` and `TokenStream`, which is right for a thing whose job is to become Rust code and wrong for everything else. An `Ident` cannot be serialised or constructed outside a proc-macro context without a `Span::call_site` that lies about where it came from, and a `TokenStream` is not `PartialEq`, so two schemas cannot even be asked whether they differ. That is the one question a migration planner exists to answer. `schema::Schema` is the same declaration with the compiler's concerns removed: `String` for `Ident`, ordered `Vec`s for `HashMap`s, no spans. It derives `PartialEq` so schemas can be diffed, and under an optional `serde` feature it derives `Serialize`, so one can be stored next to the data it describes and read back by a process that never saw the Rust type. The feature is off by default because `worktable_codegen` depends on this crate and is a proc macro: every WorkTable user compiles it for the host before anything else in their build, and they should not pay for a derive macro that serves consumers who are not the compiler. The approach is additive. Nothing in the parser or the generators changed, and the model is untouched apart from three `cfg_attr` derives on plain enums the IR reuses rather than duplicates. Inverting the parser to produce plain data directly would have been a 61-file edit across 13k lines of generators with no test proving the output was unchanged. Two emitters. `to_dsl` renders the declaration body back to text, which is what makes a drawing editable: read, change, write, compile. `to_mermaid` renders UML class notation, chosen because it is text, so it is diffable and needs no rendering dependency, and because it renders anywhere Markdown does. Columns are attributes carrying their markers, queries are operations, and the partition key is a note rather than an attribute because it is stored once per partition and no query can name it. The schema language has no foreign keys, so `infer_relations` guesses links from a single stated naming rule and returns what it guessed; `schemas_to_mermaid` draws those as dependencies rather than associations, because a dashed arrow is the honest notation for a link the declaration does not make. Ordering is a guarantee here, not an accident. `columns_map` is a `HashMap`, whose iteration order Rust randomises, so a consumer walking it draws a different table on every run; `field_positions` records the declaration order and is what the IR sorts by. The query maps have no such field, so those are sorted by name, which is at least stable, and a test says which is which. The round trip is checked against the repository rather than against a fixture. `tests/round_trip.rs` finds all 128 `worktable!` invocations in the tree, sets aside the 12 that are `macro_rules!` templates full of metavariables, and asserts `parse(emit(parse(x))) == parse(x)` for the remaining 116. Those were written by people not thinking about this crate, which makes them a better corpus than anything written here. `codegen` adds the claim this crate cannot make about itself: that emitted text is a declaration the macro accepts. That check has to live on the near side of the proc-macro boundary. Three things the corpus turned up. The emitter writes no comma after `delete`, `in_place` or `config` blocks. `parse_updates` consumes one and those three do not, so a comma there arrives at a dispatch loop as a `,` token and dies as "Unexpected identifier". Omitting it is the only form all of them accept. Expanding one declaration twice does not produce one program. Several generators iterate `columns_map` directly to emit an ordered construct, the `RowFields` and `AvaiableTypes` enums among them, so the variant order differs between two expansions in one process and can differ between two compilations of the same source. `generator_determinism` records it with the evidence and is ignored rather than deleted: the fix changes the generated code of every table and deserves reviewing on its own. It is also why `emitted_declarations` can only assert that an emitted declaration expands, not that it generates identical code.
With a schema as data on both sides, the decision a version mismatch forces can
be computed instead of hand-written. `Diff::between` says what changed, `Cost`
says what applying it costs, and `transforms_required` says which parts a person
still has to write, which are the parts that need intent rather than mechanism.
The cost model is about links, not about fields. A row is addressed by a
`Link { page_id, offset, length }`, and every index holds links, so the question
that decides what a change costs is not how many columns moved but whether a row
is still where it was. Any change to the archived layout invalidates every link
in the table at once and there is no cheaper answer than writing every row
somewhere else. A change to an index invalidates nothing: the rows have not
moved and the index can be rebuilt from them. Hence three tiers and a fourth for
the changes no diff can settle.
That last tier is the point of the exercise. A changed primary key, a changed
partition key, a renamed table and a flipped `persist` are not expensive, they
are underdetermined, and the useful thing a planner can do is say so rather than
guess. The routing key is the clearest case: it is not in the row, so which
partition a row belongs to cannot be recomputed from the row, only from where
the row already is.
The planner invents a value only when there is exactly one it could be. Widening
a column to `optional` has one answer. Narrowing it does not, and neither does
adding a required column or changing a type, so each of those comes back as a
`TransformRequest` naming the column and why. A rename is reported as a drop and
an add, because nothing in a declaration distinguishes it from a deletion beside
an unrelated addition, and guessing by type equality would be wrong exactly when
it mattered.
Three things worth knowing that the tests state as claims. A version bump on its
own costs nothing, which is what keeps bumping cheap enough to be habitual.
Reordering columns is a layout change, because declaration order is the row
struct's field order: it is the change most likely to be made by accident and
least likely to look like one. And a schema that changed without a version bump
is still detected, at the cost of comparing two small structs and reading no
rows, which is the middle branch of the load state machine.
`plan` lifts the same comparison to a set of tables, matching by name because
that is how spaces are matched on disk. A dropped table is `NeedsIntent` rather
than free: whether to delete data is a decision, not a consequence of a
declaration.
A compiled binary could not say what schema it was built against. The
information was there at expansion time and then thrown away, so a migration
planner had no "declared" side to compare against what is on disk, and a
designer could not draw a diagram of an application whose source it did not
have.
Every generated table now carries a `<NAME>_SCHEMA` const, named after
`<NAME>_VERSION` because it answers the question next to it: the version says
which schema, and this says what that schema is.
The stored form is the DSL text rather than a serialised structure. It needs no
format decision, keeps serde out of the dependency graph of every user's build,
is legible in a hex dump, and is read back by the same parser that read the
original. It is also, being a declaration, exactly what regenerates an old table
type, which is the hand-maintained `version_tables: { 1 => v1::UserV1WorkTable }`
that makes migrations something people put off. `dsl/tests/round_trip.rs` holds
the property this rests on against all 116 declarations in this repository, and
the tests here check the emitted const against the declaration it came from and
that the macro accepts it back.
In-memory tables get it too. A designer reading a crate wants every table, and
the const costs a string either way.
Two details worth the words. The second parse runs at the end of `expand` rather
than the start, so this function's diagnostics stay the ones a bad declaration
produces: both parses reject the same inputs, but only one of them knows to say
that a separate `attributes` section is not part of the 1.0 grammar. And the
const is `allow(dead_code)`, because a `worktable!` inside a function body puts
it inside that body, where nothing refers to it and `-D warnings` would fail a
user's build over a const they never asked for.
A designer opening a project, a documentation generator and a migration tool
comparing a checkout against a running database all start from the same problem:
a schema is written inside a `worktable!` invocation somewhere in a crate, and
there is no index of where.
`declarations_in_source` walks tokens rather than `syn`'s item tree, because an
invocation inside a function body is not an item and real code puts them there.
An item walk would quietly miss a table, and the caller would never learn a table
existed to be missed. Both delimiter forms are accepted: the repository uses
`worktable!( .. )` 83 times and `worktable! { .. }` 45 times, and a reader that
took only one would be wrong about the language.
The return is not a `Vec<Schema>`. Some invocations are not declarations: a
`macro_rules!` body writing `name: $name, ... using $backend` is a template whose
metavariables stand for text that exists only after the outer macro expands, and
counting those as failures would be wrong. Everything else that fails to parse is
reported with the text that failed, because a designer that silently drops a
table the compiler accepts is worse than one that says it could not read it.
The corpus round-trip test now goes through this, so it is also the evidence that
the scanner finds what is there: 116 declarations read, 12 templates set aside,
nothing rejected, across the whole repository.
The example on `PartitionSet::pinned` was `ignore`, so it never compiled. It referenced `prices`, `batch` and `tick` without defining them, which is exactly why it could not run - and it is the only documentation the API has. An `ignore` example on a new public method is worse than none: it looks checked, and it rots silently against the very signature it documents. Made self-contained and executable - declares a partitioned table, inserts, then shows the pin-once-read-many shape the surrounding prose argues for.
Four dependencies carried `=` requirements: `data_bucket`, `WorkTablesIndex`, `indexset` and `worktable_codegen`. None of them needs to. `ps-reclaim` is the exception that was never locked. A bare `"0.1.0"` already means `^0.1.0`, so beta.16 picks up 0.1.1 on any fresh resolve. It moves to `"0.1.1"` to raise the floor above the version whose `Guard` was `Send`, which is a soundness bound rather than a lock: a resolve cannot land on the version with the use-after-free window at all. Caret on a `0.0.x` version describes the same set as `=`, so widening `WorkTablesIndex` changes nothing until that crate reaches 0.1.0. Removing the `=` still matters: the file stops implying a constraint it is not expressing. `worktable_codegen` is the one that gives something up. The exact pin held the macro and the runtime it generates calls into in lockstep, and a caret admits later betas. A mismatched pair fails at expansion in a consumer rather than as a resolver conflict here. `docs/TODO.md` moves the two items that were listed as blocking beta.16, and did not stop it, into a section recording how they closed. Re-measuring the partition regression stays open, now against 0.1.1 rather than against the guard size 0.1.1 removes, and the file says plainly which set of `partition_ref` numbers not to reuse and why.
The DSL extraction added a third publishable crate to this workspace and
nothing published it. `worktable_codegen` depends on it by path with a
version requirement, so the first version-bumped merge after the
extraction would have failed the release after green CI:
error: failed to prepare local package for uploading
Caused by:
no matching package named `worktable_dsl` found
location searched: crates.io index
required by package `worktable_codegen`
Reproduced with `cargo publish --dry-run -p worktable_codegen`. The
publish step now walks all three crates in dependency order, each with
its own already-published guard, so adding a fourth is one more line
rather than a rediscovery of this failure.
The three versions also disagreed: dsl at beta.14, codegen at beta.15,
worktable at beta.17. Three crates published from one repo on three
numbers is how a macro and the runtime it generates calls into drift
apart. They now move together at 1.0.0-beta.17.
The two intra-workspace pins go back to `=`. The caret is right for
every external dependency and wrong for these two: a mismatched
macro/runtime pair fails at expansion inside a consumer's build, which
is a worse place to find it than a resolver conflict here. That is the
one concession the caret change conceded in its own description.
… not one
Three of the six block parsers consumed the comma that may follow their
block and three did not. `parse_updates`, `parse_indexes` and
`parse_queries` did; `parse_deletes`, `parse_in_place` and
`parse_configs` did not, so `config: { .. },` reached the top-level
dispatch as a `,` token and died as "Unexpected identifier". Those three
blocks happen to be written last in every declaration in this
repository, which is the only reason nobody had hit it.
It matters now because the designer emits a declaration and parses it
back. An asymmetry between which blocks may carry a comma is a round
trip that fails on the tool's own output as soon as block order changes,
and block order is the sort of thing a visual editor changes freely.
Three `try_parse_comma()` calls, strictly more permissive: every form
that parsed before still parses.
The error message was the other half of the cost. A `,` reported as an
unexpected *identifier* names the wrong category of token and sends the
reader looking for a misspelled keyword. Four dispatch arms now print
the token they saw and the set they expected.
`dsl/tests/trailing_commas.rs` was checked against the pre-fix parser:
three of its five cases go red when the `try_parse_comma()` calls are
reverted. The two that stay green are the ones asserting the fix is
permissive rather than a new rule.
`worktable_dsl` could read a declaration and could not say whether the macro would accept it. The rules lived in `worktable_codegen`, next to the code they would have generated, and a proc-macro crate exports nothing but macros, so the only way to ask was to expand. A designer cannot expand a proc macro, and an editor that finds out by compiling is not an editor. The three validators move here unchanged, still operating on `crate::model` types, which carry spans. `worktable_codegen` calls them here and its diagnostics are identical, down to the span each error points at. Same discipline as the parser extraction, same reason: a second implementation drifts, and the drift shows up as a designer that green-lights a table the compiler rejects. `check()` is the designer's entry point. It returns the schema *and* the diagnostics, because a declaration that breaks a rule is still a declaration and an editor has to draw it in order to let anyone fix it. Each diagnostic carries a stage: `Grammar` means there is nothing to draw, `Rules` means there is a schema and the macro would refuse it. That second state is the normal one in an editor and had no representation at all before this, since `Schema::parse` returns `Ok` for it. It reports every broken rule rather than the first. The macro stops at the first because it will not generate code either way; an editor has the opposite economics, where fix-recompile-find-the-next is the loop a live checker exists to remove. `index_backends_into` therefore collects instead of short-circuiting, and `validate_index_backends` is a thin first-error wrapper over it so the macro's behaviour is unchanged. Spans come back as byte ranges on the diagnostic rather than as fields on the IR. `Schema` stays plain data, serialisable and comparable across processes, which was the whole point of it; the ranges live in the result of the call that produced them, so a consumer that does not want them does not carry them. Byte offsets rather than line and column: an editor converts to whatever it needs, and a range survives being sent to one that disagrees about what a column is. They need `proc-macro2/span-locations`, and this crate is compiled for the host as part of `worktable_codegen` before anything else in a dependent's build. So it is an off-by-default `spans` feature, the same argument that keeps `serde` off. Without it the span is `None` and the messages are identical: a consumer degrades to file-level diagnostics rather than losing them. The span test asserts by slicing the source with the range rather than by comparing offsets. An off-by-one in either direction produces a plausible number and a wrong underline, and only the slice catches that.
The schema language has no foreign keys. A column called `project_id` is a `u64` like any other, and nothing in WorkTable enforces, records, or checks a relationship between two tables. `infer_relations` guesses those links from a naming convention, and the guess is right often enough to be useful and wrong often enough that presenting it as recovered fact would make a designer lie about the schema. So it is not the same kind of thing as the rest of this crate. Everything else here has a single correct answer that WorkTable owns: what the grammar accepts, what the macro rejects, what a schema change costs the storage engine. The mapping between tables belongs next to whatever application enforces the convention, which is not this one. The Mermaid emitter and the relation guessing move behind an off-by-default `uml` feature, and their tests move to a file gated on it. Nothing is deleted: it is written, it is tested, and turning it on is now a decision rather than a default. The crate docs say which layer is which, so the next person building on this does not have to work it out from the absence of a foreign key. That also makes the feature set consistent. `serde`, `spans` and `uml` are all off by default for the same reason: `worktable_codegen` depends on this crate and is a proc macro, compiled for the host before anything else in a dependent's build, so anything unconditional here is paid by every WorkTable user's first compile to serve consumers who are not the compiler.
`delete_many(keys)` and `delete_range(a..b)`, the bulk eviction a consumer needs to bound state growth. Tracked as #78, and the blocker for generational eviction in agentcode, whose durable state grows about 16.5 MB per generation with no way to drop one. Deleting was already the cheap operation the issue asked for: the row is marked deleted in place, its index entries come out, and the storage becomes reusable once no reader can reach it, with vacuum compacting pages later. Nothing about that model needed changing. What was missing was a way to do it to many rows at once. **What batching actually saves.** Not the per-row work: every index still has to lose its entry, and that is most of the cost. What it saves is the fixed cost paid per call, which is one lock acquisition over the whole key set, one grace marker, and one reclaim pass. `Data::delete_many` ghosts every link first and retires them behind a single marker, because a link must not become reusable while a later row in the same batch is still being marked, or a concurrent insert could claim it and be ghosted by this call. Measured on a 50,000-row table, scattered keys, three backends: batching is a flat 1.3 to 1.4x over a loop of `delete` at every batch size from 1 to 100. It does not improve with batch size, and that is the honest shape: the constant fraction is what batches away. **`delete_range` is ergonomic, not faster.** It converges to `delete_many` rather than beating it, and is slightly worse below a batch of 64. Two rounds of optimisation went in before that was clear, and both were worth keeping: it now takes the mutation guards *before* the walk that reads the links, so one `O(log n + k)` walk replaces `k` lookups instead of being added to them, and both paths now read rows at the link they already hold rather than spending a second primary-key lookup in `select`. The remaining per-row work is identical in both, so there is nothing left for a span to exploit. Its value is that the caller does not enumerate the keys. **Not all-or-nothing, unlike `insert_many`.** A rejected insert has published nothing, so unwinding restores a state that was real. A delete that fails partway has already ghosted rows and removed their index entries, and resurrecting them would mean republishing index entries for storage queued for reuse. So the error reports how many succeeded. A key that is not present is skipped rather than failing the batch: a caller evicting a generation cannot know which keys a concurrent writer already removed, and making them find out first is a race they cannot win. The tests were checked against broken code before being trusted, and one had to be rewritten to earn it. `deleted_rows_are_unreachable_through_every_index` originally only read the deleted rows back, which passes with secondary index removal deleted entirely: a ghosted row is filtered out of reads, so a dangling index entry is invisible through `select` until the link is reused. It now reclaims each deleted row's unique value, which a stale unique entry rejects. Skipping secondary removal fails it and the reuse test; skipping primary removal fails five.
`delete` is async and `delete_many`/`delete_range` were not, which is an inconsistency I introduced yesterday rather than one I found. The split has a cause: cell-level locking means an update waits on the readers of the cells it touches, so `update`, `upsert`, `delete` and `reinsert` are async, while `insert` has no existing cell to contend on and the batch paths take the striped mutation gate rather than cell locks. That is a real distinction and a bad thing to expose. A caller cannot be expected to know which writes happen to need a cell lock, and the penalty for guessing wrong is not a compile error: `let _ = table.upsert(row)` builds a future, drops it, and the write never happens. These two await nothing today and say so. They are async because the write surface should have one rule, and because they will need to wait once they take cell locks rather than the striped gate. `insert` and `insert_many` are the remaining exceptions and are not touched here. Making them async does not stop at the API: the macro composes `insert` internally, and `PersistenceTask::push` is synchronous and inserts into WorkTable's own `QueueInner` table, so it reaches into the persistence queue. That is its own change with its own review.
Concurrency coverage existed but was scattered and stopped at four
writers: `nonunique_arctic` races four against an arctic non-unique
index, `index_backends` recovers concurrent same-row updates, `vacuum`
runs a vacuum thread beside sequential inserts. Nothing raced writers
across all three backends, and nothing went above four.
Four is exactly the last writer count at which this engine still looks
healthy. Insert throughput is flat to four and collapses at eight, so
the one dial that would have exposed it was never turned.
Four tests per backend: eight writers losing no rows, inserts racing
deletes so an insert can claim storage a delete just freed, a contended
unique key admitting exactly one writer, and readers seeing consistent
groups while writers run. Congee appears throughout because these need
only a unique index; it has no non-unique backend.
Every shape is a parameter rather than a literal, which is the point:
hardcoding the writer count is how this hid.
WT_CONC_WRITERS=32 WT_CONC_PER_WRITER=2000 cargo test --test mod concurrency
WT_SCALE_SWEEP=1,2,4,8,16,32,64 cargo test --release --test mod insert_throughput -- --ignored
A malformed value is a hard error rather than a silent fallback: a typo
in `WT_CONC_WRITERS` that quietly runs the default is a run you believe
tested something it did not.
`insert_throughput_should_scale_past_four_writers` is `#[ignore]`d
because it fails, deliberately, recording the defect the way
`generator_determinism` records its own. Eight writers reach 0.20x of
single-writer throughput. It is not the index: all three backends
collapse to the same ~300 K/s, and arctic and congee are 1.3x faster
single-threaded before hitting the identical wall. A `sample` of the
eight-writer run puts the time in `RawRwLock::lock_exclusive_slow` and
`DataPages`, and `pages.rs` takes an exclusive write lock on the one
page named by `current_page_id`, so appends serialise by construction.
`EmptyLinkRegistry::pop_max` takes a global mutex on every insert even
when the free list is empty.
It refuses to run in a debug build. Per-operation overhead there swamps
the contention and the sweep reports eight writers as 2.79x *faster*
than one; a throughput assertion that passes for the wrong reason is
worse than none.
`DataPages::insert` calls `EmptyLinkRegistry::pop_max` on every insert, and `pop_max` took a global `FairMutex` before looking at anything. An append-only table therefore paid a contended mutex per row to discover there was nothing to reuse. `sum_links_len` already tracks the aggregate and is maintained on both sides, so one relaxed load answers the question without touching either lock. Release build, 200,000 inserts, best of three, M4 Max: | writers | before | after | | | ---: | ---: | ---: | ---: | | 1 | 1,216,309/s | 1,207,160/s | flat | | 2 | 1,216,010/s | 1,326,333/s | 1.09x | | 4 | 1,154,861/s | 1,425,324/s | 1.23x | | 8 | 310,748/s | 802,077/s | **2.58x** | | 16 | 272,121/s | 557,414/s | 2.05x | Scaling at eight writers goes from 0.26x of single-writer throughput to 0.66x, and four writers now scales positively (1.18x) where it used to degrade. So this mutex, not the page lock, was the dominant term. The page lock is presumably the remaining ceiling: `insert` still takes an exclusive write lock on the one page named by `current_page_id`, which serialises appends by construction, and 0.66x is not 1.0x. `Relaxed` is enough because the answer is a hint rather than an invariant. A push landing concurrently can leave the load reading zero, and the caller then appends a fresh row instead of reusing a link that became available a moment ago: the same outcome as having called one instruction earlier, with the link still registered for the next insert. The reverse cannot happen, since the counter is only non-zero once a link is registered, and the locked path re-checks regardless. The failure this could cause is silent in both directions, which is why it gets its own test rather than relying on the reuse tests. A counter that drifts above zero costs a pointless lock; one that drifts to zero while links remain stops reuse entirely, and inserts would simply append forever, which is a correct way to insert. `the_pop_fast_path_agrees_with_the_registry` pins the counter to the registry across a push/pop cycle, and was checked against both drifts: never decrementing fails it, and never incrementing fails it.
`assert!(!cfg!(debug_assertions))` is a constant assertion and clippy rejects it; a bare `#[cfg] panic!` makes the rest of the function unreachable and its imports unused. Reading the profile through a function keeps one code path in both builds.
They reach the same striped mutation gate by different routes: `insert` gates one key, `insert_many` gates its whole key set and sorts the stripes so a batch and a single insert cannot deadlock. That ordering is a claim worth holding down, and a deadlock shows up as a hang rather than a failure, so the arms interleave on overlapping stripes instead of staying politely apart. The persisted path already had `batches_and_singles_interleave_through_the_engine`; this is the in-memory half, across all three backends.
…e benches Two changes, both about putting things where they belong. **Key widths.** `validate_index_backends` rejects a declaration whose key type the backend cannot serve, and the accepted sets are specific: congee takes u8/u16/u32/u64/usize, arctic takes u16/u32/u64/u128. That list is a promise the macro makes, and nothing tested it. The suite instantiated u64 with both backends and u128 once, so most of the advertised matrix had never been generated. The backends test their own key widths in their own repositories, which is the right place for the data structure and the wrong place for this. What was unverified is not whether arctic handles a u16, it is whether a *generated WorkTable over an arctic u16 index* stores, finds and removes a row. Eleven cells, deliberately shallow: insert, select by that index, a key that must not resolve, then delete. Depth belongs in the backend's suite. All eleven pass, so this adds coverage rather than fixing a bug. **The throughput sweep moves to wt-benchmarks.** It reported a curve rather than asserting a property, which makes it a benchmark, and it had to be `#[ignore]`d to live in a correctness suite at all. It is now `cargo bench --bench insert_scaling` alongside a new `op_latency` bench covering single insert, upsert, delete and select in both storage modes. This file keeps the correctness tests, which is what it is for.
Owner
Author
|
Folded into #87, which is now the single beta.17 PR for this repo. Every commit from this branch is on |
Merged
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
1 participant
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.
Stacks on #88 (which stacks on #87).
DataPages::insertcallsEmptyLinkRegistry::pop_maxon every insert, andpop_maxtook a globalFairMutexbefore looking at anything. An append-onlytable therefore paid a contended mutex per row to discover there was nothing to
reuse.
sum_links_lenalready tracks the aggregate and is maintained on both sides, soone relaxed load answers the question without touching either lock.
Result
Release, 200,000 inserts, best of three, M4 Max:
Scaling at eight writers goes from 0.26x to 0.66x of single-writer
throughput, and four writers now scales positively (1.18x) where it used to
degrade.
So this mutex, not the page lock, was the dominant term. The page lock is
presumably the remaining ceiling:
insertstill takes an exclusive write lockon the one page named by
current_page_id, which serialises appends byconstruction, and 0.66x is not 1.0x. That is a design change and is not
attempted here.
Why
Relaxedis enoughThe answer is a hint, not an invariant. A push landing concurrently can leave
the load reading zero, and the caller then appends a fresh row instead of
reusing a link that became available a moment ago: the same outcome as having
called one instruction earlier, and the link stays registered for the next
insert. The reverse cannot happen, since the counter is only non-zero once a
link is registered, and the locked path re-checks regardless.
The failure mode is silent, so it gets its own test
A counter drifting above zero costs a pointless lock. One drifting to zero
while links remain stops reuse entirely, and inserts would simply append
forever, which is a correct way to insert and would never fail an assertion.
Neither shows up in the existing reuse tests.
the_pop_fast_path_agrees_with_the_registrypins the counter to the registryacross a push/pop cycle, and was checked against both drifts before being
trusted: never decrementing fails it, and never incrementing fails it.
Full suite green: 1020 tests, clippy
-D warnings,cargo fmt.Also here
insertracinginsert_manyacross all three backends. They reach the samestriped gate by different routes,
insert_manysorts its stripes so a batch anda single insert cannot deadlock, and a deadlock would hang rather than fail, so
the arms interleave on overlapping stripes deliberately. The persisted path
already had an equivalent; this is the in-memory half.