perf: resume the ByteStrings fragment hint backward as well as forward - #3526
Open
pjfanning wants to merge 1 commit into
Open
perf: resume the ByteStrings fragment hint backward as well as forward#3526pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
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
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.
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".
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, terminates atfragment 0 at the latest (fragment 0 starts at 0 and
offset >= 0, so the branch cannoteven be entered when the hint is fragment 0), and is never longer than the from-zero scan
it replaces — so backward resume is unconditional, with no distance heuristic to tune.
The hit test (already direction-agnostic), the forward resume, and the packed-long
racy-hint design are unchanged; the backward branch writes the same internally-consistent
packed pair.
Result
Backward traversal is O(1) amortised per boundary crossing, matching forward. 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.
Tests
sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec"— 244 passedByteStrings.byteAtUncheckedblock already exercises backward, alternatingand multi-threaded access (those guard correctness — this change cannot be discriminated
behaviourally, only by the measurements above, since the old code was correct but slow).
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.
manyFragments_reverse/manyFragments_sequentialfromByteString_byteAtUnchecked_Benchmarkrun before and after on the same machine; thebenchmark file's results comment is updated with both.
sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" "bench-jmh/scalafmtCheckAll"— cleanCheck / Binary Compatibilityjob; the change is aprivatemethod onan
@InternalApiclass.References
Refs #3463 — extends the fragment hint introduced there.