Conversation
Motivation: ByteStrings.resolveFragment resumed the fragment scan from the remembered fragment only when the requested offset was past it; an offset before the remembered fragment rescanned the fragment vector from index 0. Byte-wise backward traversal therefore paid a full prefix scan at every fragment boundary it crossed - O(fragments^2) steps over the whole rope - which is the access pattern of reverseIterator and of lastIndexOfSlice candidate verification. Modification: When a valid hint exists and the offset is before the remembered fragment's start, walk backward from that fragment instead of scanning from index 0. The walk keeps the invariant that `seen` is the start of fragment `pos`, terminates at fragment 0 at the latest, and is never longer than the scan it replaces, so there is no heuristic to tune. The hit test, the forward resume, and the packed-long racy-hint design are unchanged. Result: Backward traversal is O(1) amortised per boundary crossing, matching forward. Measured with the existing benchmark (numbers recorded in the benchmark file): manyFragments_reverse goes from 386 ops/s to 39969 ops/s, on par with sequential, which is unchanged within the noise. Tests: - sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec" - 244 passed - the existing byteAtUnchecked block already covers backward, alternating and concurrent access; a new test lands exactly on the first and last byte of every fragment from a far-end hint, the arithmetic the backward walk recomputes - bench-jmh ByteString_byteAtUnchecked_Benchmark manyFragments_reverse / _sequential run before and after on the same machine - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" "bench-jmh/scalafmtCheckAll" - clean References: Refs apache#3463 - extends the fragment hint introduced there
| if (offset < hintStart) { | ||
| // moving backward before the remembered fragment: walk back from it. `offset >= 0` | ||
| // and fragment 0 starts at 0, so the walk stops at fragment 0 at the latest, and it | ||
| // is never longer than the scan from fragment 0 it replaces. |
There was a problem hiding this comment.
never longer than the scan from fragment 0 it replaces isn't true. With the hint at fragment N-1 (say, after a forward pass to the end) and the next call being apply(0), this walk does N-1 steps while the from-zero scan it replaces does 0. It is still bounded by N steps per call, so nothing is asymptotically worse — but the comment asserts an invariant that does not hold, and that assertion is what justifies skipping a distance heuristic.
| // is never longer than the scan from fragment 0 it replaces. | ||
| pos = hintIdx | ||
| seen = hintStart | ||
| while (offset < seen) { |
There was a problem hiding this comment.
The loop has no lower bound on pos. fragmentHint is a non-volatile Long, and JLS 17.7 allows a non-volatile 64-bit field to be read as two separate 32-bit reads, so hintIdx and hintStart can come from different writes despite the packing. The forward path merely rescans in that case; this one drives pos below 0 and bytestrings(pos) throws IndexOutOfBoundsException. while (pos > 0 && offset < seen) costs nothing and degrades to a from-zero scan instead.
(The torn-read hazard itself predates this PR — the fast path in byteAtUnchecked would already return a wrong byte from an inconsistent pair.)
There was a problem hiding this comment.
I pushed a fix for this and made the hint volatile
Motivation: Review of apache#3526 raised two problems with the backward resume in `ByteStrings.resolveFragment`. The comment claimed the backward walk "is never longer than the scan from fragment 0 it replaces". That is false: reaching fragment k from a hint at fragment h costs h - k steps where the from-zero scan costs k, so a hint at the last fragment followed by `apply(0)` walks N-1 steps against the scan's 0. The false invariant was also the stated justification for having no distance heuristic. The walk had no lower bound on `pos`. `fragmentHint` is a non-volatile long and JLS 17.7 permits it to be read as two 32-bit halves, so `hintIdx` and `hintStart` can come from different writes despite the packing. The forward path tolerates such a pair by rescanning; the backward walk instead stepped `pos` below 0 and indexed the fragment vector out of bounds. Modification: Bound the walk with `pos > 0` and check `offset >= seen` on exit. A consistent pair always satisfies that check, since fragment 0 starts at 0 and `offset >= 0`; a torn pair does not, and now falls through to the scan from the start instead of being trusted. The `pos > 0` guard alone would not do: it would return `(pos = 0, seen > 0)`, from which `byteAtUnchecked` computes a negative index into fragment 0. State the real bound in the comment -- at most `hintIdx` steps, the same O(fragments) per-call bound as the scan it replaces but not always fewer steps than it -- and give the actual reason there is no distance heuristic: the case that loses is random access, which neither strategy serves, while backward sequential access costs one step whatever the fragment count. Also correct the `fragmentHint` comment, which claimed the packing made the pair atomic; it only keeps the halves from being paired across separate updates where the 64-bit access is itself atomic. Result: The backward resume cannot index outside the fragment vector, and the comments state bounds that hold. Performance is unchanged: the added guard and check are one comparison each on a path that already walks fragments. Tests: - `sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec"` -- 244 passed - The existing backward-boundary test already covers the walk's longest case, a far-end hint followed by offset 0. The torn-read path has no directional test: `fragmentHint` is `private[this]`, so a spec cannot poison it deterministically; the multi-threaded `byteAtUnchecked` block is the only cover. - `sbt "actor/scalafmtCheckAll"` -- clean - MiMa left to the `Check / Binary Compatibility` job; `resolveFragment` is a `private` method on an `@InternalApi` class. References: Refs apache#3526
Motivation: `ByteStrings.fragmentHint` packs a fragment index and that fragment's start offset into one non-volatile long. JLS 17.7 permits a non-volatile 64-bit field to be read as two 32-bit halves, so the index can come from one write and the start from another. `byteAtUnchecked`'s fast path then tests `offset` against the wrong fragment's length, and that test can pass: the call returns a byte from the wrong fragment. Concurrent readers of one ByteString are ordinary in Pekko -- a broadcast or pub-sub fan-out has several consumers reading the same instance, and every read updates the hint -- so this is a plain wrong-answer bug, not a theoretical one. The packing was introduced to keep the pair together; the comment claimed it made the pair atomic, which the packing alone does not do. Modification: Make `fragmentHint` volatile, which is what actually makes a 64-bit read atomic. Volatile is for atomicity here, not ordering: the value is still only a hint, so a reader that misses another thread's update just rescans, and ByteString is immutable, so a resolved mapping never becomes wrong. With the pair guaranteed internally consistent, the `pos > 0` bound and the `offset >= seen` check added to the backward walk in the previous commit are provably unreachable, so the walk returns to the plain loop: `seen` is always a real fragment start, fragment 0 starts at 0 and `offset >= 0`, so the walk stops at fragment 0 at the latest. This is a stronger fix for the same hazard -- the bound stopped the walk from stepping off the front, but a torn pair would still have produced a wrong byte on the fast path, which never reaches the walk. Result: No reader can pair one fragment's index with another's start. Cost lands on the write, which happens only when a lookup misses -- once per fragment crossed, not once per byte -- while the hit path stays a plain read on x86. Tests: - `sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec"` -- 244 passed - No directional test for the torn read: `fragmentHint` is `private[this]`, so a spec cannot poison it deterministically, and on a 64-bit VM the tear does not reproduce. The multi-threaded `byteAtUnchecked` block remains the only cover. - `ByteString_byteAtUnchecked_Benchmark`, paired run toggling only the `@volatile` keyword, -f2 -wi 5 -i 5, with manyFragments_map as an in-run control (it walks fragments directly and never consults the hint). Random and reverse move by about as much as the control (~2%); only sequential shows a possible cost, and its intervals barely separate. Numbers recorded in the benchmark file. Those 1024 one-byte fragments are the worst case, writing the hint on every access. - `sbt "actor/scalafmtCheckAll" "bench-jmh/scalafmtCheckAll"` -- clean - MiMa left to the `Check / Binary Compatibility` job; the field is `private[this]` on an `@InternalApi` class, and ByteStrings serialises through SerializationProxy, so the added modifier cannot reach the wire. References: Refs apache#3526
Motivation
ByteStrings.resolveFragmentresumes the fragment scan from the remembered fragment onlywhen the requested offset is past it; an offset before the remembered fragment rescans
the fragment vector from index 0. Byte-wise backward traversal therefore pays a full
prefix scan at every fragment boundary it crosses — O(fragments²) steps over the whole
rope. That is the access pattern of
reverseIterator(the inherited IndexedSeqimplementation drives
applywith descending indices) and oflastIndexOfSlicecandidate verification, whose candidate windows move backward through the rope.
The benchmark comment in
ByteString_byteAtUnchecked_Benchmarkhas documented thisasymmetry since the hint was introduced in #3463: "reverse access does not benefit
either, since each step lands before the remembered fragment and falls back to a scan
from the start".
Review turned up a second problem, in the hint itself.
fragmentHintpacks a fragmentindex and that fragment's start offset into one non-volatile long. JLS 17.7 permits a
non-volatile 64-bit field to be read as two 32-bit halves, so the index can come from one
write and the start from another.
byteAtUnchecked's fast path then tests the offsetagainst the wrong fragment's length, and that test can pass: the call returns a byte from
the wrong fragment. Concurrent readers of one
ByteStringare ordinary in Pekko — abroadcast or pub-sub fan-out has several consumers reading the same instance, and every
read updates the hint — so this is a wrong-answer bug, not a theoretical one. The packing
was introduced to keep the pair together; its comment claimed that made the pair atomic,
which packing alone does not do.
Modification
When a valid hint exists and the offset is before the remembered fragment's start,
resolveFragmentnow walks backward from that fragment instead of scanning from index 0.The walk keeps the invariant that
seenis the start of fragmentpos, and terminates atfragment 0 at the latest, since fragment 0 starts at 0 and
offset >= 0.The walk costs at most
hintIdxsteps — the same O(fragments) bound per call as thefrom-zero scan it replaces, but not always fewer steps than it: a hint near the end
paired with an offset near the start walks further than a from-zero scan would. No distance
heuristic guards against that, because it is the random-access pattern that neither
strategy serves, while backward sequential access — what this branch is for — costs one
step whatever the fragment count.
fragmentHintis now volatile, which is what actually makes a 64-bit read atomic. Volatileis for that atomicity, not for ordering: the value is still only a hint, so a reader that
misses another thread's update simply rescans, and
ByteStringis immutable, so a resolvedmapping never becomes wrong. The cost lands on the write, which happens only when a lookup
misses — once per fragment crossed, not once per byte — while the hit path stays a plain
read on x86.
The hit test (already direction-agnostic) and the forward resume are unchanged.
Result
Backward traversal is O(1) amortised per boundary crossing, matching forward, and no reader
can pair one fragment's index with another's start.
Measured with the existing benchmark on one machine (short run, wide error bars, recorded in
the benchmark file per its convention):
manyFragments_reversemanyFragments_sequentialReverse access is roughly 100x faster and on par with sequential; sequential is unchanged
within the noise.
Making the hint volatile costs nothing measurable outside the one-byte-fragment sequential
case. Paired run toggling only the
@volatilekeyword,-f2 -wi 5 -i 5, withmanyFragments_map— which walks the fragments directly and never consults the hint — as anin-run control:
manyFragments_map(control)manyFragments_randommanyFragments_reversemanyFragments_sequentialRandom and reverse move by about as much as the control (~2%); only sequential shows a
possible cost, and its intervals barely separate. Those 1024 one-byte fragments are the
worst case for it, since the hint is then written on every single access — at realistic
fragment sizes the write is amortised over the whole fragment.
Tests
sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec"— 244 passedByteStrings.byteAtUncheckedblock already exercises backward, alternatingand multi-threaded access. One new test lands exactly on the first and last byte of every
fragment from a far-end hint — the boundary arithmetic the backward walk recomputes,
including its longest case, a far-end hint followed by offset 0.
fragmentHintisprivate[this], so a spec cannotpoison it deterministically, and the tear does not reproduce on a 64-bit VM. The
multi-threaded block above remains the only cover.
manyFragments_reverse/manyFragments_sequentialfromByteString_byteAtUnchecked_Benchmarkrun before and after on the same machine, plus thepaired volatile comparison above; the benchmark file's results comment records both.
sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" "bench-jmh/scalafmtCheckAll"— cleanCheck / Binary Compatibilityjob:resolveFragmentis aprivatemethod and
fragmentHintaprivate[this]field on an@InternalApiclass, andByteStringsserialises throughSerializationProxy, so the added modifier cannot reachthe wire.
References
Refs #3463 — extends the fragment hint introduced there.