Skip to content

Clone what the caller asks for, not the rest of the node - #14

Merged
pathscale merged 7 commits into
masterfrom
fix/range-batch
Sep 3, 2026
Merged

Clone what the caller asks for, not the rest of the node#14
pathscale merged 7 commits into
masterfrom
fix/range-batch

Conversation

@pathscale

Copy link
Copy Markdown
Owner

range(k..=k).next() cost 212 ns, against 81 ns on 0.0.6. Reported as a
2.6x regression to be undone. It is not one, and the distinction decides what
the fix can be.

0.0.6 is not a valid baseline

It was fast because it was unsound. Its iterator was type Item = &'a T,
produced by transmute::<slice::Iter<'_, T>, slice::Iter<'a, T>> on an iterator
borrowed from a node's mutex guard: a yielded reference outlived the guard the
moment the scan advanced past that node, so iter().collect::<Vec<&T>>() read
storage nothing was holding.

Both steps since were correctness fixes, not regressions:

step cost bought
0.0.6 → 0.0.7 +41% 4052a1d, atomic select-and-lock, closing a split-migration window that silently truncated scans
0.0.7 → 0.0.8 +97% 1ef3098, owned clones instead of transmuted borrows, closing a use-after-free

So there is no route back to 81 ns that keeps the iterator sound, and no reason
to want one.

What was actually avoidable

Owned values are what soundness requires. Cloning a node's entire remainder
to produce one of them is not, and that is what install_front_batch did:

let batch = guard.iter().skip(skip).cloned().collect::<Vec<_>>();

A one-element range cloned every remaining element of whichever node it landed
in, then discarded all but the first.

The batch now starts at four and doubles per install, bounded by what the node
holds. A one-element range clones four; a long scan reaches whole-node batches
after a handful of installs and pays the same total clone count it always did.

range(k..=k).next()   212.1 ns -> 117.1 ns   (1.81x)
lookup_for_select      48.6 ns ->  40.9 ns   (untouched path, shown as a control)

That recovers the whole 1ef3098 step. What remains is 4052a1d, a
different change and not touched here. Restated honestly: this was never a 2.6x
regression against a valid baseline, it was 1.81x of avoidable waste on top of
a legitimate correctness cost
, and the waste is gone.

Correctness of a partial batch

A batch that stops short of a node's end must resume inside that node rather
than step past it, so exhausted_*_node is set only when the batch reached the
end, and the count already taken is recorded so a resume can never take less
than it already has.

Six new tests cover the paths a whole-node batch could not reach, because it
never resumed inside a node: scans across many installs forwards and backwards,
a scan confined to a single node, ranges of every width across the batch
boundaries, single-element ranges, and a double-ended scan that must remain a
partition.

They were checked against broken code. Marking a node exhausted when the batch
stopped short fails six tests, two of which predate this change.

One thing deliberately unproven

The recorded take count also guards a concurrent stall: a repositioned node can
rank the cursor below elements already yielded, the yield path drops the batch
as duplicates, and the next install computes the same skip forever.

Removing that guard leaves the whole suite green, including
a_scan_under_concurrent_mutation_terminates, which was written to reach it and
does not. It is kept as insurance against a non-terminating scan, which is the
worst failure this iterator can have, and it costs one max. The code says it
is unproven rather than claiming coverage. If you can build the interleaving
that needs it, that test is worth more than the comment.

Second commit: the documentation compiles again

104 of 107 doctests had been failing since the crate was renamed. They say
use indexset::.., which is how every consumer refers to this crate
(indexset = { package = "WorkTablesIndex", .. }), but with no [lib] section
the library took the package's name and indexset resolved to nothing inside
the crate.

[lib] name = "indexset" restores the identity the fork was published with.
Consumers that rename are unaffected, which is all of them. The benches had
drifted the other way and imported WorkTablesIndex::, so the repository
disagreed with itself about its own name; they now match.

Three doctests failed for a real reason rather than the rename and are corrected
rather than deleted: remove takes both arguments by reference while the
examples passed them by value, and iter yields owned values since the
borrow-transmuting iterator was removed, so there was nothing left to
dereference.

107 of 107 pass. 110 unit tests pass, clippy -D warnings and cargo fmt
are clean.

Note on upstream

indexset 0.15.0 upstream still carries the same type Item = &'a T plus
transmute pattern this fork removed in 0.0.8, verified by inspection. WorkTable
depends on it directly as vanilla_indexset and exposes it as the using indexset backend.

I could not turn that into a demonstrated bug in our code: WorkTable's
range_values clones each item as it is yielded, before the iterator advances
and drops that node's guard, so it never holds a reference across a node
boundary. Treat it as a footgun in the upstream API rather than a live defect
here, and note that a Miri run aborts on an unrelated pre-existing
crossbeam-skiplist violation before it can reach the question.

meh added 3 commits September 2, 2026 18:20
A range that yields one value cloned every remaining element of the node
it landed in. `range(k..=k).next()` cost 212 ns against 81 ns on 0.0.6,
and the gap had been read as a regression to be undone.

It is not, and the distinction decides what the fix can be. 0.0.6 was
fast because it was unsound: its iterator was `type Item = &'a T`,
produced by `transmute::<slice::Iter<'_, T>, slice::Iter<'a, T>>` on an
iterator borrowed from a node's mutex guard, so a reference outlived the
guard as soon as the scan advanced past that node. `1ef3098` fixed that
by yielding owned clones, and `4052a1d` before it closed a
split-migration window that silently truncated scans. Both steps bought
correctness. There is no going back to 81 ns while remaining sound, and
no reason to want to.

What was avoidable is *eager* cloning. Owned values are what soundness
needs; cloning a whole node to produce one of them is not. The batch now
starts at four elements and doubles per install, bounded by what the node
holds, so a one-element range clones four and a long scan reaches
whole-node batches after a handful of installs and pays the same total it
always did.

    range(k..=k).next()   212.1 ns -> 117.1 ns   (1.81x)
    lookup_for_select      48.6 ns ->  40.9 ns   (untouched path)

That recovers the whole `1ef3098` step. What remains is `4052a1d`, a
different change and not touched here.

A batch that stops short of a node's end has to resume inside it, so
`exhausted_*_node` is set only when the batch reached the end, and the
count already taken is recorded so a resume cannot take less than it
already has. Six new tests cover the partial-batch paths: scans across
many installs forwards and backwards, a scan confined to one node, ranges
of every width across the batch boundaries, single-element ranges, and a
double-ended scan that must remain a partition. Marking a node exhausted
when the batch stopped short fails six tests, two of which predate this
change.

One thing is deliberately unproven and says so in the code: the recorded
take count also guards a concurrent stall, where a repositioned node
ranks the cursor below elements already yielded. Removing that guard
leaves the whole suite green, including the concurrent stress test
written to reach it. It is kept as insurance against a non-terminating
scan, and annotated as unproven rather than as covered.
104 of 107 doctests had been failing since the crate was renamed. They
say `use indexset::..`, which is how every consumer refers to this crate
(`indexset = { package = "WorkTablesIndex", .. }`), but with no `[lib]`
section the library was named after the package, so inside the crate
`indexset` resolved to nothing.

Setting `[lib] name = "indexset"` restores the identity the fork was
published with: it is a fork of `indexset` under a different package name
to avoid colliding on the registry, and it is consumed under the original
name everywhere. Consumers that rename are unaffected, which is all of
them.

The benches had drifted the other way, importing `WorkTablesIndex::`, so
the repository disagreed with itself about its own name. They now match
the doctests.

Three doctests were failing for a real reason rather than the rename, and
are corrected rather than deleted: `remove` takes both arguments by
reference and the examples passed them by value, and `iter` yields owned
values since the borrow-transmuting iterator was removed, so there was
nothing left to dereference.

107 of 107 pass.
Naming the library `indexset` changed the import paths in the benches,
and those edits went in unformatted, so `cargo fmt --all --check` fails
on this branch while master is clean. That is this branch's doing, not
inherited.
meh added 3 commits September 3, 2026 10:40
0.0.9 is published, and the publish job skips when the version already
exists, so everything on this branch stayed on the shelf: the range fix,
and the 104 doctests that had not compiled since the library target was
renamed. WorkTable's requirement is a caret at minor granularity, so it
picks this up with no pin to bump.
This reverts commit 9900f7b.
This reverts commit abf7f16.
front_partial and back_partial count positions, and a position is not a
stable cursor under deletion. Removing an element the scan already yielded
shifts the unyielded remainder while the recorded count stays put, so
max(rank, position) lets the stale position win and steps over an element
that was present for the whole scan. That is the one guarantee this
iterator makes.

The value rank is authoritative wherever there is one: it counts the
elements at or below the cursor, so the batch resumes strictly past it. No
duplicates, and progress every time, which is what makes dropping the max
safe. The non-termination it guarded against was a batch that came back
all duplicates and advanced nothing, and a value-ranked resume cannot
produce one. The position still decides before anything has been yielded,
where there is no value to rank against.

Both directions were confirmed defective and are fixed, each with a test
that fails when its own fix alone is reverted. The forward case reproduces
the review's worked example exactly: key 4 skipped after key 0 is removed.

The tests do not use Vec::contains. NodeLike is in scope in this module and
its contains for Vec<T> is a binary search, so it answers nonsense for any
sequence not sorted ascending -- which a backward scan's output is. That
cost an hour and a phantom bug report: an earlier version of the backward
test failed on a complete, correct scan. Clippy suggests exactly that call
and the lint is silenced rather than followed.
@pathscale
pathscale merged commit 8163580 into master Sep 3, 2026
4 checks passed
@pathscale
pathscale deleted the fix/range-batch branch September 3, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant