Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
> `ClassReaderImpl.buffer` manipulations into an
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
>
> Please review.
>
> Thanks,
> Adam
Adam Sotona has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains three commits:
- Merge branch 'master' into JDK-8320360-bounds
# Conflicts:
#test/jdk/jdk/classfile/LimitsTest.java
- added bug # to the test
- 8320360: ClassFile.parse: Some defect class files cause unexpected
exceptions to be thrown
-
Changes: https://git.openjdk.org/jdk/pull/16762/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16762=01
Stats: 69 lines in 2 files changed: 47 ins; 0 del; 22 mod
Patch: https://git.openjdk.org/jdk/pull/16762.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16762/head:pull/16762
PR: https://git.openjdk.org/jdk/pull/16762
Re: RFR: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access) [v2]
On Wed, 6 Dec 2023 17:03:08 GMT, Maurizio Cimadamore wrote: >> This PR fixes a couple of aligned accesses when reading/writing strings. >> Such aligned accesses crept in when we optimized string read/write >> operations to work in bulk. As a result, depending on the maximum alignment >> constraints supported by the heap segment, some string operations might fail. >> >> I've added some tests to make sure that all operations work as expected with >> unaligned semantics. >> >> Note: I've considered inferring an alignment constraint from the provided >> charset, and then use aligned operations (and document that behavior), but I >> found that to be unsatisfactory: memory operations typically accept a >> layout, which allow clients to opt out from alignment checks if needed. But >> if we always infer an alignment constraint from the provided charset, >> clients would find themselves w/o an escape hatch. For this reason, I think >> the best way to fix this is to use unaligned operations when reading/writing >> the string. > > Maurizio Cimadamore has updated the pull request incrementally with two > additional commits since the last revision: > > - Make test more robust > - Simplify test LGTM. Testing looks solid. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16999#pullrequestreview-1769404275
Re: RFR: JDK-8320538: Obsolete CSS styles in collection framework doc-file
On Wed, 6 Dec 2023 16:22:24 GMT, Hannes Wallnöfer wrote:
> Please review a simple change to remove a stray inline CSS element from the
> Collection Framework index doc file. The only thing the `a {font-weight:
> bold;}` rule did was to make all links in the header and footer bold as [can
> be seen here][1]; the three content links to the other doc files are still
> displayed in bold font because they are contained in `` tags. The other
> CSS rules had no effect in the page.
>
> [1]:
> https://download.java.net/java/early_access/jdk22/docs/api/java.base/java/util/doc-files/coll-index.html
>
> Screenshot of the fixed page:
> src="https://github.com/openjdk/jdk/assets/15975/0bd3b613-e535-438c-bd4d-7c8817df5702;>
>
> I also added the `` meta tag which is present in all other pages (both doc-files
> and generated by JavaDoc).
Marked as reviewed by alanb (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/16997#pullrequestreview-1769396161
RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable
This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 time frame. It is fixing a deadlock issue between `VirtualThread` class critical sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, `getAndClearInterrupt()`, `threadState()`, `toString()`), `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. The deadlocking scenario is well described by Patricio in a bug report comment. In simple words, a virtual thread should not be suspended during 'interruptLock' critical sections. The fix is to record that a virtual thread is in a critical section (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about begin/end of critical section. This bit is used in `HandshakeState::get_op_for_self()` to filter out any `HandshakeOperation` if a target `JavaThread` is in a critical section. Some of new notifications with `notifyJvmtiSync()` method is on a performance critical path. It is why this method has been intrincified. New test was developed by Patricio: `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` The test is very nice as it reliably in 100% reproduces the deadlock without the fix. The test is never failing with this fix. Testing: - tested with newly added test: `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` - tested with mach5 tiers 1-6 - Commit messages: - added @summary to new test SuspendWithInterruptLock.java - add new test SuspendWithInterruptLock.java - 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable Changes: https://git.openjdk.org/jdk/pull/17011/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17011=00 Issue: https://bugs.openjdk.org/browse/JDK-8311218 Stats: 183 lines in 15 files changed: 178 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v8]
> Currently the vector floating-point math APIs like > `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, > which causes large performance gap on AArch64. Note that those APIs are > optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. > To close the gap, we would like to optimize these APIs for AArch64 by calling > a third-party vector library called libsleef [2], which are available in > mainstream Linux distros (e.g. [3] [4]). > > SLEEF supports multiple accuracies. To match Vector API's requirement and > implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA > instructions used stubs in libsleef for most of the operations by default, > and 2) add the vector calling convention to apply with the runtime calls to > stub code in libsleef. Note that for those APIs that libsleef does not > support 1.0 ULP, we choose 0.5 ULP instead. > > To help loading the expected libsleef library, this patch also adds an > experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. > People can use it to denote the libsleef path/name explicitly. By default, it > points to the system installed library. If the library does not exist or the > dynamic loading of it in runtime fails, the math vector ops will fall-back to > use the default scalar version without error. But a warning is printed out if > people specifies a nonexistent library explicitly. > > Note that this is a part of the original proposed patch in panama-dev [5], > just with some initial review comments addressed. And now we'd like to get > some wider feedbacks from more hotspot experts. > > [1] https://github.com/openjdk/jdk/pull/3638 > [2] https://sleef.org/ > [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/ > [4] https://packages.debian.org/bookworm/libsleef3 > [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html Xiaohong Gong has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Remove -fvisibility in makefile and add the attribute in source code - Merge branch 'jdk:master' into JDK-8312425 - Add "--with-libsleef-lib" and "--with-libsleef-include" options - Separate neon and sve functions into two source files - Merge branch 'jdk:master' into JDK-8312425 - Rename vmath to sleef in configure - Address review comments in build system - Add a bundled native lib in jdk as a bridge to libsleef - Merge 'jdk:master' into JDK-8312425 - Disable sleef by default - ... and 2 more: https://git.openjdk.org/jdk/compare/6feb6794...c55357b6 - Changes: - all: https://git.openjdk.org/jdk/pull/16234/files - new: https://git.openjdk.org/jdk/pull/16234/files/f3ff0672..c55357b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16234=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16234=06-07 Stats: 83778 lines in 1591 files changed: 38309 ins; 39305 del; 6164 mod Patch: https://git.openjdk.org/jdk/pull/16234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16234/head:pull/16234 PR: https://git.openjdk.org/jdk/pull/16234
Re: RFR: JDK-8319413: Start of release updates for JDK 23 [v8]
> Time to start making preparations for JDK 23. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update copyright year. - Changes: - all: https://git.openjdk.org/jdk/pull/16505/files - new: https://git.openjdk.org/jdk/pull/16505/files/4a871372..a1aa187c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16505=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16505=06-07 Stats: 6 lines in 6 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16505/head:pull/16505 PR: https://git.openjdk.org/jdk/pull/16505
Re: RFR: JDK-8319413: Start of release updates for JDK 23 [v7]
> Time to start making preparations for JDK 23. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Regenerate JDK 22 symbol files. - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Add symbol files. - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - ... and 11 more: https://git.openjdk.org/jdk/compare/632a3c56...4a871372 - Changes: https://git.openjdk.org/jdk/pull/16505/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16505=06 Stats: 4866 lines in 98 files changed: 4828 ins; 0 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/16505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16505/head:pull/16505 PR: https://git.openjdk.org/jdk/pull/16505
Re: RFR: 8321409: Console read line with zero out should zero out underlying buffer in JLine (redux)
On Wed, 6 Dec 2023 21:12:40 GMT, Naoto Sato wrote: > This is an additional fix to JDK-8321131, where more clearing is required in > JLine. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17004#pullrequestreview-1769310866
Re: RFR: 8320198: some reference processing tests don't wait long enough for reference processing to complete
On Mon, 4 Dec 2023 17:46:18 GMT, Tom Rodriguez wrote:
> This slightly increases the wait for reference processing to complete to
> accommodate long Xcomp compile times. Testing is underway.
test/jdk/java/lang/Object/FinalizationOption.java line 89:
> 87: static boolean checkFinalizerCalled(boolean expected) {
> 88: create();
> 89: for (int i = 0; i < 100; i++) {
Hello Tom, I think this entire loop can be replaced by using the
`jdk.test.lib.util.ForceGC` utility class available in the tests. That class
has the necessary knowledge of using the jtreg timeout factor. I think you
could replace this loop with something like (I haven't tried it):
ForceGC.wait(() -> finalizerWasCalled);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16956#discussion_r1418373430
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final [v2]
On Wed, 6 Dec 2023 17:42:47 GMT, Brett Okken wrote: >> The static AtomicInteger used for the nextHashCode should be final. > > Brett Okken has updated the pull request incrementally with one additional > commit since the last revision: > > Update full name The change looks fine to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16987#pullrequestreview-1769179862
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
On Wed, 6 Dec 2023 23:09:01 GMT, Srinivas Vamsi Parasa wrote: >>> LGTM, thanks! >> >> Thanks Jatin! > >> @vamsi-parasa, sorry, I was wrong. I missed that you need to check type >> `bt`. Latest change is more complicated than it was before. Please revert it >> back (undo last change). I will test previous version 09. > @vnkozlov > Vladimir, please see the commit reverted in the updated changes pushed now. > @vamsi-parasa, please, remind me which tests check that code in > `libsmdsort.so` is used? @vnkozlov Please see the tests for simd sort code in `test/jdk/java/util/Arrays/Sorting.java` - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843963054
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
On Wed, 6 Dec 2023 23:09:01 GMT, Srinivas Vamsi Parasa wrote: >>> LGTM, thanks! >> >> Thanks Jatin! > >> @vamsi-parasa, sorry, I was wrong. I missed that you need to check type >> `bt`. Latest change is more complicated than it was before. Please revert it >> back (undo last change). I will test previous version 09. > @vnkozlov > Vladimir, please see the commit reverted in the updated changes pushed now. @vamsi-parasa, please, remind me which tests check that code in `libsmdsort.so` is used? - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843938518
RFR: 8321180: Condition for non-latin1 string size too large exception is off by one
In the compact string implementation of non-latin1 (UTF16) strings the length is constrained by VM implementation limit on the size a byte array that can be allocated. To produce a useful exception the implementation checks the string size against the maximum byte array size. The exception message is correct "UTF16 String size is " + len + ", should be less than or equal to " + MAX_LENGTH The code should match the message, otherwise the exception thrown is: java.lang.OutOfMemoryError: Requested array size exceeds VM limit - Commit messages: - 8321180: Condition for non-latin1 string size too large is off by one Changes: https://git.openjdk.org/jdk/pull/17008/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17008=00 Issue: https://bugs.openjdk.org/browse/JDK-8321180 Stats: 129 lines in 2 files changed: 119 ins; 7 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17008.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17008/head:pull/17008 PR: https://git.openjdk.org/jdk/pull/17008
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
On Wed, 6 Dec 2023 23:12:13 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX2 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> For serial sort on random data, this PR shows upto ~7.5x improvement for >> 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the >> performance data below. >> >> For parallel sort on random data, this PR shows upto ~3.4x for 32-bit >> datatypes (int, float) as shown below. >> >> **Note:** This PR also improves the performance of AVX512 sort by upto 35%. >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | >> Speedup >> -- | -- | -- | -- | -- >> ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 >> ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 >> ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 >> ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 >> ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 >> ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 >> ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 >> ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 >> ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 >> ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 >> ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 >> ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 >> ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 >> ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 >> ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 >> ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 >> ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 >> ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 >> >> >> >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Change supported intrinsic check" > > This reverts commit 9621eb045c2958582f81ec06b237789a07481ddd. Good. I submitted testing. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843839669
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
On Wed, 6 Dec 2023 17:44:24 GMT, Srinivas Vamsi Parasa wrote: >> LGTM, thanks! > >> LGTM, thanks! > > Thanks Jatin! > @vamsi-parasa, sorry, I was wrong. I missed that you need to check type `bt`. > Latest change is more complicated than it was before. Please revert it back > (undo last change). I will test previous version 09. @vnkozlov Vladimir, please see the commit reverted in the updated changes pushed now. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843834085
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
> The goal is to develop faster sort routines for x86_64 CPUs by taking > advantage of AVX2 instructions. This enhancement provides an order of > magnitude speedup for Arrays.sort() using int, long, float and double arrays. > > For serial sort on random data, this PR shows upto ~7.5x improvement for > 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the > performance data below. > > For parallel sort on random data, this PR shows upto ~3.4x for 32-bit > datatypes (int, float) as shown below. > > **Note:** This PR also improves the performance of AVX512 sort by upto 35%. > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> > > > > > > > > > Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | > Speedup > -- | -- | -- | -- | -- > ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 > ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 > ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 > ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 > ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 > ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 > ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 > ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 > ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 > ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 > ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 > ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 > ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 > ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 > ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 > ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 > ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 > ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 > > > > > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/... Srinivas Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: Revert "Change supported intrinsic check" This reverts commit 9621eb045c2958582f81ec06b237789a07481ddd. - Changes: - all: https://git.openjdk.org/jdk/pull/16534/files - new: https://git.openjdk.org/jdk/pull/16534/files/9621eb04..eadba369 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16534=11 - incr: https://webrevs.openjdk.org/?repo=jdk=16534=10-11 Stats: 28 lines in 4 files changed: 0 ins; 20 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/16534.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16534/head:pull/16534 PR: https://git.openjdk.org/jdk/pull/16534
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 17:44:24 GMT, Srinivas Vamsi Parasa wrote: >> LGTM, thanks! > >> LGTM, thanks! > > Thanks Jatin! @vamsi-parasa, sorry, I was wrong. I missed that you need to check type `bt`. Latest change is more complicated than it was before. Please revert it back (undo last change). I will test previous version 09. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843822075
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v3]
> Re-write the IndexOf code without the use of the pcmpestri instruction, only > using AVX2 instructions. This change accelerates String.IndexOf on average > 1.3x for AVX2. The benchmark numbers: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Support UU IndexOf - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/e614b86f..5e03173e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=01-02 Stats: 20 lines in 1 file changed: 6 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit Looks good; simpler and more direct is an improvement. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16986#pullrequestreview-1768704534
Integrated: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters
On Tue, 5 Dec 2023 14:57:09 GMT, Jim Laskey wrote: > A regression is found in Java9+ creating String instance from UTF8 bytes, a > side effect of string compactation https://openjdk.org/jeps/254 that changed > the decoding logic. Specifically, when constructing a string from bytes: > > ``` > String str = new String(largeBytes, StandardCharsets.UTF_8); > ``` > > if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than > INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack > trace is slightly different, see below), regardless of jvm heap size. In > Java8, it succeeded when jvm heap size is set to be sufficient. This pull request has now been integrated. Changeset: 82796bde Author:Jim Laskey URL: https://git.openjdk.org/jdk/commit/82796bdebbf56b98ec97a6d572ed68c2842f60c6 Stats: 82 lines in 2 files changed: 75 ins; 0 del; 7 mod 8320570: NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters Reviewed-by: rriggs - PR: https://git.openjdk.org/jdk/pull/16974
RFR: 8321409: Console read line with zero out should zero out underlying buffer in JLine (redux)
This is an additional fix to JDK-8321131, where more clearing is required in JLine. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/17004/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17004=00 Issue: https://bugs.openjdk.org/browse/JDK-8321409 Stats: 12 lines in 3 files changed: 11 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17004.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17004/head:pull/17004 PR: https://git.openjdk.org/jdk/pull/17004
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
> Currently, Locale-related system properties, such as `user.language` or > `user.country`, are initialized when the `Locale` class is loaded. Making > them static properties is safer than relying on the `Locale` class loading > timing, which could potentially be changed depending on the implementation. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Reflects review comments - Merge branch 'master' into JDK-8321206-Locale-static-properties - Add exclusions in cdsHeapVerifier for new StaticProperties - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/16986/files - new: https://git.openjdk.org/jdk/pull/16986/files/929f339b..35396106 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16986=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16986=00-01 Stats: 38933 lines in 413 files changed: 13835 ins; 23669 del; 1429 mod Patch: https://git.openjdk.org/jdk/pull/16986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16986/head:pull/16986 PR: https://git.openjdk.org/jdk/pull/16986
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 18:10:05 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 415:
>>
>>> 413: public static String userRegion() {
>>> 414: return USER_REGION;
>>> 415: }
>>
>> Using methods to retrieve these makes is more complicated.
>> The bleeding of the enum values outside of Locale is undesirable.
>> Since the property values are final strings, I suggest just making the
>> fields public and keep the mapping local to the Locale class.
>
> As Alan commented, I will not use `Locale.Category.ordinal()` but instead use
> the properties keys. Would that address your suggestion?
Ended up replacing public methods with public fields
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417964470
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 18:41:26 GMT, Vladimir Kozlov wrote:
>> Srinivas Vamsi Parasa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> add missing header files
>
> src/hotspot/share/opto/library_call.cpp line 5393:
>
>> 5391: if (!Matcher::supports_simd_sort(bt)) {
>> 5392: return false;
>> 5393: }
>
> This check should be in `C2Compiler::is_intrinsic_supported()`
Hi Vladimir (@vnkozlov), please see the updated changes which use
`C2Compiler::is_intrinsic_supported(id, bt)`
> src/hotspot/share/opto/library_call.cpp line 5450:
>
>> 5448: if (!Matcher::supports_simd_sort(bt)) {
>> 5449: return false;
>> 5450: }
>
> Same.
Please see the updated changes which use C2Compiler::is_intrinsic_supported(id,
bt)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417946689
PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417946968
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v11]
> The goal is to develop faster sort routines for x86_64 CPUs by taking > advantage of AVX2 instructions. This enhancement provides an order of > magnitude speedup for Arrays.sort() using int, long, float and double arrays. > > For serial sort on random data, this PR shows upto ~7.5x improvement for > 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the > performance data below. > > For parallel sort on random data, this PR shows upto ~3.4x for 32-bit > datatypes (int, float) as shown below. > > **Note:** This PR also improves the performance of AVX512 sort by upto 35%. > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> > > > > > > > > > Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | > Speedup > -- | -- | -- | -- | -- > ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 > ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 > ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 > ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 > ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 > ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 > ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 > ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 > ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 > ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 > ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 > ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 > ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 > ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 > ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 > ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 > ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 > ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 > > > > > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/... Srinivas Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: Change supported intrinsic check - Changes: - all: https://git.openjdk.org/jdk/pull/16534/files - new: https://git.openjdk.org/jdk/pull/16534/files/7e124581..9621eb04 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16534=10 - incr: https://webrevs.openjdk.org/?repo=jdk=16534=09-10 Stats: 28 lines in 4 files changed: 20 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/16534.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16534/head:pull/16534 PR: https://git.openjdk.org/jdk/pull/16534
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v5]
> A regression is found in Java9+ creating String instance from UTF8 bytes, a > side effect of string compactation https://openjdk.org/jeps/254 that changed > the decoding logic. Specifically, when constructing a string from bytes: > > ``` > String str = new String(largeBytes, StandardCharsets.UTF_8); > ``` > > if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than > INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack > trace is slightly different, see below), regardless of jvm heap size. In > Java8, it succeeded when jvm heap size is set to be sufficient. Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8320570 - Alternate 64 bit test - Exclude 32 bit - Requested changes - Bump up memory - Cotrrect NegativeSize.java - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/16974/files - new: https://git.openjdk.org/jdk/pull/16974/files/9926adda..8ae170dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16974=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16974=03-04 Stats: 39190 lines in 426 files changed: 14144 ins; 23593 del; 1453 mod Patch: https://git.openjdk.org/jdk/pull/16974.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16974/head:pull/16974 PR: https://git.openjdk.org/jdk/pull/16974
Re: RFR: 8320759: Creation of random BigIntegers can be made faster [v3]
On Wed, 6 Dec 2023 15:07:59 GMT, fabioromano1 wrote: >> So, item 1 is a non-issue and item 2 is not likely a problem in practice. >> What, if anything, will be done about item 3? > > @bplb Maybe an assertion at the end of `randomBits(int, Random)` method, or a > test class. @fabioromano1 As suggested earlier, you might want to take a look [here](https://github.com/openjdk/jdk/blob/master/test/jdk/java/math/BigInteger/ByteArrayConstructorTest.java) to see how the fields can be accessed from your test, preferably JUnit and preferably placing it into that same folder. - PR Comment: https://git.openjdk.org/jdk/pull/16817#issuecomment-1843479248
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 17:48:04 GMT, Srinivas Vamsi Parasa wrote:
>> The goal is to develop faster sort routines for x86_64 CPUs by taking
>> advantage of AVX2 instructions. This enhancement provides an order of
>> magnitude speedup for Arrays.sort() using int, long, float and double arrays.
>>
>> For serial sort on random data, this PR shows upto ~7.5x improvement for
>> 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the
>> performance data below.
>>
>> For parallel sort on random data, this PR shows upto ~3.4x for 32-bit
>> datatypes (int, float) as shown below.
>>
>> **Note:** This PR also improves the performance of AVX512 sort by upto 35%.
>>
>> > xmlns:o="urn:schemas-microsoft-com:office:office"
>> xmlns:x="urn:schemas-microsoft-com:office:excel"
>> xmlns="http://www.w3.org/TR/REC-html40;>
>>
>>
>>
>>
>>
>> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm">
>> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml">
>>
>>
>>
>>
>>
>>
>>
>>
>> Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) |
>> Speedup
>> -- | -- | -- | -- | --
>> ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2
>> ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0
>> ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5
>> ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5
>> ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2
>> ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4
>> ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6
>> ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3
>> ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1
>> ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0
>> ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4
>> ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3
>> ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0
>> ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9
>> ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4
>> ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7
>> ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0
>> ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6
>>
>>
>>
>>
>>
>> > xmlns:o="urn:schemas-microsoft-com:office:office"
>> xmlns:x="urn:schemas-microsoft-com:office:excel"
>> xmlns="http://www.w3.org/TR/REC-html40;>
>>
>>
>>
>>
> Srinivas Vamsi Parasa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> add missing header files
src/hotspot/share/opto/library_call.cpp line 5393:
> 5391: if (!Matcher::supports_simd_sort(bt)) {
> 5392: return false;
> 5393: }
This check should be in `C2Compiler::is_intrinsic_supported()`
src/hotspot/share/opto/library_call.cpp line 5450:
> 5448: if (!Matcher::supports_simd_sort(bt)) {
> 5449: return false;
> 5450: }
Same.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417831171
PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417832246
Re: RFR: JDK-8319413: Start of release updates for JDK 23 [v6]
> Time to start making preparations for JDK 23. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Regenerate JDK 22 symbol files. - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Add symbol files. - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Merge branch 'master' into JDK-8319413 - Update symbol files to JDK 22 b26. - ... and 8 more: https://git.openjdk.org/jdk/compare/db5613af...efe849f6 - Changes: https://git.openjdk.org/jdk/pull/16505/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16505=05 Stats: 4866 lines in 98 files changed: 4828 ins; 0 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/16505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16505/head:pull/16505 PR: https://git.openjdk.org/jdk/pull/16505
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 18:26:34 GMT, Vladimir Kozlov wrote: >> @TobiHartmann @vnkozlov Please advice if we can go head and integrate this >> PR today before the fork. > >> @TobiHartmann @vnkozlov Please advice if we can go head and integrate this >> PR today before the fork. > > Too late. Changes looks fine to me (I am still on fence that we moving to C++ > implementation of intrinsics and require latest C++ compiler version). I need > to run testing for latest version of changes before approval. Lets not rush. Thanks a lot @vnkozlov, we will wait for your approval. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843454162
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 18:05:22 GMT, Sandhya Viswanathan wrote: > @TobiHartmann @vnkozlov Please advice if we can go head and integrate this PR > today before the fork. Too late. Changes looks fine to me (I am still on fence that we moving to C++ implementation of intrinsics and require latest C++ compiler version). I need to run testing for latest version of changes before approval. Lets not rush. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843446956
Re: RFR: 8321206: Make Locale related system properties `StaticProperty`
On Wed, 6 Dec 2023 15:18:27 GMT, Roger Riggs wrote:
>> Currently, Locale-related system properties, such as `user.language` or
>> `user.country`, are initialized when the `Locale` class is loaded. Making
>> them static properties is safer than relying on the `Locale` class loading
>> timing, which could potentially be changed depending on the implementation.
>
> src/java.base/share/classes/java/util/Locale.java line 1061:
>
>> 1059: if (sm != null) {
>> 1060: sm.checkPropertiesAccess();
>> 1061: }
>
> This SM check is no longer needed; the use of privilegedGetProperties made
> access unconditional.
> The static properties are always accessible to the locale implementation.
Was wondering so. Will remove them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417796853
Re: RFR: 8321206: Make Locale related system properties `StaticProperty`
On Wed, 6 Dec 2023 15:27:48 GMT, Roger Riggs wrote:
>> Currently, Locale-related system properties, such as `user.language` or
>> `user.country`, are initialized when the `Locale` class is loaded. Making
>> them static properties is safer than relying on the `Locale` class loading
>> timing, which could potentially be changed depending on the implementation.
>
> src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 113:
>
>> 111: USER_EXTENSIONS_DISPLAY = getProperty(props,
>> "user.extensions.display", USER_EXTENSIONS);
>> 112: USER_EXTENSIONS_FORMAT = getProperty(props,
>> "user.extensions.format", USER_EXTENSIONS);
>> 113: USER_REGION = getProperty(props, "user.region", "");
>
> Computing the defaults for these properties should be in the Locale class,
> close to the initialization of the default locale. Splitting the
> responsibility across files makes it harder to follow what happens where/when.
If I am not mistaken, these assignments should stay here in `StaticProperty`,
as that is the whole purpose of this change. Depending on `Locale` class
loading time might be fragile, and this change guarantees the properties are
initialized and frozen in `System.initPhase1()`
> src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 415:
>
>> 413: public static String userRegion() {
>> 414: return USER_REGION;
>> 415: }
>
> Using methods to retrieve these makes is more complicated.
> The bleeding of the enum values outside of Locale is undesirable.
> Since the property values are final strings, I suggest just making the fields
> public and keep the mapping local to the Locale class.
As Alan commented, I will not use `Locale.Category.ordinal()` but instead use
the properties keys. Would that address your suggestion?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417793505
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417785427
Re: RFR: 8321206: Make Locale related system properties static properties
On Wed, 6 Dec 2023 08:14:05 GMT, Alan Bateman wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > src/java.base/share/classes/java/util/Locale.java line 1099: > >> 1097: StaticProperty.userCountry(category.ordinal() + 1), >> 1098: StaticProperty.userVariant(category.ordinal() + 1), >> 1099: >> getDefaultExtensions(StaticProperty.userExtensions(category.ordinal() + 1)) > > I suspect this would be more readable, and maintainable, if you add > non-public languageKey/scriptKey/countryKey/variantKey methods to > Locale.Category. That would change this to > getInstance(category.languageKey(), ...) so no sight of the ordinal at the > use-site. Right. Will change it in the next iteration - PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417769586
Re: RFR: 8321206: Make Locale related system properties static properties
On Wed, 6 Dec 2023 02:14:13 GMT, David Holmes wrote: > I'm not following the changes to cdsHeapVerifier.cpp. You've marked the new > entries as `C` but the definition is: > > ``` > // [C] A non-final static string that is assigned a string literal during > class > // initialization; this string is never changed during -Xshare:dump. > ``` > > and these are final static strings not non-final. ??? I simply followed the fix to https://bugs.openjdk.org/browse/JDK-8295295 where the last time I added a `StaticProperty`, it broke the CDS test. Since I am not familiar in this area, I am happy to have the reasoning corrected. I would appreciate suggestions. > You also have a large number of test failures with this PR. Are you referring to GHA tests? I am not sure they are relevant to this fix, as my run for mach5 did not cause any regression test failures. > Can I also suggest that you change the bug and PR synopsis to say > StaticProperty rather than static properties as I found it quite confusing to > understand the issue. Modified. >> Making them static properties is safer than relying on the class >> initialization timing > "class initialization" refers to the static initialization of a class. I > assume that is not what you mean here but the creation of the instance of the > class? Even StaticProperty still initializes these things during class > initialization, so I'm unclear exactly what this is trying to say. Modified it to specifically saying `Locale` class loading time. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1843406352
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 17:48:04 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX2 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> For serial sort on random data, this PR shows upto ~7.5x improvement for >> 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the >> performance data below. >> >> For parallel sort on random data, this PR shows upto ~3.4x for 32-bit >> datatypes (int, float) as shown below. >> >> **Note:** This PR also improves the performance of AVX512 sort by upto 35%. >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | >> Speedup >> -- | -- | -- | -- | -- >> ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 >> ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 >> ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 >> ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 >> ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 >> ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 >> ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 >> ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 >> ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 >> ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 >> ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 >> ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 >> ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 >> ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 >> ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 >> ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 >> ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 >> ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 >> >> >> >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > add missing header files @TobiHartmann @vnkozlov Please advice if we can go head and integrate this PR today before the fork. - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843407940
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v4]
On Wed, 6 Dec 2023 17:56:58 GMT, Jim Laskey wrote: >> A regression is found in Java9+ creating String instance from UTF8 bytes, a >> side effect of string compactation https://openjdk.org/jeps/254 that changed >> the decoding logic. Specifically, when constructing a string from bytes: >> >> ``` >> String str = new String(largeBytes, StandardCharsets.UTF_8); >> ``` >> >> if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than >> INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack >> trace is slightly different, see below), regardless of jvm heap size. In >> Java8, it succeeded when jvm heap size is set to be sufficient. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Alternate 64 bit test > - Exclude 32 bit Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16974#pullrequestreview-1768284373
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v4]
> A regression is found in Java9+ creating String instance from UTF8 bytes, a > side effect of string compactation https://openjdk.org/jeps/254 that changed > the decoding logic. Specifically, when constructing a string from bytes: > > ``` > String str = new String(largeBytes, StandardCharsets.UTF_8); > ``` > > if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than > INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack > trace is slightly different, see below), regardless of jvm heap size. In > Java8, it succeeded when jvm heap size is set to be sufficient. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Alternate 64 bit test - Exclude 32 bit - Changes: - all: https://git.openjdk.org/jdk/pull/16974/files - new: https://git.openjdk.org/jdk/pull/16974/files/144b5161..9926adda Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16974=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16974=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16974.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16974/head:pull/16974 PR: https://git.openjdk.org/jdk/pull/16974
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
> The goal is to develop faster sort routines for x86_64 CPUs by taking > advantage of AVX2 instructions. This enhancement provides an order of > magnitude speedup for Arrays.sort() using int, long, float and double arrays. > > For serial sort on random data, this PR shows upto ~7.5x improvement for > 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the > performance data below. > > For parallel sort on random data, this PR shows upto ~3.4x for 32-bit > datatypes (int, float) as shown below. > > **Note:** This PR also improves the performance of AVX512 sort by upto 35%. > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> > > > > > > > > > Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | > Speedup > -- | -- | -- | -- | -- > ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 > ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 > ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 > ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 > ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 > ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 > ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 > ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 > ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 > ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 > ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 > ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 > ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 > ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 > ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 > ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 > ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 > ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 > > > > > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/... Srinivas Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: add missing header files - Changes: - all: https://git.openjdk.org/jdk/pull/16534/files - new: https://git.openjdk.org/jdk/pull/16534/files/c143e0b9..7e124581 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16534=09 - incr: https://webrevs.openjdk.org/?repo=jdk=16534=08-09 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16534.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16534/head:pull/16534 PR: https://git.openjdk.org/jdk/pull/16534
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 17:42:39 GMT, Jatin Bhateja wrote: > LGTM, thanks! Thanks Jatin! - PR Comment: https://git.openjdk.org/jdk/pull/16534#issuecomment-1843372385
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v10]
On Wed, 6 Dec 2023 17:44:25 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX2 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> For serial sort on random data, this PR shows upto ~7.5x improvement for >> 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the >> performance data below. >> >> For parallel sort on random data, this PR shows upto ~3.4x for 32-bit >> datatypes (int, float) as shown below. >> >> **Note:** This PR also improves the performance of AVX512 sort by upto 35%. >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | >> Speedup >> -- | -- | -- | -- | -- >> ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 >> ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 >> ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 >> ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 >> ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 >> ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 >> ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 >> ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 >> ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 >> ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 >> ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 >> ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 >> ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 >> ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 >> ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 >> ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 >> ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 >> ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 >> >> >> >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > add missing header files LGTM, thanks! - Marked as reviewed by jbhateja (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16534#pullrequestreview-1768255412
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final [v2]
> The static AtomicInteger used for the nextHashCode should be final. Brett Okken has updated the pull request incrementally with one additional commit since the last revision: Update full name - Changes: - all: https://git.openjdk.org/jdk/pull/16987/files - new: https://git.openjdk.org/jdk/pull/16987/files/9e276138..d05272a3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16987=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16987=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16987.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16987/head:pull/16987 PR: https://git.openjdk.org/jdk/pull/16987
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final
On Wed, 6 Dec 2023 00:52:48 GMT, Brett Okken wrote: > The static AtomicInteger used for the nextHashCode should be final. Looks okay to me! Since this is new contribution, I would like someone else to take a look. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16987#pullrequestreview-1768229984
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v9]
> The goal is to develop faster sort routines for x86_64 CPUs by taking > advantage of AVX2 instructions. This enhancement provides an order of > magnitude speedup for Arrays.sort() using int, long, float and double arrays. > > For serial sort on random data, this PR shows upto ~7.5x improvement for > 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the > performance data below. > > For parallel sort on random data, this PR shows upto ~3.4x for 32-bit > datatypes (int, float) as shown below. > > **Note:** This PR also improves the performance of AVX512 sort by upto 35%. > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> > > > > > > > > > Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | > Speedup > -- | -- | -- | -- | -- > ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 > ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 > ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 > ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 > ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 > ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 > ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 > ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 > ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 > ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 > ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 > ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 > ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 > ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 > ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 > ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 > ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 > ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 > > > > > > xmlns:o="urn:schemas-microsoft-com:office:office" > xmlns:x="urn:schemas-microsoft-com:office:excel" > xmlns="http://www.w3.org/TR/REC-html40;> > > > > > > href="file:///C:/Users/... Srinivas Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: remove unused avx2 64 bit sort functions; add assertions - Changes: - all: https://git.openjdk.org/jdk/pull/16534/files - new: https://git.openjdk.org/jdk/pull/16534/files/bc590d9f..c143e0b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16534=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16534=07-08 Stats: 128 lines in 4 files changed: 12 ins; 116 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16534.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16534/head:pull/16534 PR: https://git.openjdk.org/jdk/pull/16534
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v8]
On Tue, 5 Dec 2023 19:37:34 GMT, Jatin Bhateja wrote: >> Srinivas Vamsi Parasa has updated the pull request with a new target base >> due to a merge or a rebase. The incremental webrev excludes the unrelated >> changes brought in by the merge/rebase. The pull request contains 17 >> additional commits since the last revision: >> >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - add GCC version guards >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - Remove C++17 from C flags >> - add avoid masked stores operation >> - update the code to check for supported simd sort cpus >> - Disable AVX2 sort for 64-bit types >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - fix jcheck failures due to windows encoding >> - fix carriage return and change insertion sort thresholds >> - ... and 7 more: https://git.openjdk.org/jdk/compare/d4151e5b...bc590d9f > > src/java.base/linux/native/libsimdsort/avx2-emu-funcs.hpp line 64: > >> 62: } >> 63: return lut; >> 64: }(); > > Lut64 is needed for compress64 emulation, can be removed. Removed in the latest commit... > src/java.base/linux/native/libsimdsort/avx2-emu-funcs.hpp line 234: > >> 232: >> 233: vtype::mask_storeu(leftStore, left, temp); >> 234: } > > Can be removed if not being used. Removed in the latest commit... > src/java.base/linux/native/libsimdsort/avx2-emu-funcs.hpp line 277: > >> 275: >> 276: return _mm_popcnt_u32(shortMask); >> 277: } > > Can be removed if not being used. Removed in the latest commit... > src/java.base/linux/native/libsimdsort/avx2-linux-qsort.cpp line 44: > >> 42: break; >> 43: case JVM_T_FLOAT: >> 44: avx2_fast_sort((float*)array, from_index, to_index, >> INSERTION_SORT_THRESHOLD_32BIT); > > Assertions for unsupported types. Added in the latest commit... > src/java.base/linux/native/libsimdsort/avx2-linux-qsort.cpp line 56: > >> 54: case JVM_T_FLOAT: >> 55: avx2_fast_partition((float*)array, from_index, to_index, >> pivot_indices, index_pivot1, index_pivot2); >> 56: break; > > Please add assertion for unsupported types. Added in the latest commit... - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417701182 PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417702999 PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417702251 PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417701469 PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417701705
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final
On Wed, 6 Dec 2023 00:52:48 GMT, Brett Okken wrote: > The static AtomicInteger used for the nextHashCode should be final. Submitted: https://bugs.openjdk.org/browse/JDK-8321470 Please change this PR title to "8321470: ThreadLocal.nextHashCode can be static final", and bots would do the rest. - PR Comment: https://git.openjdk.org/jdk/pull/16987#issuecomment-1842976493
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v9]
On Tue, 5 Dec 2023 19:19:23 GMT, Jatin Bhateja wrote: >> Srinivas Vamsi Parasa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> remove unused avx2 64 bit sort functions; add assertions > > src/java.base/linux/native/libsimdsort/avx2-linux-qsort.cpp line 50: > >> 48: case JVM_T_DOUBLE: >> 49: avx2_fast_sort((double*)array, from_index, to_index, >> INSERTION_SORT_THRESHOLD_64BIT); >> 50: break; > > Please add safe assertions for missing types. This is from an older (but outdated) commit. The assertions have been added in other cases. - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417706670
RFR: 8321470: ThreadLocal.nextHashCode can be static final
The static AtomicInteger used for the nextHashCode should be final. - Commit messages: - Merge remote-tracking branch 'upstream/master' into threadlocal_final - make ThreadLocal.nextHashCode final Changes: https://git.openjdk.org/jdk/pull/16987/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16987=00 Issue: https://bugs.openjdk.org/browse/JDK-8321470 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16987.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16987/head:pull/16987 PR: https://git.openjdk.org/jdk/pull/16987
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v9]
On Wed, 6 Dec 2023 11:59:19 GMT, Magnus Ihse Bursie wrote: >> Hi Magnus (@magicus), >> >>> Are you saying that when compiling with GCC 6, it will just silently ignore >>> `-std=c++17`? I'd have assumed that it printed a warning or error about an >>> unknown or invalid option, if C++17 is not supported. >> >> The GCC complier for versions 6 (and even 5) silently ignores the flag >> `-std=c++17`. It does not print any warning or error. I tested it with a toy >> C++ program and also by building OpenJDK using GCC 6. >> >>> You can't check for if compiler options should be enabled or not inside >>> source code files. >> >> what I meant was, there are #ifdef guards using predefined macros in the >> C++ source code to check for GCC version and make the simdsort code >> available for compilation or not based on the GCC version >> >> >> // src/java.base/linux/native/libsimdsort/simdsort-support.hpp >> #if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 7) || ((__GNUC__ == >> 7) && (__GNUC_MINOR__ >= 5 >> #define __SIMDSORT_SUPPORTED_LINUX >> #endif >> >> >> >> //src/java.base/linux/native/libsimdsort/avx2-linux-qsort.cpp >> #include "simdsort-support.hpp" >> #ifdef __SIMDSORT_SUPPORTED_LINUX >> >> #endif > > Okay, then I guess I am fine with this. Thank you Magnus! - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417707661
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v8]
On Tue, 5 Dec 2023 19:33:48 GMT, Jatin Bhateja wrote: >> Srinivas Vamsi Parasa has updated the pull request with a new target base >> due to a merge or a rebase. The incremental webrev excludes the unrelated >> changes brought in by the merge/rebase. The pull request contains 17 >> additional commits since the last revision: >> >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - add GCC version guards >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - Remove C++17 from C flags >> - add avoid masked stores operation >> - update the code to check for supported simd sort cpus >> - Disable AVX2 sort for 64-bit types >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort >> - fix jcheck failures due to windows encoding >> - fix carriage return and change insertion sort thresholds >> - ... and 7 more: https://git.openjdk.org/jdk/compare/d8b29378...bc590d9f > > src/java.base/linux/native/libsimdsort/avx512-32bit-qsort.hpp line 235: > >> 233: return avx512_double_compressstore>( >> 234: left_addr, right_addr, k, reg); >> 235: } > > Can be removed. This is needed for AVX512 sort... - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417690992
Re: RFR: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access) [v2]
> This PR fixes a couple of aligned accesses when reading/writing strings. Such > aligned accesses crept in when we optimized string read/write operations to > work in bulk. As a result, depending on the maximum alignment constraints > supported by the heap segment, some string operations might fail. > > I've added some tests to make sure that all operations work as expected with > unaligned semantics. > > Note: I've considered inferring an alignment constraint from the provided > charset, and then use aligned operations (and document that behavior), but I > found that to be unsatisfactory: memory operations typically accept a layout, > which allow clients to opt out from alignment checks if needed. But if we > always infer an alignment constraint from the provided charset, clients would > find themselves w/o an escape hatch. For this reason, I think the best way to > fix this is to use unaligned operations when reading/writing the string. Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Make test more robust - Simplify test - Changes: - all: https://git.openjdk.org/jdk/pull/16999/files - new: https://git.openjdk.org/jdk/pull/16999/files/856d3e14..12cf9e3a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16999=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16999=00-01 Stats: 51 lines in 1 file changed: 0 ins; 26 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/16999.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16999/head:pull/16999 PR: https://git.openjdk.org/jdk/pull/16999
RFR: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access)
This PR fixes a couple of aligned accesses when reading/writing strings. Such aligned accesses crept in when we optimized string read/write operations to work in bulk. As a result, depending on the maximum alignment constraints supported by the heap segment, some string operations might fail. I've added some tests to make sure that all operations work as expected with unaligned semantics. Note: I've considered inferring an alignment constraint from the provided charset, and then use aligned operations (and document that behavior), but I found that to be unsatisfactory: memory operations typically accept a layout, which allow clients to opt out from alignment checks if needed. But if we always infer an alignment constraint from the provided charset, clients would find themselves w/o an escape hatch. For this reason, I think the best way to fix this is to use unaligned operations when reading/writing the string. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/16999/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16999=00 Issue: https://bugs.openjdk.org/browse/JDK-8321467 Stats: 79 lines in 2 files changed: 69 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16999.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16999/head:pull/16999 PR: https://git.openjdk.org/jdk/pull/16999
Re: RFR: 8320759: Creation of random BigIntegers can be made faster [v3]
On Tue, 5 Dec 2023 22:01:04 GMT, Brian Burkhalter wrote: >> fabioromano1 has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Create RandomBigIntegers.java >> >> Create a benchmark to measure the performance of BigInteger(int, Random) >> constructor implementation. > > So, item 1 is a non-issue and item 2 is not likely a problem in practice. > What, if anything, will be done about item 3? > @bplb Maybe an assertion at the end of `randomBits(int, Random)` method, or a > test class. @fabioromano1 A test class would be better as then we would be able to catch any problems during routine testing. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16817#issuecomment-1843266956
RFR: JDK-8320538: Obsolete CSS styles in collection framework doc-file
Please review a simple change to remove a stray inline CSS element from the
Collection Framework index doc file. The only thing the `a {font-weight:
bold;}` rule did was to make all links in the header and footer bold as [can be
seen here][1]; the three content links to the other doc files are still
displayed in bold font because they are contained in `` tags. The other CSS
rules had no effect in the page.
[1]:
https://download.java.net/java/early_access/jdk22/docs/api/java.base/java/util/doc-files/coll-index.html
I also added the `` meta tag which is present in all other pages (both doc-files
and generated by JavaDoc).
-
Commit messages:
- JDK-8320538: Obsolete CSS styles in collection framework doc-file
Changes: https://git.openjdk.org/jdk/pull/16997/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=16997=00
Issue: https://bugs.openjdk.org/browse/JDK-8320538
Stats: 10 lines in 1 file changed: 0 ins; 9 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/16997.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16997/head:pull/16997
PR: https://git.openjdk.org/jdk/pull/16997
Integrated: JDK-8319662 ForkJoinPool trims worker threads too slowly
On Sun, 19 Nov 2023 17:36:01 GMT, Doug Lea wrote: > This update cascades timeouts to trim subsequent workers after the first > keepAlive inactive period. This pull request has now been integrated. Changeset: cc25d8b1 Author:Doug Lea URL: https://git.openjdk.org/jdk/commit/cc25d8b12bbab9dde9ade7762927dcb8d27e23c5 Stats: 251 lines in 2 files changed: 79 ins; 45 del; 127 mod 8319662: ForkJoinPool trims worker threads too slowly 8319498: ForkJoinPool.invoke(ForkJoinTask) does not specify behavior when task throws checked exception Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/16725
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException [v3]
On Wed, 6 Dec 2023 15:32:54 GMT, Per Minborg wrote: >> This PR proposes to change the exception type for exceptions thrown for >> certain methods with a parameter of type `MemorySegment` when it is >> `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException` >> was specified but in some cases, in reality, an `IllegalArgumentException` >> was thrown. >> >> The principle used in this PR is that operations acting on an MS where the >> MS is `this` should throw an `UnsupportedOperationException` whereas in >> cases where the MS is a *parameter* an `IllegalArgumentException` should be >> thrown. >> >> It should be noted that this PR retains the previous behavior for MS >> VarHandle access (even though the MS is a parameter to the accessor methods, >> the first parameter can be said to represent `this`). > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Update throws docs fror SegmentAllocator There are still more changes than required, I think - PR Review: https://git.openjdk.org/jdk/pull/16993#pullrequestreview-1767913213
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException [v3]
On Wed, 6 Dec 2023 15:32:54 GMT, Per Minborg wrote:
>> This PR proposes to change the exception type for exceptions thrown for
>> certain methods with a parameter of type `MemorySegment` when it is
>> `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException`
>> was specified but in some cases, in reality, an `IllegalArgumentException`
>> was thrown.
>>
>> The principle used in this PR is that operations acting on an MS where the
>> MS is `this` should throw an `UnsupportedOperationException` whereas in
>> cases where the MS is a *parameter* an `IllegalArgumentException` should be
>> thrown.
>>
>> It should be noted that this PR retains the previous behavior for MS
>> VarHandle access (even though the MS is a parameter to the accessor methods,
>> the first parameter can be said to represent `this`).
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update throws docs fror SegmentAllocator
src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 95:
> 93: * @param str the Java string to be converted into a C string
> 94: * @return a new native segment containing the converted C string
> 95: * @throws IllegalArgumentException if the allocated segment is
I don't think the changes here are useful. What does it mean for an allocated
segment to be read-only? I think all these conditions are tied to
`prefixAllocator` blindly accepting read-only segments, which should NOT be the
case. I suggest to revert all the chnages here and document (and throw) a new
exception for when a prefix allocator is created from a read-only segment.
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 367:
> 365:
> 366: @ForceInline
> 367: public void checkAccess(long offset, long length, AccessConstraint
> access) {
These changes should be reverted, I don't think we use the UOE mode anymore.
Just revert the impl to what it was before (also true for VarHandle templates).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417525637
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417528416
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
On Wed, 6 Dec 2023 10:40:51 GMT, Adam Sotona wrote: >> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an >> optional argument and does not respect >> `ClassFile.ClassHierarchyResolverOption` of the actual context. >> Parsing, building and transforming take options from the actual `ClassFile` >> context and verification should follow the same API pattern. >> >> This patch removes `ClassModel::verify` methods and introduces new top level >> methods: >> >> List ClassFile::verify(ClassModel model); >> List ClassFile::verify(byte[] bytes); >> List ClassFile::verify(Path path); >> >> >> Impact of the API change is minimal as the API has not been released yet. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > reverted modified test Thank you for the reviews. Yes, next step is to improve verifier range and also cover it with negative tests (see https://github.com/openjdk/jdk/pull/16809/files#diff-a23859572e86e946a9ce66361e64e4b7e4473f134bb49a248fccd70d2ac96ea2) - PR Comment: https://git.openjdk.org/jdk/pull/16947#issuecomment-1843120837
Integrated: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Mon, 4 Dec 2023 11:12:37 GMT, Adam Sotona wrote: > ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an > optional argument and does not respect > `ClassFile.ClassHierarchyResolverOption` of the actual context. > Parsing, building and transforming take options from the actual `ClassFile` > context and verification should follow the same API pattern. > > This patch removes `ClassModel::verify` methods and introduces new top level > methods: > > List ClassFile::verify(ClassModel model); > List ClassFile::verify(byte[] bytes); > List ClassFile::verify(Path path); > > > Impact of the API change is minimal as the API has not been released yet. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 0217b5ac Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/0217b5ac8b25db96fce026ac027b18024e25a329 Stats: 94 lines in 11 files changed: 44 ins; 32 del; 18 mod 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API Reviewed-by: jlahoda, mcimadamore - PR: https://git.openjdk.org/jdk/pull/16947
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException [v3]
> This PR proposes to change the exception type for exceptions thrown for > certain methods with a parameter of type `MemorySegment` when it is > `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException` > was specified but in some cases, in reality, an `IllegalArgumentException` > was thrown. > > The principle used in this PR is that operations acting on an MS where the MS > is `this` should throw an `UnsupportedOperationException` whereas in cases > where the MS is a *parameter* an `IllegalArgumentException` should be thrown. > > It should be noted that this PR retains the previous behavior for MS > VarHandle access (even though the MS is a parameter to the accessor methods, > the first parameter can be said to represent `this`). Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update throws docs fror SegmentAllocator - Changes: - all: https://git.openjdk.org/jdk/pull/16993/files - new: https://git.openjdk.org/jdk/pull/16993/files/8ba6278c..d7b08a40 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16993=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16993=01-02 Stats: 44 lines in 1 file changed: 44 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16993.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16993/head:pull/16993 PR: https://git.openjdk.org/jdk/pull/16993
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 GMT, Naoto Sato wrote:
> Currently, Locale-related system properties, such as `user.language` or
> `user.country`, are initialized when the `Locale` class is loaded. Making
> them static properties is safer than relying on the class initialization
> timing, which could potentially be changed depending on the implementation.
src/java.base/share/classes/java/util/Locale.java line 1061:
> 1059: if (sm != null) {
> 1060: sm.checkPropertiesAccess();
> 1061: }
This SM check is no longer needed; the use of privilegedGetProperties made
access unconditional.
The static properties are always accessible to the locale implementation.
src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 113:
> 111: USER_EXTENSIONS_DISPLAY = getProperty(props,
> "user.extensions.display", USER_EXTENSIONS);
> 112: USER_EXTENSIONS_FORMAT = getProperty(props,
> "user.extensions.format", USER_EXTENSIONS);
> 113: USER_REGION = getProperty(props, "user.region", "");
Computing the defaults for these properties should be in the Locale class,
close to the initialization of the default locale. Splitting the responsibility
across files makes it harder to follow what happens where/when.
src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 415:
> 413: public static String userRegion() {
> 414: return USER_REGION;
> 415: }
Using methods to retrieve these makes is more complicated.
The bleeding of the enum values outside of Locale is undesirable.
Since the property values are final strings, I suggest just making the fields
public and keep the mapping local to the Locale class.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417495473
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417510556
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417485546
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException [v2]
> This PR proposes to change the exception type for exceptions thrown for > certain methods with a parameter of type `MemorySegment` when it is > `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException` > was specified but in some cases, in reality, an `IllegalArgumentException` > was thrown. > > The principle used in this PR is that operations acting on an MS where the MS > is `this` should throw an `UnsupportedOperationException` whereas in cases > where the MS is a *parameter* an `IllegalArgumentException` should be thrown. > > It should be noted that this PR retains the previous behavior for MS > VarHandle access (even though the MS is a parameter to the accessor methods, > the first parameter can be said to represent `this`). Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update after comments - Changes: - all: https://git.openjdk.org/jdk/pull/16993/files - new: https://git.openjdk.org/jdk/pull/16993/files/ac7859ab..8ba6278c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16993=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16993=00-01 Stats: 75 lines in 6 files changed: 17 ins; 0 del; 58 mod Patch: https://git.openjdk.org/jdk/pull/16993.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16993/head:pull/16993 PR: https://git.openjdk.org/jdk/pull/16993
Re: RFR: 8320759: Creation of random BigIntegers can be made faster [v3]
On Tue, 5 Dec 2023 22:01:04 GMT, Brian Burkhalter wrote: >> fabioromano1 has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Create RandomBigIntegers.java >> >> Create a benchmark to measure the performance of BigInteger(int, Random) >> constructor implementation. > > So, item 1 is a non-issue and item 2 is not likely a problem in practice. > What, if anything, will be done about item 3? @bplb Maybe an assertion at the end of `randomBits(int, Random)` method, or a test class. - PR Comment: https://git.openjdk.org/jdk/pull/16817#issuecomment-1843077150
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v3]
On Wed, 6 Dec 2023 11:47:13 GMT, Jim Laskey wrote: >> A regression is found in Java9+ creating String instance from UTF8 bytes, a >> side effect of string compactation https://openjdk.org/jeps/254 that changed >> the decoding logic. Specifically, when constructing a string from bytes: >> >> ``` >> String str = new String(largeBytes, StandardCharsets.UTF_8); >> ``` >> >> if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than >> INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack >> trace is slightly different, see below), regardless of jvm heap size. In >> Java8, it succeeded when jvm heap size is set to be sufficient. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Requested changes Looks good, thanks for the updates. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16974#pullrequestreview-1767792457
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
On Wed, 6 Dec 2023 10:40:51 GMT, Adam Sotona wrote: >> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an >> optional argument and does not respect >> `ClassFile.ClassHierarchyResolverOption` of the actual context. >> Parsing, building and transforming take options from the actual `ClassFile` >> context and verification should follow the same API pattern. >> >> This patch removes `ClassModel::verify` methods and introduces new top level >> methods: >> >> List ClassFile::verify(ClassModel model); >> List ClassFile::verify(byte[] bytes); >> List ClassFile::verify(Path path); >> >> >> Impact of the API change is minimal as the API has not been released yet. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > reverted modified test Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16947#pullrequestreview-1767786746
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
On Wed, 6 Dec 2023 10:40:51 GMT, Adam Sotona wrote: >> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an >> optional argument and does not respect >> `ClassFile.ClassHierarchyResolverOption` of the actual context. >> Parsing, building and transforming take options from the actual `ClassFile` >> context and verification should follow the same API pattern. >> >> This patch removes `ClassModel::verify` methods and introduces new top level >> methods: >> >> List ClassFile::verify(ClassModel model); >> List ClassFile::verify(byte[] bytes); >> List ClassFile::verify(Path path); >> >> >> Impact of the API change is minimal as the API has not been released yet. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > reverted modified test I guess the tests could be made much more tight. E.g. by not only checking passes/fails verification, but also checking the exact errors reported (for tests that just check that verify "failed", without checking the exact errors, it may happen the verification fails for a wrong reason). And the tests could probably be made much broader. But I don't think that needs to happen here. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16947#pullrequestreview-1767775716
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException
On Wed, 6 Dec 2023 14:09:31 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
>> line 153:
>>
>>> 151: static void set(VarHandle ob, Object obb, long base, $type$ value)
>>> {
>>> 152: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
>>> 153: AbstractMemorySegmentImpl bb = checkAddress(obb, base,
>>> handle.length, ON_WRITE_UOE);
>>
>> I don't think this should throw UOE. It should throw IAE, because the bad
>> segment is passed to the var handle. I also realize that this exception is
>> not captured in the documentation of MemoryLayout::varHandle. So that should
>> be fixed as well.
>
> This is actually tricky: for var handles it seems like the right exception is
> IAE - but then for MemorySegment::get the right exception is UOE (because the
> receiver itself is read-only). We might need to give up consistency a bit, as
> otherwise I don't think memory segment getter/setter can reuse the VarHandle
> implementation.
After offline discussion: the distinction between UOE and IAE is mostly
fictional and doesn't add much. In fact, such distinction is harmful because it
would mean that code written using var handles cannot cleanly migrate to use
memory segment getter/setter (and vice-versa). As such, I recommend that the
implementation is left alone, and IAE is thrown by the var handle accessors for
read-only segments. Memory segment setters should similarly throw IAE for
read-only, because the setters are just thin easy to use wrappers around the
var handle.
A similar argument holds for MemorySegment::copyFrom - here throwing UOE would
be more apt, but given we explain this method as calling the static
MemorySegment::copy (which throws IAE) I would again avoid exception mismatch
which would be detrimental to refactoring.
At which point we're left with MemorySegment::fill. While UOE would be better
here, it would be the only method in MS issuing UOE for a read-only issue, so I
think it would be just best to regularize and use IAE here too.
tl;dr; the implementation is already throwing the correct set of exceptions
(which is not perfect, but better than the alternatives). The javadoc should be
updated to reflect that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417426024
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException
On Wed, 6 Dec 2023 14:06:00 GMT, Maurizio Cimadamore
wrote:
>> This PR proposes to change the exception type for exceptions thrown for
>> certain methods with a parameter of type `MemorySegment` when it is
>> `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException`
>> was specified but in some cases, in reality, an `IllegalArgumentException`
>> was thrown.
>>
>> The principle used in this PR is that operations acting on an MS where the
>> MS is `this` should throw an `UnsupportedOperationException` whereas in
>> cases where the MS is a *parameter* an `IllegalArgumentException` should be
>> thrown.
>>
>> It should be noted that this PR retains the previous behavior for MS
>> VarHandle access (even though the MS is a parameter to the accessor methods,
>> the first parameter can be said to represent `this`).
>
> src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
> line 153:
>
>> 151: static void set(VarHandle ob, Object obb, long base, $type$ value) {
>> 152: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
>> 153: AbstractMemorySegmentImpl bb = checkAddress(obb, base,
>> handle.length, ON_WRITE_UOE);
>
> I don't think this should throw UOE. It should throw IAE, because the bad
> segment is passed to the var handle. I also realize that this exception is
> not captured in the documentation of MemoryLayout::varHandle. So that should
> be fixed as well.
This is actually tricky: for var handles it seems like the right exception is
IAE - but then for MemorySegment::get the right exception is UOE (because the
receiver itself is read-only). We might need to give up consistency a bit, as
otherwise I don't think memory segment getter/setter can reuse the VarHandle
implementation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417386458
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException
On Wed, 6 Dec 2023 13:52:37 GMT, Per Minborg wrote:
> This PR proposes to change the exception type for exceptions thrown for
> certain methods with a parameter of type `MemorySegment` when it is
> `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException`
> was specified but in some cases, in reality, an `IllegalArgumentException`
> was thrown.
>
> The principle used in this PR is that operations acting on an MS where the MS
> is `this` should throw an `UnsupportedOperationException` whereas in cases
> where the MS is a *parameter* an `IllegalArgumentException` should be thrown.
>
> It should be noted that this PR retains the previous behavior for MS
> VarHandle access (even though the MS is a parameter to the accessor methods,
> the first parameter can be said to represent `this`).
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 363:
> 361:
> 362: public enum AccessConstraint {
> 363: READ_ONLY, ON_WRITE_UOE, ON_WRITE_IAE
I think the names are a bit confusing. Perhaps having NONE instead of READ_ONLY
would be better.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417378162
Re: RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException
On Wed, 6 Dec 2023 13:52:37 GMT, Per Minborg wrote:
> This PR proposes to change the exception type for exceptions thrown for
> certain methods with a parameter of type `MemorySegment` when it is
> `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException`
> was specified but in some cases, in reality, an `IllegalArgumentException`
> was thrown.
>
> The principle used in this PR is that operations acting on an MS where the MS
> is `this` should throw an `UnsupportedOperationException` whereas in cases
> where the MS is a *parameter* an `IllegalArgumentException` should be thrown.
>
> It should be noted that this PR retains the previous behavior for MS
> VarHandle access (even though the MS is a parameter to the accessor methods,
> the first parameter can be said to represent `this`).
src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
line 153:
> 151: static void set(VarHandle ob, Object obb, long base, $type$ value) {
> 152: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
> 153: AbstractMemorySegmentImpl bb = checkAddress(obb, base,
> handle.length, ON_WRITE_UOE);
I don't think this should throw UOE. It should throw IAE, because the bad
segment is passed to the var handle. I also realize that this exception is not
captured in the documentation of MemoryLayout::varHandle. So that should be
fixed as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16993#discussion_r1417381854
RFR: 8321387: SegmentAllocator:allocateFrom(AddressLayout, MemorySegment) does not throw stated UnsupportedOperationException
This PR proposes to change the exception type for exceptions thrown for certain methods with a parameter of type `MemorySegment` when it is `MemorySegment::isReadOnly`. Previously an `UnsupportedOperationException` was specified but in some cases, in reality, an `IllegalArgumentException` was thrown. The principle used in this PR is that operations acting on an MS where the MS is `this` should throw an `UnsupportedOperationException` whereas in cases where the MS is a *parameter* an `IllegalArgumentException` should be thrown. It should be noted that this PR retains the previous behavior for MS VarHandle access (even though the MS is a parameter to the accessor methods, the first parameter can be said to represent `this`). - Commit messages: - Retain old behavior for MS VarHandle access - Change some throws from UOE to IAE Changes: https://git.openjdk.org/jdk/pull/16993/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16993=00 Issue: https://bugs.openjdk.org/browse/JDK-8321387 Stats: 94 lines in 8 files changed: 39 ins; 4 del; 51 mod Patch: https://git.openjdk.org/jdk/pull/16993.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16993/head:pull/16993 PR: https://git.openjdk.org/jdk/pull/16993
Re: RFR: JDK-8319662 ForkJoinPool trims worker threads too slowly [v8]
On Mon, 4 Dec 2023 13:56:55 GMT, Doug Lea wrote: >> This update cascades timeouts to trim subsequent workers after the first >> keepAlive inactive period. > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8319662 > - Remove unnecessary re-interrupt > - Merge branch 'openjdk:master' into JDK-8319662 > - Reduce oversignalling and contention; add test > - Revert 2 lines in method scan > - Merge branch 'openjdk:master' into JDK-8319662 > - Split up method awaitWork; other associated changes. > - Merge branch 'openjdk:master' into JDK-8319662 > - tweak cascades; reinstate an @Contended; resolve JDK-8319498 > - Support cascading idle timeouts @DougLea @AlanBateman I've had a look and I think we should integrate. - PR Comment: https://git.openjdk.org/jdk/pull/16725#issuecomment-1842832564
Re: RFR: JDK-8319662 ForkJoinPool trims worker threads too slowly [v8]
On Mon, 4 Dec 2023 13:56:55 GMT, Doug Lea wrote: >> This update cascades timeouts to trim subsequent workers after the first >> keepAlive inactive period. > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8319662 > - Remove unnecessary re-interrupt > - Merge branch 'openjdk:master' into JDK-8319662 > - Reduce oversignalling and contention; add test > - Revert 2 lines in method scan > - Merge branch 'openjdk:master' into JDK-8319662 > - Split up method awaitWork; other associated changes. > - Merge branch 'openjdk:master' into JDK-8319662 > - tweak cascades; reinstate an @Contended; resolve JDK-8319498 > - Support cascading idle timeouts I've done some testing with the latest changes. The changes to trimming workers looks okay to me. When Viktor is done reviewing then I think we should integrate this. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16725#pullrequestreview-1767414023
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v8]
On Tue, 5 Dec 2023 17:26:06 GMT, Srinivas Vamsi Parasa wrote: >> That sounds weird. You can't check for if compiler options should be enabled >> or not inside source code files. >> >> Are you saying that when compiling with GCC 6, it will just silently ignore >> `-std=c++17`? I'd have assumed that it printed a warning or error about an >> unknown or invalid option, if C++17 is not supported. > > Hi Magnus (@magicus), > >> Are you saying that when compiling with GCC 6, it will just silently ignore >> `-std=c++17`? I'd have assumed that it printed a warning or error about an >> unknown or invalid option, if C++17 is not supported. > > The GCC complier for versions 6 (and even 5) silently ignores the flag > `-std=c++17`. It does not print any warning or error. I tested it with a toy > C++ program and also by building OpenJDK using GCC 6. > >> You can't check for if compiler options should be enabled or not inside >> source code files. > > what I meant was, there are #ifdef guards using predefined macros in the C++ > source code to check for GCC version and make the simdsort code available for > compilation or not based on the GCC version > > > // src/java.base/linux/native/libsimdsort/simdsort-support.hpp > #if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 7) || ((__GNUC__ == > 7) && (__GNUC_MINOR__ >= 5 > #define __SIMDSORT_SUPPORTED_LINUX > #endif > > > > //src/java.base/linux/native/libsimdsort/avx2-linux-qsort.cpp > #include "simdsort-support.hpp" > #ifdef __SIMDSORT_SUPPORTED_LINUX > > #endif Okay, then I guess I am fine with this. - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1417170882
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v8]
On Mon, 4 Dec 2023 22:15:24 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX2 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> For serial sort on random data, this PR shows upto ~7.5x improvement for >> 32-bit datatypes (int, float) on Intel TigerLake machine as shown in the >> performance data below. >> >> For parallel sort on random data, this PR shows upto ~3.4x for 32-bit >> datatypes (int, float) as shown below. >> >> **Note:** This PR also improves the performance of AVX512 sort by upto 35%. >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> Benchmark (Serial Sort) | Size | Baseline (us/op) | AVX2 (us/op) | >> Speedup >> -- | -- | -- | -- | -- >> ArraysSort.intSort | 10 | 0.034 | 0.029 | 1.2 >> ArraysSort.intSort | 25 | 0.088 | 0.044 | 2.0 >> ArraysSort.intSort | 50 | 0.239 | 0.159 | 1.5 >> ArraysSort.intSort | 75 | 0.417 | 0.27 | 1.5 >> ArraysSort.intSort | 100 | 0.572 | 0.265 | 2.2 >> ArraysSort.intSort | 1000 | 10.098 | 4.282 | 2.4 >> ArraysSort.intSort | 1 | 330.065 | 43.383 | 7.6 >> ArraysSort.intSort | 10 | 4099.527 | 778.943 | 5.3 >> ArraysSort.intSort | 100 | 49150.16 | 9634.335 | 5.1 >> ArraysSort.floatSort | 10 | 0.045 | 0.043 | 1.0 >> ArraysSort.floatSort | 25 | 0.105 | 0.073 | 1.4 >> ArraysSort.floatSort | 50 | 0.278 | 0.216 | 1.3 >> ArraysSort.floatSort | 75 | 0.476 | 0.241 | 2.0 >> ArraysSort.floatSort | 100 | 0.583 | 0.313 | 1.9 >> ArraysSort.floatSort | 1000 | 10.182 | 4.329 | 2.4 >> ArraysSort.floatSort | 1 | 323.136 | 57.175 | 5.7 >> ArraysSort.floatSort | 10 | 4299.519 | 862.63 | 5.0 >> ArraysSort.floatSort | 100 | 50889.4 | 10972.19 | 4.6 >> >> >> >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> > Srinivas Vamsi Parasa has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 17 additional > commits since the last revision: > > - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort > - add GCC version guards > - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort > - Remove C++17 from C flags > - add avoid masked stores operation > - update the code to check for supported simd sort cpus > - Disable AVX2 sort for 64-bit types > - Merge branch 'master' of https://git.openjdk.java.net/jdk into simdsort > - fix jcheck failures due to windows encoding > - fix carriage return and change insertion sort thresholds > - ... and 7 more: https://git.openjdk.org/jdk/compare/dbbc5f0a...bc590d9f Build changes look fine. You will still need the usual 2 reviewers from hotspot. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16534#pullrequestreview-1767374468
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v7]
On Wed, 6 Dec 2023 09:14:58 GMT, Xiaohong Gong wrote: >> Currently the vector floating-point math APIs like >> `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, >> which causes large performance gap on AArch64. Note that those APIs are >> optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. >> To close the gap, we would like to optimize these APIs for AArch64 by >> calling a third-party vector library called libsleef [2], which are >> available in mainstream Linux distros (e.g. [3] [4]). >> >> SLEEF supports multiple accuracies. To match Vector API's requirement and >> implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA >> instructions used stubs in libsleef for most of the operations by default, >> and 2) add the vector calling convention to apply with the runtime calls to >> stub code in libsleef. Note that for those APIs that libsleef does not >> support 1.0 ULP, we choose 0.5 ULP instead. >> >> To help loading the expected libsleef library, this patch also adds an >> experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. >> People can use it to denote the libsleef path/name explicitly. By default, >> it points to the system installed library. If the library does not exist or >> the dynamic loading of it in runtime fails, the math vector ops will >> fall-back to use the default scalar version without error. But a warning is >> printed out if people specifies a nonexistent library explicitly. >> >> Note that this is a part of the original proposed patch in panama-dev [5], >> just with some initial review comments addressed. And now we'd like to get >> some wider feedbacks from more hotspot experts. >> >> [1] https://github.com/openjdk/jdk/pull/3638 >> [2] https://sleef.org/ >> [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/ >> [4] https://packages.debian.org/bookworm/libsleef3 >> [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html > > Xiaohong Gong has updated the pull request incrementally with one additional > commit since the last revision: > > Add "--with-libsleef-lib" and "--with-libsleef-include" options All the makefile changes we've discussed previously now look good. However, I just noticed the additional -f flag. Why are you not exporting the functions from source code instead? That is the way we normally do it in JDK libraries. In your case, it seems like you only need to add the export to the macro. - PR Comment: https://git.openjdk.org/jdk/pull/16234#issuecomment-1842720243
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v7]
On Wed, 6 Dec 2023 09:14:58 GMT, Xiaohong Gong wrote: >> Currently the vector floating-point math APIs like >> `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, >> which causes large performance gap on AArch64. Note that those APIs are >> optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. >> To close the gap, we would like to optimize these APIs for AArch64 by >> calling a third-party vector library called libsleef [2], which are >> available in mainstream Linux distros (e.g. [3] [4]). >> >> SLEEF supports multiple accuracies. To match Vector API's requirement and >> implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA >> instructions used stubs in libsleef for most of the operations by default, >> and 2) add the vector calling convention to apply with the runtime calls to >> stub code in libsleef. Note that for those APIs that libsleef does not >> support 1.0 ULP, we choose 0.5 ULP instead. >> >> To help loading the expected libsleef library, this patch also adds an >> experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. >> People can use it to denote the libsleef path/name explicitly. By default, >> it points to the system installed library. If the library does not exist or >> the dynamic loading of it in runtime fails, the math vector ops will >> fall-back to use the default scalar version without error. But a warning is >> printed out if people specifies a nonexistent library explicitly. >> >> Note that this is a part of the original proposed patch in panama-dev [5], >> just with some initial review comments addressed. And now we'd like to get >> some wider feedbacks from more hotspot experts. >> >> [1] https://github.com/openjdk/jdk/pull/3638 >> [2] https://sleef.org/ >> [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/ >> [4] https://packages.debian.org/bookworm/libsleef3 >> [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html > > Xiaohong Gong has updated the pull request incrementally with one additional > commit since the last revision: > > Add "--with-libsleef-lib" and "--with-libsleef-include" options make/modules/jdk.incubator.vector/Lib.gmk line 45: > 43: $(eval $(call SetupJdkLibrary, BUILD_LIBVMATH, \ > 44: NAME := vmath, \ > 45: CFLAGS := $(CFLAGS_JDKLIB) $(LIBSLEEF_CFLAGS) -fvisibility=default, > \ Why `-fvisibility=default`? (Sorry, only noticed this now) - PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1417156921
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v2]
On Tue, 5 Dec 2023 20:44:54 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Bump up memory
>> - Cotrrect NegativeSize.java
>
> test/jdk/java/lang/String/CompactString/NegativeSize.java line 30:
>
>> 28: * @test
>> 29: * @bug 8077559
>> 30: * @summary Tests Compact String for negative size.
>
> It might be useful to require the larger memory; to avoid getting run when
> there's insufficient memory available.
>
> * @requires os.maxMemory >= 4G
Updated
> test/jdk/java/lang/String/CompactString/NegativeSize.java line 63:
>
>> 61: System.out.println(inStr.substring(1_200_000_000));
>> 62: } catch (OutOfMemoryError ex) {
>> 63: System.out.println("Succeeded with OutOfMemoryError");
>
> It might be good to check that it is the expected OOME whose message starts
> with `UTF16 String size is `.
> No just any "Java heap memory" OOME.
Updated
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16974#discussion_r1417154205
PR Review Comment: https://git.openjdk.org/jdk/pull/16974#discussion_r1417154093
Re: RFR: JDK-8320570 NegativeArraySizeException decoding >1G UTF8 bytes with non-ascii characters [v3]
> A regression is found in Java9+ creating String instance from UTF8 bytes, a > side effect of string compactation https://openjdk.org/jeps/254 that changed > the decoding logic. Specifically, when constructing a string from bytes: > > ``` > String str = new String(largeBytes, StandardCharsets.UTF_8); > ``` > > if the size of largeBytes is greater than 2^30 (>1 GB) but smaller than > INT_MAX (2 GB), it fails on Java9+ (including 11, 17, 21, though the stack > trace is slightly different, see below), regardless of jvm heap size. In > Java8, it succeeded when jvm heap size is set to be sufficient. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Requested changes - Changes: - all: https://git.openjdk.org/jdk/pull/16974/files - new: https://git.openjdk.org/jdk/pull/16974/files/ddc7acc2..144b5161 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16974=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16974=01-02 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16974.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16974/head:pull/16974 PR: https://git.openjdk.org/jdk/pull/16974
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API [v2]
> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an > optional argument and does not respect > `ClassFile.ClassHierarchyResolverOption` of the actual context. > Parsing, building and transforming take options from the actual `ClassFile` > context and verification should follow the same API pattern. > > This patch removes `ClassModel::verify` methods and introduces new top level > methods: > > List ClassFile::verify(ClassModel model); > List ClassFile::verify(byte[] bytes); > List ClassFile::verify(Path path); > > > Impact of the API change is minimal as the API has not been released yet. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: reverted modified test - Changes: - all: https://git.openjdk.org/jdk/pull/16947/files - new: https://git.openjdk.org/jdk/pull/16947/files/4485a369..bacdf481 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16947=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16947=00-01 Stats: 30 lines in 1 file changed: 30 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16947.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16947/head:pull/16947 PR: https://git.openjdk.org/jdk/pull/16947
Re: RFR: 8318809: java/util/concurrent/ConcurrentLinkedQueue/WhiteBox.java shows intermittent failures on linux ppc64le and aarch64
On Tue, 5 Dec 2023 09:08:59 GMT, Andrew Haley wrote: >> We've seen some rare failures of the CLQ Whitebox test on "less-strong" >> architectures, and the only thing which -- given my research -- could be the >> culprit is spuriously failing weakCAS (which is correct in terms of the >> implementation of CLQ). >> >> After discussion with @DougLea, it was decided as the CLQ implementation >> does not guarantee what the failing test tests, and modifying the test would >> mean that it would generally not be able to enforce anything, the test is >> invalid and should be removed -- hence this PR. > > Few AArch64 HotSpot systems implement weak CAS as anything other than plain > CAS. In order to get to the root cause of this problem, it would help to know > on which AArch64 hardware this test failed. > @theRealAph I think the problem in this case was that the whitebox test in > this case relied on a presumption that was only true for stronger consistency > architectures, and rewriting the test would essentially be asserting that "a > lot of permutations are valid, and only internally observable" which is a > low-value test. Oh right, so nothing to do with weak CAS, then. Fair enough. - PR Comment: https://git.openjdk.org/jdk/pull/16786#issuecomment-1842584686
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Wed, 6 Dec 2023 09:42:49 GMT, Adam Sotona wrote:
>> test/jdk/jdk/classfile/VerifierSelfTest.java line 62:
>>
>>> 60:
>>> 61: @Test
>>> 62: void testFailedDump() throws IOException {
>>
>> Why is this removed?
>
> Dump (print) of the classfile to an optional log (ConsumerString
> argument) has been removed from the API.
> It was a relic from early phase of the ClassFile API development and it has
> no use except for this test.
> This functionality can be replaced by explicit use of ClassPrinter.
I'll rename the test to "testFailed" and keep it without the "dump" part.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16947#discussion_r1417013768
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Wed, 6 Dec 2023 09:26:29 GMT, Maurizio Cimadamore
wrote:
>> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an
>> optional argument and does not respect
>> `ClassFile.ClassHierarchyResolverOption` of the actual context.
>> Parsing, building and transforming take options from the actual `ClassFile`
>> context and verification should follow the same API pattern.
>>
>> This patch removes `ClassModel::verify` methods and introduces new top level
>> methods:
>>
>> List ClassFile::verify(ClassModel model);
>> List ClassFile::verify(byte[] bytes);
>> List ClassFile::verify(Path path);
>>
>>
>> Impact of the API change is minimal as the API has not been released yet.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> test/jdk/jdk/classfile/VerifierSelfTest.java line 62:
>
>> 60:
>> 61: @Test
>> 62: void testFailedDump() throws IOException {
>
> Why is this removed?
Dump (print) of the classfile to an optional log (ConsumerString
argument) has been removed from the API.
It was a relic from early phase of the ClassFile API development and it has no
use except for this test.
This functionality can be replaced by explicit use of ClassPrinter.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16947#discussion_r1417008753
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v12]
On Tue, 5 Dec 2023 19:15:53 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a "jmodless" jlink mode to the JDK. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a >> `JmodLessArchive` class which has all the info of what constitutes the final >> jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.internal.vm.compiler.management.jmod >> jdk.jlink.jmod jdk.naming.dns.j... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 46 commits: > > - Add @enablePreview for JImageValidator that uses classfile API > - Fix SystemModulesPlugin after merge > - Merge branch 'master' into jdk-8311302-jmodless-link > - Don't show the verbose hint when already verbose > - Use '_files' over '_resources' as the suffix for listing resources > - Remove the hidden option hint. > >Also adjust the messages being printed when performing >a run-time image link. > - Localize messages, switch expression > - Rename RunImageArchive => JRTArchive and RunImageLinkException => > RuntimeImageLinkException > >Also moved the stamp file to jdk.jlink module. The resources files per >module now get unconditionally created (empty if no resources not in the >jimage). > - First round of addressing review feedback. > >- Share resource names (JlinkTask and JlinkResourcesListPlugin) >- Exclude resources in JlinkResourcesListPlugin the same way > as done for other plugins. > - Rename AddRunImageResourcesPlugin => JlinkResourcesListPlugin > - ... and 36 more: https://git.openjdk.org/jdk/compare/87516e29...a797ea69 The x86 test failure related to stream gatherers seems unrelated. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1842514733
Re: RFR: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
On Mon, 4 Dec 2023 11:12:37 GMT, Adam Sotona wrote:
> ClassFile API `ClassModel::verify` accepts `ClassHierarchyResolver` as an
> optional argument and does not respect
> `ClassFile.ClassHierarchyResolverOption` of the actual context.
> Parsing, building and transforming take options from the actual `ClassFile`
> context and verification should follow the same API pattern.
>
> This patch removes `ClassModel::verify` methods and introduces new top level
> methods:
>
> List ClassFile::verify(ClassModel model);
> List ClassFile::verify(byte[] bytes);
> List ClassFile::verify(Path path);
>
>
> Impact of the API change is minimal as the API has not been released yet.
>
> Please review.
>
> Thanks,
> Adam
test/jdk/jdk/classfile/VerifierSelfTest.java line 62:
> 60:
> 61: @Test
> 62: void testFailedDump() throws IOException {
Why is this removed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16947#discussion_r1416985296
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v7]
> Currently the vector floating-point math APIs like > `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, > which causes large performance gap on AArch64. Note that those APIs are > optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. > To close the gap, we would like to optimize these APIs for AArch64 by calling > a third-party vector library called libsleef [2], which are available in > mainstream Linux distros (e.g. [3] [4]). > > SLEEF supports multiple accuracies. To match Vector API's requirement and > implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA > instructions used stubs in libsleef for most of the operations by default, > and 2) add the vector calling convention to apply with the runtime calls to > stub code in libsleef. Note that for those APIs that libsleef does not > support 1.0 ULP, we choose 0.5 ULP instead. > > To help loading the expected libsleef library, this patch also adds an > experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. > People can use it to denote the libsleef path/name explicitly. By default, it > points to the system installed library. If the library does not exist or the > dynamic loading of it in runtime fails, the math vector ops will fall-back to > use the default scalar version without error. But a warning is printed out if > people specifies a nonexistent library explicitly. > > Note that this is a part of the original proposed patch in panama-dev [5], > just with some initial review comments addressed. And now we'd like to get > some wider feedbacks from more hotspot experts. > > [1] https://github.com/openjdk/jdk/pull/3638 > [2] https://sleef.org/ > [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/ > [4] https://packages.debian.org/bookworm/libsleef3 > [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision: Add "--with-libsleef-lib" and "--with-libsleef-include" options - Changes: - all: https://git.openjdk.org/jdk/pull/16234/files - new: https://git.openjdk.org/jdk/pull/16234/files/ee5caf6d..f3ff0672 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16234=06 - incr: https://webrevs.openjdk.org/?repo=jdk=16234=05-06 Stats: 124 lines in 3 files changed: 67 ins; 33 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/16234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16234/head:pull/16234 PR: https://git.openjdk.org/jdk/pull/16234
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v5]
On Tue, 5 Dec 2023 13:03:22 GMT, Magnus Ihse Bursie wrote: >> Thanks for the suggestion @magicus ! >> >> The check in current `lib-sleef.m4` is very common: >> >> - If user has specified libsleef root by '--with-libsleef', we assume it is >> the manually built sleef lib. So only `lib/` and `include/` is checked. And >> the flags are set based on that path. >> - If user has not specified the libsleef root, and no `SYSROOT` is set, we >> try `PKG_CHECK` (like what you suggested) >> - Otherwise, check `sleef.h` >>- We assume the sleef module is installed under one of the valid system >> paths if the header can be found. So just linking with `-lsleef` will >> success. >> >> It's an issue in current flow like what @theRealAph met. I will add the >> options like `--with-libsleef-lib` and `--with-libsleef-include` like ffi. >> Regarding to extending the check for`--with-libsleef`, I think we can just >> make it simple like what it is now. Or, we have to check all the potential >> valid lib paths like `lib/`, `lib64/`, or maybe `lib/aarch64-linux-gnu`. The >> same to the `include` part. @theRealAph @magicus , WDYT? > > I'm fine with adding just --with-libsleef-lib and --with-libsleef-include to > specify them directly. This makes it at least possible to use, if not overly > convenient, for people using a system like Andrew's. If it annoys someone too > much, we can extend it later. Added these two options in latest commit. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1416962901
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Wed, 6 Dec 2023 01:56:46 GMT, David Holmes wrote: > FWIW exitCode out numbers exitValue in source code 3:1 (and > 5:1 in test > code). That might be the case, but both the called function returning the value and the function we are changing uses the name exitValue. It seems irrelevant that other places in the code uses exitCode. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1842489162
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v6]
On Fri, 1 Dec 2023 16:26:02 GMT, Magnus Ihse Bursie wrote: >> Xiaohong Gong has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains ten additional >> commits since the last revision: >> >> - Separate neon and sve functions into two source files >> - Merge branch 'jdk:master' into JDK-8312425 >> - Rename vmath to sleef in configure >> - Address review comments in build system >> - Add a bundled native lib in jdk as a bridge to libsleef >> - Merge 'jdk:master' into JDK-8312425 >> - Disable sleef by default >> - Merge 'jdk:master' into JDK-8312425 >> - 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF > > doc/building.md line 639: > >> 637: >> 638: libsleef, the [SIMD Library for Evaluating Elementary Functions]( >> 639: https://sleef.org/) is required when building libvmath.so on >> Linux/aarch64 > > This is incorrect. The library is not required, but if it is present, we will > build libvmath with it. > > Edit: Or rather, this is misleading. Technically it is correct, since you > state that it is required when building libvmath.so, but it is easy to > mistake for being required for building the JDK. The reader presumably does > not know what libvmath.so is or how it is used. > > Please rephrase this to so that it is clear that this is optional, but will > provide performance benefits to the resulting JDK if present. You do not need > to mention libvmath.so here, for no other dependency do we declare what parts > of the JDK that require it -- it is not essential for this document. > > Also see if you can make this paragraph and the one at the end be a bit more > tighter, not the last paragraph seems to be both repeat and contradict this > one. Hi @magicus , the doc is updated. Thanks for your comment on this! - PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1416964362
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 GMT, Naoto Sato wrote: > Currently, Locale-related system properties, such as `user.language` or > `user.country`, are initialized when the `Locale` class is loaded. Making > them static properties is safer than relying on the class initialization > timing, which could potentially be changed depending on the implementation. src/java.base/share/classes/java/util/Locale.java line 1099: > 1097: StaticProperty.userCountry(category.ordinal() + 1), > 1098: StaticProperty.userVariant(category.ordinal() + 1), > 1099: > getDefaultExtensions(StaticProperty.userExtensions(category.ordinal() + 1)) I suspect this would be more readable, and maintainable, if you add non-public languageKey/scriptKey/countryKey/variantKey methods to Locale.Category. That would change this to getInstance(category.languageKey(), ...) so no sight of the ordinal at the use-site. - PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1416885432
Integrated: 8321159: SymbolLookup.libraryLookup(Path, Arena) Assumes default Filesystem
On Mon, 4 Dec 2023 07:29:37 GMT, Per Minborg wrote: > This PR proposes to reject paths provided to the > `SymbolLookup.libraryLookup(Path path, Arena arena)` method if a path is not > in the default file system. This pull request has now been integrated. Changeset: a0920aa4 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/a0920aa436943b88b53a81f46752e8c4bb0a0fc7 Stats: 42 lines in 2 files changed: 42 ins; 0 del; 0 mod 8321159: SymbolLookup.libraryLookup(Path, Arena) Assumes default Filesystem Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/16944
