perf(plc4j/spi/buffers): speed up byte-aligned integer read/write#2650
Merged
sruehl merged 4 commits intoJul 21, 2026
Merged
Conversation
The byte buffers sit on every driver's hottest path (every primitive field of every message). Measured with JMH on a 32x uint16 batch (JDK 21): ~6x faster reads, ~5.6x faster writes, ~38% less allocation, with all 775 buffer tests (incl. fuzz) and the Modbus parser/serializer suite green. - AbstractBuffer: use ArrayDeque for the option-context stack instead of java.util.Stack (a synchronized Vector). A buffer is a single-threaded, per-message scratch object -- positionInBits and the backing array are already unsynchronized -- so the monitor enter/exit on every getContext() guarded nothing. (Changing the protected context field type is binary-incompatible for subclasses that read it directly; all in-repo buffer modules recompile.) - ByteOrderBigEndian: expose a shared stateless INSTANCE and return it from getByteOrder() instead of allocating one per field. - Read/WriteBufferByteBased: add a zero-allocation fast path for whole-byte, byte-aligned, big-endian, plain-binary integer fields (unsigned and signed short/int/long) that reads/writes straight from the backing array. Gated on the concrete EncodingUnsignedBinary / EncodingTwosComplement (not the broader EncodingDefault) so BCD/float/etc. fall through to the slow path unchanged; covered by new regression tests. Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes PLC4X’s byte-based buffers on the hot path by removing avoidable synchronization and allocations, and by adding a zero-allocation fast path for byte-aligned, whole-byte, big-endian integer reads/writes while preserving existing behavior for non-binary encodings.
Changes:
- Replaced the option-context stack in
AbstractBufferfromjava.util.StacktoArrayDequeto remove unnecessary synchronization overhead. - Added a reusable
ByteOrderBigEndian.INSTANCEand updated default byte order resolution to reuse it instead of allocating per field. - Introduced guarded fast paths in
ReadBufferByteBased/WriteBufferByteBased(shared helpers inAbstractBufferByteBased) and added regression tests to ensure binary vs BCD and signed sign-extension behavior remain correct.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plc4j/spi/buffers/api/src/main/java/org/apache/plc4x/java/spi/buffers/api/AbstractBuffer.java | Swaps context stack implementation to ArrayDeque for lower overhead. |
| plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/AbstractBufferByteBased.java | Adds shared fast-path guards and signExtend, and reuses shared big-endian instance. |
| plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/ReadBufferByteBased.java | Adds byte-aligned big-endian integer read fast path reading directly from backing array. |
| plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/WriteBufferByteBased.java | Adds byte-aligned big-endian integer write fast path writing directly into backing array. |
| plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/byteorder/ByteOrderBigEndian.java | Introduces shared stateless INSTANCE to avoid repeated allocations. |
| plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/ReadBufferByteBasedTest.java | Adds tests guarding fast-path gating (BCD must not take binary path) and signed sign-extension. |
| plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/WriteBufferByteBasedTest.java | Adds tests guarding fast-path gating (BCD) and signed two’s-complement emission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ub-buffers isAligned() (used by the whole-byte arraycopy fast paths in readBits/writeBits, and now by the integer fast-path guards) tested only positionInBits, ignoring startBit. A sub-buffer created at a non-byte-aligned offset has a non-zero startBit while its own positionInBits is 0, so a whole-byte read/write took the byte-copy fast path and indexed the backing array by (startBit + positionInBits) / 8 -- truncating the sub-byte offset and reading/writing from the wrong byte. Test it on the absolute bit index. Also gate the integer fast-path guards on isAligned() (absolute) instead of positionInBits alone, and use `getByteOrder() instanceof ByteOrderBigEndian` instead of reference-equality with the singleton, so an explicitly big-endian-configured buffer (ServiceLoader instance) is still eligible for the fast path. Adds a regression test: a whole-byte unsigned read from a non-byte-aligned sub-buffer now returns the correct value. Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
… field The fast-path eligibility guards resolved the encoding and byte order, and when the fast path was not taken (e.g. a little-endian buffer or a non-binary encoding) the slow path resolved both again -- an extra context/manager lookup per whole-byte aligned integer field. Resolve them once in each read/write method and reuse the resolved values for both the eligibility check and the slow path. The BE-specific guards collapse into a single structural isByteAlignedWholeBytes check combined with instanceof tests on the already resolved values. (EncodingRaw paths now resolve the byte order they do not use -- a negligible constant -- in exchange for exactly-once resolution everywhere else.) Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
…th on exact codec classes Two hardening items in the same alignment/fast-path family as the previous commits: - writeBit indexed the backing array by positionInBits alone, ignoring startBit -- unlike readBit and the whole-byte arraycopy path in writeBits, which both use the absolute bit index. A WriteBufferByteBased constructed with a non-zero startBit therefore wrote from the start of the backing array. Index by (startBit + positionInBits), mirroring readBit, with a regression test (writeHonoursNonByteAlignedStartBit). - The integer fast-path gates checked `instanceof EncodingUnsignedBinary` / `EncodingTwosComplement` / `ByteOrderBigEndian`. The slow path dispatches virtually, so a subclass registered through the Encoding/ByteOrder managers with overridden codec behaviour would have been silently bypassed whenever a field was byte-aligned. Gate on the exact class (getClass() == ...) instead: multiple instances remain eligible, subclasses fall through to the virtual-dispatch slow path. Also document that writeAlignedBytesBE relies on the caller's ensureAvailable(numBits) (it has no internal capacity check), and drop the dead value mask in writeUnsignedShort's fast path (the range check already guarantees a non-negative value, and the mask was inconsistent with the other five write sites). Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The shared byte buffers in
plc4j/spi/bufferssit on every driver's hottest path — everyprimitive field of every message is read/written through them (~70+ files across
plc4j— drivers,spi, transports, test-utils — construct these byte buffers). Profiling a 32×
uint16batch (atypical Modbus/S7-style read) with JMH showed two avoidable costs per field:
java.util.Stack(a synchronizedVector), so everygetContext()did a monitor enter/exit that guarded nothing; andallocating an intermediate
byte[]and a freshByteOrderobject per field.This change removes both. Wire behaviour is preserved on every previously-correct path; the review
follow-ups below additionally fixed a pre-existing alignment bug for buffers and sub-buffers that
start at a non-byte-aligned bit position (which affected the whole-byte paths on
developtoo).Changes
AbstractBuffer— useArrayDequefor the option-context stack instead ofjava.util.Stack.A buffer is a single-threaded, per-message scratch object (
positionInBitsand the backing arrayare already unsynchronized), so
Stack's synchronization protected nothing here.ByteOrderBigEndian— expose a shared statelessINSTANCEand return it fromgetByteOrder()instead of allocating one per field (
process()is the identity, so the instance is reusable).Read/WriteBufferByteBased(with shared guards + asignExtendhelper inAbstractBufferByteBased) — add a zero-allocation fast path for whole-byte, byte-aligned,big-endian, plain-binary integer fields (unsigned and signed short/int/long) that reads/writes
straight from the backing array. It is gated on the concrete
EncodingUnsignedBinary/EncodingTwosComplement(not the broaderEncodingDefault), so BCD / float / etc. fall throughto the existing slow path unchanged.
Benchmarks
JMH, JDK 21, 32×
uint16batch,-prof gc:Thread-safety
No regression. Buffers are per-message throwaway objects — no pooling, no static/
ThreadLocalsharing — and
positionInBits/ the backing array were already unsynchronized, so each buffer isconfined to a single thread for the duration of one message's parse/serialize. The
Stack'ssynchronization was incidental, not an anti-interleaving guarantee: concurrent read/write requests
each get their own buffer.
ArrayDequeis a drop-in — the code uses the context field only as aLIFO stack via
push/pop/peek/isEmpty/size, all of which behave identically, and neveriterates it or reads it by index.
Binary compatibility
Changing
AbstractBuffer'scontextfield type fromStacktoDequeis source-compatible butbinary-incompatible for any subclass that reads the
protected contextfield directly. Within therepo exactly one class does so —
WriteBufferXmlBased(context.isEmpty()/context.size()) —and it compiles and recompiles cleanly against
Deque; the other buffer subclasses(
ReadBufferXmlBased, the ascii-box and byte buffers) never touch the field. All three buffermodules (
byte,xml,ascii-box) recompile in a normal/CI build; only a stale incremental build(Maven not recompiling the
xmlmodule afterapichanged), or an out-of-repo precompiled subclassthat reads
context, would hit aNoSuchFieldError. Flagging it here for downstream awareness.Review follow-ups (commits after the initial review)
isAligned(), which thepre-existing
readBits/writeBitswhole-byte arraycopy paths also use) tested onlypositionInBits, ignoringstartBit; a sub-buffer created at a non-byte-aligned offset read/wrotethe wrong bytes — on
developtoo.isAligned()now tests the absolute bit index, andwriteBit(which ignored
startBitentirely, unlikereadBit) now indexes absolutely as well. Regressiontests cover both directions.
eligibility check and the slow path (previously a non-eligible aligned field resolved them twice).
getClass() == EncodingUnsignedBinary.class/EncodingTwosComplement.class/ByteOrderBigEndian.classrather thaninstanceof, so a codecsubclass registered through the managers keeps its overridden (virtually-dispatched) slow-path
behaviour instead of being silently bypassed.
Testing
regression tests: the unsigned byte-aligned fast path returns/emits the same value as the slow path
(read & write), BCD-must-not-take-the-binary-fast-path (read & write), signed sign-extension /
two's-complement (read & write), and the alignment fixes (a whole-byte read from a non-byte-aligned
sub-buffer, and a write through a buffer constructed with a non-byte-aligned
startBit).