Re: RFR: 8268276: Base64 Decoding optimization for x86 using AVX-512 [v5]
On Fri, 18 Jun 2021 22:12:11 GMT, Scott Gibbons wrote: >> Add the Base64 Decode intrinsic for x86 to utilize AVX-512 for acceleration. >> Also allows for performance improvement for non-AVX-512 enabled platforms. >> Due to the nature of MIME-encoded inputs, modify the intrinsic signature to >> accept an additional parameter (isMIME) for fast-path MIME decoding. >> >> A change was made to the signature of DecodeBlock in Base64.java to provide >> the intrinsic information as to whether MIME decoding was being done. This >> allows for the intrinsic to bypass the expensive setup of zmm registers from >> AVX tables, knowing there may be invalid Base64 characters every 76 >> characters or so. A change was also made here removing the restriction that >> the intrinsic must return an even multiple of 3 bytes decoded. This >> implementation handles the pad characters at the end of the string and will >> return the actual number of characters decoded. >> >> The AVX portion of this code will decode in blocks of 256 bytes per loop >> iteration, then in chunks of 64 bytes, followed by end fixup decoding. The >> non-AVX code is an assembly-optimized version of the java DecodeBlock and >> behaves identically. >> >> Running the Base64Decode benchmark, this change increases decode performance >> by an average of 2.6x with a maximum 19.7x for buffers > ~20k. The numbers >> are given in the table below. >> >> **Base Score** is without intrinsic support, **Optimized Score** is using >> this intrinsic, and **Gain** is **Base** / **Optimized**. >> >> >> Benchmark Name | Base Score | Optimized Score | Gain >> -- | -- | -- | -- >> testBase64Decode size 1 | 15.36 | 15.32 | 1.00 >> testBase64Decode size 3 | 17.00 | 16.72 | 1.02 >> testBase64Decode size 7 | 20.60 | 18.82 | 1.09 >> testBase64Decode size 32 | 34.21 | 26.77 | 1.28 >> testBase64Decode size 64 | 54.43 | 38.35 | 1.42 >> testBase64Decode size 80 | 66.40 | 48.34 | 1.37 >> testBase64Decode size 96 | 73.16 | 52.90 | 1.38 >> testBase64Decode size 112 | 84.93 | 51.82 | 1.64 >> testBase64Decode size 512 | 288.81 | 32.04 | 9.01 >> testBase64Decode size 1000 | 560.48 | 40.79 | 13.74 >> testBase64Decode size 2 | 9530.28 | 483.37 | 19.72 >> testBase64Decode size 5 | 24552.24 | 1735.07 | 14.15 >> testBase64MIMEDecode size 1 | 22.87 | 21.36 | 1.07 >> testBase64MIMEDecode size 3 | 27.79 | 25.32 | 1.10 >> testBase64MIMEDecode size 7 | 44.74 | 43.81 | 1.02 >> testBase64MIMEDecode size 32 | 142.69 | 129.56 | 1.10 >> testBase64MIMEDecode size 64 | 256.90 | 243.80 | 1.05 >> testBase64MIMEDecode size 80 | 311.60 | 310.80 | 1.00 >> testBase64MIMEDecode size 96 | 364.00 | 346.66 | 1.05 >> testBase64MIMEDecode size 112 | 472.88 | 394.78 | 1.20 >> testBase64MIMEDecode size 512 | 1814.96 | 1671.28 | 1.09 >> testBase64MIMEDecode size 1000 | 3623.50 | 3227.61 | 1.12 >> testBase64MIMEDecode size 2 | 70484.09 | 64940.77 | 1.09 >> testBase64MIMEDecode size 5 | 191732.34 | 158158.95 | 1.21 >> testBase64WithErrorInputsDecode size 1 | 1531.02 | 1185.19 | 1.29 >> testBase64WithErrorInputsDecode size 3 | 1306.59 | 1170.99 | 1.12 >> testBase64WithErrorInputsDecode size 7 | 1238.11 | 1176.62 | 1.05 >> testBase64WithErrorInputsDecode size 32 | 1346.46 | 1138.47 | 1.18 >> testBase64WithErrorInputsDecode size 64 | 1195.28 | 1172.52 | 1.02 >> testBase64WithErrorInputsDecode size 80 | 1469.00 | 1180.94 | 1.24 >> testBase64WithErrorInputsDecode size 96 | 1434.48 | 1167.74 | 1.23 >> testBase64WithErrorInputsDecode size 112 | 1440.06 | 1162.56 | 1.24 >> testBase64WithErrorInputsDecode size 512 | 1362.79 | 1193.42 | 1.14 >> testBase64WithErrorInputsDecode size 1000 | 1426.07 | 1194.44 | 1.19 >> testBase64WithErrorInputsDecode size 2 | 1398.44 | 1138.17 | 1.23 >> testBase64WithErrorInputsDecode size 5 | 1409.41 | 1114.16 | 1.26 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Added comments. Streamlined flow for decode. src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6004: > 6002: __ BIND(L_continue); > 6003: > 6004: __ vpxor(errorvec, errorvec, errorvec, Assembler::AVX_512bit); Why clearing errorvec is needed here? src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6023: > 6021: __ evmovdquq(tmp16_op3, pack16_op, Assembler::AVX_512bit); > 6022: __ evmovdquq(tmp16_op2, pack16_op, Assembler::AVX_512bit); > 6023: __ evmovdquq(tmp16_op1, pack16_op, Assembler::AVX_512bit); Why do we need 3 additional copies of pack16_op? src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6026: > 6024: __ evmovdquq(tmp32_op3, pack32_op, Assembler::AVX_512bit); > 6025: __ evmovdquq(tmp32_op2, pack32_op, Assembler::AVX_512bit); > 6026: __ evmovdquq(tmp32_op1, pack32_op, Assembler::AVX_512bit); Why do we need 3 additional copies of pack32_op? src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6051: > 6049: __ vpternlogd(t0, 0xfe, input1, input2,
Integrated: Merge jdk17
On Fri, 18 Jun 2021 22:17:41 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: b7d78a5b Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/b7d78a5b661e2b00f271298db3b6cc873cf754e7 Stats: 12229 lines in 119 files changed: 6768 ins; 5337 del; 124 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4533
Re: RFR: Merge jdk17 [v2]
> Forwardport JDK 17 -> JDK 18 Jesper Wilhelmsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Merge - 8267042: bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header Co-authored-by: Chris Cole Reviewed-by: dsamersoff - 8268964: Remove unused ReferenceProcessorAtomicMutator Reviewed-by: tschatzl, pliden - 8268900: com/sun/net/httpserver/Headers.java: Fix indentation and whitespace Reviewed-by: dfuchs, chegar, michaelm - Merge - 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired Reviewed-by: xuelei - 8267189: Remove duplicated unregistered classes from dynamic archive Reviewed-by: ccheung, minqi - 8268638: semaphores of AsyncLogWriter may be broken when JVM is exiting. Reviewed-by: dholmes, phh - 8268556: Use bitmap for storing regions that failed evacuation Reviewed-by: kbarrett, iwalulya, sjohanss - 8268294: Reusing HttpClient in a WebSocket.Listener hangs. Reviewed-by: dfuchs - ... and 36 more: https://git.openjdk.java.net/jdk/compare/b8f073be...ed622f4b - Changes: https://git.openjdk.java.net/jdk/pull/4533/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4533=01 Stats: 8681 lines in 159 files changed: 7992 ins; 386 del; 303 mod Patch: https://git.openjdk.java.net/jdk/pull/4533.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4533/head:pull/4533 PR: https://git.openjdk.java.net/jdk/pull/4533
Re: RFR: 8268974: jpackage fails to handle --dest option containing "bin" folder
On Fri, 18 Jun 2021 22:46:24 GMT, Alexey Semenyuk wrote: > GetApplicationHomeFromDll() fails if the path to libjli.so contains "bin" > component (/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so). TruncatePath() > looks for "/bin/" first in "/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so" > string and then it looks for "/lib/". But this is wrong order as it should > look for "/lib/" first. I.e. TruncatePath() should look for "/bin/" and then > for "/lib/" if called from GetApplicationHome() and for "/lib/" first and > then for "/bin/" if called from GetApplicationHomeFromDll(). Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4534
RFR: 8268974: jpackage fails to handle --dest option containing "bin" folder
GetApplicationHomeFromDll() fails if the path to libjli.so contains "bin" component (/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so). TruncatePath() looks for "/bin/" first in "/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so" string and then it looks for "/lib/". But this is wrong order as it should look for "/lib/" first. I.e. TruncatePath() should look for "/bin/" and then for "/lib/" if called from GetApplicationHome() and for "/lib/" first and then for "/bin/" if called from GetApplicationHomeFromDll(). - Commit messages: - 8268974: jpackage fails to handle --dest option containing "bin" folder Changes: https://git.openjdk.java.net/jdk/pull/4534/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4534=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268974 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4534.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4534/head:pull/4534 PR: https://git.openjdk.java.net/jdk/pull/4534
Withdrawn: 8200559: Java agents doing instrumentation need a means to define auxiliary classes
On Fri, 16 Apr 2021 13:44:16 GMT, Rafael Winterhalter wrote: > To allow agents the definition of auxiliary classes, an API is needed to > allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or > `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed > from `sun.misc.Unsafe`. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/3546
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters
On Wed, 16 Jun 2021 20:22:17 GMT, Roger Riggs wrote: > Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory > property. > Fix description in the example of a filter allowing platform classes. > Suppress some warnings about use of SecurityManager in tests. OVERRIDE is also mentioned in `src/java.base/share/conf/security/java.security b/src/java.base/share/conf/security/java.security` - Changes requested by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/85
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18 - Commit messages: - Merge - 8268316: Typo in JFR jdk.Deserialization event - 8268638: semaphores of AsyncLogWriter may be broken when JVM is exiting. - 8264775: ClhsdbFindPC still fails with java.lang.RuntimeException: 'In java stack' missing from stdout/stderr - 8265073: XML transformation and indentation when using xml:space - 8269025: jsig/Testjsig.java doesn't check exit code - 8266518: Refactor and expand scatter/gather tests - 8268903: JFR: RecordingStream::dump is missing @since - 8265369: [macos-aarch64] java/net/MulticastSocket/Promiscuous.java failed with "SocketException: Cannot allocate memory" - 8268564: mark hotspot serviceability/attach tests which ignore external VM flags - ... and 13 more: https://git.openjdk.java.net/jdk/compare/8f2456e5...ed622f4b The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.java.net/jdk/pull/4533/files Stats: 12229 lines in 119 files changed: 6768 ins; 5337 del; 124 mod Patch: https://git.openjdk.java.net/jdk/pull/4533.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4533/head:pull/4533 PR: https://git.openjdk.java.net/jdk/pull/4533
Re: RFR: 8268276: Base64 Decoding optimization for x86 using AVX-512 [v5]
> Add the Base64 Decode intrinsic for x86 to utilize AVX-512 for acceleration. > Also allows for performance improvement for non-AVX-512 enabled platforms. > Due to the nature of MIME-encoded inputs, modify the intrinsic signature to > accept an additional parameter (isMIME) for fast-path MIME decoding. > > A change was made to the signature of DecodeBlock in Base64.java to provide > the intrinsic information as to whether MIME decoding was being done. This > allows for the intrinsic to bypass the expensive setup of zmm registers from > AVX tables, knowing there may be invalid Base64 characters every 76 > characters or so. A change was also made here removing the restriction that > the intrinsic must return an even multiple of 3 bytes decoded. This > implementation handles the pad characters at the end of the string and will > return the actual number of characters decoded. > > The AVX portion of this code will decode in blocks of 256 bytes per loop > iteration, then in chunks of 64 bytes, followed by end fixup decoding. The > non-AVX code is an assembly-optimized version of the java DecodeBlock and > behaves identically. > > Running the Base64Decode benchmark, this change increases decode performance > by an average of 2.6x with a maximum 19.7x for buffers > ~20k. The numbers > are given in the table below. > > **Base Score** is without intrinsic support, **Optimized Score** is using > this intrinsic, and **Gain** is **Base** / **Optimized**. > > > Benchmark Name | Base Score | Optimized Score | Gain > -- | -- | -- | -- > testBase64Decode size 1 | 15.36 | 15.32 | 1.00 > testBase64Decode size 3 | 17.00 | 16.72 | 1.02 > testBase64Decode size 7 | 20.60 | 18.82 | 1.09 > testBase64Decode size 32 | 34.21 | 26.77 | 1.28 > testBase64Decode size 64 | 54.43 | 38.35 | 1.42 > testBase64Decode size 80 | 66.40 | 48.34 | 1.37 > testBase64Decode size 96 | 73.16 | 52.90 | 1.38 > testBase64Decode size 112 | 84.93 | 51.82 | 1.64 > testBase64Decode size 512 | 288.81 | 32.04 | 9.01 > testBase64Decode size 1000 | 560.48 | 40.79 | 13.74 > testBase64Decode size 2 | 9530.28 | 483.37 | 19.72 > testBase64Decode size 5 | 24552.24 | 1735.07 | 14.15 > testBase64MIMEDecode size 1 | 22.87 | 21.36 | 1.07 > testBase64MIMEDecode size 3 | 27.79 | 25.32 | 1.10 > testBase64MIMEDecode size 7 | 44.74 | 43.81 | 1.02 > testBase64MIMEDecode size 32 | 142.69 | 129.56 | 1.10 > testBase64MIMEDecode size 64 | 256.90 | 243.80 | 1.05 > testBase64MIMEDecode size 80 | 311.60 | 310.80 | 1.00 > testBase64MIMEDecode size 96 | 364.00 | 346.66 | 1.05 > testBase64MIMEDecode size 112 | 472.88 | 394.78 | 1.20 > testBase64MIMEDecode size 512 | 1814.96 | 1671.28 | 1.09 > testBase64MIMEDecode size 1000 | 3623.50 | 3227.61 | 1.12 > testBase64MIMEDecode size 2 | 70484.09 | 64940.77 | 1.09 > testBase64MIMEDecode size 5 | 191732.34 | 158158.95 | 1.21 > testBase64WithErrorInputsDecode size 1 | 1531.02 | 1185.19 | 1.29 > testBase64WithErrorInputsDecode size 3 | 1306.59 | 1170.99 | 1.12 > testBase64WithErrorInputsDecode size 7 | 1238.11 | 1176.62 | 1.05 > testBase64WithErrorInputsDecode size 32 | 1346.46 | 1138.47 | 1.18 > testBase64WithErrorInputsDecode size 64 | 1195.28 | 1172.52 | 1.02 > testBase64WithErrorInputsDecode size 80 | 1469.00 | 1180.94 | 1.24 > testBase64WithErrorInputsDecode size 96 | 1434.48 | 1167.74 | 1.23 > testBase64WithErrorInputsDecode size 112 | 1440.06 | 1162.56 | 1.24 > testBase64WithErrorInputsDecode size 512 | 1362.79 | 1193.42 | 1.14 > testBase64WithErrorInputsDecode size 1000 | 1426.07 | 1194.44 | 1.19 > testBase64WithErrorInputsDecode size 2 | 1398.44 | 1138.17 | 1.23 > testBase64WithErrorInputsDecode size 5 | 1409.41 | 1114.16 | 1.26 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Added comments. Streamlined flow for decode. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4368/files - new: https://git.openjdk.java.net/jdk/pull/4368/files/247f2245..bb73df6c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4368=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4368=03-04 Stats: 44 lines in 1 file changed: 18 ins; 10 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/4368.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4368/head:pull/4368 PR: https://git.openjdk.java.net/jdk/pull/4368
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v2]
On Fri, 18 Jun 2021 20:27:13 GMT, Naoto Sato wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflect the review comments > > src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToHTMLStream.java > line 1454: > >> 1452: writer.write(ch); // no escaping in this case >> 1453: } >> 1454: else > > I was suggesting removing the entire comment-out block if it is not needed > (and confusing), but I will defer the decision to Joe. I agree. It's very obsolete. The comment-out block from line 1445 to 1454 can be removed. - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v2]
On Fri, 18 Jun 2021 04:56:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > Reflect the review comments src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToHTMLStream.java line 1454: > 1452: writer.write(ch); // no escaping in this case > 1453: } > 1454: else I was suggesting removing the entire comment-out block if it is not needed (and confusing), but I will defer the decision to Joe. test/jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest1.xml line 4: > 2: > 3: ட > 4: Add a new line at the end of the file (and other newly created files too). - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Mon, 14 Jun 2021 11:34:50 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. > I checked only places where `Vector` was used as local variable. The change to `PKCS7::verify(byte[])` looks fine. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 05:54:01 GMT, Yi Yang wrote:
>> After JDK-8265518(#3615), it's possible to replace all variants of
>> checkIndex by
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
>> the whole JDK codebase.
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> restore IndexOfOufBoundsException; split exception line
@kahatlen for cases earlier in VM startup you need to avoid method references
and lambda expressions. See the implementation of
`outOfBoundsExceptionFormatter`, and see the usage in `VarHandle` for two
examples.
Custom exception formaters should ideally be constants held in static final
fields.
I think the API complexity comes down to whether it is necessary to preserve
existing exception messages or not when converting existing code to use the
precondition methods. The API is designed to do that and composes reasonably
well for default exception messages with a non-default exceptions. We could
argue (i would!) it is preferable to have a consistent exception messages for
index out of bounds exceptions, thereby we could collapse and simplify, but
this is sometimes considered a change in behaviour.
src/java.base/share/classes/java/util/Base64.java line 935:
> 933: throw new IOException("Stream is closed");
> 934: Preconditions.checkFromIndexSize(len, off, b.length,
> 935:
> Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
`outOfBoundsExceptionFormatter` will allocate. Store the result of
`Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new))`
in a public static final on `Preconditions`, and replace the method ref with a
inner class (thereby making it usable earlier at VM startup.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Jun 18, 2021, at 6:36 AM, Alan Bateman mailto:al...@openjdk.java.net>> wrote: Adding an override of transferTo may require new tests. @bplb Do you if we have good tests for all the scenarios where input stream returned by Channels.newInputStream is the source? There are not a lot of tests for the `InputStream` returned by `Channels.newInputStream`: `java/nio/channels/Channels`: `Basic.java` - reading from a wrapped `FileChannel` `ReadByte.java` - trivial `Basic2.java` - reading at random offsets from a wrapped `AsynchronousSocketChannel` `ReadOffset.java` - trivial Elsewhere: `sun/nio/ch/TempBuffer.java` `jdk/nio/zipfs/ZipFSTester.java` I don’t see that `transferTo()` on such a stream is tested anywhere.
Re: RFR: 8266054: VectorAPI rotate operation optimization [v8]
On Tue, 8 Jun 2021 10:29:44 GMT, Jatin Bhateja wrote: >> Current VectorAPI Java side implementation expresses rotateLeft and >> rotateRight operation using following operations:- >> >> vec1 = lanewise(VectorOperators.LSHL, n) >> vec2 = lanewise(VectorOperators.LSHR, n) >> res = lanewise(VectorOperations.OR, vec1 , vec2) >> >> This patch moves above handling from Java side to C2 compiler which >> facilitates dismantling the rotate operation if target ISA does not support >> a direct rotate instruction. >> >> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over >> long and integer type vectors. For other cases (i.e. sub-word type vectors >> or for targets which do not support direct rotate operations ) instruction >> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted. >> >> Please find below the performance data for included JMH benchmark. >> Machine: Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz) >> >> >> Benchmark | (TESTSIZE) | Shift | Baseline AVX3 (ops/ms) | Withopt AVX3 >> (ops/ms) | Gain % | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain % >> -- | -- | -- | -- | -- | -- | -- | -- | -- >> | | | | | | | | >> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 17223.35 | 17094.69 | >> -0.75 | 17008.32 | 17488.06 | 2.82 >> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 8944.98 | 8811.34 | -1.49 >> | 8878.17 | 9218.68 | 3.84 >> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 17195.75 | 17137.32 | >> -0.34 | 16789.01 | 17780.34 | 5.90 >> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 9052.67 | 8838.60 | -2.36 >> | 8814.62 | 9206.01 | 4.44 >> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 17100.19 | 16950.64 | >> -0.87 | 16827.73 | 17720.37 | 5.30 >> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 9079.95 | 8471.26 | -6.70 >> | .44 | 9167.68 | 3.14 >> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 21231.33 | 21513.08 | 1.33 >> | 21824.51 | 21479.48 | -1.58 >> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 11103.62 | 11180.16 | 0.69 >> | 11173.67 | 11529.22 | 3.18 >> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 21119.14 | 21552.04 | >> 2.05 | 21693.05 | 21915.37 | 1.02 >> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 11048.68 | 11094.20 | >> 0.41 | 11049.90 | 11439.07 | 3.52 >> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 21506.31 | 21391.41 | >> -0.53 | 21263.18 | 21986.29 | 3.40 >> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 11056.12 | 11232.78 | >> 1.60 | 10941.59 | 11397.09 | 4.16 >> RotateBenchmark.testRotateLeftB | 512.00 | 7.00 | 17976.56 | 18180.85 | 1.14 >> | 1212.26 | 2533.34 | 108.98 >> RotateBenchmark.testRotateLeftB | 512.00 | 15.00 | 17553.70 | 18219.07 | >> 3.79 | 1256.73 | 2537.41 | 101.91 >> RotateBenchmark.testRotateLeftB | 512.00 | 31.00 | 17618.03 | 17738.15 | >> 0.68 | 1214.69 | 2533.83 | 108.60 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 7258.87 | 7468.88 | 2.89 | >> 7115.12 | 7117.26 | 0.03 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 3586.65 | 3950.85 | 10.15 >> | 3532.17 | 3595.80 | 1.80 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 1835.07 | 1999.68 | 8.97 | >> 1789.90 | 1819.93 | 1.68 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 7273.36 | 7410.91 | 1.89 >> | 7198.60 | 6994.79 | -2.83 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 3674.98 | 3926.27 | 6.84 >> | 3549.90 | 3755.09 | 5.78 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 1840.94 | 1882.25 | 2.24 >> | 1801.56 | 1872.89 | 3.96 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 7457.11 | 7361.48 | -1.28 >> | 6975.33 | 7385.94 | 5.89 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 3570.74 | 3929.30 | 10.04 >> | 3635.37 | 3736.67 | 2.79 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 1902.32 | 1960.46 | 3.06 >> | 1812.32 | 1813.88 | 0.09 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 11174.24 | 12044.52 | 7.79 >> | 11509.87 | 11273.44 | -2.05 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 5981.47 | 6073.70 | 1.54 | >> 5593.66 | 5661.93 | 1.22 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 2932.49 | 3069.54 | 4.67 | >> 2950.86 | 2892.42 | -1.98 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 11764.11 | 12098.63 | >> 2.84 | 11069.52 | 11476.93 | 3.68 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 5855.20 | 6080.40 | 3.85 >> | 5919.11 | 5607.04 | -5.27 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 2989.05 | 3048.56 | 1.99 >> | 2902.63 | 2821.83 | -2.78 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 11652.84 | 11965.40 | >> 2.68 | 11525.62 | 11459.83 | -0.57 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 5851.82 | 6164.94 | 5.35 >> | 5882.60 | 5842.30 | -0.69 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 3015.99 | 3043.79 | 0.92 >> | 2963.71 | 2947.97 | -0.53 >> RotateBenchmark.testRotateLeftI | 512.00 | 7.00 |
Re: [jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
On Fri, 18 Jun 2021 09:50:49 GMT, Aleksei Voitylov wrote: > Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 > against JDK17. > > This fixes the deadlock in ClassLoader between the two lock objects - a lock > object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library > load operation. > > Further details can be found in the original PR. > > Testing: jtreg and jck testing with no regressions. A new regression test was > developed. Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/96
[jdk17] Integrated: 8265073: XML transformation and indentation when using xml:space
On Thu, 17 Jun 2021 16:13:49 GMT, Joe Wang wrote: > The issue was that the attribute was processed before the variable was set > (e.g. m_preserveSpaces.push). Reversing the order fixed it. This pull request has now been integrated. Changeset: 7e03cf29 Author:Joe Wang URL: https://git.openjdk.java.net/jdk17/commit/7e03cf2916a69f947c46ac85b222ee7a99f68ad8 Stats: 60 lines in 2 files changed: 52 ins; 6 del; 2 mod 8265073: XML transformation and indentation when using xml:space Reviewed-by: naoto, lancea, iris - PR: https://git.openjdk.java.net/jdk17/pull/89
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v3]
On Fri, 18 Jun 2021 13:23:57 GMT, Jan Lahoda wrote:
>> Currently, an enum switch with patterns is desugared in a very non-standard,
>> and potentially slow, way. It would be better to use the standard
>> `typeSwitch` bootstrap to classify the enum constants. The bootstrap needs
>> to accept enum constants as labels in order to allow this. A complication is
>> that if an enum constant is missing, that is not an incompatible change for
>> the switch, and the switch should simply work as if the case for the missing
>> constant didn't exist. So, the proposed solution is to have a new bootstrap
>> `enumConstant` that converts the enum constant name to the enum constant,
>> returning `null`, if the constant does not exist. It delegates to
>> `ConstantBootstraps.enumConstant` to do the actual conversion. And
>> `typeSwitch` accepts `null`s as padding.
>>
>> How does this look?
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updating javadoc, code and tests as suggested.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 175:
> 173: * Bootstrap method for linking an {@code invokedynamic} call site
> that
> 174: * implements a {@code switch} on a target of an enum type. The
> static
> 175: * arguments are an array of case labels which must be non-null and
> of type
This sentence can still be improved and made cleared. Example:
> The static arguments are used to encode the case labels associated to the
> `switch` construct, where each label can be encoded as a `String` (e.g. to
> represent an enum constant), or, alternatively, as a `Class` (e.g. to
> represent a type test pattern whose type is an enum type).
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 179:
> 177: *
> 178: * The returned {@code CallSite}'s method handle will have
> 179: * a return type of {@code int} and accepts two parameters: the
> first argument
By writing the javadoc text above, it came to mind that, perhaps, some
validation is in order for the static arguments. For instance:
* the enum type of a Class parameter doesn't match that of the BSM
* the static arguments contain more than one Class parameter (I think even if
they are all of the same "correct" type, that's bogus, right?)
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Fri, 18 Jun 2021 09:47:29 GMT, Rémi Forax wrote:
>> This is to represent cases like:
>>
>> E sel = null;
>> switch (sel) {
>> case A -> {}
>> case E e && "B".equals(e.name()) -> {}
>> case C -> {}
>> case E e -> {}
>> }
>>
>>
>> The method needs to know which cases represent constants and which represent
>> patterns (even though the primary type of all the patterns will be the enum
>> type), so we cannot easily put the `Class` first (or elide it), and then a
>> `String...`, unless we represent the patterns in the `String...` array
>> somehow.
>
> Ok got it,
> At some point, we will have to represent patterns either as String and parse
> them or as Condy of condy if we want a more structured recursive way.
Of course - I missed it! Thanks for the explanation.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
[jdk17] Integrated: 8266518: Refactor and expand scatter/gather tests
On Mon, 14 Jun 2021 16:26:17 GMT, Paul Sandoz wrote: > Refactor scatter/gather tests to be included in the load/store test classes > and expand to support access between `ShortVector` and and `char[]`, and > access between `ByteVector` and `boolean[]`. > > Vector tests pass on linux-x64 linux-aarch64 macosx-x64, and windows-x64. This pull request has now been integrated. Changeset: dab00ee5 Author:Paul Sandoz URL: https://git.openjdk.java.net/jdk17/commit/dab00ee59b73bcd5b8632d127b3d0a324e48e4e5 Stats: 11673 lines in 72 files changed: 6367 ins; 5271 del; 35 mod 8266518: Refactor and expand scatter/gather tests Reviewed-by: sviswanathan - PR: https://git.openjdk.java.net/jdk17/pull/48
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Sun, 30 May 2021 17:30:56 GMT, Markus KARG wrote: > This PR-*draft* is **work in progress** and an invitation to discuss a > possible solution for issue > [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not > yet* intended for a final review. > > As proposed in JDK-8265891, this PR provides an implementation for > `Channels.newInputStream().transferTo()` which provide superior performance > compared to the current implementation. The changes are: > * Prevents transfers through the JVM heap as much as possibly by offloading > to deeper levels via NIO, hence allowing the operating system to optimize the > transfer. > * Using more JRE heap in the fallback case when no NIO is possible (still > only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern > hardware / fast I/O devides. > > Using JMH I have benchmarked both, the original implementation and this > implementation, and (depending on the used hardware and use case) performance > change was approx. doubled performance. So this PoC proofs that it makes > sense to finalize this work and turn it into an actual OpenJDK contribution. > > I encourage everybody to discuss this draft: > * Are there valid arguments for *not* doing this change? > * Is there a *better* way to improve performance of > `Channels.newInputStream().transferTo()`? > * How to go on from here: What is missing to get this ready for an actual > review? Adding an override of transferTo may require new tests. @bplb Do you if we have good tests for all the scenarios where input stream returned by Channels.newInputStream is the source? I think we also need to decide if this PR is about Channels.newInputStream or the input stream return by Files.newInputStream as the latter could return its own input stream implementation, it doesn't need to ChannelInputStream. If the approach on the table goes ahead then ChannelInputStream.transferTo can be split into 4 simple methods to make it easier to maintain. You can use something like "total" or "bytesWritten" for the total number of bytes written rather than "i". - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v3]
> Currently, an enum switch with patterns is desugared in a very non-standard, > and potentially slow, way. It would be better to use the standard > `typeSwitch` bootstrap to classify the enum constants. The bootstrap needs to > accept enum constants as labels in order to allow this. A complication is > that if an enum constant is missing, that is not an incompatible change for > the switch, and the switch should simply work as if the case for the missing > constant didn't exist. So, the proposed solution is to have a new bootstrap > `enumConstant` that converts the enum constant name to the enum constant, > returning `null`, if the constant does not exist. It delegates to > `ConstantBootstraps.enumConstant` to do the actual conversion. And > `typeSwitch` accepts `null`s as padding. > > How does this look? Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Updating javadoc, code and tests as suggested. - Changes: - all: https://git.openjdk.java.net/jdk17/pull/81/files - new: https://git.openjdk.java.net/jdk17/pull/81/files/4e1977a7..06a1bebf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=81=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=81=01-02 Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk17/pull/81.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/81/head:pull/81 PR: https://git.openjdk.java.net/jdk17/pull/81
[jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 against JDK17. This fixes the deadlock in ClassLoader between the two lock objects - a lock object associated with the class being loaded, and the ClassLoader.loadedLibraryNames hash map, locked during the native library load operation. Further details can be found in the original PR. Testing: jtreg and jck testing with no regressions. A new regression test was developed. - Commit messages: - JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class Changes: https://git.openjdk.java.net/jdk17/pull/96/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=96=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266310 Stats: 913 lines in 10 files changed: 894 ins; 1 del; 18 mod Patch: https://git.openjdk.java.net/jdk17/pull/96.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/96/head:pull/96 PR: https://git.openjdk.java.net/jdk17/pull/96
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Fri, 18 Jun 2021 09:08:04 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:
>>
>>> 220: String invocationName,
>>> 221: MethodType invocationType,
>>> 222: Object... labels) throws
>>> Throwable {
>>
>> Is it not better to take a Class and a String... as separated parameters
>> instead of taking Object... and doing the conversion to a Class and an array
>> of String later in Java
>
> This is to represent cases like:
>
> E sel = null;
> switch (sel) {
> case A -> {}
> case E e && "B".equals(e.name()) -> {}
> case C -> {}
> case E e -> {}
> }
>
>
> The method needs to know which cases represent constants and which represent
> patterns (even though the primary type of all the patterns will be the enum
> type), so we cannot easily put the `Class` first (or elide it), and then a
> `String...`, unless we represent the patterns in the `String...` array
> somehow.
Ok got it,
At some point, we will have to represent patterns either as String and parse
them or as Condy of condy if we want a more structured recursive way.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Thu, 17 Jun 2021 21:21:14 GMT, Maurizio Cimadamore wrote: > Very good piece of work. I like all the code that can be removed because of > this. Thanks! > > I assume that the new code only kicks in if there's at least a pattern in the > switch, otherwise we fallback to legacy translation (meaning that compiling > with source < 17 is still ok), right? Yes, it is only for pattern matching switches. Traditional switches are still desugared in the way they were. > > I left some comments to help and clarify the javadoc text of the enum BSM. - PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Thu, 17 Jun 2021 21:56:21 GMT, Rémi Forax wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Creating a new bootstrap method for (pattern matching) enum switches, as
>> suggested.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:
>
>> 220: String invocationName,
>> 221: MethodType invocationType,
>> 222: Object... labels) throws
>> Throwable {
>
> Is it not better to take a Class and a String... as separated parameters
> instead of taking Object... and doing the conversion to a Class and an array
> of String later in Java
This is to represent cases like:
E sel = null;
switch (sel) {
case A -> {}
case E e && "B".equals(e.name()) -> {}
case C -> {}
case E e -> {}
}
The method needs to know which cases represent constants and which represent
patterns (even though the primary type of all the patterns will be the enum
type), so we cannot easily put the `Class` first (or elide it), and then a
`String...`, unless we represent the patterns in the `String...` array somehow.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Thu, 17 Jun 2021 21:03:58 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Creating a new bootstrap method for (pattern matching) enum switches, as
>> suggested.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 173:
>
>> 171:
>> 172: /**
>> 173: * Bootstrap method for linking an {@code invokedynamic} call site
>> that
>
> This should be made clearer - e.g. the first argument is of type `Class` and
> represents the enum we want to switch on. The remaining constants should be
> of type `String`, the names of the various constants.
That is not quite what the labels represent. The target Enum type is inferred
from the bootstrap method's invocation MethodType. (Alternatively, we can add a
new Class parameter to the bootstrap method.)
For the labels, please consider this switch:
E sel = null;
switch (sel) {
case A -> {}
case E e && "B".equals(e.name()) -> {}
case C -> {}
case E e -> {}
}
The patterns and the constants are mixed, and the order needs to be represented
somehow in the labels array, so that the switch will classify the input
correctly. The method in this proposal will use `{"A", E.class, "C", E.class}`
as the labels array (which is mostly consistent with the `typeSwitch` method),
but we could use different encodings if needed.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Thu, 17 Jun 2021 20:08:08 GMT, Michael Bien
wrote:
>> This PR-*draft* is **work in progress** and an invitation to discuss a
>> possible solution for issue
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not
>> yet* intended for a final review.
>>
>> As proposed in JDK-8265891, this PR provides an implementation for
>> `Channels.newInputStream().transferTo()` which provide superior performance
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading
>> to deeper levels via NIO, hence allowing the operating system to optimize
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern
>> hardware / fast I/O devides.
>>
>> Using JMH I have benchmarked both, the original implementation and this
>> implementation, and (depending on the used hardware and use case)
>> performance change was approx. doubled performance. So this PoC proofs that
>> it makes sense to finalize this work and turn it into an actual OpenJDK
>> contribution.
>>
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual
>> review?
>
> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 216:
>
>> 214: }
>> 215:
>> 216: return super.transferTo(out);
>
> (i am no reviewer)
> wouldn't this method be more intuitive if it wouldn't try to avoid using
> "else if" and "else"? If there are sequential if blocks with return in them,
> part of me is always trying to find the fall-through scenario, but in this
> case its all distinct code paths. Using branches would make that obvious + if
> there would be a fall through in future, the compiler would generate a
> "missing return" error right away.
>
> example:
>
> if (out instanceof ChannelOutputStream cos) {
>
> WritableByteChannel oc = cos.channel();
> long i = 0L;
>
> if (ch instanceof FileChannel fc) {
>
> long pos = fc.position();
> long size = fc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferTo(pos + i, Long.MAX_VALUE, oc);
> return i;
> } finally {
> fc.position(pos + i);
> }
>
> } else if (oc instanceof FileChannel fc) {
>
> long fcpos = fc.position();
>
> if (ch instanceof SeekableByteChannel sbc) {
>
> long pos = sbc.position();
> long size = sbc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferFrom(ch, fcpos + i,
> Long.MAX_VALUE);
> return i;
> } finally {
> fc.position(fcpos + i);
> }
>
> } else {
>
> ByteBuffer bb =
> Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> int r;
> do {
> i += fc.transferFrom(ch, fcpos + i,
> Long.MAX_VALUE);
> r = ch.read(bb); // detect end-of-stream
> if (r > -1) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> } while (r > -1);
> return i;
> } finally {
> fc.position(fcpos + i);
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
>
> } else {
>
> ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> for (int r = ch.read(bb); r > -1; r = ch.read(bb)) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> return i;
> } finally {
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
> } else {
> return
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 05:54:01 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > restore IndexOfOufBoundsException; split exception line > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [serviceability-dev](mailto:serviceability-...@mail.openjdk.java.net):_ > > On 17/06/2021 8:50 pm, Alan Bateman wrote: > > > On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes > > wrote: > > > There are a lot more tests than just tier1. :) I don't expect many, if > > > any, tests to be looking for a specific IOOBE message, and I can't see an > > > easy way to find such tests without running them. If core-libs folk are > > > okay with this update then I guess we can just handle any test failures > > > if they arise. But I'll run this through our tier 1-3 testing. > > > > > > It would be good to run tier 1-3. Off hand I can't think of any tests that > > are dependent on the exception message, I think I'm more concerned about > > changing behavior (once you throw a more specific IOOBE is some of the very > > core classes then it's hard to change it because libraries get dependent on > > the more specific exception). > > tiers 1-3 on Linux-x64 passed okay. > > Any change to the exact type of exception thrown should be affirmed > through a CSR request. > > Cheers, > David Thank you David for running these tests! - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 15:45:47 GMT, Paul Sandoz wrote:
>> src/java.base/share/classes/java/util/Base64.java line 934:
>>
>>> 932: if (closed)
>>> 933: throw new IOException("Stream is closed");
>>> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
>>> xb) -> new ArrayIndexOutOfBoundsException());
>>
>> You might want to split this really long line too, to avoid inconsistent
>> line length in this source file.
>
> I would suggest factoring out `(xa, xb) -> new
> ArrayIndexOutOfBoundsException()` as a reusable component on `Preconditions`,
> and maybe even supplying an exception message (since it is rather useful, and
> way better than no message).
>
> See the usages of `Preconditions.outOfBoundsExceptionFormatter` (where there
> just happens to be many repeated cases of supplying AIOOBE with a message,
> that could also be consolidated, separate fix perhaps).
Ok, I've replaced
Preconditions.checkFromIndexSize(len, off, b.length, (xa, xb) -> new
ArrayIndexOutOfBoundsException());
with
Preconditions.checkFromIndexSize(len, off, b.length,
Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
I am curious about the motivations of current APIs:
public static
int checkIndex(int index, int length,
BiFunction, X> oobef) {
if (index < 0 || index >= length)
throw outOfBoundsCheckIndex(oobef, index, length);
return index;
}
Are they over-engineered? When I checked all checkIndex-like patterns in the
whole jdk codebase, I found that in most cases, providing an API that accepts
custom exception should be enough for scalability:
int checkIndex(int index, int length, IndexOutOfBoundException iooe) {
if (index < 0 || index >= length)
throw iooe;
return index;
}
In addition to the ease-of-use problem, there is another problem with the
current API design.
Some methods in j.l.String are good replacement candidates for
Preconditions.check{Index,...}:
https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/java/lang/String.java#L4558-L4604
But we **can not** do such replacement after my practice.
The third parameter of Preconditions.checkIndex is a BiFunction, which can be
passed a lambda as its argument. The generated lambda method exercises many
other codes like MethodHandles, j.l.Package, etc, eventually it called
j.l.String.checkIndex itself, so if we replace j.l.String.checkIndex with
`Preconditions.checkIndex(a,b,(x1,x2)->)`, a StackOverflowError would occur
at VM startup.
If there is an API that accepts user-defined exceptions, I think we can apply
Preconditions in more places.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
