Optimize two-part ByteString concatenation#2924
Conversation
|
@pjfanning I think this will help the pekko-grpc case |
|
@hepin this code does not appear to support JDK Serialization of the new class |
|
@He-Pin there are probably a few cases where we use ++ to append 2 ByteStrings but where we could build up the binary data ourselves in a byte array and create a ByteString.fromArrayUnsafe. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a specialized internal ByteString representation for the common case of concatenating exactly two non-empty fragments, reducing allocations/flattening on hot paths and extending benchmarks and tests to validate cross-fragment reads.
Changes:
- Add
ByteString2(two-fragment) to avoid promoting 2-part concatenations intoVector-backed ropes. - Add unchecked byte access + new SWAR helpers to speed up already-bounds-checked primitive reads across fragment boundaries.
- Extend JMH benchmarks and add a targeted spec for cross-boundary primitive reads.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| bench-jmh/src/main/scala/org/apache/pekko/util/ByteString_append_Benchmark.scala | Adds benchmarks for 2-part append, header reads (including cross-boundary), and copy-to-buffer/array. |
| actor/src/main/scala/org/apache/pekko/util/SWARUtil.scala | Adds byte-parameter SWAR helpers for assembling primitives without array access. |
| actor/src/main/scala/org/apache/pekko/util/ByteString.scala | Introduces ByteString2, updates concatenation logic, and adds byteAtUnchecked for optimized internal access. |
| actor/src/main/resources/reference.conf | Registers ByteString2 in serialization bindings. |
| actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala | Adds a test for primitive reads across ByteString2 fragment boundaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the review comments in 3a2bca8:
Local verification passed:
|
|
Follow-up pushed in 147572b:
Local verification passed:
|
|
For me, this is a lot of new code to suit one or 2 use cases where we build a ByteString from precisely 2 underlying byte strings. |
|
Merged current main into this branch in e6e9f56 and resolved the two conflicts:
On the design concern: I left the ByteString2 implementation in place for this draft because the simpler copy-to-compact fast path would trade the Vector allocation for copying payload bytes on header+payload appends. The current approach keeps that use case zero-copy while promoting to ByteStrings as soon as a third fragment is added. If the preferred direction is to keep ByteString smaller and accept the copy for two-fragment appends, I can rework the PR that way. Local verification passed:
All review threads are currently resolved. |
|
Having an extra, 4th, ByteString impl comes with too large a maintenance overhead. Lots more code and lots more tests and easy to miss having test coverage. I don't think there are too many places where we benefit - instances where we concat exactly 2 byte strings that we don't later concat or that have been previously concatenated. |
|
This is for the case pekko-grpc， where body + headers |
I know the use case and it don't think this is worth it. I documented my reasoning in #2924 (comment) |
|
In Scala stdlib there are seq1234 ,map1234 and set1234, we need make it been well tested covered. |
|
@pjfanning Fair maintenance concern — let me put numbers behind the use case so we can decide on data, not vibes. Pushed in c962213:
JDK 25, G1GC, single fork, 5×1s measurement. Numbers are ops/μs (higher is better). Build the frame (the operation pekko-grpc actually does per outgoing message):
Build the frame and read the header (parser hot path):
Build the frame and write to a
Build the frame and
Where bs2 is slower than copy — pre-built downstream reads on the merged result:
This is the honest cost: when every other op is amortized away, contiguous wins because the bounds check / vtable dispatch is cheaper. But the "build, then do one downstream op" workload — which is what real frame producers and consumers do — wins on Re: "I/O dwarfs the difference" — for a streaming RPC at ~1 M small messages/s, the per-frame Happy to drop this if you still don't think the maintenance is worth it — but I wanted to make sure the decision was informed by the post-fix numbers and a reproducible benchmark, since the pre-fix iterator regression made the pre-built ops look much worse than they actually are. |
pjfanning
force-pushed
the
optimize-bytestring-two-part
branch
from
c962213 to
ac2a6a7
Compare
|
@hepin there are commits in this PR for 'stabilising' unrelated tests - can you remove any commits not related to ByteString changes |
|
Yes， will do |
Motivation: Small ByteString concatenation currently promotes two fragments into ByteStrings backed by a Vector, which adds allocation and hurts hot paths that append a header and payload before reading or copying. Modification: Add an internal two-fragment ByteString representation for simple non-empty fragments, flatten it when appending beyond two fragments, and add focused JMH coverage for append, read, copy, and cross-boundary header reads. Result: JMH shows appendTwo allocation dropping from 160 B/op on main to 32 B/op on this branch, with improved read and copy throughput while ByteStringSpec and MiMa pass.
|
the code is a bit outdated as it doesn't take into account some recent ByteString changes for copyToBuffer, endsWith/startsWith and map. pjfanning#51 has suggested changes. |
Motivation: ByteString2.iterator built MultiByteArrayIterator from `LazyList(...)`. That allocates two cons cells plus two thunk closures per iterator, and each call to MultiByteArrayIterator.normalize() walks the LinearSeq and forces a thunk. For the gRPC-shaped "5-byte header ++ payload" hot path the iterator+read overhead dominated, so reading via `iterator.getInt` on the result was ~30x slower than the equivalent `ByteString1C` baseline. Modification: * ByteString2.iterator now uses a plain two-element `List` -- same shape that MultiByteArrayIterator already accepts (LinearSeq[ByteArrayIterator]) but with no thunks and half the allocations. * Add ByteString_concat3way_Benchmark: directional comparison of the two candidate strategies for the hot path (`copy` via fromArrayUnsafe vs `bs2` via ByteString2.apply) across realistic gRPC payload sizes (64B / 1KB / 16KB / 64KB / 256KB), covering concat alone, concat+iterator reads, concat+readIntBE, copyToBuffer, asByteBuffers, drop(5)/take(5), and the same operations on pre-built results. * Add a cross-check test in ByteStringSpec that builds the same logical bytes as `ByteString1C` (reference) and as `ByteString2` with every possible split point, then asserts equivalence across `apply`, full byte-by-byte iteration, `iterator.getShort/Int/Long` (BE+LE) at every offset, `readShortBE/LE`, `readIntBE/LE`, `readLongBE/LE`, `take/drop/takeRight/ dropRight/slice`, `copyToBuffer`, `copyToArray`, `indexOf`, `compact`, equality and hashCode. Result: JMH on JDK 25 G1GC, single fork, 5x1s measurement (ops/us, higher is better): benchmark size before after speedup downRead_bs2 64 16.4 730.8 44x downRead_bs2 1024 27.2 730.8 27x downRead_bs2 16384 25.7 727.5 28x downRead_bs2 65536 24.4 731.0 30x downRead_bs2 262144 18.4 730.6 40x concatRead_bs2 64 27.2 616.0 23x concatRead_bs2 1024 16.2 609.5 38x concatRead_bs2 16384 21.5 608.8 28x concatRead_bs2 65536 22.1 616.7 28x concatRead_bs2 262144 25.7 612.8 24x The previously dominant iterator overhead is gone: ByteString2 read paths are now within ~1.6x of the contiguous `ByteString1C` baseline (down from 30-50x slower), while concat-and-read is several times faster than the copy path because we avoid the System.arraycopy. All 212 ByteStringSpec tests pass including the new cross-check.
He-Pin
force-pushed
the
optimize-bytestring-two-part
branch
from
ae2ae64 to
05fdf87
Compare
…tartsWith/endsWith (#2950) - Add private[pekko] matchesAt to abstract ByteString class (default byte-by-byte impl) - Override matchesAt in ByteString1C and ByteString1 with SWAR 8-byte comparisons - Add optimized copyToBuffer(buffer, offset) to ByteString2 (avoids drop() allocation) - Improve startsWith in ByteString2 to use matchesAt (SWAR-based) instead of byte-by-byte byteAt - Improve endsWith in ByteString2 to use matchesAt (SWAR-based) instead of byte-by-byte byteAt - Add optimized map to ByteString2 (pre-allocated array + byteAtUnchecked while-loop) - foreach in ByteString2 already optimal (delegates to first/second fragments) Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/cf258ced-c5f8-4bef-8ec4-3de15df750d3 Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
Motivation:
Small ByteString concatenation currently promotes two fragments into ByteStrings backed by a Vector. This is expensive for hot paths that build a frame from a header and payload before reading or copying it.
Modification:
Result:
Focused JMH, JDK 25, short local run with -prof gc:
Additional A/B on this branch showed byteAtUnchecked improves cross-fragment reads: