ByteStringBuilder.putInt/putLong: eliminate closure allocation via SWARUtil VarHandle writes#2879
Conversation
…SWARUtil VarHandle writes Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/c0a324ab-b053-4254-8d3b-e472278559b9 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
…lder.putInt/putLong Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/d3bbcf71-0623-473b-b9cc-52a23c034666 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
|
I'll look into creating a JMH benchmark |
|
nice improvement |
Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/710c7bd8-24f1-4665-b722-dd9020049450 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
|
…re-allocated arrays Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/770d8c57-8a12-4f06-b900-1f6271577fdd Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
| val out = new ByteArrayOutputStream | ||
| writeInt(out, snapshotSerializer.identifier) | ||
|
|
||
| val ms = migrateManifestIfNecessary(Serializers.manifestFor(snapshotSerializer, snapshot)) |
There was a problem hiding this comment.
The SnapshotSerializer changes refactor headerToBinary and the serialization method to use direct byte arrays + SWARUtil.putInt instead of ByteArrayOutputStream + writeInt. This is a significant change to the serialization format logic.
However, no tests are added or modified for SnapshotSerializer to verify that:
- The serialized binary format is identical (same byte order, same structure)
- Round-trip serialization/deserialization works correctly
- The
migrateManifestIfNecessarypath still produces correct output
Please add tests for SnapshotSerializer to cover these cases, or point to existing tests that would catch regressions here.
There was a problem hiding this comment.
#2891 has extra tests and they appear in this PR too
| if (byteOrder == ByteOrder.BIG_ENDIAN) { | ||
| if (intBeArrayViewSupported) { | ||
| intBeArrayView.set(array, index, value) | ||
| } else { |
There was a problem hiding this comment.
Correctness concern: The putInt/putLong/putShort methods silently treat any non-BIG_ENDIAN byte order as LITTLE_ENDIAN. The original ByteStringBuilder.putInt threw IllegalArgumentException for unknown byte orders:
} else throw new IllegalArgumentException("Unknown byte order " + byteOrder)While ByteOrder only has BE/LE instances in practice, this is still a behavioral regression. Consider adding an explicit validation or at least documenting this change.
There was a problem hiding this comment.
there are only 2 ByteOrders in the Java class - https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/nio/ByteOrder.html
| @OutputTimeUnit(TimeUnit.MILLISECONDS) | ||
| @BenchmarkMode(Array(Mode.Throughput)) | ||
| @Fork(2) | ||
| @Warmup(iterations = 5) |
There was a problem hiding this comment.
Performance concern: The benchmark only measures the new implementation. There is no baseline @Benchmark method using the old fillArray approach for before/after comparison.
To validate the performance claim (eliminating closure allocation), the benchmark should either:
- Include a method that replicates the old
fillArraypattern for direct comparison, or - Document that reviewers should run this benchmark against both
mainand the PR branch.
Without a baseline, the benchmark cannot demonstrate the claimed improvement.
There was a problem hiding this comment.
this is the case with all our benchmarks - it is not hard to run the benchmark with and without the main source changes
…Float/putDouble tests, SnapshotSerializer format tests Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/3a10e6a7-4e61-4f22-9ca1-fdbad5891385 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
…utFloat/putDouble and SnapshotSerializer format tests Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/3a10e6a7-4e61-4f22-9ca1-fdbad5891385 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
ByteStringBuilder.putIntandputLongpassed lambdas tofillArray, allocating a closure object on every call. This replaces those closures with direct inlined writes using newSWARUtil.putInt/putLonghelpers.Changes
SWARUtilputInt(array, index, value, byteOrder)andputLong(array, index, value, byteOrder)— VarHandleset-based writes mirroring the existinggetInt/getLongreadersputIntBEWithoutMethodHandle,putIntLEWithoutMethodHandle,putLongBEWithoutMethodHandle,putLongLEWithoutMethodHandle) for environments without VarHandle supportByteStringBuilderputIntandputLongnow callensureTempSize+SWARUtil.putInt/putLongdirectly, updating_tempLength/_lengthinline — same structure asfillArraybut without the closureputFloatandputDoublebenefit automatically since they delegate toputInt/putLong.Tests
SWARUtilSpecputInt,putIntBEWithoutMethodHandle,putIntLEWithoutMethodHandle,putLong,putLongBEWithoutMethodHandle,putLongLEWithoutMethodHandle) covering specific byte layouts at multiple offsets, boundary values (Int/LongMaxValue/MinValue), offset correctness (surrounding bytes not clobbered), and round-trip consistency with the correspondingget*fallbacks.ByteStringSpecputIntandputLongtest blocks under"A ByteStringBuilder"covering: known byte sequences for both byte orders, boundary values, length assertions, multi-put accumulation, and property-based round-trips againstByteBufferfor both big-endian and little-endian.