On Tue, 3 Sep 2024 00:38:40 GMT, Shaojin Wen <s...@openjdk.org> wrote:
>> Use fast path for ascii characters 1 to 127 to improve the performance of
>> writing Utf8Entry to BufferWriter.
>
> Shaojin Wen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - code style
> - remove unsafe
I think this is looking much better now, thanks! There's some polish I'd like
to see made, though.
src/java.base/share/classes/java/lang/System.java line 2597:
> 2595: }
> 2596:
> 2597: public byte stringCoder(String s) {
I think it'd be better to do `boolean isLatin1(String s)` here. It's still
somewhat leaky, but keeps things a bit more high-level, containing more of the
low-level details. Seems all uses basically just checks `coder == 0` anyhow.
src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java line
182:
> 180: byte[] elems = this.elems;
> 181:
> 182: str.getBytes(0, countGreaterThanZero, elems, offset);
Might be beneficial to non-latin1 case to move this up into the latin1 block.
Could you verify the 256 limit is useful? Add a couple of cases to the micro
with much longer latin1 strings; both ASCII and with non-ASCII at the end.
Would be nice if we can keep this simple and the "too long" check feels a bit
premature.
src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java line
201:
> 199: }
> 200: int utflen = offset - start - 2;
> 201: if (utflen > 65535) {
Previously we'd count bytes and throw before writing to the byte array. Perhaps
this is still preferable, even if it comes at a cost.
test/micro/org/openjdk/bench/java/lang/classfile/Utf8EntryWriteTo.java line 76:
> 74: byte[] bytes = HexFormat.of().parseHex(
> 75: switch (charType) {
> 76: case "ascii" -> "78";
Might be good to randomize these inputs a bit.
-------------
Changes requested by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20772#pullrequestreview-2276820291
PR Review Comment: https://git.openjdk.org/jdk/pull/20772#discussion_r1741719346
PR Review Comment: https://git.openjdk.org/jdk/pull/20772#discussion_r1741713216
PR Review Comment: https://git.openjdk.org/jdk/pull/20772#discussion_r1741717901
PR Review Comment: https://git.openjdk.org/jdk/pull/20772#discussion_r1741688429
