Copilot commented on code in PR #2650:
URL: https://github.com/apache/plc4x/pull/2650#discussion_r3615116790
##########
plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/AbstractBufferByteBased.java:
##########
@@ -65,7 +67,37 @@ 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) {
+ if (options.length != 0 || (positionInBits & 7) != 0 || (numBits & 7)
!= 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getUnsignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingUnsignedBinary
+ && getByteOrder() == ByteOrderBigEndian.INSTANCE;
+ }
+
+ protected boolean isFastSignedTwosComplementBE(int numBits, WithOption[]
options) {
+ if (options.length != 0 || (positionInBits & 7) != 0 || (numBits & 7)
!= 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getSignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingTwosComplement
+ && getByteOrder() == ByteOrderBigEndian.INSTANCE;
+ }
Review Comment:
Same alignment / byte-order-instance issues as in `isFastUnsignedBinaryBE`:
the fast-path check should use absolute bit alignment `((startBit +
positionInBits) & 7) == 0`, and the big-endian check should not rely on
reference equality with `ByteOrderBigEndian.INSTANCE` (ServiceLoader provides a
different `ByteOrderBigEndian` instance).
##########
plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/AbstractBufferByteBased.java:
##########
@@ -65,7 +67,37 @@ 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) {
+ if (options.length != 0 || (positionInBits & 7) != 0 || (numBits & 7)
!= 0) {
+ return false;
+ }
+ Optional<Encoding> enc = getUnsignedIntegerEncoding();
+ return enc.isPresent() && enc.get() instanceof EncodingUnsignedBinary
+ && getByteOrder() == ByteOrderBigEndian.INSTANCE;
+ }
Review Comment:
The byte-aligned integer fast-path eligibility check only considers
`positionInBits` alignment, but sub-buffers can have a non-byte-aligned
`startBit` (see `ReadBufferByteBased#createSubBuffer`, which sets `startBit +
positionInBits`). In that case `positionInBits` can be 0 (byte-aligned) while
the absolute bit index is not, causing the fast path to read/write from the
wrong byte offset.
Also, `getByteOrder() == ByteOrderBigEndian.INSTANCE` is too strict: when
BIG_ENDIAN is provided via options/context, `getByteOrder()` returns the
ServiceLoader-created `ByteOrderBigEndian` instance from `ByteOrderManager`,
which is not reference-equal to `INSTANCE`, so the fast path is unnecessarily
disabled.
--
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]
