From b0450dc7d66249efb151aa6b777e51b183476ff8 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Thu, 3 Sep 2026 16:29:57 +0100 Subject: [PATCH 1/3] perf: resume the ByteStrings fragment hint backward as well as forward 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 #3463 - extends the fragment hint introduced there --- .../apache/pekko/util/ByteStringSpec.scala | 16 ++++++++++++++ .../org/apache/pekko/util/ByteString.scala | 21 ++++++++++++++++--- ...ByteString_byteAtUnchecked_Benchmark.scala | 16 ++++++++++++-- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala b/actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala index 6cb0118ab3..271d982c0e 100644 --- a/actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala +++ b/actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala @@ -2088,6 +2088,22 @@ class ByteStringSpec extends AnyWordSpec with Matchers with Checkers { rope.iterator.toArray should ===(expected) } + "return the right byte when stepping backward onto fragment boundaries" in { + // the backward resume recomputes fragment starts by subtracting lengths, so land exactly + // on the first and last byte of every fragment, from a hint planted at the far end + val freshRope = fragmentLengths + .foldLeft((ByteString.empty, 0)) { case ((acc, offset), len) => + (acc ++ ByteString(expected.slice(offset, offset + len)), offset + len) + } + ._1 + val starts = fragmentLengths.scanLeft(0)(_ + _).init + val lasts = fragmentLengths.scanLeft(0)(_ + _).tail.map(_ - 1) + freshRope(expected.length - 1) should ===(expected(expected.length - 1)) + for (s <- starts.reverse) withClue(s"fragment start $s: ")(freshRope(s) should ===(expected(s))) + freshRope(expected.length - 1) should ===(expected(expected.length - 1)) + for (e <- lasts.reverse) withClue(s"fragment last byte $e: ")(freshRope(e) should ===(expected(e))) + } + "still reject out of range indices" in { an[IndexOutOfBoundsException] should be thrownBy rope(-1) an[IndexOutOfBoundsException] should be thrownBy rope(expected.length) diff --git a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala index 2b672aa7bb..1caa3d62c4 100644 --- a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala +++ b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala @@ -1540,8 +1540,8 @@ object ByteString { else throw new IndexOutOfBoundsException(idx.toString) // Remembers the fragment resolved by the last byteAtUnchecked call, so sequential access -- - // the dominant pattern -- stays on the same fragment or steps to the next one instead of - // rescanning the fragment vector from index 0 for every byte. + // the dominant pattern -- stays on the same fragment or steps to the neighbouring one, in + // either direction, instead of rescanning the fragment vector from index 0 for every byte. // // Packed into a single long so the triple is read and written atomically: the fragment index // in the high 32 bits and its start offset in the low 32. A reader can therefore never pair @@ -1581,12 +1581,27 @@ object ByteString { /** * Scans for the fragment containing `offset` and records it as the hint. `hintIdx` and * `hintStart` are a previously read hint that missed, used to resume the scan from that - * fragment rather than from the start. + * fragment -- forward or backward, whichever side of it `offset` is on -- rather than + * from the start. */ private def resolveFragment(offset: Int, hintIdx: Int, hintStart: Int): Long = { var pos = 0 var seen = 0 if (hintIdx >= 0) { + 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. + pos = hintIdx + seen = hintStart + while (offset < seen) { + pos -= 1 + seen -= bytestrings(pos).length + } + val located = (pos.toLong << 32) | (seen.toLong & 0xFFFFFFFFL) + fragmentHint = located + return located + } val hintEnd = hintStart + bytestrings(hintIdx).length if (offset >= hintEnd && hintIdx + 1 < bytestrings.length) { // moving forward past the remembered fragment: resume the scan from it diff --git a/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala b/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala index 95e2e89325..33b33b3d53 100644 --- a/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala +++ b/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala @@ -64,8 +64,20 @@ class ByteString_byteAtUnchecked_Benchmark { manyFragments_sequential thrpt 3 54838.759 ± 29275.476 ops/s Sequential access is roughly 84x faster. Random access is unchanged (the hint never hits) and - reverse access does not benefit either, since each step lands before the remembered fragment - and falls back to a scan from the start -- both stay within the noise of the previous numbers. + reverse access did not benefit at that point, since each step landed before the remembered + fragment and fell back to a scan from the start -- both stayed within the noise of the + previous numbers. + + After resolveFragment also resumes backward from the remembered fragment (same short run, + same wide error bars): + + manyFragments_reverse thrpt 3 386.052 ± 550.638 ops/s (before, on this machine) + manyFragments_reverse thrpt 3 39969.362 ± 49806.718 ops/s (after) + manyFragments_sequential thrpt 3 43415.035 ± 45223.244 ops/s (before, on this machine) + manyFragments_sequential thrpt 3 44624.286 ± 42770.609 ops/s (after) + + Reverse access is roughly 100x faster and on par with sequential; sequential is unchanged + within the noise. */ private val randomIndices: Array[Int] = { From 1d33182c42d0e58dd3cd31950cec39f0d747a30d Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Mon, 7 Sep 2026 08:06:13 +0100 Subject: [PATCH 2/3] fix: bound the backward fragment walk and correct its cost claim Motivation: Review of #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 #3526 --- .../org/apache/pekko/util/ByteString.scala | 51 +++++++++++++------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala index 1caa3d62c4..c394a3da22 100644 --- a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala +++ b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala @@ -1543,9 +1543,11 @@ object ByteString { // the dominant pattern -- stays on the same fragment or steps to the neighbouring one, in // either direction, instead of rescanning the fragment vector from index 0 for every byte. // - // Packed into a single long so the triple is read and written atomically: the fragment index - // in the high 32 bits and its start offset in the low 32. A reader can therefore never pair - // the index of one fragment with the start of another, which separate int fields would allow. + // Packed into a single long -- the fragment index in the high 32 bits, its start offset in + // the low 32 -- so the two travel together instead of in separate int fields a reader could + // pair across two different updates. JLS 17.7 still permits a non-volatile long to be read + // as two 32-bit halves, so a reader must treat the pair as advisory rather than assume it is + // internally consistent; `resolveFragment` checks the backward walk it drives for that. // The end offset is not stored; it is recomputed as start + fragment.length, which is a // cheap array read. The field is deliberately not volatile: it is only a hint, so a reader // that misses another thread's update simply rescans, and ByteString is immutable, so a @@ -1589,24 +1591,41 @@ object ByteString { var seen = 0 if (hintIdx >= 0) { 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. + // Moving backward before the remembered fragment: walk back from it, keeping `seen` at + // the start of fragment `pos`, so the walk stops at the fragment holding `offset`. That + // is at most `hintIdx` steps -- the same O(fragments) bound per call as the scan from + // fragment 0 it replaces, though 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. + // + // `pos > 0` and the consistency check are not redundant: a torn read of `fragmentHint` + // (see above) can pair `hintIdx` with the `hintStart` of a different fragment, and an + // unbounded walk would then step `pos` off the front of the vector. On a consistent + // pair the walk always ends with `offset >= seen`, since fragment 0 starts at 0 and + // `offset >= 0`; anything else means the hint was torn, so fall through to the scan + // from the start rather than trust it. pos = hintIdx seen = hintStart - while (offset < seen) { + while (pos > 0 && offset < seen) { pos -= 1 seen -= bytestrings(pos).length } - val located = (pos.toLong << 32) | (seen.toLong & 0xFFFFFFFFL) - fragmentHint = located - return located - } - val hintEnd = hintStart + bytestrings(hintIdx).length - if (offset >= hintEnd && hintIdx + 1 < bytestrings.length) { - // moving forward past the remembered fragment: resume the scan from it - pos = hintIdx + 1 - seen = hintEnd + if (offset >= seen) { + val located = (pos.toLong << 32) | (seen.toLong & 0xFFFFFFFFL) + fragmentHint = located + return located + } + pos = 0 + seen = 0 + } else { + val hintEnd = hintStart + bytestrings(hintIdx).length + if (offset >= hintEnd && hintIdx + 1 < bytestrings.length) { + // moving forward past the remembered fragment: resume the scan from it + pos = hintIdx + 1 + seen = hintEnd + } } } var frag = bytestrings(pos) From 206ab86ae135868cdef6691a660de3295995eb41 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Mon, 7 Sep 2026 08:49:41 +0100 Subject: [PATCH 3/3] fix: make the ByteStrings fragment hint pair atomic 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 #3526 --- .../org/apache/pekko/util/ByteString.scala | 60 ++++++++----------- ...ByteString_byteAtUnchecked_Benchmark.scala | 17 ++++++ 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala index c394a3da22..b948a84706 100644 --- a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala +++ b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala @@ -1543,19 +1543,22 @@ object ByteString { // the dominant pattern -- stays on the same fragment or steps to the neighbouring one, in // either direction, instead of rescanning the fragment vector from index 0 for every byte. // - // Packed into a single long -- the fragment index in the high 32 bits, its start offset in - // the low 32 -- so the two travel together instead of in separate int fields a reader could - // pair across two different updates. JLS 17.7 still permits a non-volatile long to be read - // as two 32-bit halves, so a reader must treat the pair as advisory rather than assume it is - // internally consistent; `resolveFragment` checks the backward walk it drives for that. - // The end offset is not stored; it is recomputed as start + fragment.length, which is a - // cheap array read. The field is deliberately not volatile: it is only a hint, so a reader + // Packed into a single long -- the fragment index in the high 32 bits, its start offset in the + // low 32 -- and volatile so that the two always come from the same write. JLS 17.7 lets a + // non-volatile 64-bit field be read as two 32-bit halves, which would pair the index of one + // fragment with the start of another; the fast path below tests that pair against the wrong + // fragment's length and can pass, returning a byte from the wrong fragment. + // + // Volatile is for that atomicity, not for ordering: the value remains only a hint, so a reader // that misses another thread's update simply rescans, and ByteString is immutable, so a - // resolved mapping never becomes wrong. - private[this] var fragmentHint: Long = ByteStrings.NoHint + // resolved mapping never becomes wrong. Reads are the hot path; the write happens only when a + // lookup misses, which is once per fragment crossed rather than once per byte. + // + // The end offset is not stored; it is recomputed as start + fragment.length, a cheap array read. + @volatile private[this] var fragmentHint: Long = ByteStrings.NoHint private[pekko] override def byteAtUnchecked(offset: Int): Byte = { - val hint = fragmentHint // single read: index and start below are mutually consistent + val hint = fragmentHint // single read: index and start below come from the same write val hintIdx = (hint >>> 32).toInt val hintStart = hint.toInt if (hintIdx >= 0) { @@ -1573,7 +1576,7 @@ object ByteString { * the hint uses. `offset` must be within this ByteString. */ private def locateFragment(offset: Int): Long = { - val hint = fragmentHint // single read: index and start below are mutually consistent + val hint = fragmentHint // single read: index and start below come from the same write val hintIdx = (hint >>> 32).toInt val hintStart = hint.toInt if (hintIdx >= 0 && offset >= hintStart && offset - hintStart < bytestrings(hintIdx).length) hint @@ -1591,34 +1594,23 @@ object ByteString { var seen = 0 if (hintIdx >= 0) { if (offset < hintStart) { - // Moving backward before the remembered fragment: walk back from it, keeping `seen` at - // the start of fragment `pos`, so the walk stops at the fragment holding `offset`. That - // is at most `hintIdx` steps -- the same O(fragments) bound per call as the scan from - // fragment 0 it replaces, though 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. - // - // `pos > 0` and the consistency check are not redundant: a torn read of `fragmentHint` - // (see above) can pair `hintIdx` with the `hintStart` of a different fragment, and an - // unbounded walk would then step `pos` off the front of the vector. On a consistent - // pair the walk always ends with `offset >= seen`, since fragment 0 starts at 0 and - // `offset >= 0`; anything else means the hint was torn, so fall through to the scan - // from the start rather than trust it. + // Moving backward before the remembered fragment: walk back from it, keeping `seen` at the + // start of fragment `pos`. The hint pair is atomic (see above), so `seen` is a real + // fragment start; fragment 0 starts at 0 and `offset >= 0`, so the walk stops at fragment + // 0 at the latest. At most `hintIdx` steps -- the same O(fragments) bound per call as the + // from-zero scan it replaces, but not always fewer steps than it, since a far-end hint + // with a near-zero offset walks the whole way back. That case is random access, which + // neither strategy serves; backward sequential access, the case this is for, costs one + // step. Hence no distance heuristic. pos = hintIdx seen = hintStart - while (pos > 0 && offset < seen) { + while (offset < seen) { pos -= 1 seen -= bytestrings(pos).length } - if (offset >= seen) { - val located = (pos.toLong << 32) | (seen.toLong & 0xFFFFFFFFL) - fragmentHint = located - return located - } - pos = 0 - seen = 0 + val located = (pos.toLong << 32) | (seen.toLong & 0xFFFFFFFFL) + fragmentHint = located + return located } else { val hintEnd = hintStart + bytestrings(hintIdx).length if (offset >= hintEnd && hintIdx + 1 < bytestrings.length) { diff --git a/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala b/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala index 33b33b3d53..cbb787552c 100644 --- a/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala +++ b/bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_byteAtUnchecked_Benchmark.scala @@ -78,6 +78,23 @@ class ByteString_byteAtUnchecked_Benchmark { Reverse access is roughly 100x faster and on par with sequential; sequential is unchanged within the noise. + + Making the hint volatile, so that its index and start cannot be read as two halves of + different writes (JLS 17.7), costs nothing measurable outside the one-byte-fragment + sequential case. Paired run toggling only the `@volatile` keyword, -f2 -wi 5 -i 5, with + manyFragments_map -- which walks the fragments directly and never consults the hint -- as + an in-run control: + + plain long volatile + manyFragments_map 143209.374 ± 8645.210 139763.871 ± 9327.740 ops/s + manyFragments_random 911.739 ± 76.206 901.787 ± 154.965 ops/s + manyFragments_reverse 43961.948 ± 9780.313 42858.330 ± 949.351 ops/s + manyFragments_sequential 48989.773 ± 6972.720 41100.395 ± 3682.974 ops/s + + Random and reverse move by about as much as the control (~2%); only sequential shows a + possible cost, and its intervals barely separate. These 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. */ private val randomIndices: Array[Int] = {