From 4538c3968fa521141dac775b79da4884aa4857d6 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Tue, 25 Aug 2026 22:19:20 +0100 Subject: [PATCH] perf: avoid copying the temp buffer in ByteStringBuilder.result Motivation: `ByteStringBuilder` accumulates writes from `putByte`/`putBytes`/`putInt`/ `asOutputStream` into a temporary array and flushes it in `clearTemp`, which always does `java.util.Arrays.copyOf(_temp, _tempLength)`. On the final flush performed by `result()` that copy is avoidable: the buffer is not needed for further writes at that point, so every builder result pays a full copy of its tail chunk for nothing. Modification: Add a private `clearTempForResult` used only by `result()`. When more than half of the temp buffer holds data, the buffer is handed over to the resulting `ByteString1` instead of being copied, and `_temp`/`_tempCapacity` are reset so that a later write to the builder allocates a fresh buffer rather than mutating the bytes that were handed over. When only a small part of the buffer is used it is still copied, so a small result does not retain a much larger array. The mid-stream `clearTemp` calls made by `addAll` are unchanged, since the buffer is still reused there. Result: No copy of the tail chunk on `result()` for builders that are at least half full. A result produced by the hand-over path can be a non-compact `ByteString1`; that is copy-neutral, because a consumer calling `toArrayUnsafe` on it pays exactly the copy that used to happen inside `clearTemp`, and `ByteStringUtils.toProtoByteStringUnsafe` still takes its zero-copy `asByteBuffers` path for it. Tests: - `sbt "actor-tests/testOnly org.apache.pekko.util.ByteStringSpec"` - 218 tests succeeded, 0 failed - `ByteStringSpec` gains cases for the mostly-used, exactly-filled and small-write-in-a-large-buffer flush paths, that a returned `ByteString` is unaffected by later writes to the builder, and that the content is correct for every fill level from 0 to 200 bytes - `scalafmt --list --mode diff-ref=upstream/main` - no files reported - `git diff --check` - clean - Broader run of `actor-tests/testOnly org.apache.pekko.util.*`, the stream io/compression/framing suites and `actor/mimaReportBinaryIssues` was started but stopped before completing, so it is not recorded as a pass. MiMa is expected to be unaffected: the change adds one private method and alters no signature. References: None - follows the same no-copy approach as apache/pekko-http#1235 --- .../apache/pekko/util/ByteStringSpec.scala | 56 +++++++++++++++++++ .../org/apache/pekko/util/ByteString.scala | 23 +++++++- 2 files changed, 78 insertions(+), 1 deletion(-) 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 e7d15c7541d..70d3e3e06ed 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 @@ -2409,6 +2409,62 @@ class ByteStringSpec extends AnyWordSpec with Matchers with Checkers { (ByteString1(Array[Byte](1, 2, 3), 0, Int.MinValue) should be).theSameInstanceAs(ByteString1.empty) } + "ByteStringBuilder.result hands over a mostly used temp buffer without copying" in { + val builder = ByteString.newBuilder + builder.sizeHint(64) + val data = Array.tabulate[Byte](40)(i => i.toByte) + builder.putBytes(data) + val result = builder.result() + result should ===(ByteString(data)) + // the 64 byte buffer is wrapped rather than copied, so the result covers only part of its array + result.isCompact should ===(false) + } + + "ByteStringBuilder.result hands over an exactly filled temp buffer without copying" in { + val builder = ByteString.newBuilder + builder.sizeHint(64) + val data = Array.tabulate[Byte](64)(i => i.toByte) + builder.putBytes(data) + val result = builder.result() + result should ===(ByteString(data)) + result.isCompact should ===(true) + } + + "ByteStringBuilder.result copies when only a small part of the temp buffer is used" in { + val builder = ByteString.newBuilder + builder.sizeHint(64) + val data = Array.tabulate[Byte](10)(i => i.toByte) + builder.putBytes(data) + val result = builder.result() + result should ===(ByteString(data)) + // a small result must not retain the much larger buffer + result.isCompact should ===(true) + result.toArrayUnsafe().length should ===(10) + } + + "ByteStringBuilder.result is not affected by later writes to the builder" in { + val builder = ByteString.newBuilder + builder.sizeHint(64) + val data = Array.fill[Byte](40)(1) + builder.putBytes(data) + val first = builder.result() + builder.clear() + builder.putBytes(Array.fill[Byte](40)(2)) + val second = builder.result() + first should ===(ByteString(data)) + second should ===(ByteString(Array.fill[Byte](40)(2))) + } + + "ByteStringBuilder.result returns the written bytes for any fill level of the temp buffer" in { + for (n <- 0 to 200) { + val builder = ByteString.newBuilder + builder.sizeHint(128) + val data = Array.tabulate[Byte](n)(i => (i % 128).toByte) + builder.putBytes(data) + builder.result() should ===(ByteString(data)) + } + } + "ByteStringBuilder.sizeHint does not shrink existing capacity" in { val builder = ByteString.newBuilder builder.sizeHint(100) 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 af10c991a99..73ffdc6d53d 100644 --- a/actor/src/main/scala/org/apache/pekko/util/ByteString.scala +++ b/actor/src/main/scala/org/apache/pekko/util/ByteString.scala @@ -2853,6 +2853,27 @@ final class ByteStringBuilder extends Builder[Byte, ByteString] { } } + /** + * Like `clearTemp` but for the final flush done by `result()`, where the temporary buffer does not + * have to be kept around for further writes. When most of the buffer holds data, it is handed over + * to the resulting `ByteString` instead of being copied; the reference to it is dropped here so that + * a later write to this builder allocates a fresh buffer rather than mutating the handed over bytes. + * When only a small part of the buffer is used it is still copied, so that a small result does not + * retain a much larger array. + */ + private def clearTempForResult(): Unit = { + if (_tempLength > 0) { + if (_tempLength > (_tempCapacity >> 1)) { + _builder += ByteString1(_temp, 0, _tempLength) + _temp = null + _tempCapacity = 0 + } else { + _builder += ByteString1(java.util.Arrays.copyOf(_temp, _tempLength)) + } + _tempLength = 0 + } + } + private def resizeTemp(size: Int): Unit = { val newtemp = if (_temp eq null) new Array[Byte](size) else java.util.Arrays.copyOf(_temp, size) _temp = newtemp @@ -3090,7 +3111,7 @@ final class ByteStringBuilder extends Builder[Byte, ByteString] { def result(): ByteString = if (_length == 0) ByteString.empty else { - clearTemp() + clearTempForResult() val bytestrings = _builder.result() if (bytestrings.size == 1) bytestrings.head