xiangfu0 commented on code in PR #20153:
URL: https://github.com/apache/pinot/pull/20153#discussion_r4474666376
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Utf8Utils.java:
##########
@@ -18,57 +18,71 @@
*/
package org.apache.pinot.spi.utils;
-import com.google.common.base.Utf8;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
+import java.nio.ByteOrder;
import java.nio.charset.StandardCharsets;
+/// UTF-8 helpers whose sizing and encoding agree: [#encodedLength] returns
exactly the length of the array that
+/// [#encode] produces for the same string.
public class Utf8Utils {
private Utf8Utils() {
}
+ // Byte order is irrelevant: isAscii() only ORs the bytes together
+ private static final VarHandle LONG_VIEW =
+ MethodHandles.byteArrayViewVarHandle(long[].class,
ByteOrder.nativeOrder());
+
public static byte[] encode(String s) {
return s.getBytes(StandardCharsets.UTF_8);
}
+ /// Returns the number of bytes in the UTF-8 encoding of `s`, which is
exactly `encode(s).length`. An unpaired
+ /// surrogate counts as the single replacement byte that [String#getBytes]
writes for it, so this never throws.
public static int encodedLength(String s) {
Review Comment:
Removing the public `encodedLengthWithReplacement(String)` breaks binary
compatibility. It shipped in release-1.6.0, and the Binary Compatibility Check
fails on exactly this
(`Utf8Utils.encodedLengthWithReplacement(java.lang.String):METHOD_REMOVED`).
Consider keeping it as a `@Deprecated` one-line delegate to `encodedLength`, or
get explicit maintainer sign-off on the break.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5)
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Utf8Utils.java:
##########
@@ -18,57 +18,71 @@
*/
package org.apache.pinot.spi.utils;
-import com.google.common.base.Utf8;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
+import java.nio.ByteOrder;
import java.nio.charset.StandardCharsets;
+/// UTF-8 helpers whose sizing and encoding agree: [#encodedLength] returns
exactly the length of the array that
+/// [#encode] produces for the same string.
public class Utf8Utils {
private Utf8Utils() {
}
+ // Byte order is irrelevant: isAscii() only ORs the bytes together
+ private static final VarHandle LONG_VIEW =
+ MethodHandles.byteArrayViewVarHandle(long[].class,
ByteOrder.nativeOrder());
+
public static byte[] encode(String s) {
return s.getBytes(StandardCharsets.UTF_8);
}
+ /// Returns the number of bytes in the UTF-8 encoding of `s`, which is
exactly `encode(s).length`. An unpaired
+ /// surrogate counts as the single replacement byte that [String#getBytes]
writes for it, so this never throws.
public static int encodedLength(String s) {
- return Utf8.encodedLength(s);
- }
-
- /// Returns the UTF-8 byte length using the same malformed UTF-16
replacement behavior as [String#getBytes()].
- /// Unlike [#encodedLength(String)], this method does not reject unpaired
surrogate code units.
- public static int encodedLengthWithReplacement(String s) {
- int length = 0;
- int index = 0;
- while (index < s.length()) {
- char character = s.charAt(index);
- if (character <= 0x7f) {
- length++;
- } else if (character <= 0x7ff) {
- length += 2;
- } else if (Character.isHighSurrogate(character) && index + 1 < s.length()
- && Character.isLowSurrogate(s.charAt(index + 1))) {
- length += 4;
- index++;
- } else if (Character.isSurrogate(character)) {
- length++;
+ int utf16Length = s.length();
+ // Fast path for the common all-ASCII prefix, one byte per char
+ int i = 0;
+ while (i < utf16Length && s.charAt(i) < 0x80) {
+ i++;
+ }
+ int utf8Length = i;
+ for (; i < utf16Length; i++) {
+ char c = s.charAt(i);
+ if (c < 0x80) {
+ utf8Length++;
+ } else if (c < 0x800) {
Review Comment:
Measured slowdown versus the Guava baseline on non-ASCII text. JMH on JDK
25.0.5 over 1024 strings of 64 chars, 2 forks × 5 iterations, reproduced in a
second run:
- Latin text with 1 in 8 chars `é`: 22.8 → 52.4 µs/op (2.3x)
- CJK: 19.1 → 34.8 µs/op (1.8x)
- ASCII: unchanged
- Emoji: about 10% faster
A branchless form avoids the mispredicted `c < 0x80` branch. Start from
`utf8Length = utf16Length`, add `(0x7f - c) >>> 31` when `c < 0x800`, add 2 for
other non-surrogate chars and for a surrogate pair (then `i++`), and add
nothing for an unpaired surrogate. That brings Latin back to 23.0 µs and CJK to
26.4 µs, and it agreed with `getBytes(UTF_8).length` on 2M fuzzed strings. This
is not blocking, but it is a per-value ingestion hot path.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
