Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v3]
On Sat, 8 Jun 2024 23:30:38 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > change method name, putHex -> putHex4, and fix comments > > You are right, ByteArray and ByteArrayLittleEndian have good performance > > after removing Unsafe. This is similar to the previous version of > > java.io.Bits > > Do you have evidence that `VarHandle` affects startup time? If there is good > evidence to support this, I would go ahead and rewrite #14636. @cl4es has fixed startup regression issues, such as this https://github.com/openjdk/jdk/pull/15836 - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156332130
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v3]
On Sun, 9 Jun 2024 01:16:49 GMT, Shaojin Wen wrote: > You are right, ByteArray and ByteArrayLittleEndian have good performance > after removing Unsafe. This is similar to the previous version of java.io.Bits Do you have evidence that `VarHandle` affects startup time? If there is good evidence to support this, I would go ahead and rewrite https://github.com/openjdk/jdk/pull/14636. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156311883
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v3]
On Sat, 8 Jun 2024 23:30:38 GMT, Shaojin Wen wrote:
>> After PR #16245, C2 optimizes stores into primitive arrays by combining
>> values into larger stores. In the UUID.toString method,
>> ByteArrayLittleEndian can be removed, making the code more elegant and
>> faster.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change method name, putHex -> putHex4, and fix comments
> I think we don't need to change them back everywhere, but only need to
> rewrite `ByteArrayLittleEndian` and `ByteArray` so that they no longer use
> `VarHandle`.
>
> Maybe I should rewrite #14636 without using `Unsafe`, so more people might
> agree with it.
You are right, ByteArray and ByteArrayLittleEndian have good performance after
removing Unsafe. This is similar to the previous version of java.io.Bits
class ByteArrayLittleEndian {
public static void setInt(byte[] array, int offset, int value) {
array[offset] = (byte) value;
array[offset + 1] = (byte) (value >> 8);
array[offset + 2] = (byte) (value >> 16);
array[offset + 3] = (byte) (value >> 24);
}
}
class HexDigits {
public static void putHex4(byte[] array, int offset, int value) {
// Prepare an int value so C2 generates a 4-byte write instead of two
2-byte writes
ByteArrayLittleEndian.setInt(
array,
offset,
(DIGITS[value & 0xff] << 16) | DIGITS[(value >> 8) & 0xff]);
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156255748
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 18:00:43 GMT, Chen Liang wrote:
>> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 122:
>>
>>> 120: * @param value to convert
>>> 121: */
>>> 122: public static void putHex(byte[] buffer, int off, int i) {
>>
>> Should there be 2 methods - for 2 and 4 bytes respectively?
>> Does c2 optimize 8 byte writes as well?
>
> The 4-byte unsigned int input for 8-byte write sounds plausible, I personally
> am fine either with or without it.
>
>> Does c2 optimize 8 byte writes as well?
>
> From the first few lines of #16245:
>
>> Merging multiple consecutive small stores (e.g. 8 byte stores) into larger
>> stores (e.g. one long store) can lead to speedup.
8-byte writing requires converting int to long. The performance is similar to
the current version, but an additional method putHex8 needs to be added. The
current version has less code.
The following is the code for writing 8 bytes:
class UUID {
@Override
public String toString() {
byte[] buf = new byte[36];
HexDigits.putHex8(buf, 0, (int) (mostSigBits >> 32));
HexDigits.putHex10(buf, 8, (int) mostSigBits);
HexDigits.putHex10(buf, 18, (int) (leastSigBits >> 32));
HexDigits.putHex8(buf, 28, (int) leastSigBits);
try {
return jla.newStringNoRepl(buf, StandardCharsets.ISO_8859_1);
} catch (CharacterCodingException cce) {
throw new AssertionError(cce);
}
}
}
class HexDigits {
public static void putHex8(byte[] bytes, int off, int i) {
long v = (((long) DIGITS[(i >> 16) & 0xff]) << 48)
| (((long) DIGITS[(i >> 24) & 0xff]) << 32)
| (DIGITS[ i& 0xff] << 16)
| (DIGITS[(i >> 8 ) & 0xff]);
bytes[off] = (byte) v;
bytes[off + 1] = (byte) (v >> 8);
bytes[off + 2] = (byte) (v >> 16);
bytes[off + 3] = (byte) (v >> 24);
bytes[off + 4] = (byte) (v >> 32);
bytes[off + 5] = (byte) (v >> 40);
bytes[off + 6] = (byte) (v >> 48);
bytes[off + 7] = (byte) (v >> 56);
}
public static void putHex10(byte[] bytes, int off, int i) {
int v0 = (DIGITS[(i >> 16) & 0xff] << 16)
| DIGITS[(i >> 24) & 0xff];
int v1 = (DIGITS[i & 0xff] << 16)
| DIGITS[(i >> 8 ) & 0xff];
bytes[off] = '-';
bytes[off + 1] = (byte) v0;
bytes[off + 2] = (byte) (v0 >> 8);
bytes[off + 3] = (byte) (v0 >> 16);
bytes[off + 4] = (byte) (v0 >> 24);
bytes[off + 5] = '-';
bytes[off + 6] = (byte) v1;
bytes[off + 7] = (byte) (v1 >> 8);
bytes[off + 8] = (byte) (v1 >> 16);
bytes[off + 9] = (byte) (v1 >> 24);
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632133853
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v3]
> After PR #16245, C2 optimizes stores into primitive arrays by combining > values into larger stores. In the UUID.toString method, > ByteArrayLittleEndian can be removed, making the code more elegant and faster. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: change method name, putHex -> putHex4, and fix comments - Changes: - all: https://git.openjdk.org/jdk/pull/19610/files - new: https://git.openjdk.org/jdk/pull/19610/files/ace7a9f6..d6b4ed25 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19610&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19610&range=01-02 Stats: 12 lines in 2 files changed: 1 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19610.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19610/head:pull/19610 PR: https://git.openjdk.org/jdk/pull/19610
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments Changes requested by e...@github.com (no known OpenJDK username). src/java.base/share/classes/jdk/internal/util/HexDigits.java line 120: > 118: * @param buffer byte buffer to copy into > 119: * @param off insert point > 120: * @param value to convert The parameter is called i. Maybe also add "Only least significant 16 bits are used." Even when it's only a internal api. - PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2105936973 PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632105594
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 15:51:13 GMT, Brett Okken wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add comments
>
> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 117:
>
>> 115:
>> 116: /**
>> 117: * Insert the int into the buffer as 4 hexadecimal digits
>
> 4 hex digits is 2 bytes, right?
> should this take a short instead?
> At a minimum, seems documentation should be clear as to which 2 bytes are
> used.
Yes, 4 hex digits requires 2 bytes of input integer; we always take the least
significant bytes. I think taking an int is better than taking a short, as
subint types are int in bytecode, and short's signedness may be bug-prone.
> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 122:
>
>> 120: * @param value to convert
>> 121: */
>> 122: public static void putHex(byte[] buffer, int off, int i) {
>
> Should there be 2 methods - for 2 and 4 bytes respectively?
> Does c2 optimize 8 byte writes as well?
The 4-byte unsigned int input for 8-byte write sounds plausible, I personally
am fine either with or without it.
> Does c2 optimize 8 byte writes as well?
>From the first few lines of #16245:
> Merging multiple consecutive small stores (e.g. 8 byte stores) into larger
> stores (e.g. one long store) can lead to speedup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632081700
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632082369
Re: RFR: 8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Sun, 22 Oct 2023 17:26:52 GMT, Laurent Bourgès wrote: >> * improved mixed insertion sort (makes whole sorting faster) >> * introduced Radix which sort shows several times boost of performance and >> has linear complexity instead of n*ln(n) >> * improved merging sort for almost sorted data >> * optimized parallel sorting >> * improved step for pivot candidates and pivot partitioning >> * extended existing tests >> * added benchmarking JMH tests >> * suggested better buffer allocation: if no memory, it is switched to >> in-place sorting with no OutOfMemoryError, threshold is 1/16th heap >> >> I am going on previous PR by Vladimir Yaroslavskyi: >> https://github.com/openjdk/jdk/pull/3938 > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > add @SuppressWarnings (serial) Version for other data types is being prepared - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-2156111460
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote:
>> After PR #16245, C2 optimizes stores into primitive arrays by combining
>> values into larger stores. In the UUID.toString method,
>> ByteArrayLittleEndian can be removed, making the code more elegant and
>> faster.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add comments
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 122:
> 120: * @param value to convert
> 121: */
> 122: public static void putHex(byte[] buffer, int off, int i) {
Should there be 2 methods - for 2 and 4 bytes respectively?
Does c2 optimize 8 byte writes as well?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632068305
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments src/java.base/share/classes/jdk/internal/util/HexDigits.java line 117: > 115: > 116: /** > 117: * Insert the int into the buffer as 4 hexadecimal digits 4 hex digits is 2 bytes, right? should this take a short instead? At a minimum, seems documentation should be clear as to which 2 bytes are used. - PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632064197
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments I think we don't need to change them back everywhere, but only need to rewrite `ByteArrayLittleEndian` and `ByteArray` so that they no longer use `VarHandle`. Maybe I should rewrite https://github.com/openjdk/jdk/pull/14636 without using `Unsafe`, so more people might agree with it. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156063742
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2105860441
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments And in addition, VarHandle is not initialized unless it's necessary; thus, programs that use UUIDs but not VarHandle no longer need to initialize VarHandle. See #15386 where JDK startup has a performance degradation because it had to initialize VarHandle after using BALE. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156044329
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments The C2 optimization brought by PR #16245 makes many of the previous performance improvement techniques based on VarHandle/ByteArray/Unsafe no longer meaningful, and many optimizations based on this need to be changed back. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156040853
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 12:25:54 GMT, Sunmisc Unsafe wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add comments > > As far as I know, ByteArrayLittleEndian uses the VarHandle mechanism, which > more efficiently writes different primitives into the array, unlike the basic @sunmisc BALE uses byte array view VH which still uses Unsafe: https://github.com/openjdk/jdk/blob/8d2f9e57c3797c01c84df007f4d2bfdcd645d0c0/src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template#L141 Please take a look at #16245; you will see that C2 now JIT compiles these compatible "different primitives" like Unsafe would do, yet there's a bit of requirement on code shape. Thus I recommended the comment for wenshao, so future changes won't accidentally destroy the code shape and the optimization. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156036174
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
On Fri, 7 Jun 2024 18:58:36 GMT, Claes Redestad wrote: >> This PR refactors type matching in BootstrapMethodInvoker and adds a few >> types, seeking to improve bootstrap overheads of some ConstantBootstraps and >> in particular the ProxyGenerator condys generated for e.g. annotation >> proxies since [JDK-8332457](https://bugs.openjdk.org/browse/JDK-8332457) >> >> I've adjusted the micro-benchmark added by JDK-8332457 to not only generate >> a proxy but also call into one of the proxied methodt (`Object::hashCode`). >> >> Running org.openjdk.bench.java.lang.reflect.ProxyGenBench as a one-off >> startup benchmark sees significant improvement (-9% instructions, -6% >> cycles): >> >> Name Cnt Base ErrorTest >> Error Unit Change >> Perfstartup-JMH 20154,000 ±8,165 148,000 ± >> 23,164ms/op 1,04x (p = 0,352 ) >> :.cycles925335973,200 ± 47147600,262 842221278,800 ± >> 46836254,964 cycles 0,91x (p = 0,000*) >> :.instructions 2101588857,600 ± 81105850,361 1966307798,400 ± >> 22011043,908 instructions 0,94x (p = 0,000*) >> :.taskclock 291,500 ± 16,494 262,000 ± >> 15,328 ms 0,90x (p = 0,000*) >> * = significant >> >> Number of classes loaded drops from 1096 to 1092 >> >> Running the micro regularly shows no significant difference: >> >> Name Cnt Base ErrorTest Error Unit >> Change >> ProxyGenBench.generateAndProxy100 10 26,827 ± 8,954 26,855 ± 7,531 ms/op >> 1,00x (p = 0,991 ) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Adress review comments, add ConstantBootstraps#invoke to list of recognized > type signatures I think Jorn is recommending to use invokeBasic with erased types as how LambdaForm does it. We can try that approach, but we need new tests to verify that bootstrap method type mismatches throw the same exceptions, so that should be another RFE. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19598#pullrequestreview-2105846859
Integrated: 8333749: Consolidate ConstantDesc conversion in java.base
On Thu, 6 Jun 2024 18:35:01 GMT, Chen Liang wrote: > In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. This pull request has now been integrated. Changeset: 8d2f9e57 Author:Chen Liang Committer: Claes Redestad URL: https://git.openjdk.org/jdk/commit/8d2f9e57c3797c01c84df007f4d2bfdcd645d0c0 Stats: 255 lines in 13 files changed: 113 ins; 47 del; 95 mod 8333749: Consolidate ConstantDesc conversion in java.base Co-authored-by: Claes Redestad Reviewed-by: redestad, jvernee - PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
On Fri, 7 Jun 2024 19:22:43 GMT, Chen Liang wrote: >> It's not the intent of this PR to exhaustively enumerate all methods in >> `ConstantBootstraps`, primarily those shown to be bootstrap sensitive in >> some app. I've so far never seen a use of `primitiveClass` (and I admit >> being ignorant as to why this even exists as a BSM) so I don't have any >> reason to believe special-casing it will carry its own weight. > > For its existence, I think it was the same as null, that they weren't > representable by cp entries for bootstrap method args. They are now backed by > PrimitiveClassDescImpl, a type of condy, and ClassFile API currently writes > loadConstant(primitiveCd) as a condy. I am sure there's a perfectly good explanation why we've done it like that rather than translating things like javac would (AFAICT a load from `Integer/Short/Byte/...TYPE`). - PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1632041009
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments As far as I know, ByteArrayLittleEndian uses the VarHandle mechanism, which more efficiently writes different primitives into the array, unlike the basic - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156018093
