RFR: 8316641: VarHandle template classes can share code in the base class
VarHandle implementations have many static fields and methods that can be pulled to the common superclass to avoid repeated initialization and code duplication. In addition, the Unsafe-based Buffer field access are replaced by usage of public methods or JavaNioAccess. - Commit messages: - Clean up VarHandle template classes Changes: https://git.openjdk.org/jdk/pull/15854/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15854=00 Issue: https://bugs.openjdk.org/browse/JDK-8316641 Stats: 136 lines in 5 files changed: 31 ins; 50 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/15854.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15854/head:pull/15854 PR: https://git.openjdk.org/jdk/pull/15854
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v8]
On Wed, 20 Sep 2023 21:51:19 GMT, Brian Burkhalter wrote: >> Add a `finally` block to delete the created files. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8315960: Address additional reviewer comments Now that we're using junit, we can start using assertions test/jdk/java/io/File/TempDirDoesNotExist.java line 142: > 140: OutputAnalyzer originalOutput = > ProcessTools.executeTestJvm(options); > 141: List list = originalOutput.asLines().stream().filter(line > 142: -> line.equalsIgnoreCase(WARNING)).toList(); You could use `count` instead of `toList`; the actual list is never used in this test test/jdk/java/io/File/TempDirDoesNotExist.java line 143: > 141: List list = originalOutput.asLines().stream().filter(line > 142: -> line.equalsIgnoreCase(WARNING)).toList(); > 143: if (list.size() != 1) Use assertEquals test/jdk/java/io/File/TempDirDoesNotExist.java line 148: > 146: > originalOutput.asLines().toString()); > 147: int exitValue = originalOutput.getExitValue(); > 148: if (exitValue != 0) Use assertEquals - PR Review: https://git.openjdk.org/jdk/pull/15757#pullrequestreview-1636914632 PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332479846 PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332474537 PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332474679
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v40]
On Wed, 20 Sep 2023 22:46:16 GMT, Srinivas Vamsi Parasa wrote: > Hi Vladimir, > > Just trying to understand: is there a reason to use > `DualPivotQuicksort_RadixForParallel.java` and > `DualPivotQuicksort_RadixForAll.java`? > > Would it not be sufficient to do the following two runs: > > 1. Baseline (Stock JDK) vs. AVX512 sort for`sort()`and `parallelSort()` ? > 2. AVX512 sort vs. Radix sort for `sort()` and `parallelSort()` ? > > > [1] current implementation in JDK [2] your AVX12 version based on [1], from > > this PR [3] my new version with Radix sort for parallel case plus your > > AVX12 changes [4] my new version with Radix sort for all cases plus your > > AVX12 changes > > Thanks, Vamsi Hi @vamsi-parasa , Please also add C1 intrinsification support for newly carved sort / partition intrinsics in your follow up PR https://bugs.openjdk.org/browse/JDK-8315382 , it may further improve the overall runtime for large sized array sort in tiered compilation mode. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1728877814
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
On Mon, 18 Sep 2023 23:00:09 GMT, Mandy Chung wrote: > `JVM_MoreStackWalk` has a bug that always assumes that the Java frame > stream is currently at the frame decoded in the last patch and so always > advances to the next frame before filling in the new batch of stack frame. > However `JVM_MoreStackWalk` may return 0. The library will set > the continuation to its parent. It then call `JVM_MoreStackWalk` to continue > the stack walking but the last decoded frame has already been advanced. > The Java frame stream is already at the top frame of the parent continuation. > . > The current implementation skips "Continuation::yield0" mistakenly. This > only happens with `-XX:+ShowHiddenFrames` or > `StackWalker.Option.SHOW_HIDDEN_FRAMES`. > > The fix is to pass the number of frames decoded in the last batch to > `JVM_MoreStackWalk` > so that the VM will determine if the current frame should be skipped or not. > > `test/jdk/jdk/internal/vm/Continuation/Scoped.java` test now correctly checks > the expected result where "yield0" exists between "enter" and "run" frames. src/java.base/share/classes/java/lang/StackStreamFactory.java line 443: > 441: > 442: // If the last batch didn't fetch any frames, keep the > current batch size. > 443: int lastBatchFrameCount = frameBuffer.numFrames(); I run some tests to understand the issue and I got the same failure if I now set MIN_BATCH_SIZE to 7. This just forces the same situation where Continuation::enter is the last frame in the buffer, otherwise since the patch also changes the batch sizes we don't fall into that issue anymore. The problem is with this numFrames() method which still returns a number > 0 after the fetch attempt that returns with no frames. Maybe there is a reset missing for origin and fence when fetching the next batch? - PR Review Comment: https://git.openjdk.org/jdk/pull/15804#discussion_r1332471224
Re: RFR: 8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl [v2]
> Like #12077, this uses JDK's internal utilities to speed up ASCII reading in > Class files, where most identifiers, from conventions to attribute names, are > ASCII. See the JBS issue for more in-depth explanations. > > Before: (Master) > > Benchmark Mode CntScore Error Units > ReadMetadata.jdkReadMemberNames thrpt4 167.623 ± 8.522 ops/s > > > After: (This patch, first revision) > > Benchmark Mode CntScore Error Units > ReadMetadata.jdkReadMemberNames thrpt4 175.908 ± 4.766 ops/s Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Fix logical bug with char array filling - Changes: - all: https://git.openjdk.org/jdk/pull/15837/files - new: https://git.openjdk.org/jdk/pull/15837/files/d0ce1181..6f5e3d72 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15837=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15837=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15837.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15837/head:pull/15837 PR: https://git.openjdk.org/jdk/pull/15837
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Thank you Naoto for the update. This looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636736028
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v4]
On Wed, 20 Sep 2023 23:48:13 GMT, Brian Burkhalter wrote: >> In the Windows implementation of java.io.File.getCanonicalPath, strip any >> long path or UNC prefix before canonicalizing the remainder of the pathname. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8287843: Move prefix stripping to separate method; add to isAbsolute Support for `isAbsolute` was added. All `jdk_core` tests still pass. Test cases still need to be added to `GetAbsolutePath.java` and `IsAbsolute.java`. These tests also appear ripe for conversion to JUnit 5. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728602641
Withdrawn: 8313718: make container at requires command configurable
On Tue, 29 Aug 2023 22:01:22 GMT, Mikhailo Seledtsov wrote: > Container ecosystem is growing. It would be beneficial to define custom > command to figure out whether a specific test host or environment allows for > container testing. This enhancement seeks to make the command used by jtreg > "requires" extension configurable, specifically > test/jtreg-ext/requires/VMProps.java checkContainerSupport(). This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/15475
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v4]
> In the Windows implementation of java.io.File.getCanonicalPath, strip any > long path or UNC prefix before canonicalizing the remainder of the pathname. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8287843: Move prefix stripping to separate method; add to isAbsolute - Changes: - all: https://git.openjdk.org/jdk/pull/15603/files - new: https://git.openjdk.org/jdk/pull/15603/files/a8726fbb..2ea422c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15603=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15603=02-03 Stats: 42 lines in 1 file changed: 31 ins; 7 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15603.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15603/head:pull/15603 PR: https://git.openjdk.org/jdk/pull/15603
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636650667
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v40]
On Wed, 20 Sep 2023 17:19:42 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > change variable names of indexPivot* to pivotIndex* > Hi Vamsi, > > In this comment [#13568 > (comment)](https://github.com/openjdk/jdk/pull/13568#issuecomment-1728082819) > Paul suggested comparing of performance. > > Could you please run benchmarking of the following 4 class? > > [1] current implementation in JDK [2] your AVX12 version based on [1], from > this PR [3] my new version with Radix sort for parallel case plus your AVX12 > changes > https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_RadixForParallel.java > [4] my new version with Radix sort for all cases plus your AVX12 changes >
RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit
Please review this PR which converts some tests under _Calendar_ to use JUnit. These tests either previously used the internal _IntlTest_, or used no framework at all. Any files named BugXXX.java will be renamed after review. - Commit messages: - Separate data generation and data testing in BuddhistCalendarTest - init Changes: https://git.openjdk.org/jdk/pull/15853/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15853=00 Issue: https://bugs.openjdk.org/browse/JDK-8316559 Stats: 679 lines in 10 files changed: 206 ins; 224 del; 249 mod Patch: https://git.openjdk.org/jdk/pull/15853.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15853/head:pull/15853 PR: https://git.openjdk.org/jdk/pull/15853
Re: RFR: 8316000: File.setExecutable silently fails if file does not exist [v4]
> On Windows, do not return `true` from the `java.io.File` methods > `setReadable(boolean, boolean)` and `setExecutable(boolean, boolean)` if the > file does not exist. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8316000: Move apiNotes to normative text; update @return descriptions - Changes: - all: https://git.openjdk.org/jdk/pull/15673/files - new: https://git.openjdk.org/jdk/pull/15673/files/9712535c..8640decd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15673=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15673=02-03 Stats: 23 lines in 1 file changed: 0 ins; 4 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/15673.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15673/head:pull/15673 PR: https://git.openjdk.org/jdk/pull/15673
Re: RFR: JDK-8316629: j.text.DateFormatSymbols setZoneStrings() exception is unhelpful
On Wed, 20 Sep 2023 22:10:16 GMT, Justin Lu wrote: > Please review this PR, which updates the exception message for > java.text.DateFormatSymbols.setZoneStrings > > `setZoneStrings()` takes a multi dimensional array as input. If any row does > not have a length of at least 5, an _IllegalArgumentException_ is thrown. The > exception should indicate why it was thrown. Initially, I thought `%d` would fit here since `i` is an `int`, but would be a bit odd if the localized number were inserted in the English exception message. So `%s` which simply calls `toString()` is fine to me. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15849#pullrequestreview-1636620860
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v40]
On Wed, 20 Sep 2023 17:19:42 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > change variable names of indexPivot* to pivotIndex* Hi Vladimir, Sure, will run the benchmarks and post the JMH performance data. Just trying to understand: is there a reason to use `DualPivotQuicksort_RadixForParallel.java` and `DualPivotQuicksort_RadixForAll.java`? Would it not be sufficient to do the following two runs: 1. Baseline (Stock JDK) vs. AVX512 sort for` sort() `and `parallelSort()` ? 2. AVX512 sort vs. Radix sort for `sort()` and `parallelSort(`) ? > [1] current implementation in JDK [2] your AVX12 version based on [1], from > this PR [3] my new version with Radix sort for parallel case plus your AVX12 >
RFR: JDK-8316629: j.text.DateFormatSymbols setZoneStrings() exception is unhelpful
Please review this PR, which updates the exception message for java.text.DateFormatSymbols.setZoneStrings `setZoneStrings()` takes a multi dimensional array as input. If any row does not have a length of at least 5, an _IllegalArgumentException_ is thrown. The exception should indicate why it was thrown. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/15849/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15849=00 Issue: https://bugs.openjdk.org/browse/JDK-8316629 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15849.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15849/head:pull/15849 PR: https://git.openjdk.org/jdk/pull/15849
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636561895
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v7]
On Wed, 20 Sep 2023 15:42:27 GMT, Brian Burkhalter wrote:
>> test/jdk/java/io/File/TempDirDoesNotExist.java line 153:
>>
>>> 151: @ParameterizedTest
>>> 152: @MethodSource("tempDirSource")
>>> 153: public void existingMessage(int exitValue, String errorMsg,
>>
>> `exitValue` is always zero, and `errorMsg` is always `WARNING`; do you need
>> to use parameters here?
>
> Right. I intentionally left it that way to match the original but it should
> probably be changed.
Extraneous parameters removed in b3ac8c7b1242281bda86f7dc98efccf2bcefc7f6.
>> test/jdk/java/io/File/TempDirDoesNotExist.java line 161:
>>
>>> 159: @ParameterizedTest
>>> 160: @MethodSource("noTempDirSource")
>>> 161: public void nonexistentMessage(int exitValue, String errorMsg,
>>
>> exitValue is always zero, and errorMsg is always WARNING; do you need to use
>> parameters here?
>
> Same comment as above re: line 100.
Extraneous parameters removed in b3ac8c7b1242281bda86f7dc98efccf2bcefc7f6.
>> test/jdk/java/io/File/TempDirDoesNotExist.java line 170:
>>
>>> 168: @MethodSource("counterSource")
>>> 169: public void messageCounter(int exitValue, String... options)
>>> 170: throws Exception {
>>
>> Suggestion:
>>
>> public void messageCounter(int exitValue, String... options)
>> throws Exception {
>
> Thanks; will fix.
Fixed in b3ac8c7b1242281bda86f7dc98efccf2bcefc7f6.
>> test/jdk/java/io/File/TempDirDoesNotExist.java line 174:
>>
>>> 172: List list =
>>> originalOutput.asLines().stream().filter(line
>>> 173: -> line.equalsIgnoreCase(WARNING)).toList();
>>> 174: if (list.size() != 1 || originalOutput.getExitValue() !=
>>> exitValue)
>>
>> (preexisting) the exception message doesn't make much sense in the second
>> case (`originalOutput.getExitValue() != exitValue`)
>
> Will reconsider this.
Improved in b3ac8c7b1242281bda86f7dc98efccf2bcefc7f6.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332215332
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332215487
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332215855
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332215701
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v8]
> Add a `finally` block to delete the created files. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8315960: Address additional reviewer comments - Changes: - all: https://git.openjdk.org/jdk/pull/15757/files - new: https://git.openjdk.org/jdk/pull/15757/files/511c511f..b3ac8c7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15757=07 - incr: https://webrevs.openjdk.org/?repo=jdk=15757=06-07 Stats: 64 lines in 1 file changed: 4 ins; 31 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/15757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15757/head:pull/15757 PR: https://git.openjdk.org/jdk/pull/15757
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v7]
On Wed, 20 Sep 2023 21:18:43 GMT, Roger Riggs wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8315960: Fix indentation
>
> test/jdk/java/io/File/TempDirDoesNotExist.java line 98:
>
>> 96: new String[] {
>> 97: "-Djava.io.tmpdir=" +
>> 98: tempDir(),
>
> These lines could be joined. Or perhaps move the "+" to the beginning of the
> next line as in the others below.
Fixed in b3ac8c7b1242281bda86f7dc98efccf2bcefc7f6.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332216006
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
On Wed, 20 Sep 2023 16:33:56 GMT, Paul Sandoz wrote: > I think its definitely a better fit, but another aspect of my previous > comment was wondering if we need a radix sort if the vectorized quicksort > implementation is fast enough. IMO we need to compare performance results > with the vectorized quick sort, and be aware of future enhancements to that. In this comment https://github.com/openjdk/jdk/pull/14227#issuecomment-1728440839 I asked Vamsi to compare current version in JDK (base), base + AVX512, Radix sort for all cases + AVX512, Radix sort for parallel case + AVX512. - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1728444843
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v7]
On Tue, 19 Sep 2023 21:45:13 GMT, Brian Burkhalter wrote:
>> Add a `finally` block to delete the created files.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8315960: Fix indentation
test/jdk/java/io/File/TempDirDoesNotExist.java line 98:
> 96: new String[] {
> 97: "-Djava.io.tmpdir=" +
> 98: tempDir(),
These lines could be joined. Or perhaps move the "+" to the beginning of the
next line as in the others below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1332196855
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v40]
On Wed, 20 Sep 2023 17:19:42 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > change variable names of indexPivot* to pivotIndex* Hi Vamsi, In this comment https://github.com/openjdk/jdk/pull/13568#issuecomment-1728082819 Paul suggested comparing of performance. Could you please run benchmarking of the following 4 class? [1] current implementation in JDK [2] your AVX12 version based on [1], from this PR [3] my new version with Radix sort for parallel case plus your AVX12 changes https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_RadixForParallel.java [4] my new version with Radix sort for all cases plus your AVX12 changes
Re: Should we build jrt-fs.jar again with the "Build JDK" ?
Hello Andrew, The bootcycle-images target is AFAIK just a test. It's certainly not meant to be the authoritative way of building the JDK. Using JDK N-1 as bootjdk is the official way of producing a JDK of version JDK N. For convenience, and because it should work, we also allow JDK N. Vendors should definitely not be encouraged to use bootcycle builds to produce their JDK binaries. Switching the compiler to interim would help with the reproducibility issue. I would support that change. I don't think we can reasonably do something about the jar tool. /Erik On 9/20/23 08:12, Andrew Leonard wrote: Hi Magnus, So yes, jrt-fs.jar can be different between a normal build and a bootcycle build, which is where I sort of came in here... For example, building say jdk-21 using a jdk-20 bootjdk, you will find that there is an extra inner class in the standard build of jrt-fs.jar, due to the fact that the jdk-21 compiler optimized the inner class generation for enum's somewhere, such that jdk/internal/jrtfs/JrtFileAttributeView$1.class only exists in a jdk-20 compiled jrt-fs.jar! I did experiment, and you can simply switch jrt-fs.jar to be COMPILER="interim", however when it comes to the jar's construction via "jar", it obviously uses the bootjdk "jar" command since the "interim-compiler" is just a compiler In summary, I suspect this is just eluding to what the real purpose of "bootcycle-images" is, which I think is essentially a "test", and I suspect most vendors will either just do a standard "product-images" build, or perform their own bootcycle by doing two builds... Cheers Andrew On Wed, Sep 20, 2023 at 2:44 PM Magnus Ihse Bursie wrote: On 2023-09-20 09:38, Andrew Leonard wrote: Thanks Alan, So different gcc, glibc, Xcode,.. agree, they need to be the same for identical bits. However, at the moment using the same toolchains, if you do a standard product build, and then a bootcycle build, of the same source, jrt-fs.jar will differ. I'll do some investigation of the make files to see if a "Build JDK" rebuild of jrt-fs.jar is feasible. I would not in general assume that a normal build and a bootcycle build produce identical results. A bootcycle build will build the product using a newer version of the JDK (viz. the one you just build from the sources), and as such, changes to javac can result in different class file outputs, etc. That being said, for large time periods of the JDK source code, a normal build and a bootcycle build can certainly result in the same output, since no changes have been made in the product that affects how .class files are generated. But that is not guaranteed, nor is a difference between normal and bootcycle build a sign of trouble or a defect. If jrt-fs.jar is consistently different between a bootcycle build and a normal build, that sounds a bit odd, though. Especially since it should be built with `--release 8` (or something like that) to ensure it is usable on older Java; and that output ought not to really change as the JDK develops. (Also, questions about the build process is preferably handled on the build-dev list) /Magnus Cheers Andrew On Tue, Sep 19, 2023 at 5:42 PM Alan Bateman wrote: On 18/09/2023 14:51, Andrew Leonard wrote: > Thanks for the clarification Alan. > > To ensure the reproducibility of the whole JDK image regardless of the > specific bootjdk used, would it make sense once the "Build JDK" has > been built, we re-build jrt-fs.jar again using the "Build JDK" ? Thus > jrt-fs.jar will be consistent with the rest of the image in terms of > what it is compiled with. > The boot JDK will be JDK N-1, or the newly built JDK in the case of boot cycle builds. It seems a bit of a stretch to have builds using different tool chains to produce identical bits but maybe you mean something else. In any case, for jrt-fs.jar the important thing is that they are compiled to --release 8 (that might rev at some points) so that IDEs/tools can open a target run-time image as a file system and access the classes/resources. -Alan.
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v3]
On Wed, 20 Sep 2023 18:10:17 GMT, Brian Burkhalter wrote: >> In the Windows implementation of java.io.File.getCanonicalPath, strip any >> long path or UNC prefix before canonicalizing the remainder of the pathname. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8287843: Move \\?\ prefix stripping to resolve(File) It might be that the long prefix needs to be stripped local to several different methods. Will investigate. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728370663
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
On Wed, 20 Sep 2023 18:00:40 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove the UseHeavyMonitors develop >> option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) >> option. Tested with tier1 locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Update option comment. Thanks Dan and Alan for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15845#issuecomment-1728285358
Integrated: 8315869: UseHeavyMonitors not used
On Wed, 20 Sep 2023 17:36:06 GMT, Coleen Phillimore wrote: > Please review this trivial change to remove the UseHeavyMonitors develop > option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) > option. Tested with tier1 locally. This pull request has now been integrated. Changeset: 3301fb1e Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/3301fb1e8ad11d7de01a052e0a2d6178a7579ba6 Stats: 16 lines in 4 files changed: 0 ins; 13 del; 3 mod 8315869: UseHeavyMonitors not used Reviewed-by: dcubed, alanb - PR: https://git.openjdk.org/jdk/pull/15845
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v3]
On Wed, 20 Sep 2023 18:47:33 GMT, Brian Burkhalter wrote: > > File::isAbsolute, it looks like it will return true for input like > > `\\\?\\foo` but it will be treated by toAbsolutePath as a relative path. > > Right on: Ideally the prefix would be removed at the front door, meaning when creating the File object, but it may be 20 years too late to do that change. From a compatibility perspective then limiting the it to canonicalization should be okay but it the path needs to make absolute again after stripping. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728282600
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
On Wed, 20 Sep 2023 18:00:40 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove the UseHeavyMonitors develop >> option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) >> option. Tested with tier1 locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Update option comment. Yes, I agree that this is a trivial fix. (Sorry I missed that earlier.) - PR Comment: https://git.openjdk.org/jdk/pull/15845#issuecomment-1728278222
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
On Wed, 20 Sep 2023 18:00:40 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove the UseHeavyMonitors develop >> option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) >> option. Tested with tier1 locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Update option comment. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15845#pullrequestreview-1636307098
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
On Wed, 20 Sep 2023 18:00:40 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove the UseHeavyMonitors develop >> option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) >> option. Tested with tier1 locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Update option comment. Thanks Dan. Do you agree that it's trivial? - PR Comment: https://git.openjdk.org/jdk/pull/15845#issuecomment-1728260950
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v3]
On Wed, 20 Sep 2023 18:42:31 GMT, Alan Bateman wrote:
> File::isAbsolute, it looks like it will return true for input like
> `\\\?\\foo` but it will be treated by toAbsolutePath as a relative path.
Right on:
jshell> File f = new File("\\\?\\foo")
f ==> \?\foo
jshell> f.isAbsolute()
$2 ==> true
jshell> f.getAbsolutePath()
$3 ==> "C:\\Users\\bpb\\foo"
jshell> f.getCanonicalPath()
$4 ==> "C:\\Users\\bpb\\foo"
-
PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728260652
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v3]
On Wed, 20 Sep 2023 18:10:17 GMT, Brian Burkhalter wrote: >> In the Windows implementation of java.io.File.getCanonicalPath, strip any >> long path or UNC prefix before canonicalizing the remainder of the pathname. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8287843: Move \\?\ prefix stripping to resolve(File) > So changed in > [a8726fb](https://github.com/openjdk/jdk/commit/a8726fbb8a070388f2b9756ee88c746b12c18397) > which also adds a couple of test cases. Perhaps some cases should be added > to the `GetAbsolutePath` test as well. Yes, I think it will need tests. The next thing is ask is the behavior of File::isAbsolute, it looks like it will return true for input like `\\\?\\foo` but it will be treated by toAbsolutePath as a relative path. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728254382
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
On Wed, 20 Sep 2023 18:00:40 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove the UseHeavyMonitors develop >> option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) >> option. Tested with tier1 locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Update option comment. Thumbs up. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15845#pullrequestreview-1636241551
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v2]
On Wed, 20 Sep 2023 07:26:02 GMT, Alan Bateman wrote: > the one-arg WinNTFileSystem.resolve may be the right place to check for the > prefix So changed in a8726fbb8a070388f2b9756ee88c746b12c18397 which also adds a couple of test cases. Perhaps some cases should be added to the `GetAbsolutePath` test as well. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1728207029
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v3]
> In the Windows implementation of java.io.File.getCanonicalPath, strip any > long path or UNC prefix before canonicalizing the remainder of the pathname. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8287843: Move \\?\ prefix stripping to resolve(File) - Changes: - all: https://git.openjdk.org/jdk/pull/15603/files - new: https://git.openjdk.org/jdk/pull/15603/files/d9d84b8e..a8726fbb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15603=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15603=01-02 Stats: 47 lines in 2 files changed: 24 ins; 14 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/15603.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15603/head:pull/15603 PR: https://git.openjdk.org/jdk/pull/15603
Re: RFR: 8315869: UseHeavyMonitors not used [v2]
> Please review this trivial change to remove the UseHeavyMonitors develop > option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) > option. Tested with tier1 locally. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Update option comment. - Changes: - all: https://git.openjdk.org/jdk/pull/15845/files - new: https://git.openjdk.org/jdk/pull/15845/files/acecf6f2..5d00c551 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15845=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15845=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15845.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15845/head:pull/15845 PR: https://git.openjdk.org/jdk/pull/15845
Re: RFR: 8315869: UseHeavyMonitors not used
On Wed, 20 Sep 2023 17:36:06 GMT, Coleen Phillimore wrote: > Please review this trivial change to remove the UseHeavyMonitors develop > option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) > option. Tested with tier1 locally. Changes requested by dcubed (Reviewer). src/hotspot/share/runtime/globals.hpp line 1064: > 1062: develop(bool, VerifyHeavyMonitors, false, > \ > 1063: "Checks that no stack locking happens when using " > \ > 1064: "-XX:LockingMode=LM_MONITOR") > \ s/LM_MONITOR/0/ I don't think you can specify the `LM_MONITOR` value on the actual cmd line. - PR Review: https://git.openjdk.org/jdk/pull/15845#pullrequestreview-1636204262 PR Review Comment: https://git.openjdk.org/jdk/pull/15845#discussion_r1331988698
RFR: 8315869: UseHeavyMonitors not used
Please review this trivial change to remove the UseHeavyMonitors develop option, in favor of the now non-experimental LockingMode=LM_MONITOR (0) option. Tested with tier1 locally. - Commit messages: - 8315869: UseHeavyMonitors not used Changes: https://git.openjdk.org/jdk/pull/15845/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15845=00 Issue: https://bugs.openjdk.org/browse/JDK-8315869 Stats: 16 lines in 4 files changed: 0 ins; 13 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15845.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15845/head:pull/15845 PR: https://git.openjdk.org/jdk/pull/15845
Integrated: 8296246: Update Unicode Data Files to Version 15.1.0
On Wed, 13 Sep 2023 20:15:09 GMT, Naoto Sato wrote:
> This PR is to incorporate the latest Unicode 15.1, which was released
> yesterday. Besides the usual character data update, an upgraded
> implementation of RegEx which reflects the Indic Conjunct Break specified in
> the latest [Unicode Annex #29 ("Unicode Text
> Segmentation")](https://unicode.org/reports/tr29/) is included. A
> corresponding CSR has also been drafted.
This pull request has now been integrated.
Changeset: 7c991cc5
Author:Naoto Sato
URL:
https://git.openjdk.org/jdk/commit/7c991cc567bfe8cfa56774c545de689ee20f699a
Stats: 1534 lines in 22 files changed: 1303 ins; 16 del; 215 mod
8296246: Update Unicode Data Files to Version 15.1.0
Reviewed-by: erikj, joehw, srl, rriggs
-
PR: https://git.openjdk.org/jdk/pull/15728
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v39]
On Wed, 20 Sep 2023 07:18:55 GMT, iaroslavski wrote: > ... and suggestion to improve naming: there are inconsistent new names: > pivotIndices, indexPivot1 and indexPivot2. I think names pivotIndices, > pivotIndex1 and pivotIndex2 will be better. Do you agree? Please see the variable names updated as suggested, in the latest commit just pushed. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1728138009
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v40]
> The goal is to develop faster sort routines for x86_64 CPUs by taking > advantage of AVX512 instructions. This enhancement provides an order of > magnitude speedup for Arrays.sort() using int, long, float and double arrays. > > This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and upto > ~4.5x improvement for 64-bit datatypes (long, double) as shown in the > performance data below. > > > **Arrays.sort performance data using JMH benchmarks for arrays with random > data** > > | Arrays.sort benchmark | Array Size | Baseline > (us/op)| AVX512 Sort (us/op) | Speedup | > | --- | --- | --- | --- | --- > | > | ArraysSort.doubleSort | 10 | 0.034 | 0.035 > | 1.0 | > | ArraysSort.doubleSort | 25 | 0.116 | 0.089 > | 1.3 | > | ArraysSort.doubleSort | 50 | 0.282 | 0.291 > | 1.0 | > | ArraysSort.doubleSort | 75 | 0.474 | 0.358 > | 1.3 | > | ArraysSort.doubleSort | 100 | 0.654 | 0.623 > | 1.0 | > | ArraysSort.doubleSort | 1000| 9.274 | 6.331 > | 1.5 | > | ArraysSort.doubleSort | 1 | 323.339 | 71.228 > | **4.5** | > | ArraysSort.doubleSort | 10 | 4471.871| > 1002.748| **4.5** | > | ArraysSort.doubleSort | 100 | 51660.742 | > 12921.295 | **4.0** | > | ArraysSort.floatSort| 10 | 0.045 | 0.046 > | 1.0 | > | ArraysSort.floatSort| 25 | 0.103 | 0.084 > | 1.2 | > | ArraysSort.floatSort| 50 | 0.285 | 0.33 > | 0.9 | > | ArraysSort.floatSort| 75 | 0.492 | 0.346 > | 1.4 | > | ArraysSort.floatSort| 100 | 0.597 | 0.326 > | 1.8 | > | ArraysSort.floatSort| 1000| 9.811 | 5.294 > | 1.9 | > | ArraysSort.floatSort| 1 | 323.955 | 50.547 > | **6.4** | > | ArraysSort.floatSort| 10 | 4326.38 | 731.152 > | **5.9** | > | ArraysSort.floatSort| 100 | 52413.88| > 8409.193| **6.2** | > | ArraysSort.intSort | 10 | 0.033 | 0.033 > | 1.0 | > | ArraysSort.intSort | 25 | 0.086 | 0.051 > | 1.7 | > | ArraysSort.intSort | 50 | 0.236 | 0.151 > | 1.6 | > | ArraysSort.intSort | 75 | 0.416 | 0.332 > | 1.3 | > | ArraysSort.intSort | 100 | 0.63| 0.521 > | 1.2 | > | ArraysSort.intSort | 1000| 10.518 | 4.698 > | 2.2 | > | ArraysSort.intSort | 1 | 309.659 | 42.518 > | **7.3** | > | ArraysSort.intSort | 10 | 4130.917| > 573.956 | **7.2** | > | ArraysSort.intSort | 100 | 49876.307 | > 6712.812| **7.4** | > | ArraysSort.longSort | 10 | 0.036 | 0.037 > | 1.0 | > | ArraysSort.longSort | 25 | 0.094 | 0.08 > | 1.2 | > | ArraysSort.longSort | 50 | 0.218 | 0.227 > | 1.0 | > | ArraysSort.longSort | 75 | 0.466 | 0.402 > | 1.2 | > | ArraysSort.longSort | 100 | 0.76| 0.58 > | 1.3 | > | ArraysSort.longSort | 1000| 10.449 | 6.239 > | 1.7 | > | ArraysSort.longSort | 1 | 307.074 | 70.284 > | **4.4** | > | ArraysSor... Srinivas Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: change variable names of indexPivot* to pivotIndex* - Changes: - all: https://git.openjdk.org/jdk/pull/14227/files - new: https://git.openjdk.org/jdk/pull/14227/files/3e0b8cfc..b04cb6c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14227=39 - incr: https://webrevs.openjdk.org/?repo=jdk=14227=38-39 Stats: 49 lines in 1 file changed: 0 ins; 6 del; 43 mod Patch: https://git.openjdk.org/jdk/pull/14227.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14227/head:pull/14227 PR: https://git.openjdk.org/jdk/pull/14227
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 07:00:23 GMT, Justin Lu wrote: >> Please review this PR which restricts sub-classing of the internal calendar >> system in sun.util.calendar to only the existing implementations. Drive by >> cleanup included. >> >> As the implementation is long-standing and complete with no intent for >> future sub-classing, the CalendarSystem should be made sealed. Modifiers >> adjusted accordingly (`JulianCalendar.Date` must now have package >> visibility). >> >> >> This system has the following structure, >> >> `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar` >> extended by >> (`Gregorian, JulianCalendar, LocalGregorianCalendar`) >> >> `CalendarDate` extended by `BaseCalendar.Date` extended by >> (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date, >> LocalGregorianCalendar.Date`) >> >> Additionally, CalendarUtils was made `final`, as it is a utility class >> composed of static util methods. > > Justin Lu has updated the pull request incrementally with two additional > commits since the last revision: > > - cleanup CalendarDate after revert > - Revert "Replace sprintf0d with Formatter" > >This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15803#pullrequestreview-1636151330
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 08:54:55 GMT, Andrey Turbanov wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - cleanup CalendarDate after revert
>> - Revert "Replace sprintf0d with Formatter"
>>
>>This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc.
>
> src/java.base/share/classes/sun/util/calendar/CalendarDate.java line 63:
>
>> 61: */
>> 62: public sealed abstract class CalendarDate implements Cloneable
>> 63: permits BaseCalendar.Date {
>
> Can we just merge `CalendarDate` and `BaseCalendar.Date` to be the one class?
> I think it will greatly simplify the code.
`BaseCalendar` is for Gregorian based calendars, so its `Date` class also
represents dates for those calendars, while `CalendarDate` is an abstract class
for all calendar implementations. So I don't think merging them is the right
direction.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1331943059
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v39]
On Wed, 20 Sep 2023 16:27:19 GMT, Srinivas Vamsi Parasa wrote: > This API was suggested to me and was already reviewed by others. Confirming so, this was my suggestion. We use the _double-register_ addressing mode (as commonly applied in unsafe), thereby the intrinsic is capable of being used on and off-heap. To reduce the number of intrinsic methods we pass the element class as the first argument. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1728089648
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
On Fri, 15 Sep 2023 20:17:17 GMT, Paul Sandoz wrote: >>> Alan, you mentioned that DualPivotQuicksort will need detailed review. Can >>> we go ahead and start reviewing? Laurent checked performance, JMH results >>> look fine. >> >> As before, I think the main question with this change is whether adding >> radix sort to the mix is worth the complexity and additional code to >> maintain. Also as we discussed in the previous PR, the additional memory >> needed for the radix sort may have an effect on other things that are going >> on concurrently. I know it has been updated to handle OOME but I think >> potential reviewers would need to be comfortable with that part. > >> > Alan, you mentioned that DualPivotQuicksort will need detailed review. Can >> > we go ahead and start reviewing? Laurent checked performance, JMH results >> > look fine. >> >> As before, I think the main question with this change is whether adding >> radix sort to the mix is worth the complexity and additional code to >> maintain. Also as we discussed in the previous PR, the additional memory >> needed for the radix sort may have an effect on other things that are going >> on concurrently. I know it has been updated to handle OOME but I think >> potential reviewers would need to be comfortable with that part. > > I too share concerns about the potential increased use of memory for sorting > ints/longs/floats/doubles. With modern SIMD hardware and data parallel > techniques we can apply quicksort much more efficiently. I think it is > important to determine to what extent this reduces the need for radix sort. > To determine that we would need to carefully measure against the AVX-512 > implementation (with ongoing improvements) with appropriately initialized > data to sort, and further measure against an AVX2 version. > Hi Paul (@PaulSandoz), Alan (@AlanBateman), Any update? Do you agree with > Radix sort in parallel case only? I think its definitely a better fit, but another aspect of my previous comment was wondering if we need a radix sort if the vectorized quicksort implementation is fast enough. IMO we need to compare performance results with the vectorized quick sort, and be aware of future enhancements to that. - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1728082819
RFR: 8271268: Fix Javadoc links for Stream.mapMulti
Fix Javadoc links for Stream.mapMulti, Stream.MapMultiInt, Stream.mapMultiToInt, Stream.mapMultiToLong and Stream.mapMultiToDouble. - Commit messages: - Fix Javadoc links for Stream.mapMulti. Changes: https://git.openjdk.org/jdk/pull/15794/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15794=00 Issue: https://bugs.openjdk.org/browse/JDK-8271268 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15794.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15794/head:pull/15794 PR: https://git.openjdk.org/jdk/pull/15794
Re: RFR: 8271268: Fix Javadoc links for Stream.mapMulti
On Mon, 18 Sep 2023 18:09:57 GMT, Mourad Abbay wrote: > Fix Javadoc links for Stream.mapMulti, Stream.MapMultiInt, > Stream.mapMultiToInt, Stream.mapMultiToLong and Stream.mapMultiToDouble. Note to others. I suggested @mabbay pick up the dropped [PR](https://github.com/openjdk/jdk/pull/3909/), as a starter issue to help achieve OpenJDK author status. - PR Comment: https://git.openjdk.org/jdk/pull/15794#issuecomment-1724417397
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v39]
On Tue, 19 Sep 2023 01:57:44 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with two > additional commits since the last revision: > > - Update DualPivotQuicksort.java > - Rename arraySort and arrayPartition Java methods to sort and partition. > Cleanup some comments Hello Vladimir, > The first parameter of introduced method `sort(Class elemType, A array, > long offset, int low, int high, SortOperation so)` is `elemType` > (int.class, long,class etc.). The third parameter is `offset` and it depends > on `elemType`. The reason for using the `sort(Class elemType, A array, long offset,...)` API is to have a general API which can support sorting for `MemorySegment` in future. Also, a native heap backed MemorySegment will have to specify the `elemType` and
Re: RFR: 8311084: Add typeSymbol() API for applicable constant pool entries [v3]
On Wed, 20 Sep 2023 07:35:40 GMT, Chen Liang wrote: >> 5 Constant Pool entries, namely ConstantDynamicEntry, InvokeDynamicEntry, >> FieldRefEntry, MethodRefEntry, and InterfaceMethodRefEntry should have a >> typeSymbol() API to return the nominal/symbolic descriptor (ClassDesc or >> MethodTypeDesc). >> >> This API is not added to NameAndTypeEntry itself, for a NameAndTypeEntry >> only knows if its type should be a field or method type from the other >> entries that refer to it. >> >> This is one of the issues discussed in this mailing list thread: >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html > > Chen Liang 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: > > - Merge branch 'master' into feature/cpentry-typesym > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/cpentry-typesym > - Use typeSymbol in asSymbol > - Add typeSymbol API for a few constant pool entries It looks good, thanks for the patch. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14706#pullrequestreview-1636043719
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Thanks, Jai. Since `assertCurrentDate()` is apart from the actual JVM invoking test method `testEmptySysPropValue()`, I thought it would be safer to use UTC in all test cases so that if someone calls `assertCurrentDate()` in other test methods, the test wouldn't break. But you are right that they are not needed in other locations right now. I removed those locations and instead added some instructions in `assertCurrentDate()` for future proof. - PR Comment: https://git.openjdk.org/jdk/pull/15829#issuecomment-1728050059
Re: RFR: 8313612: Use JUnit in lib-test/jdk tests [v4]
On Tue, 19 Sep 2023 18:30:10 GMT, Qing Xiao wrote: >> Modified all tests under lib-test/jdk to use JUnit > > Qing Xiao has updated the pull request incrementally with three additional > commits since the last revision: > > - Update test/lib-test/jdk/test/lib/hexdump/ObjectStreamPrinterTest.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Update test/lib-test/jdk/test/lib/hexdump/HexPrinterTest.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Update test/lib-test/jdk/test/lib/format/ArrayDiffTest.java > >Co-authored-by: Christian Stein Marked as reviewed by asotona (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15131#pullrequestreview-1636039145
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
> Fixing a test case that fails in some time zones. Making sure the test is run > in `UTC` zone will fix the issue. Confirmed the fix by manually setting > machine's time zone to Europe/Dublin. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15829/files - new: https://git.openjdk.org/jdk/pull/15829/files/429b2369..30b0af53 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15829=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15829=00-01 Stats: 12 lines in 1 file changed: 3 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15829.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15829/head:pull/15829 PR: https://git.openjdk.org/jdk/pull/15829
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v7]
On Wed, 20 Sep 2023 04:29:00 GMT, Daniel Jeliński wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8315960: Fix indentation
>
> test/jdk/java/io/File/TempDirDoesNotExist.java line 153:
>
>> 151: @ParameterizedTest
>> 152: @MethodSource("tempDirSource")
>> 153: public void existingMessage(int exitValue, String errorMsg,
>
> `exitValue` is always zero, and `errorMsg` is always `WARNING`; do you need
> to use parameters here?
Right. I intentionally left it that way to match the original but it should
probably be changed.
> test/jdk/java/io/File/TempDirDoesNotExist.java line 161:
>
>> 159: @ParameterizedTest
>> 160: @MethodSource("noTempDirSource")
>> 161: public void nonexistentMessage(int exitValue, String errorMsg,
>
> exitValue is always zero, and errorMsg is always WARNING; do you need to use
> parameters here?
Same comment as above re: line 100.
> test/jdk/java/io/File/TempDirDoesNotExist.java line 170:
>
>> 168: @MethodSource("counterSource")
>> 169: public void messageCounter(int exitValue, String... options)
>> 170: throws Exception {
>
> Suggestion:
>
> public void messageCounter(int exitValue, String... options)
> throws Exception {
Thanks; will fix.
> test/jdk/java/io/File/TempDirDoesNotExist.java line 174:
>
>> 172: List list =
>> originalOutput.asLines().stream().filter(line
>> 173: -> line.equalsIgnoreCase(WARNING)).toList();
>> 174: if (list.size() != 1 || originalOutput.getExitValue() !=
>> exitValue)
>
> (preexisting) the exception message doesn't make much sense in the second
> case (`originalOutput.getExitValue() != exitValue`)
Will reconsider this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1331832086
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1331832598
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1331834543
PR Review Comment: https://git.openjdk.org/jdk/pull/15757#discussion_r1331833630
Re: Should we build jrt-fs.jar again with the "Build JDK" ?
Hi Magnus, So yes, jrt-fs.jar can be different between a normal build and a bootcycle build, which is where I sort of came in here... For example, building say jdk-21 using a jdk-20 bootjdk, you will find that there is an extra inner class in the standard build of jrt-fs.jar, due to the fact that the jdk-21 compiler optimized the inner class generation for enum's somewhere, such that jdk/internal/jrtfs/JrtFileAttributeView$1.class only exists in a jdk-20 compiled jrt-fs.jar! I did experiment, and you can simply switch jrt-fs.jar to be COMPILER="interim", however when it comes to the jar's construction via "jar", it obviously uses the bootjdk "jar" command since the "interim-compiler" is just a compiler In summary, I suspect this is just eluding to what the real purpose of "bootcycle-images" is, which I think is essentially a "test", and I suspect most vendors will either just do a standard "product-images" build, or perform their own bootcycle by doing two builds... Cheers Andrew On Wed, Sep 20, 2023 at 2:44 PM Magnus Ihse Bursie < magnus.ihse.bur...@oracle.com> wrote: > On 2023-09-20 09:38, Andrew Leonard wrote: > > Thanks Alan, > > So different gcc, glibc, Xcode,.. agree, they need to be the same for > identical bits. > However, at the moment using the same toolchains, if you do a standard > product build, > and then a bootcycle build, of the same source, jrt-fs.jar will differ. > I'll do some investigation of the make files to see if a "Build JDK" > rebuild of jrt-fs.jar is > feasible. > > I would not in general assume that a normal build and a bootcycle build > produce identical results. A bootcycle build will build the product using a > newer version of the JDK (viz. the one you just build from the sources), > and as such, changes to javac can result in different class file outputs, > etc. That being said, for large time periods of the JDK source code, a > normal build and a bootcycle build can certainly result in the same output, > since no changes have been made in the product that affects how .class > files are generated. But that is not guaranteed, nor is a difference > between normal and bootcycle build a sign of trouble or a defect. > > If jrt-fs.jar is consistently different between a bootcycle build and a > normal build, that sounds a bit odd, though. Especially since it should be > built with `--release 8` (or something like that) to ensure it is usable on > older Java; and that output ought not to really change as the JDK develops. > > (Also, questions about the build process is preferably handled on the > build-dev list) > > /Magnus > > > > Cheers > Andrew > > > On Tue, Sep 19, 2023 at 5:42 PM Alan Bateman > wrote: > >> On 18/09/2023 14:51, Andrew Leonard wrote: >> > Thanks for the clarification Alan. >> > >> > To ensure the reproducibility of the whole JDK image regardless of the >> > specific bootjdk used, would it make sense once the "Build JDK" has >> > been built, we re-build jrt-fs.jar again using the "Build JDK" ? Thus >> > jrt-fs.jar will be consistent with the rest of the image in terms of >> > what it is compiled with. >> > >> >> The boot JDK will be JDK N-1, or the newly built JDK in the case of boot >> cycle builds. It seems a bit of a stretch to have builds using different >> tool chains to produce identical bits but maybe you mean something else. >> >> In any case, for jrt-fs.jar the important thing is that they are >> compiled to --release 8 (that might rev at some points) so that >> IDEs/tools can open a target run-time image as a file system and access >> the classes/resources. >> >> -Alan. >> >>
Re: RFR: 8316421: libjava should load shell32.dll eagerly [v2]
On Wed, 20 Sep 2023 14:41:59 GMT, Jorn Vernee wrote: >> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove unused define > > src/java.base/windows/native/libjava/java_props_md.c line 214: > >> 212: HRESULT hr; >> 213: WCHAR *tmpPath = NULL; >> 214: hr = SHGetKnownFolderPath(_Profile, >> KF_FLAG_DONT_VERIFY, NULL, ); > > I'd say inline the variable declaration here as well > Suggestion: > > HRESULT hr = SHGetKnownFolderPath(_Profile, > KF_FLAG_DONT_VERIFY, NULL, ); (And you will have to remove the declaration from the line above) - PR Review Comment: https://git.openjdk.org/jdk/pull/15789#discussion_r1331745594
Re: RFR: 8316421: libjava should load shell32.dll eagerly [v2]
On Mon, 18 Sep 2023 18:10:12 GMT, Daniel Jeliński wrote: >> Please review this patch that makes java.dll load shell32.dll earlier. >> Delay-loading requires some additional code (delayimp.lib), and offers no >> benefits since we always load shell32 during JVM startup. >> >> Other than removing the delayload clause, the patch also cleans up the >> `getHomeFromShell32` method: >> - the WinXP code path is removed. The documentation states that [the older >> function just calls the new one with translated >> parameters](https://learn.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetfolderpatha). >> - `CoTaskMemFree` is now called if `SHGetKnownFolderPath` fails. This is >> suggested by the documentation, but probably never needed in practice. >> >> This change shouldn't have any observable effect on behavior on any of the >> supported operating systems. It reduced the size of java.dll by 2KB on my >> machine. >> No new tests. Existing tier1-2 tests continue to pass. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused define LGTM src/java.base/windows/native/libjava/java_props_md.c line 214: > 212: HRESULT hr; > 213: WCHAR *tmpPath = NULL; > 214: hr = SHGetKnownFolderPath(_Profile, > KF_FLAG_DONT_VERIFY, NULL, ); I'd say inline the variable declaration here as well Suggestion: HRESULT hr = SHGetKnownFolderPath(_Profile, KF_FLAG_DONT_VERIFY, NULL, ); - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15789#pullrequestreview-1635822883 PR Review Comment: https://git.openjdk.org/jdk/pull/15789#discussion_r1331744667
Re: Should we build jrt-fs.jar again with the "Build JDK" ?
On 2023-09-20 09:38, Andrew Leonard wrote: Thanks Alan, So different gcc, glibc, Xcode,.. agree, they need to be the same for identical bits. However, at the moment using the same toolchains, if you do a standard product build, and then a bootcycle build, of the same source, jrt-fs.jar will differ. I'll do some investigation of the make files to see if a "Build JDK" rebuild of jrt-fs.jar is feasible. I would not in general assume that a normal build and a bootcycle build produce identical results. A bootcycle build will build the product using a newer version of the JDK (viz. the one you just build from the sources), and as such, changes to javac can result in different class file outputs, etc. That being said, for large time periods of the JDK source code, a normal build and a bootcycle build can certainly result in the same output, since no changes have been made in the product that affects how .class files are generated. But that is not guaranteed, nor is a difference between normal and bootcycle build a sign of trouble or a defect. If jrt-fs.jar is consistently different between a bootcycle build and a normal build, that sounds a bit odd, though. Especially since it should be built with `--release 8` (or something like that) to ensure it is usable on older Java; and that output ought not to really change as the JDK develops. (Also, questions about the build process is preferably handled on the build-dev list) /Magnus Cheers Andrew On Tue, Sep 19, 2023 at 5:42 PM Alan Bateman wrote: On 18/09/2023 14:51, Andrew Leonard wrote: > Thanks for the clarification Alan. > > To ensure the reproducibility of the whole JDK image regardless of the > specific bootjdk used, would it make sense once the "Build JDK" has > been built, we re-build jrt-fs.jar again using the "Build JDK" ? Thus > jrt-fs.jar will be consistent with the rest of the image in terms of > what it is compiled with. > The boot JDK will be JDK N-1, or the newly built JDK in the case of boot cycle builds. It seems a bit of a stretch to have builds using different tool chains to produce identical bits but maybe you mean something else. In any case, for jrt-fs.jar the important thing is that they are compiled to --release 8 (that might rev at some points) so that IDEs/tools can open a target run-time image as a file system and access the classes/resources. -Alan.
Integrated: 8308995: Update Network IO JFR events to be static mirror events
On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing wrote: > The socket read/write JFR events currently use instrumentation of java.base > code using templates in the jdk.jfr modules. This results in some java.base > code residing in the jdk.jfr module which is undesirable. > > JDK19 added static support for event classes. The old instrumentor classes > should be replaced with mirror events using the static support. > > In the java.base module: > Added two new events, jdk.internal.event.SocketReadEvent and > jdk.internal.event.SocketWriteEvent. > java.net.Socket and sun.nio.ch.SocketChannelImpl were changed to make use of > the new events. > > In the jdk.jfr module: > jdk.jfr.events.SocketReadEvent and jdk.jfr.events.SocketWriteEvent were > changed to be mirror events. > In the package jdk.jfr.internal.instrument, the classes > SocketChannelImplInstrumentor, SocketInputStreamInstrumentor, and > SocketOutputStreamInstrumentor were removed. The JDKEvents class was updated > to reflect all of those changes. > > The existing tests in test/jdk/jdk/jfr/event/io continue to pass with the new > implementation: > Passed: jdk/jfr/event/io/TestSocketChannelEvents.java > Passed: jdk/jfr/event/io/TestSocketEvents.java > > I added a micro benchmark which measures the overhead of handling the jfr > socket events. > test/micro/org/openjdk/bench/java/net/SocketEventOverhead.java. > It needs access the jdk.internal.event package, which is done at runtime with > annotations that add the extra arguments. > At compile time the build arguments had to be augmented in > make/test/BuildMicrobenchmark.gmk This pull request has now been integrated. Changeset: b275bdd9 Author:Tim Prinzing Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/b275bdd9b55b567cfe60c389d5ef8b70615928f4 Stats: 906 lines in 13 files changed: 519 ins; 379 del; 8 mod 8308995: Update Network IO JFR events to be static mirror events Reviewed-by: egahlin, alanb - PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Tue, 19 Sep 2023 20:51:41 GMT, Tim Prinzing wrote: > The existing JFR tests TestSocketChannelEvents and TestSocketEvents in > jdk.jfr.event.io verify the events are still emitted as expected. Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727528861
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Wed, 20 Sep 2023 11:21:51 GMT, Daniel Fuchs wrote: > Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? I don't think so as these tests are just used to check that changes haven't broken anything. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727532006
Re: RFR: 8316456: StackWalker may skip Continuation::yield0 frame mistakenly
On Mon, 18 Sep 2023 23:00:09 GMT, Mandy Chung wrote: > `JVM_MoreStackWalk` has a bug that always assumes that the Java frame > stream is currently at the frame decoded in the last patch and so always > advances to the next frame before filling in the new batch of stack frame. > However `JVM_MoreStackWalk` may return 0. The library will set > the continuation to its parent. It then call `JVM_MoreStackWalk` to continue > the stack walking but the last decoded frame has already been advanced. > The Java frame stream is already at the top frame of the parent continuation. > . > The current implementation skips "Continuation::yield0" mistakenly. This > only happens with `-XX:+ShowHiddenFrames` or > `StackWalker.Option.SHOW_HIDDEN_FRAMES`. > > The fix is to pass the number of frames decoded in the last batch to > `JVM_MoreStackWalk` > so that the VM will determine if the current frame should be skipped or not. > > `test/jdk/jdk/internal/vm/Continuation/Scoped.java` test now correctly checks > the expected result where "yield0" exists between "enter" and "run" frames. Marked as reviewed by rpressler (Committer). - PR Review: https://git.openjdk.org/jdk/pull/15804#pullrequestreview-1635330981
Re: RFR: 8313612: Use JUnit in lib-test/jdk tests [v4]
On Tue, 19 Sep 2023 18:30:10 GMT, Qing Xiao wrote: >> Modified all tests under lib-test/jdk to use JUnit > > Qing Xiao has updated the pull request incrementally with three additional > commits since the last revision: > > - Update test/lib-test/jdk/test/lib/hexdump/ObjectStreamPrinterTest.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Update test/lib-test/jdk/test/lib/hexdump/HexPrinterTest.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Update test/lib-test/jdk/test/lib/format/ArrayDiffTest.java > >Co-authored-by: Christian Stein Looks good to me. Tested with `--test test/lib-test/jdk/test/lib`. - Marked as reviewed by cstein (Committer). PR Review: https://git.openjdk.org/jdk/pull/15131#pullrequestreview-1635286817
RFR: 8316587: Use ArraysSupport.vectorizedHashCode in Utf8EntryImpl
Like #12077, this uses JDK's internal utilities to speed up ASCII reading in Class files, where most identifiers, from conventions to attribute names, are ASCII. See the JBS issue for more in-depth explanations. Before: (Master) Benchmark Mode CntScore Error Units ReadMetadata.jdkReadMemberNames thrpt4 167.623 ± 8.522 ops/s After: (This patch, first revision) Benchmark Mode CntScore Error Units ReadMetadata.jdkReadMemberNames thrpt4 175.908 ± 4.766 ops/s - Commit messages: - Use JLA string utilities for utf8 entry reading Changes: https://git.openjdk.org/jdk/pull/15837/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15837=00 Issue: https://bugs.openjdk.org/browse/JDK-8316587 Stats: 47 lines in 2 files changed: 24 ins; 13 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/15837.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15837/head:pull/15837 PR: https://git.openjdk.org/jdk/pull/15837
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 07:00:23 GMT, Justin Lu wrote:
>> Please review this PR which restricts sub-classing of the internal calendar
>> system in sun.util.calendar to only the existing implementations. Drive by
>> cleanup included.
>>
>> As the implementation is long-standing and complete with no intent for
>> future sub-classing, the CalendarSystem should be made sealed. Modifiers
>> adjusted accordingly (`JulianCalendar.Date` must now have package
>> visibility).
>>
>>
>> This system has the following structure,
>>
>> `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar`
>> extended by
>> (`Gregorian, JulianCalendar, LocalGregorianCalendar`)
>>
>> `CalendarDate` extended by `BaseCalendar.Date` extended by
>> (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date,
>> LocalGregorianCalendar.Date`)
>>
>> Additionally, CalendarUtils was made `final`, as it is a utility class
>> composed of static util methods.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - cleanup CalendarDate after revert
> - Revert "Replace sprintf0d with Formatter"
>
>This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc.
src/java.base/share/classes/sun/util/calendar/CalendarDate.java line 63:
> 61: */
> 62: public sealed abstract class CalendarDate implements Cloneable
> 63: permits BaseCalendar.Date {
Can we just merge `CalendarDate` and `BaseCalendar.Date` to be the one class?
I think it will greatly simplify the code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1331283073
Re: RFR: 8316000: File.setExecutable silently fails if file does not exist [v3]
On Tue, 19 Sep 2023 22:48:20 GMT, Brian Burkhalter wrote: >> On Windows, do not return `true` from the `java.io.File` methods >> `setReadable(boolean, boolean)` and `setExecutable(boolean, boolean)` if the >> file does not exist. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8316000: Add the API notes to the corresponding single parameter methods I agree this one needs a spec clarification as changing the implementation would be an incompatible change leading to the methods apparently failing for cases where they didn't previously fail. I don't think it will work to just add an apiNote, it needs to normative. Also the descriptions of the return value say that the method fails (which can be interpreted to mean return false) when the file system doesn't support the permission. - PR Comment: https://git.openjdk.org/jdk/pull/15673#issuecomment-1727217278
Re: RFR: 8311084: Add typeSymbol() API for applicable constant pool entries [v3]
> 5 Constant Pool entries, namely ConstantDynamicEntry, InvokeDynamicEntry, > FieldRefEntry, MethodRefEntry, and InterfaceMethodRefEntry should have a > typeSymbol() API to return the nominal/symbolic descriptor (ClassDesc or > MethodTypeDesc). > > This API is not added to NameAndTypeEntry itself, for a NameAndTypeEntry only > knows if its type should be a field or method type from the other entries > that refer to it. > > This is one of the issues discussed in this mailing list thread: > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html Chen Liang 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: - Merge branch 'master' into feature/cpentry-typesym - Merge branch 'master' of https://github.com/openjdk/jdk into feature/cpentry-typesym - Use typeSymbol in asSymbol - Add typeSymbol API for a few constant pool entries - Changes: - all: https://git.openjdk.org/jdk/pull/14706/files - new: https://git.openjdk.org/jdk/pull/14706/files/3ae616ef..dd288733 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14706=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14706=01-02 Stats: 249978 lines in 5218 files changed: 100290 ins; 97603 del; 52085 mod Patch: https://git.openjdk.org/jdk/pull/14706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14706/head:pull/14706 PR: https://git.openjdk.org/jdk/pull/14706
Re: Should we build jrt-fs.jar again with the "Build JDK" ?
Thanks Alan, So different gcc, glibc, Xcode,.. agree, they need to be the same for identical bits. However, at the moment using the same toolchains, if you do a standard product build, and then a bootcycle build, of the same source, jrt-fs.jar will differ. I'll do some investigation of the make files to see if a "Build JDK" rebuild of jrt-fs.jar is feasible. Cheers Andrew On Tue, Sep 19, 2023 at 5:42 PM Alan Bateman wrote: > On 18/09/2023 14:51, Andrew Leonard wrote: > > Thanks for the clarification Alan. > > > > To ensure the reproducibility of the whole JDK image regardless of the > > specific bootjdk used, would it make sense once the "Build JDK" has > > been built, we re-build jrt-fs.jar again using the "Build JDK" ? Thus > > jrt-fs.jar will be consistent with the rest of the image in terms of > > what it is compiled with. > > > > The boot JDK will be JDK N-1, or the newly built JDK in the case of boot > cycle builds. It seems a bit of a stretch to have builds using different > tool chains to produce identical bits but maybe you mean something else. > > In any case, for jrt-fs.jar the important thing is that they are > compiled to --release 8 (that might rev at some points) so that > IDEs/tools can open a target run-time image as a file system and access > the classes/resources. > > -Alan. > >
Re: RFR: 8311084: Add typeSymbol() API for applicable constant pool entries [v2]
On Fri, 30 Jun 2023 02:42:08 GMT, Chen Liang wrote: >> 5 Constant Pool entries, namely ConstantDynamicEntry, InvokeDynamicEntry, >> FieldRefEntry, MethodRefEntry, and InterfaceMethodRefEntry should have a >> typeSymbol() API to return the nominal/symbolic descriptor (ClassDesc or >> MethodTypeDesc). >> >> This API is not added to NameAndTypeEntry itself, for a NameAndTypeEntry >> only knows if its type should be a field or method type from the other >> entries that refer to it. >> >> This is one of the issues discussed in this mailing list thread: >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Use typeSymbol in asSymbol @asotona Can you review this Classfile API utility addition? - PR Comment: https://git.openjdk.org/jdk/pull/14706#issuecomment-1727129377
Re: RFR: 8287843: File::getCanonicalFile doesn't work for \?\C:\ style paths DOS device paths [v2]
On Wed, 20 Sep 2023 02:14:48 GMT, Brian Burkhalter wrote: > It could be that if after stripping the `\\\?\` prefix the result is relative > or drive-relative, then it should first be resolved in the same way before > proceeding. Right, which is why the one-arg WinNTFileSystem.resolve may be the right place to check for the prefix as it would fix the issue for both getAbsolutePath too. - PR Comment: https://git.openjdk.org/jdk/pull/15603#issuecomment-1727121877
Re: RFR: 8316493: Make immutable maps @ValueBased [v2]
On Wed, 20 Sep 2023 06:04:26 GMT, Per Minborg wrote: >> This PR outlines a solution for making immutable maps `@ValueBased` by >> removing cacheing of certain values in `AbstractMap`. >> >> By removing these caching fields in `AbstractMap`, we can make the immutable >> maps `@ValueBased` and at the same time, performance is likely improved >> because the JVM is probably able to optimize away object creation anyway via >> escape analysis. Also, all maps will occupy less space as we get rid of a >> number of objects and references stored for each map. >> >> We need to benchmark this solution to better understand its implications. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant impl spec parts We might need a CSR for the removal of `@implSpec` and equality behavior that @ExE-Boss described. - PR Comment: https://git.openjdk.org/jdk/pull/15614#issuecomment-1727119718
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
On Fri, 15 Sep 2023 20:17:17 GMT, Paul Sandoz wrote: >>> Alan, you mentioned that DualPivotQuicksort will need detailed review. Can >>> we go ahead and start reviewing? Laurent checked performance, JMH results >>> look fine. >> >> As before, I think the main question with this change is whether adding >> radix sort to the mix is worth the complexity and additional code to >> maintain. Also as we discussed in the previous PR, the additional memory >> needed for the radix sort may have an effect on other things that are going >> on concurrently. I know it has been updated to handle OOME but I think >> potential reviewers would need to be comfortable with that part. > >> > Alan, you mentioned that DualPivotQuicksort will need detailed review. Can >> > we go ahead and start reviewing? Laurent checked performance, JMH results >> > look fine. >> >> As before, I think the main question with this change is whether adding >> radix sort to the mix is worth the complexity and additional code to >> maintain. Also as we discussed in the previous PR, the additional memory >> needed for the radix sort may have an effect on other things that are going >> on concurrently. I know it has been updated to handle OOME but I think >> potential reviewers would need to be comfortable with that part. > > I too share concerns about the potential increased use of memory for sorting > ints/longs/floats/doubles. With modern SIMD hardware and data parallel > techniques we can apply quicksort much more efficiently. I think it is > important to determine to what extent this reduces the need for radix sort. > To determine that we would need to carefully measure against the AVX-512 > implementation (with ongoing improvements) with appropriately initialized > data to sort, and further measure against an AVX2 version. Hi Paul (@PaulSandoz), Alan (@AlanBateman), Any update? Do you agree with Radix sort in parallel case only? - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1727115667
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v39]
On Tue, 19 Sep 2023 01:57:44 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with two > additional commits since the last revision: > > - Update DualPivotQuicksort.java > - Rename arraySort and arrayPartition Java methods to sort and partition. > Cleanup some comments ... and suggestion to improve naming: there are inconsistent new names: pivotIndices, indexPivot1 and indexPivot2. I think names pivotIndices, pivotIndex1 and pivotIndex2 will be better. Do you agree? - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1727112096
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v39]
On Tue, 19 Sep 2023 21:44:00 GMT, iaroslavski wrote: >> Srinivas Vamsi Parasa has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Update DualPivotQuicksort.java >> - Rename arraySort and arrayPartition Java methods to sort and partition. >> Cleanup some comments > > Hi Vamsi, > > The first parameter of introduced method ``sort(Class elemType, A array, > long offset, int low, int high, SortOperation so)`` > is ``elemType`` (int.class, long,class etc.). The third parameter is > ``offset`` and it depends on ``elemType``. For example, > if ``elemType`` is int.class, offset is Unsafe.ARRAY_INT_BASE_OFFSET etc. > > Can you detect offset inside intrinsic (C++ code) and remove it from Java > code? > Hello Vladimir (@iaroslavski ), > > Could you please file a separate PR for integrating the `ArraysSort.java` JMH > benchmark? > > > 2. You introduced benchmarking class > > test/micro/org/openjdk/bench/java/util/ArraysSort.java, that's great, > >but there is the same class in PR > > https://raw.githubusercontent.com/openjdk/jdk/42e17e45b1adc4d77ba5549770ce591d96d4b1fe/test/micro/org/openjdk/bench/java/util/ArraysSort.java > >which covers all types (not int/long/float/double only) and there are > > different data inputs (not random only). > >Could you please switch to the more powerful ArraysSort class? > > Thanks, Vamsi Hi Vamsi, I'm not sure about new PR for ArraysSort.java. Why do we need separate PR? You can take my version of ArraysSort.java from https://github.com/openjdk/jdk/pull/13568, no any objections from my side. - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1727103206
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
> Please review this PR which restricts sub-classing of the internal calendar > system in sun.util.calendar to only the existing implementations. Drive by > cleanup included. > > As the implementation is long-standing and complete with no intent for future > sub-classing, the CalendarSystem should be made sealed. Modifiers adjusted > accordingly (`JulianCalendar.Date` must now have package visibility). > > > This system has the following structure, > > `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar` > extended by > (`Gregorian, JulianCalendar, LocalGregorianCalendar`) > > `CalendarDate` extended by `BaseCalendar.Date` extended by > (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date, > LocalGregorianCalendar.Date`) > > Additionally, CalendarUtils was made `final`, as it is a utility class > composed of static util methods. Justin Lu has updated the pull request incrementally with two additional commits since the last revision: - cleanup CalendarDate after revert - Revert "Replace sprintf0d with Formatter" This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc. - Changes: - all: https://git.openjdk.org/jdk/pull/15803/files - new: https://git.openjdk.org/jdk/pull/15803/files/790f7506..aaee9aa9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15803=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15803=03-04 Stats: 85 lines in 7 files changed: 45 ins; 5 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/15803.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15803/head:pull/15803 PR: https://git.openjdk.org/jdk/pull/15803
Re: RFR: 8316493: Make immutable maps @ValueBased [v2]
> This PR outlines a solution for making immutable maps `@ValueBased` by > removing cacheing of certain values in `AbstractMap`. > > By removing these caching fields in `AbstractMap`, we can make the immutable > maps `@ValueBased` and at the same time, performance is likely improved > because the JVM is probably able to optimize away object creation anyway via > escape analysis. Also, all maps will occupy less space as we get rid of a > number of objects and references stored for each map. > > We need to benchmark this solution to better understand its implications. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove redundant impl spec parts - Changes: - all: https://git.openjdk.org/jdk/pull/15614/files - new: https://git.openjdk.org/jdk/pull/15614/files/f0410ee3..2cb090b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15614=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15614=00-01 Stats: 8 lines in 1 file changed: 0 ins; 8 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15614/head:pull/15614 PR: https://git.openjdk.org/jdk/pull/15614
