Skip to content

feat: add ByteString slice-free copyToArray and decodeString overloads - #3464

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:bytestring-slice-apis
Open

feat: add ByteString slice-free copyToArray and decodeString overloads#3464
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:bytestring-slice-apis

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Adds two ByteString APIs for callers that repeatedly read sub-ranges of a larger buffer. Both come from an analysis of the pekko-http parsers, where the slice-then-use pattern is common in header and line parsing.

copyToArray(srcOffset, dest, destOffset, len)

The existing copyToArray(dest, destOffset, len) can only copy from index 0, so copying from an offset means drop(n) or slice(...) first — which allocates a new ByteString, and for a fragmented one a new fragment vector too.

decodeString(charset, from, until)

Decodes a sub-range directly. The array-backed layouts pass the offset and length straight to new String(bytes, offset, len, charset), so neither an intermediate ByteString nor an intermediate array is created.

Implementation

Both are defined on ByteString with a correct generic fallback and overridden in ByteString1C, ByteString1, ByteString2 and ByteStrings. The single-array layouts use System.arraycopy and the offset-taking String constructor; ByteStrings locates the starting fragment and copies whole runs out of consecutive fragments.

Out of range arguments are clamped rather than throwing, matching the existing copyToArray overloads: the copy is reduced to what is available in the source and what fits in the destination, and an empty or inverted range decodes to "". This is a deliberate choice for consistency — worth a reviewer's opinion if you would rather they threw.

Measurements

Allocation is the reliable signal here and is the point of both APIs. Copying 4096 bytes from offset 2048 of an 8192-byte ByteString:

layout drop(n).copyToArray copyToArray(srcOffset, …)
128 × 64B fragments 202 B/op 0 B/op
16 × 512B fragments 144 B/op 0 B/op

Decoding a 45-byte header out of a fragmented buffer: slice(…).decodeString 328 B/op → decodeString(cs, from, until) 152 B/op.

One case where the new API is slower, which I want to flag rather than bury: with very small fragments (1024 × 8B), copyToArray(srcOffset, …) runs slower than drop(n).copyToArray (~15µs vs ~11µs) even though it allocates nothing. The reason is that drop produces a ByteString whose existing copyToArray walks bytestrings.iterator once, whereas the new implementation indexes bytestrings(fragIdx) per fragment, and Vector.apply is O(log n). I tried an iterator-based walk; it fixed the timing but reintroduced allocation, which defeats the purpose, so I kept the allocation-free version. 8-byte fragments are also not a realistic shape for the parser use case this targets. Happy to revisit if a reviewer disagrees with that trade.

Timings on this machine were noisy run to run; the allocation figures are stable and reproducible, so I would treat the timing numbers as indicative only.

Relationship to the other ByteString PRs

Independent of #3463. I stacked this branch on top of it to check: the two merge cleanly, all tests pass, and the small-fragment gap above persists, because copyToArray copies via Array.copy per fragment and never goes through the memoised byteAtUnchecked that #3463 adds. The one place they do interact is the generic copyToArray fallback on the abstract class, which reads byteAtUnchecked per byte — that path gets faster if #3463 lands, but no built-in layout uses it. Neither PR needs the other, and they can merge in either order.

Tests

8 new tests in ByteStringSpec:

  • both APIs compared against the slice(...)-based expression they replace, across compacted, sliced, two-fragment and multi-fragment representations, over all combinations of in and out of range offsets and lengths
  • clamping of every out of range argument
  • not writing past the end of the destination
  • empty ByteString and empty/inverted ranges
  • a multi-byte UTF-8 character split across two fragments

actor-tests/testOnly org.apache.pekko.util.ByteStringSpec passes (221 tests). sbt actor/mimaReportBinaryIssues is clean — both APIs are additive. scalafmt run on both modules.

I did not add a JMH benchmark here, since the win is allocation rather than throughput and the existing ByteString benchmarks are throughput oriented. Happy to add one if preferred.

Two APIs for callers that repeatedly read sub-ranges of a larger buffer,
which today have to allocate an intermediate ByteString first. Both were
suggested by an analysis of the pekko-http parsers, where the
slice-then-use pattern is common in header and line parsing.

`copyToArray(srcOffset, dest, destOffset, len)` copies from an offset
within this ByteString. The existing overload can only copy from index 0,
so copying from an offset requires `drop(n)` or `slice(...)` first, which
allocates a new ByteString (and, for a fragmented one, a new fragment
vector).

`decodeString(charset, from, until)` decodes a sub-range directly. The
array-backed layouts pass the offset and length straight to the String
constructor, so neither an intermediate ByteString nor an intermediate
array is created.

Both are implemented on ByteString with a correct generic fallback and
overridden in ByteString1C, ByteString1, ByteString2 and ByteStrings so
the array-backed layouts use System.arraycopy and the offset-taking
String constructor directly.

Out of range arguments are clamped rather than throwing, matching the
behaviour of the existing copyToArray overloads: the range is reduced to
what is available in the source and what fits in the destination, and an
empty or inverted range decodes to the empty string.

Tests compare both against the slice-then-operate expression they
replace, across every internal representation and over all combinations
of in and out of range offsets and lengths, including a multi-byte UTF-8
character split across two fragments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant