Copilot commented on code in PR #2650:
URL: https://github.com/apache/plc4x/pull/2650#discussion_r3619899144
##########
plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/AbstractBufferByteBased.java:
##########
@@ -65,7 +67,40 @@ protected ByteOrder getByteOrder(WithOption... options) {
return byteOrder.get();
}
}
- return new ByteOrderBigEndian();
+ return ByteOrderBigEndian.INSTANCE;
+ }
+
+ // ---- Byte-aligned integer fast-path helpers (shared by Read/Write byte
buffers) ----
+ // Eligible only when there are no per-field option overrides, the
position is byte-aligned, a
+ // whole number of bytes is requested, byte order is big-endian, and the
resolved integer encoding
+ // is plain binary (unsigned) / two's-complement (signed). Gated on the
concrete encoding class,
+ // NOT the broader EncodingDefault, so BCD/float/etc. correctly fall
through to the slow path.
+
+ protected boolean isFastUnsignedBinaryBE(int numBits, WithOption[]
options) {
+ // isAligned() tests the ABSOLUTE bit index (startBit +
positionInBits) — the same predicate the
+ // readBits/writeBits whole-byte fast paths use — so a
non-byte-aligned sub-buffer correctly
+ // falls through to the generic path (readAlignedBytesBE indexes by
(startBit+positionInBits)/8).
+ if (options.length != 0 || !isAligned() || (numBits & 7) != 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getUnsignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingUnsignedBinary
+ && getByteOrder() instanceof ByteOrderBigEndian;
+ }
Review Comment:
`isFastUnsignedBinaryBE` resolves encoding/byte order via
`getUnsignedIntegerEncoding()` and `getByteOrder()` even when the fast path is
not taken (e.g., LITTLE_ENDIAN or non-binary encodings). The caller then
resolves the same encoding/byte order again for the slow path, adding an extra
manager/context lookup on every whole-byte aligned integer field that can’t use
the fast path (a performance regression for those cases). Consider refactoring
so the encoding and byte order are resolved once in the caller and reused for
both the eligibility check and the slow path (or pass the resolved
encoding/byte order into this helper).
##########
plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/AbstractBufferByteBased.java:
##########
@@ -65,7 +67,40 @@ protected ByteOrder getByteOrder(WithOption... options) {
return byteOrder.get();
}
}
- return new ByteOrderBigEndian();
+ return ByteOrderBigEndian.INSTANCE;
+ }
+
+ // ---- Byte-aligned integer fast-path helpers (shared by Read/Write byte
buffers) ----
+ // Eligible only when there are no per-field option overrides, the
position is byte-aligned, a
+ // whole number of bytes is requested, byte order is big-endian, and the
resolved integer encoding
+ // is plain binary (unsigned) / two's-complement (signed). Gated on the
concrete encoding class,
+ // NOT the broader EncodingDefault, so BCD/float/etc. correctly fall
through to the slow path.
+
+ protected boolean isFastUnsignedBinaryBE(int numBits, WithOption[]
options) {
+ // isAligned() tests the ABSOLUTE bit index (startBit +
positionInBits) — the same predicate the
+ // readBits/writeBits whole-byte fast paths use — so a
non-byte-aligned sub-buffer correctly
+ // falls through to the generic path (readAlignedBytesBE indexes by
(startBit+positionInBits)/8).
+ if (options.length != 0 || !isAligned() || (numBits & 7) != 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getUnsignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingUnsignedBinary
+ && getByteOrder() instanceof ByteOrderBigEndian;
+ }
+
+ protected boolean isFastSignedTwosComplementBE(int numBits, WithOption[]
options) {
+ if (options.length != 0 || !isAligned() || (numBits & 7) != 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getSignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingTwosComplement
+ && getByteOrder() instanceof ByteOrderBigEndian;
+ }
Review Comment:
`isFastSignedTwosComplementBE` does the same repeated work as the unsigned
helper: it calls `getSignedIntegerEncoding()` and `getByteOrder()` to decide
fast-path eligibility, but the slow path immediately re-resolves encoding/byte
order. This adds extra overhead for whole-byte aligned signed integer fields
that aren’t eligible (e.g., LITTLE_ENDIAN), counteracting some of the intended
perf wins.
--
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]
