This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pekko.git
The following commit(s) were added to refs/heads/main by this push:
new d23eb51f3a perf: avoid copying the temp buffer in
ByteStringBuilder.result (#3471)
d23eb51f3a is described below
commit d23eb51f3aab73a35051e0d2b11629e2ae04606e
Author: PJ Fanning <[email protected]>
AuthorDate: Fri Aug 28 07:30:17 2026 +0100
perf: avoid copying the temp buffer in ByteStringBuilder.result (#3471)
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
---
.../org/apache/pekko/util/ByteStringSpec.scala | 56 ++++++++++++++++++++++
.../scala/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 e7d15c7541..70d3e3e06e 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 af10c991a9..73ffdc6d53 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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
