pjfanning opened a new pull request, #3463: URL: https://github.com/apache/pekko/pull/3463
Two related performance fixes in `ByteString`, in separate commits. ## 1. `equals` compared bytes through the generic `Seq` path `ByteString` inherited equality from `Seq`, which walks both sides through `iterator` and boxes every `Byte`. Comparing two 64KB ByteStrings ran about 60x slower than `java.util.Arrays.equals` over the same bytes. `equals` now compares the underlying arrays. The comparison is driven from whichever side is a single compacted array so it can reuse the existing SWAR-based `matchesAt`; when neither side is, a new `compareBytesTo` walks the fragments in place instead of compacting. Two fragmented ByteStrings are compared with independent cursors, so neither side re-locates a fragment for each run of bytes. `ByteStrings` also gains a `matchesAt` override, so the existing `indexOfSlice` / `startsWith` paths no longer fall back to the per-byte implementation inherited from `ByteString`. `hashCode` is untouched. It is content based and independent of the internal layout, so it still agrees with the new `equals` — the tests assert this across every representation. ## 2. `ByteStrings` resolved every index by scanning from the start `ByteStrings.apply` scanned the fragment vector from index 0 on every call, and `byteAtUnchecked` was not overridden, so it fell through to `apply`. Walking a fragmented ByteString by index was quadratic in the number of fragments. `byteAtUnchecked` now remembers the fragment resolved by the previous call, so sequential access either stays inside that fragment or resumes the scan from it. `apply` becomes a bounds check in front of `byteAtUnchecked`, so both share the memoised lookup and the duplicated scan is gone. The remembered triple is held in one immutable object published by a single reference write, so a reader can never observe the start of one fragment paired with the index of another — which three separate `int` fields would allow. The field is deliberately not `volatile`: it is only a hint, a reader that misses another thread's update just rescans, and `ByteString` is immutable so a resolved mapping never becomes wrong. There is a test that hammers it from 8 threads. ### Effect on artery TCP framing `ByteStringParser`'s `ByteReader` reads sequentially via `apply`, and the `read*Unchecked` helpers that `ByteStrings` does not override are built from `apply`. Simulating that read pattern over a ByteString assembled from 1024 TCP chunks: | | before | after | |---|---|---| | sequential `readByte`-style | 409.7 ms/pass | 3.1 ms/pass | | `readIntLE`-style | 415.5 ms/pass | 3.5 ms/pass | I looked at adding a `ByteStrings.readIntLEUnchecked` override on top of this. With the memoised lookup in place it lands within about 2x of the compacted SWAR path, so the remaining headroom is small and I left it out rather than adding an override for a modest gain. ## Tests Added to `ByteStringSpec` (`actor-tests`), 10 new tests: - equality and `hashCode` agreement between all internal representations of the same content — compacted, sliced, two-fragment and multi-fragment — at 17 sizes from 0 to 1000 bytes - content differing in the first byte and in the last byte, and differing lengths - equality with other `Seq[Byte]` implementations (`Vector`, `List`) in both directions, and `hashCode` agreement with them - `Set` membership across representations - `byteAtUnchecked` under forward, backward, repeated and alternating access; agreement with `toArray` and the iterator; out of range indices; and concurrent reads from 8 threads I verified the new tests actually fail against the un-fixed code by reintroducing a sentinel bug in the fragment hint: 2 of the new tests plus 4 pre-existing ones fail, and all pass once corrected. `actor-tests/testOnly org.apache.pekko.util.*` passes (483 tests). `sbt actor/mimaReportBinaryIssues` is clean — no public signatures change; the new members are `private` or `private[pekko]`. `scalafmt` run on all three modules; headers generated with `sbt headerCreateAll`. I did not run the full stream/remote suites locally and am relying on CI for those. ## Benchmarks Two new JMH benchmarks, `ByteString_equals_Benchmark` and `ByteString_byteAtUnchecked_Benchmark`, each carrying measured before/after numbers in a comment in the file, in the style of the existing ByteString benchmarks. Both were run on the same machine with `-f1 -wi 3 -i 3`; the error bars are wide at that iteration count, and the comments say so. Headline numbers: - `flat_equal_flat` 17772 → 70767 ops/s, `rope_equal_rope` 960 → 25819 ops/s - `manyFragments_sequential` 651 → 54839 ops/s (~84x) The `byteAtUnchecked` benchmark also records the cases that do **not** improve — random access is unchanged because the hint never hits, and reverse access falls back to a scan each step. Both stay within noise of the previous numbers. ## References - `actor/src/main/scala/org/apache/pekko/util/ByteString.scala` - `actor-tests/src/test/scala/org/apache/pekko/util/ByteStringSpec.scala` - `ByteStringParser.ByteReader` in `stream/src/main/scala/org/apache/pekko/stream/impl/io/ByteStringParser.scala` (the framing read 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]
