mkurz commented on PR #1190:
URL: https://github.com/apache/pekko-http/pull/1190#issuecomment-5123578152
I let both gpt-5.6-sol (max effort) as well as claudes Opus 5 (max effort)
review your PR and **both would _not_ merge your PR**:
# gpt-5.6-sol
**Findings**
1. **High: the 128 KiB `sizeHint` is an allocation regression for typical
traffic.**
With the default `max-allocation = 256 KiB`, every call to `inflate`
eagerly allocates 128 KiB, even for an empty or tiny message. Previously, the
initial allocation was 1 KiB. Since `inflate` runs once per compressed frame,
fragmented messages multiply this cost. Many tiny compressed fragments could
create substantial GC pressure.
`maxAllocation` is a safety ceiling, not a reasonable estimate of the
next frame’s decompressed size. [Changed
code](https://github.com/apache/pekko-http/blob/dec939933b74e8b9760701ff3f88bb2fcc84863f/http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/PerMessageDeflate.scala#L245-L262)
2. **Medium: the claimed final-copy elimination does not occur.**
`ByteStringBuilder.result()` calls `clearTemp()`, which allocates an
exact-sized array and copies `_temp` into it. That is equivalent to the final
`ByteArrayOutputStream.toByteArray` copy. Output above 128 KiB also causes
`_temp` to resize first. [Pekko 2.0.0-M3
implementation](https://github.com/apache/pekko/blob/v2.0.0-M3/actor/src/main/scala/org/apache/pekko/util/ByteString.scala#L2785-L2803)
Therefore, the change can reduce intermediate growth allocations for
large outputs near the limit, but it does not generally reduce peak memory from
roughly 3x to 2x as claimed. For small outputs, it is substantially worse.
3. **Low: the change lacks an allocation benchmark.**
Existing tests and the currently green CI establish functional
equivalence, but cannot validate a memory optimization. This should have a JMH
benchmark covering small messages, 8–256 KiB outputs, highly compressible
input, and fragmented messages.
**Process Note**
The title says “during deflate,” but the changed path performs
inflation/decompression. The commit message is also missing the ticket
reference requested by the [contribution
guide](https://github.com/apache/pekko-http/blob/main/CONTRIBUTING.md#creating-commits-and-writing-commit-messages).
**Verdict**
I would not merge [PR #1190](https://github.com/apache/pekko-http/pull/1190)
as written. Its functional behavior looks correct, but the proposed
optimization likely increases allocation rates for the common small-message
case and the central copy-elimination claim is incorrect. A chunked
`ByteString` approach might genuinely avoid the final whole-output copy, but
should be benchmarked against the current implementation first.
# Opus 5
Reviewed. Here's my read — I verified the claims against the actual
`ByteStringBuilder` source (pekko-actor 1.5.0,
`org/apache/pekko/util/ByteString.scala`).
## The two claims in the PR description are both wrong
**1. "`result()` avoids the extra copy that `toByteArray()` made" — no, it
does exactly the same copy.**
```scala
private def clearTemp(): Unit = {
if (_tempLength > 0) {
val arr = new Array[Byte](_tempLength)
System.arraycopy(_temp, 0, arr, 0, _tempLength) // ← full copy, always
_builder += ByteString1(arr)
...
```
`result()` calls `clearTemp()` unconditionally, which allocates an
exactly-sized array and `arraycopy`s into it — byte for byte what
`ByteArrayOutputStream.toByteArray()` did. There is no `_tempLength ==
_temp.length` fast path. So peak memory does **not** go from 3x to 2x; it's
unchanged.
**2. "avoids over-allocating for small ones" — it does the opposite.**
```scala
override def sizeHint(len: Int): Unit = {
resizeTemp(len - (_length - _tempLength)) // resizeTemp = new
Array[Byte](size), unconditional
}
```
`sizeHint` is not a hint, it's an immediate allocation. With the default
`max-allocation = 256 * 1024`, `maxInitialCapacity` is 128 KB, so **every call
to `inflate` now allocates a 131072-byte array up front** — for a 12-byte JSON
message, for a fragment of a fragmented message, for every frame on every
connection. The old `ByteArrayOutputStream(1024)` allocated 1 KB and only grew
if needed.
That's the common WebSocket case (small, frequent messages), and this turns
~1 KB of garbage per frame into ~128 KB. On a server with thousands of chatty
connections that's a meaningful allocation-rate regression.
## What's actually gained
For messages larger than 128 KB: you skip 7 doublings of small arrays
(1K→2K→…→128K, ~256 KB of cumulative short-lived garbage) and beyond 128 KB
`ensureTempSize` doubles identically to `ByteArrayOutputStream`. So: a small,
real win in the large-message case, paid for with a large regression in the
small-message case. Net negative for typical traffic, in my view.
## Smaller points
- The sizing is conceptually mismatched:
`decompressedMessageBytes`/`maxAllocation` is a **per-message** budget
accumulated across frames, but `output` is **per-frame**. Sizing a per-frame
buffer from a per-message cap over-allocates by design for fragmented messages.
- Branch name is `WebSocket-Compression-Bomb`, but nothing about the guard
changes — the `maxAllocation` check at `PerMessageDeflate.scala:253` is
untouched. No security delta here; worth not implying one in the commit/PR
title.
- No benchmark backing the perf claim, and no test (fair enough for a pure
refactor, but then the claim needs numbers).
- `deflate()` at line 348 still uses `ByteArrayOutputStream(1024)`, so the
codebase is now inconsistent for no stated reason.
## What I'd suggest instead
If the goal is genuinely lower peak memory, the copy you want to eliminate
is the final compaction, and that means *not* going through a single contiguous
temp buffer:
```scala
val output = new ByteStringBuilder
var count = inflater.inflate(buffer)
while (count > 0) {
decompressedMessageBytes += count
if (settings.maxAllocation > 0 && decompressedMessageBytes >
settings.maxAllocation)
throw new ProtocolException("WebSocket decompressed message exceeds
configured maximum allocation")
output ++= ByteString.fromArrayUnsafe(java.util.Arrays.copyOfRange(buffer,
0, count))
count = inflater.inflate(buffer)
}
output.result()
```
Peak becomes exactly N (one 8 KB copy per chunk, no doubling, no final
compaction), and small messages allocate proportionally to their size. The
tradeoff is a rope-backed `ByteStrings` result, so it's worth checking whether
anything downstream calls `toArrayUnsafe()`/`compact` on it and would re-copy.
The cheap alternative, if you want to keep it contiguous: size the hint from
the *input*, which you already have — e.g. `math.max(1024, math.min(cap,
input.length * 4))` — instead of a fixed 128 KB floor.
**Bottom line:** I'd not merge as-is. The rationale doesn't survive reading
`ByteStringBuilder`, and the change pessimizes the common path.
--
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]
