On Sun, 9 Aug 2026 08:20:38 GMT, Liam Miller-Cushon <[email protected]> wrote:
> This change generalizes `String#bytesCompatible` and > `String#copyToSegmentRaw` to support UTF-16, see this comment in the existing > code: > > https://github.com/openjdk/jdk/blob/479d43dc63674e9d7e6b543a899e76e070a632fc/src/java.base/share/classes/java/lang/String.java#L2135-L2137 > > Those APIs are used to avoid copies in `SegmentAllocator#allocateFrom` and > `MemorySegment#copy` when `String` data is compatible with the destination > charset. This is possible for UTF-16 when the platform and target charset > endianness match (UTF-16LE on little endian platforms, or UTF-16BE on big > endian) and when the UTF-16 string contains no unpaired surrogates (which > require copying the data and handling replacement characters). > > * Test coverage has been added in > `test/jdk/java/foreign/TestStringEncoding.java`, and existing coverage was > generalized to exercise multi-byte charsets like UTF-16. > * UTF-16 constants were added in `sun.nio.cs` to allow reference comparisons > of charsets, similar to existing handling of other charsets. > * An allocation-free fast path was added to `encodedLength` for UTF-16. > > (This is partly related to [this panama-dev@ > thread](https://mail.openjdk.org/archives/list/[email protected]/thread/YSPX3TBXZTZEFZNXWDYOZCWLUQPC62P6/). > UTF-16 is a less widely used charset for data outside the JVM, which is why > the existing `bytesCompatible` / `copyToSegmentRaw` fast paths focused on > UTF-8. Having fast paths for UTF-16 is interesting inside the JVM because of > UTF-16's treatment in the String APIs, it's the only option for zero-copy > String operations on non-latin1 Strings.) > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). I've left some remarks. Note that I'm not involved with FFM, Panama, etc. Hence, I cannot judge the usefulness of the functionality. I'm just a `String` stakeholder and sharing my 5 cents on the correctness. src/java.base/share/classes/java/lang/String.java line 1655: > 1653: } > 1654: sp++; > 1655: } Does this pre-scan bring any measurable benefits? Have you considered collapsing the two loops and retaining the fast checks? Consider the following: for (int sp = off, sl = off + len; sp < sl;) { char c = StringUTF16.getChar(val, sp++); if (c < Character.MIN_HIGH_SURROGATE) { continue; } if (c > Character.MAX_LOW_SURROGATE) { continue; } if (sp == sl || !Character.isLowSurrogate(StringUTF16.getChar(val, sp))) { return true; } sp++; } return false; src/java.base/share/classes/java/lang/String.java line 1672: > 1670: } > 1671: } > 1672: return false; Can we simplify this as follows? while (sp < sl) { char c = StringUTF16.getChar(val, sp++); if (Character.isSurrogate(c)) { if (!Character.isHighSurrogate(c) || sp == sl || !Character.isLowSurrogate(StringUTF16.getChar(val, sp))) { return true; } sp++; } } return false; src/java.base/share/classes/java/lang/String.java line 2159: > 2157: return length() << 1; > 2158: } else if (cs == UTF_16.INSTANCE) { > 2159: return (length() << 1) + 2; // BOM These expressions can become negative or wrap. We need to add OOME guards similar to the ones in other paths, e.g., `encodedLengthUTF8_UTF16`. These edge cases should be tested too. src/java.base/share/classes/java/lang/String.java line 2179: > 2177: } > 2178: > 2179: // This method is intended to be used together with bytesCompatible. Can we make this a Javadoc with a valid link to the `bytesCompatible` method, please? This will make IDE-assisted jumping/linking in between two easier. src/java.base/share/classes/java/lang/String.java line 2180: > 2178: > 2179: // This method is intended to be used together with bytesCompatible. > 2180: int copyToSegmentRaw(MemorySegment segment, long offset, int > srcIndex, int srcLength) { Not a direct goal of this PR, but since you're substantially changing this method anyway... Shouldn't this method be checking its arguments? src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 1: > 1: /* Copyright year needs to be updated. src/java.base/share/classes/java/nio/charset/StandardCharsets.java line 1: > 1: /* Copyright year needs to be updated. src/java.base/share/classes/jdk/internal/foreign/StringSupport.java line 1: > 1: /* Copyright year needs to be updated. src/java.base/share/classes/sun/nio/cs/UTF_16.java line 1: > 1: /* Copyright year needs to be updated. src/java.base/share/classes/sun/nio/cs/UTF_16BE.java line 1: > 1: /* Copyright year needs to be updated. src/java.base/share/classes/sun/nio/cs/UTF_16LE.java line 1: > 1: /* Copyright year needs to be updated. test/jdk/java/foreign/TestStringEncoding.java line 612: > 610: for (Charset charset : standardCharsets()) { > 611: boolean expected = compatibleCharsets.contains(charset); > 612: boolean actual = StringSupport.bytesCompatible(string, > charset, 0, string.length()); Shall we also exercise non-zero `srcIndex`? Consider split surrogate pairs. (This can also be a separate test with only this concern.) test/jdk/java/foreign/TestStringEncoding.java line 613: > 611: boolean expected = compatibleCharsets.contains(charset); > 612: boolean actual = StringSupport.bytesCompatible(string, > charset, 0, string.length()); > 613: assertEquals(actual, expected); On failures, the detail will be very coarse. That is, I will get an `AssertionFailure` for the input, say, { List.of("", "hello world", "123"), Set.of( StandardCharsets.US_ASCII, StandardCharsets.ISO_8859_1, StandardCharsets.UTF_8), } But it won't be clear which case has failed. `"123" & US_ASCII`? `"hello world" & UTF_8`? What do you think about flattening the output of `stringsAndCompatibleCharsets`? Or adding some detail message to `assertEquals`? test/jdk/java/foreign/TestStringEncoding.java line 627: > 625: String substring = > string.substring(srcIndex, srcIndex + numChars); > 626: var segment = > arena.allocate(substring.encodedLength(charset)); > 627: StringSupport.copyToSegmentRaw(string, > segment, 0, srcIndex, numChars); Shall we also exercise non-zero `offset`? (This can also be a separate test with only this concern.) test/jdk/java/foreign/TestStringEncoding.java line 628: > 626: var segment = > arena.allocate(substring.encodedLength(charset)); > 627: StringSupport.copyToSegmentRaw(string, > segment, 0, srcIndex, numChars); > 628: assertEquals(segment.toArray(JAVA_BYTE), > substring.getBytes(charset)); What do you think about adding some detail message to `assertEquals`? test/jdk/java/foreign/TestStringEncoding.java line 790: > 788: return new Object[][] { > 789: { > 790: List.of("", "hello world", "123"), `""` would work for `nativeUtf16` too, no? test/jdk/java/foreign/TestStringEncoding.java line 805: > 803: "cjk \u4E00\u4E8C", > 804: "rainbow \uD83C\uDF08", > 805: "\uD83D\uDE00"), Can we extend this with a valid UTF-16 code unit above U+DFFF? test/jdk/java/foreign/TestStringEncoding.java line 809: > 807: }, > 808: { > 809: List.of("unpaired surrogate \uD83C", "\uD83D", "\uDC00", > "\uDC00\uD83C"), Set.of(), I'd appreciate it if we can change this `strings` list such that 1. Start all with a valid explanation: `"unpaired high surrogate: \uD83C", ...` 2. Add `"high surrogate followed by a non-low surrogate: \uD83C\uE000"` 3. Add `"valid pair followed by an unpaired low surrogate: \uD83D\uDE00\uDC00"` ------------- PR Review: https://git.openjdk.org/jdk/pull/32268#pullrequestreview-4895056307 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3748006172 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747984782 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749424713 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3748149683 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3748182735 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747957596 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747956622 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747955817 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747954728 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747954330 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3747953698 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749163712 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3748675983 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749170388 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3748811560 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749240802 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749201175 PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3749295322
