You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/08/16 22:42:50 UTC
[GitHub] [geode] dschneider-pivotal opened a new pull request #6765: added appendAsciiDigitsToByteBuf to Coder so that ints and longs
dschneider-pivotal opened a new pull request #6765:
URL: https://github.com/apache/geode/pull/6765
can more efficiently by added so a ByteBuf. The goal was to reduce
the number of short lived object allocations.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r690750209
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
Review comment:
This char can be replaced with the `bMINUS` constant.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -176,4 +180,25 @@ public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input
};
}
+ @Test
+ public void verify_appendAsciiDigitsToByteBuf_conversions() {
+ verify_appendAsciiDigitsToByteBuf(Long.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Long.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MIN_VALUE);
+ for (long i = Byte.MIN_VALUE; i <= Byte.MAX_VALUE; i++) {
+ verify_appendAsciiDigitsToByteBuf(i);
+ }
+ }
+
+ private void verify_appendAsciiDigitsToByteBuf(long value) {
+ String expected = Long.toString(value);
+ ByteBuf buf = ByteBufAllocator.DEFAULT.heapBuffer();
+
+ Coder.appendAsciiDigitsToByteBuf(value, buf);
+
+ assertThat(buf.toString(Charset.defaultCharset())).isEqualTo(expected);
Review comment:
I think we might want to explicitly use UTF-8 as the charset here, as that's what Redis and the Coder class use.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
If we want to follow the "important note" about not using chars mentioned above, then these arrays will need to use the UTF-8 byte values associated with these chars, which will probably make it look horrible.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -155,12 +156,12 @@ private static void writeCollectionOrString(ByteBuf buffer, Object next) throws
public static ByteBuf getScanResponse(ByteBuf buffer, BigInteger cursor,
List<?> scanResult) {
buffer.writeByte(ARRAY_ID);
- buffer.writeBytes(intToBytes(2));
+ buffer.writeByte('2');
Review comment:
Per a comment at the top of `StringBytesGlossary` (which was originally in this class before being pulled out):
```
* Important note
* <p>
* Do not use '' <-- java primitive chars. Redis uses {@link Coder#CHARSET} encoding so we should
* not risk java handling char to byte conversions, rather just hard code {@link Coder#CHARSET}
* chars as bytes
```
Do we still want to follow this? If so, I think the solution is to introduce a `byte` constant to the `StringBytesGlossary` class for this char. We already have constants for 0 and 1, so this wouldn't be too out of the ordinary.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #6765: GEODE-9514: optimize how Coder reads and writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #6765:
URL: https://github.com/apache/geode/pull/6765
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691708615
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
these static arrays are now initialized with some static code using the byte constants.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691708372
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
Review comment:
done
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691574576
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
Maybe what made the old code safe is that after the jdk converted a long to a String we then called stringToBytes which converted to String chars to the hardcoded UTF-8 charset. In this new code that is no longer happening so if they are using a default charset that does not map '0' to the ASCII code for 0 then this code will be broken. I'll convert it over to using the constants.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r690750209
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
Review comment:
This char can be replaced with the `bMINUS` constant.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -176,4 +180,25 @@ public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input
};
}
+ @Test
+ public void verify_appendAsciiDigitsToByteBuf_conversions() {
+ verify_appendAsciiDigitsToByteBuf(Long.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Long.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MIN_VALUE);
+ for (long i = Byte.MIN_VALUE; i <= Byte.MAX_VALUE; i++) {
+ verify_appendAsciiDigitsToByteBuf(i);
+ }
+ }
+
+ private void verify_appendAsciiDigitsToByteBuf(long value) {
+ String expected = Long.toString(value);
+ ByteBuf buf = ByteBufAllocator.DEFAULT.heapBuffer();
+
+ Coder.appendAsciiDigitsToByteBuf(value, buf);
+
+ assertThat(buf.toString(Charset.defaultCharset())).isEqualTo(expected);
Review comment:
I think we might want to explicitly use UTF-8 as the charset here, as that's what Redis and the Coder class use.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
If we want to follow the "important note" about not using chars mentioned above, then these arrays will need to use the UTF-8 byte values associated with these chars, which will probably make it look horrible.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -155,12 +156,12 @@ private static void writeCollectionOrString(ByteBuf buffer, Object next) throws
public static ByteBuf getScanResponse(ByteBuf buffer, BigInteger cursor,
List<?> scanResult) {
buffer.writeByte(ARRAY_ID);
- buffer.writeBytes(intToBytes(2));
+ buffer.writeByte('2');
Review comment:
Per a comment at the top of `StringBytesGlossary` (which was originally in this class before being pulled out):
```
* Important note
* <p>
* Do not use '' <-- java primitive chars. Redis uses {@link Coder#CHARSET} encoding so we should
* not risk java handling char to byte conversions, rather just hard code {@link Coder#CHARSET}
* chars as bytes
```
Do we still want to follow this? If so, I think the solution is to introduce a `byte` constant to the `StringBytesGlossary` class for this char. We already have constants for 0 and 1, so this wouldn't be too out of the ordinary.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691566685
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
these static byte arrays came straight from the jdk so our old code was already using them when it called String.valueOf(long) which called Long.toString(long) which ended up in this code. Even the other code you commented on was for the most part from the jdk and was using '0' and '-'. So either the old code was already broken or the new code is okay.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691708161
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -155,12 +156,12 @@ private static void writeCollectionOrString(ByteBuf buffer, Object next) throws
public static ByteBuf getScanResponse(ByteBuf buffer, BigInteger cursor,
List<?> scanResult) {
buffer.writeByte(ARRAY_ID);
- buffer.writeBytes(intToBytes(2));
+ buffer.writeByte('2');
Review comment:
Thanks for pointing this out. It was helpful and I think it is a good idea. I have switch all the code over to using these byte constants instead of java char literals
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6765: GEODE-9514: optimize how Coder reads and writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6765:
URL: https://github.com/apache/geode/pull/6765#issuecomment-902168323
I like these most recent changes.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691708800
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
Review comment:
done
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6765: GEODE-9514: optimize how Coder reads and writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r692377660
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -332,9 +343,55 @@ public static BigDecimal bytesToBigDecimal(byte[] bytes) {
}
public static long bytesToLong(byte[] bytes) {
- return Long.parseLong(bytesToString(bytes));
+ return parseLong(bytes);
+ }
+
+ private static NumberFormatException createNumberFormatException(byte[] bytes) {
+ return new NumberFormatException("For input string: \"" + bytesToString(bytes) + "\"");
+ }
+
+ /**
+ * This method was derived from openjdk Long.java parseLong
+ */
+ public static long parseLong(byte[] bytes) throws NumberFormatException {
+ final int len = bytes.length;
+ if (len <= 0) {
+ throw createNumberFormatException(bytes);
+ }
+ int i = 0;
+ long limit = -Long.MAX_VALUE;
+ boolean negative = false;
+ byte firstByte = bytes[0];
+ if (firstByte < NUMBER_0_BYTE) { // Possible leading "+" or "-"
+ if (firstByte == bMINUS) {
+ negative = true;
+ limit = Long.MIN_VALUE;
+ } else if (firstByte != bPLUS) {
Review comment:
If we want to match Redis' behaviour exactly, we should also throw an exception if we're given a number that starts with "+" or with "-0". These could be done as separate tickets though, if you feel it's not worth including them here. In fact, we already have [GEODE-9482](https://issues.apache.org/jira/browse/GEODE-9482) to track the second case.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r691708291
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
Review comment:
done
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -176,4 +180,25 @@ public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input
};
}
+ @Test
+ public void verify_appendAsciiDigitsToByteBuf_conversions() {
+ verify_appendAsciiDigitsToByteBuf(Long.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Long.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MIN_VALUE);
+ for (long i = Byte.MIN_VALUE; i <= Byte.MAX_VALUE; i++) {
+ verify_appendAsciiDigitsToByteBuf(i);
+ }
+ }
+
+ private void verify_appendAsciiDigitsToByteBuf(long value) {
+ String expected = Long.toString(value);
+ ByteBuf buf = ByteBufAllocator.DEFAULT.heapBuffer();
+
+ Coder.appendAsciiDigitsToByteBuf(value, buf);
+
+ assertThat(buf.toString(Charset.defaultCharset())).isEqualTo(expected);
Review comment:
done
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6765: GEODE-9514: optimize how Coder reads and writes "int" and "long" values
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r692668692
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -332,9 +343,55 @@ public static BigDecimal bytesToBigDecimal(byte[] bytes) {
}
public static long bytesToLong(byte[] bytes) {
- return Long.parseLong(bytesToString(bytes));
+ return parseLong(bytes);
+ }
+
+ private static NumberFormatException createNumberFormatException(byte[] bytes) {
+ return new NumberFormatException("For input string: \"" + bytesToString(bytes) + "\"");
+ }
+
+ /**
+ * This method was derived from openjdk Long.java parseLong
+ */
+ public static long parseLong(byte[] bytes) throws NumberFormatException {
+ final int len = bytes.length;
+ if (len <= 0) {
+ throw createNumberFormatException(bytes);
+ }
+ int i = 0;
+ long limit = -Long.MAX_VALUE;
+ boolean negative = false;
+ byte firstByte = bytes[0];
+ if (firstByte < NUMBER_0_BYTE) { // Possible leading "+" or "-"
+ if (firstByte == bMINUS) {
+ negative = true;
+ limit = Long.MIN_VALUE;
+ } else if (firstByte != bPLUS) {
Review comment:
lets do that on another ticket. It does seem we might get some other benefits of having our own code to do the conversion instead of just using the jdk.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org