He-Pin commented on PR #2924:
URL: https://github.com/apache/pekko/pull/2924#issuecomment-4412536130
@pjfanning Fair maintenance concern — let me put numbers behind the use case
so we can decide on data, not vibes.
Pushed in c962213cbf:
1. **Removed a `LazyList` allocation in `ByteString2.iterator`** — replaced
`LazyList(...)` with a 2-element `List`. That alone ~30× the read-side
throughput on `iterator.getInt` / `readIntBE` paths and was the reason the
pre-built downstream numbers looked unconvincing earlier.
2. **Added a directional JMH benchmark** (`ByteString_concat3way_Benchmark`)
that mirrors the gRPC frame shape (5-byte header ++ payload, 64 B / 1 KB / 16
KB / 64 KB / 256 KB) and compares two strategies:
- `copy` — assemble into a fresh array, wrap with `fromArrayUnsafe` (the
alternative you suggested in
https://github.com/apache/pekko/pull/2924#issuecomment-4326704648)
- `bs2` — `ByteString2.apply(header, payload)` (this PR)
3. **Added a cross-check property test** in `ByteStringSpec` — the same
logical bytes built as `ByteString1C` (reference) and as `ByteString2` with
**every possible split point**, asserting equivalence across `apply`, full
byte-by-byte iteration, `iterator.getShort/Int/Long` (BE+LE) at every offset,
`readShortBE/LE`/`readIntBE/LE`/`readLongBE/LE` at every offset,
`take/drop/takeRight/dropRight/slice` at every cut, `copyToBuffer`,
`copyToArray`, `indexOf`, `compact`, equality and hashCode. That should cover
the "easy to miss having test coverage" concern — a regression in any
read/transform path that diverges from `ByteString1C` will be caught.
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):
| size | copy | bs2 |
|---:|---:|---:|
| 64 B | 163.60 | **501.90** (3.07×) |
| 1 KB | 22.82 | **527.13** (23.10×) |
| 16 KB | 1.67 | **516.72** (308×) |
| 64 KB | 0.63 | **510.53** (815×) |
| 256 KB | 0.15 | **444.32** (3038×) |
**Build the frame and read the header** (parser hot path):
| size | copy | bs2 |
|---:|---:|---:|
| 64 B | 147.03 | **616.02** (4.19×) |
| 1 KB | 23.67 | **609.47** (25.75×) |
| 16 KB | 1.97 | **608.79** (308×) |
| 64 KB | 0.59 | **616.68** (1053×) |
| 256 KB | 0.14 | **612.79** (4367×) |
**Build the frame and write to a `ByteBuffer`** (Netty / NIO sink):
| size | copy | bs2 |
|---:|---:|---:|
| 64 B | 102.93 | **154.73** (1.50×) |
| 1 KB | 17.23 | **49.49** (2.87×) |
| 16 KB | 1.29 | **2.99** (2.31×) |
| 64 KB | 0.34 | **0.65** (1.95×) |
| 256 KB | 0.08 | **0.25** (2.93×) |
**Build the frame and `drop(5)` (strip header)** — the inbound side:
| size | copy | bs2 |
|---:|---:|---:|
| 64 B | 150.22 | **909.84** (6.06×) |
| 1 KB | 23.67 | **917.58** (38.76×) |
| 256 KB | 0.14 | **913.26** (6607×) |
**Where bs2 is *slower* than copy** — pre-built downstream reads on the
merged result:
| benchmark | size | copy | bs2 | bs2 / copy |
|---|---:|---:|---:|---:|
| `readIntBE(0) + apply(4)` | 1 KB | 1196.56 | 730.76 | 0.61× |
| `iterator.getInt + apply(4)` | 1 KB | 909.99 | 400.68 | 0.44× |
| `asByteBuffers` | 1 KB | 200.44 | 98.90 | 0.49× |
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 `bs2` by 2–4000× because we never pay the
`System.arraycopy`. And per
[pekko-grpc#686](https://github.com/apache/pekko-grpc/pull/686/files#diff-8c921b2d5583d7729d19bc797839f0f5e811a008becb746926a33003ea6a50e6R106),
that arraycopy *is* the alternative we're being compared against.
**Re: "I/O dwarfs the difference"** — for a streaming RPC at ~1 M small
messages/s, the per-frame `System.arraycopy(payload)` is exactly proportional
to throughput in CPU and allocation. On a 16 KB payload that's 16 GB/s of
additional memory traffic at 1 M frames/s; on a 64 KB payload, 64 GB/s. That's
not nothing even when an I/O wait sits on top.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
