Copilot commented on code in PR #10524:
URL: https://github.com/apache/rocketmq/pull/10524#discussion_r3427262573
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RocketMQSerializable.java:
##########
@@ -28,6 +28,74 @@
public class RocketMQSerializable {
private static final Charset CHARSET_UTF8 = StandardCharsets.UTF_8;
+ private static final String[] SINGLE_BYTE_STRINGS = new String[128];
+ static {
+ for (int i = 0; i < 128; i++) {
+ SINGLE_BYTE_STRINGS[i] = String.valueOf((char) i);
+ }
+ }
+
+ public static void writeDecimalLong(ByteBuf buf, long value) {
+ int lenIndex = buf.writerIndex();
+ buf.writeInt(0);
+ int start = buf.writerIndex();
+ if (value == 0) {
+ buf.writeByte('0');
+ } else {
+ boolean neg = value < 0;
+ if (neg) {
+ buf.writeByte('-');
+ if (value == Long.MIN_VALUE) {
+ writePositiveDigits(buf, 922337203685477580L);
+ buf.writeByte('8');
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ return;
+ }
+ value = -value;
+ }
+ writePositiveDigits(buf, value);
+ }
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ }
+
+ public static void writeDecimalInt(ByteBuf buf, int value) {
+ int lenIndex = buf.writerIndex();
+ buf.writeInt(0);
+ int start = buf.writerIndex();
+ if (value == 0) {
+ buf.writeByte('0');
+ } else {
+ boolean neg = value < 0;
+ if (neg) {
+ buf.writeByte('-');
+ if (value == Integer.MIN_VALUE) {
+ writePositiveDigits(buf, 214748364L);
+ buf.writeByte('8');
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ return;
+ }
+ value = -value;
+ }
+ writePositiveDigits(buf, value);
+ }
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ }
+
+ private static void writePositiveDigits(ByteBuf buf, long value) {
+ int digitStart = buf.writerIndex();
+ int numDigits = 0;
+ long temp = value;
+ while (temp > 0) {
+ numDigits++;
+ temp /= 10;
+ }
+ buf.writerIndex(digitStart + numDigits);
+ temp = value;
+ for (int i = numDigits - 1; i >= 0; i--) {
+ buf.setByte(digitStart + i, (byte) ('0' + (int) (temp % 10)));
+ temp /= 10;
+ }
+ }
Review Comment:
`buf.writerIndex(digitStart + numDigits)` can throw
`IndexOutOfBoundsException` if the `ByteBuf` does not already have enough
writable capacity for `numDigits`. Unlike `writeByte(...)`, manually moving the
writer index and using `setByte(...)` does not guarantee capacity expansion.
Call `buf.ensureWritable(numDigits)` (or otherwise reserve capacity) before
advancing the writer index.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RocketMQSerializable.java:
##########
@@ -28,6 +28,74 @@
public class RocketMQSerializable {
private static final Charset CHARSET_UTF8 = StandardCharsets.UTF_8;
+ private static final String[] SINGLE_BYTE_STRINGS = new String[128];
+ static {
+ for (int i = 0; i < 128; i++) {
+ SINGLE_BYTE_STRINGS[i] = String.valueOf((char) i);
+ }
+ }
+
+ public static void writeDecimalLong(ByteBuf buf, long value) {
+ int lenIndex = buf.writerIndex();
+ buf.writeInt(0);
+ int start = buf.writerIndex();
+ if (value == 0) {
+ buf.writeByte('0');
+ } else {
+ boolean neg = value < 0;
+ if (neg) {
+ buf.writeByte('-');
+ if (value == Long.MIN_VALUE) {
+ writePositiveDigits(buf, 922337203685477580L);
+ buf.writeByte('8');
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ return;
+ }
+ value = -value;
+ }
+ writePositiveDigits(buf, value);
+ }
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ }
+
+ public static void writeDecimalInt(ByteBuf buf, int value) {
+ int lenIndex = buf.writerIndex();
+ buf.writeInt(0);
+ int start = buf.writerIndex();
+ if (value == 0) {
+ buf.writeByte('0');
+ } else {
+ boolean neg = value < 0;
+ if (neg) {
+ buf.writeByte('-');
+ if (value == Integer.MIN_VALUE) {
+ writePositiveDigits(buf, 214748364L);
+ buf.writeByte('8');
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ return;
+ }
+ value = -value;
+ }
+ writePositiveDigits(buf, value);
+ }
+ buf.setInt(lenIndex, buf.writerIndex() - start);
+ }
Review Comment:
`writeDecimalLong(...)` and `writeDecimalInt(...)` duplicate most of their
logic (length prefix, sign handling, MIN_VALUE special-case, and digit
writing). Consider consolidating into a single implementation (e.g., one
`writeDecimalLong` used by the `int` variant after widening + MIN_VALUE
handling), to reduce the chance of future divergence/bugs.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RocketMQSerializable.java:
##########
@@ -231,7 +306,7 @@ public static RemotingCommand rocketMQProtocolDecode(final
ByteBuf headerBuffer,
public static HashMap<String, String> mapDeserialize(ByteBuf byteBuffer,
int len) throws RemotingCommandException {
- HashMap<String, String> map = new HashMap<>(128);
+ HashMap<String, String> map = new HashMap<>(24);
Review Comment:
The new initial capacity `24` is a magic number. If it’s based on an
expected average field count, add a brief comment explaining the rationale (or
derive it from an available constant/heuristic) to make future tuning safer and
clearer.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RocketMQSerializable.java:
##########
@@ -52,6 +120,13 @@ private static String readStr(ByteBuf buf, boolean
useShortLength, int limit) th
if (len > limit) {
throw new RemotingCommandException("string length exceed limit:" +
limit);
}
+ if (len == 1) {
+ byte b = buf.readByte();
+ if (b >= 0) {
+ return SINGLE_BYTE_STRINGS[b];
+ }
+ return new String(new byte[]{b}, StandardCharsets.UTF_8);
+ }
CharSequence cs = buf.readCharSequence(len, StandardCharsets.UTF_8);
return cs == null ? null : cs.toString();
Review Comment:
This method uses `StandardCharsets.UTF_8` directly even though the class
already defines `private static final Charset CHARSET_UTF8 =
StandardCharsets.UTF_8;`. Using the existing constant here would keep charset
usage consistent and avoid duplicating the configuration/choice across the
class.
--
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]
