Re: RFR: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
On Thu, 5 Jun 2025 07:47:17 GMT, Viktor Klang wrote: > It's too fragile to depend on generated NPE messages Sorry Viktor but I strongly disagree. It is far too easy to have a test "pass" because it catches the wrong instance of a thrown exception and hide an underlying problem. - PR Comment: https://git.openjdk.org/jdk/pull/25655#issuecomment-2943190741
Re: RFR: 8357995: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [core] [v5]
> Passes the `Charset` read from the `stdin.encoding` system property while > creating `InputStreamReader` or `Scanner` instances for `System.in`. > > `stdin.encoding` is a recently added property for Java 25 in > [JDK-8350703](https://bugs.openjdk.org/browse/JDK-8350703). Employing it > throughout the entire code base is addressed by the parent ticket > [JDK-8356893](https://bugs.openjdk.org/browse/JDK-8356893). JDK-8357995 this > PR is addressing is a sub-task of JDK-8356893 and is concerned with only > areas related to core libraries. Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision: Improve code style Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/25544/files - new: https://git.openjdk.org/jdk/pull/25544/files/78e8de51..92248bb3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25544&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25544&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25544.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25544/head:pull/25544 PR: https://git.openjdk.org/jdk/pull/25544
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Move the timestamp to before the try block, change bytes to bytesWritten and remove unnecessary code - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/8beb194a..45adc081 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=07-08 Stats: 35 lines in 4 files changed: 0 ins; 16 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v8]
On Thu, 5 Jun 2025 09:37:45 GMT, Erik Gahlin wrote:
>> Could I have review of an enhancement that adds rate-limited sampling to
>> Java events, including five events in the JDK (SocketRead, SocketWrite,
>> FileRead, FileWrite, and JavaExceptionThrow).
>>
>> Testing: test/jdk/jdk/jfr
>>
>> Thanks
>> Erik
>
> Erik Gahlin has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove the mistakenly added file.
> - Fix whitespace
src/java.base/share/classes/java/io/FileInputStream.java line 219:
> 217: }
> 218: } finally {
> 219: FileReadEvent.offer(start, path, bytesRead);
Thanks for this update, this looks much better. It think would be better again
if `start = FileReadEvent.timestamp()` is moved to before the try.
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 328:
> 326: } finally {
> 327: long end = FileReadEvent.timestamp();
> 328: FileReadEvent.offer(start, path, bytesRead);
I don't think "end" is needed now.
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 382:
> 380: FileWriteEvent.offer(start, path, bytes);
> 381: }
> 382: return bytes;
The method returns the number of bytes written and traceImplRead uses bytesRead
for the number of bytes read. So I think better to leave it as bytesWritten.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128400058
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128401565
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128406420
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v3]
On Thu, 5 Jun 2025 00:21:45 GMT, Stuart Marks wrote: >> We need some indication of which events are throttleable and looking at the >> mirror event may not work in some scenarios. >> >> We need to sample the endTime, because the startTime may be several minutes >> in the past. We could use commit(startTime, endTime, ...) and calculate the >> duration inside the method, but it may be confusing because the fields in >> the event are startTime and duration. We would also need to calculate the >> duration twice, both for shouldCommit and commit. >> >> When we get value types and perhaps value events, much of the ugliness of >> static methods could be avoided. > > I think we (from both the java.base and jdk.jfr perspectives) need to keep an > eye on the complexity of the use sites. The new throttling stuff requires a > new local variable. By itself this isn't a big deal, but there are 12 events > being updated here. In addition, each requires a start, end, and duration, > and clearly duration = end - start. These are all long values, and the > different calls require different long values, so sooner or later somebody is > going to get this wrong. > > To a certain extent we can do more cleanup on the java.base side, by using > things like SocketReadEvent's offer() method instead of its emit() method. > Unfortunately only one site uses offer() -- NIO SocketChannelImpl -- and note > that it didn't need to be updated! The other events should have something > like the offer() method, which groups together the testing of > shouldCommit/shouldThrottleCommit with the committing of the event. (This > also should avoid recalculating the duration, but really, this is only a > subtraction of two longs, so it should take only one cycle.) > > But note what we're doing here is constructing an internal API within > java.base, between the use sites (like java.net.Socket) and the > java.base-side JFR stuff (jdk.internal.event.SocketReadEvent). Maybe after > things are cleaned up and consolidated here we can see if the API between > jdk.internal.event and jdk.jfr can be improved. I updated FileRead and FileWrite to use an offer method like SocketRead and SocketWrite, reducing boilerplate. - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128398824
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v8]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with two additional commits since the last revision: - Remove the mistakenly added file. - Fix whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/7f26f172..8beb194a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=06-07 Stats: 414 lines in 3 files changed: 0 ins; 410 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v7]
On Thu, 5 Jun 2025 09:26:13 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Use offer method t.txt line 1: > 1: diff --git a/src/java.base/share/classes/java/io/FileInputStream.java > b/src/java.base/share/classes/java/io/FileInputStream.java t.txt added by mistake. - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128388583
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v3]
On Thu, 5 Jun 2025 09:31:39 GMT, Erik Gahlin wrote: >> I think we (from both the java.base and jdk.jfr perspectives) need to keep >> an eye on the complexity of the use sites. The new throttling stuff requires >> a new local variable. By itself this isn't a big deal, but there are 12 >> events being updated here. In addition, each requires a start, end, and >> duration, and clearly duration = end - start. These are all long values, and >> the different calls require different long values, so sooner or later >> somebody is going to get this wrong. >> >> To a certain extent we can do more cleanup on the java.base side, by using >> things like SocketReadEvent's offer() method instead of its emit() method. >> Unfortunately only one site uses offer() -- NIO SocketChannelImpl -- and >> note that it didn't need to be updated! The other events should have >> something like the offer() method, which groups together the testing of >> shouldCommit/shouldThrottleCommit with the committing of the event. (This >> also should avoid recalculating the duration, but really, this is only a >> subtraction of two longs, so it should take only one cycle.) >> >> But note what we're doing here is constructing an internal API within >> java.base, between the use sites (like java.net.Socket) and the >> java.base-side JFR stuff (jdk.internal.event.SocketReadEvent). Maybe after >> things are cleaned up and consolidated here we can see if the API between >> jdk.internal.event and jdk.jfr can be improved. > > I updated FileRead and FileWrite to use an offer method like SocketRead and > SocketWrite, reducing boilerplate. Thank you for this update, it looks much better now. I chatted briefly with Stuart yesterday and mostly agreed there would need to be some follow-up cleanup of the use-sites. Doing in now is good as it addresses our grumblings. - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128410969
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Thu, 29 May 2025 01:44:49 GMT, Xiaohong Gong wrote: >> test/micro/org/openjdk/bench/jdk/incubator/vector/MaskCompareNotBenchmark.java >> line 49: >> >>> 47: private static final VectorSpecies L_SPECIES = >>> LongVector.SPECIES_MAX; >>> 48: private static final VectorSpecies F_SPECIES = >>> FloatVector.SPECIES_MAX; >>> 49: private static final VectorSpecies D_SPECIES = >>> DoubleVector.SPECIES_MAX; >> >> Are you taking `SPECIES_MAX` on purpose here, or could we take >> `SPECIES_PREFERRED` instead? >> @jatin-bhateja What is the best to do in these tests? I suppose best would >> be to test with all vector lengths... > > Thanks for pointing out this @eme64 ! Per my understanding, `SPECIES_MAX` is > almost the same with `SPECIES_PREFERRED` in this case which are all specified > to the max vector size of a hardware. Since the max vector size is different > on different architectures, not all vector lengths are supported to be > intrinsified on a specified architecture like AArch64, especially the SVE > arch with different vector register size. Hence, just testing the max species > makes sense to me as this is a mid-end common transformation. Changed to use `ofLargestShape()` because on x64 the max vector length is related to data types. - PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128383805
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v7]
On Thu, 5 Jun 2025 09:32:15 GMT, erifan wrote:
> > FYI: `BoolTest::negate` already does what you want: `mask negate( ) const {
> > return mask(_test^4); }` I think you should use that instead :)
>
> Indeed, I hadn't noticed that, thank you.
Oh I think we still cannot use `BoolTest::negate`, because we cannot
instantiate a `BoolTest` object with **unsigned** comparison.
`BoolTest::negate` is a non-static function.
-
PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2943500455
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v8]
On Thu, 5 Jun 2025 09:41:10 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove the mistakenly added file. > - Fix whitespace src/jdk.jfr/share/conf/jfr/default.jfc line 845: > 843: true > 844: true > 845:control="exceptions-throttle-rate">100/s What was used to proposed 100/s as the default and 300/s in profile.jfc? - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128442250
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v7]
On Thu, 5 Jun 2025 09:24:10 GMT, Emanuel Peter wrote:
> FYI: `BoolTest::negate` already does what you want: `mask negate( ) const {
> return mask(_test^4); }` I think you should use that instead :)
Indeed, I hadn't noticed that, thank you.
-
PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2943449327
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
On Thu, 5 Jun 2025 10:10:41 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move the timestamp to before the try block, change bytes to bytesWritten > and remove unnecessary code The usages are much cleaner in the update so I think this all good. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25559#pullrequestreview-2899855765
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
On Thu, 5 Jun 2025 10:35:10 GMT, Alan Bateman wrote: >> Erik Gahlin has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move the timestamp to before the try block, change bytes to bytesWritten >> and remove unnecessary code > > src/java.base/share/classes/java/lang/Throwable.java line 124: > >> 122: * exceptions should be traced by JFR. >> 123: */ >> 124: static boolean jfrTracing; > > Are you sure this is okay? When JFR starts the JVM is brought to safepoint so the whole heap becomes visible. We have used it for socket events without any issues. - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128580995
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
On Thu, 5 Jun 2025 10:10:41 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move the timestamp to before the try block, change bytes to bytesWritten > and remove unnecessary code Thanks for the reviews, the offer methods turned out nicely. - PR Comment: https://git.openjdk.org/jdk/pull/25559#issuecomment-2943787262
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v7]
On Thu, 5 Jun 2025 09:48:46 GMT, erifan wrote: > Oh I think we still cannot use `BoolTest::negate`, because we cannot > instantiate a `BoolTest` object with **unsigned** comparison. > `BoolTest::negate` is a non-static function. I see. Ok. Hmm. I still think that the logic should be in `BoolTest`, because that is where the exact implementation of the enum values is. In that context it is easier to see why `^4` does the negation. And imagine we were ever to change the enum values, then it would be harder to find your code and fix it. Maybe it could be called `BoolTest::negate_mask(mast btm)` and explain in a comment that both signed and unsigned is supported. - PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2943747849
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Thu, 29 May 2025 07:55:06 GMT, erifan wrote:
>> Also: You now cast `(VectorMaskCmpNode*) in1` twice. Can we not do
>> `as_VectorMaskCmp()`? Or could we at least cast it only once, and then use
>> it as `in1_mask_cmp` instead?
>
>> What is the hard-coded ^ 4 here?
>
> This is to negate the comparison condition. We can't use `BoolTest::negate()`
> here because the comparison condition may be **unsigned** comparison. Since
> there's already a `negate()` function in `BoolTest`, so I tend to add a new
> function `get_negative_predicate` for this into class `VectorMaskCmpNode`.
>
>> Also: You now cast (VectorMaskCmpNode*) in1 twice. Can we not do
>> as_VectorMaskCmp()? Or could we at least cast it only once, and then use it
>> as in1_mask_cmp instead?
>
> For the first cast, I think you mean
>
> if (in1->Opcode() != Op_VectorMaskCmp ||
> in1->outcnt() > 1 ||
> !((VectorMaskCmpNode*) in1)->predicate_can_be_negated() ||
> !VectorNode::is_all_ones_vector(in2)) {
> return nullptr;
> }
>
> To remove one cast, then we have to split the above `if` because `in1` may
> not be a `VectorMaskCmpNode`.
>
> if (in1->Opcode() != Op_VectorMaskCmp) {
> return nullptr;
> }
> VectorMaskCmpNode* in1_as_mask_cmp = (VectorMaskCmpNode*) in1;
> if (in1->outcnt() > 1 ||
> !in1_as_mask_cmp->predicate_can_be_negated() ||
> !VectorNode::is_all_ones_vector(in2)) {
> return nullptr;
> }
> BoolTest::mask neg_cond = (BoolTest::mask) (in1_as_mask_cmp->get_predicate()
> ^ 4);
>
> Does this look better to you ?
For now I kept the current approach, as I feel it's a little more compact.
Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128358563
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v7]
On Thu, 5 Jun 2025 09:12:30 GMT, erifan wrote:
>> This patch optimizes the following patterns:
>> For integer types:
>>
>> (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1))
>> => (VectorMaskCmp src1 src2 ncond)
>> (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1))
>> => (VectorMaskCmp src1 src2 ncond)
>>
>> cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>> negative comparison of cond.
>>
>> For float and double types:
>>
>> (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1))
>> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>> (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1))
>> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>
>> cond can be eq or ne.
>>
>> Benchmarks on Nvidia Grace machine with 128-bit SVE2: With option
>> `-XX:UseSVE=2`:
>>
>> BenchmarkUnitBefore Score Error After
>> Score Error Uplift
>> testCompareEQMaskNotByte ops/s 7912127.225 2677.289518
>> 10266136.26 8955.008548 1.29
>> testCompareEQMaskNotDouble ops/s 884737.6799 446.963779
>> 1179760.772 448.031844 1.33
>> testCompareEQMaskNotFloatops/s 1765045.787 682.332214
>> 2359520.803 896.305743 1.33
>> testCompareEQMaskNotInt ops/s 1787221.411 977.743935
>> 2353952.519 960.069976 1.31
>> testCompareEQMaskNotLong ops/s 895297.1974 673.44808
>> 1178449.02 323.804205 1.31
>> testCompareEQMaskNotShortops/s 3339987.002 3415.2226
>> 4712761.965 2110.862053 1.41
>> testCompareGEMaskNotByte ops/s 7907615.16 4094.243652
>> 10251646.9 9486.699831 1.29
>> testCompareGEMaskNotInt ops/s 1683738.958 4233.813092
>> 2352855.205 1251.952546 1.39
>> testCompareGEMaskNotLong ops/s 854496.1561 8594.598885
>> 1177811.493 521.12291.37
>> testCompareGEMaskNotShortops/s 3341860.309 1578.975338
>> 4714008.434 1681.10365 1.41
>> testCompareGTMaskNotByte ops/s 7910823.674 2993.367032
>> 10245063.58 9774.75138 1.29
>> testCompareGTMaskNotInt ops/s 1673393.928 3153.099431
>> 2353654.521 1190.848583 1.4
>> testCompareGTMaskNotLong ops/s 849405.9159 2432.858159
>> 1177952.041 359.96413 1.38
>> testCompareGTMaskNotShortops/s 3339509.141 3339.976585
>> 4711442.496 2673.364893 1.41
>> testCompareLEMaskNotByte ops/s 7911340.004 3114.69191
>> 10231626.5 27134.20035 1.29
>> testCompareLEMaskNotInt ops/s 1675812.113 1340.969885
>> 2353255.341 1452.4522 1.4
>> testCompareLEMaskNotLong ops/s 848862.8036 6564.841731
>> 1177763.623 539.290106 1.38
>> testCompareLEMaskNotShortops/s 3324951.54 2380.29473
>> 4712116.251 1544.559684 1.41
>> testCompareLTMaskNotByte ops/s 7910390.844 2630.861436
>> 10239567.69 6487.441672 1.29
>> testCompareLTMaskNotInt ops/s 16721...
>
> erifan has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains 12 additional commits since the
> last revision:
>
> - Addressed some review comments
> - Merge branch 'master' into JDK-8354242
> - Refactor the JTReg tests for compare.xor(maskAll)
>
>Also made a bit change to support pattern `VectorMask.fromLong()`.
> - Merge branch 'master' into JDK-8354242
> - Refactor code
>
>Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this
>optimization, making the code more modular.
> - Merge branch 'master' into JDK-8354242
> - Update the jtreg test
> - Merge branch 'master' into JDK-8354242
> - Addressed some review comments
>
>1. Call VectorNode::Ideal() only once in XorVNode::Ideal.
>2. Improve code comments.
> - Merge branch 'master' into JDK-8354242
> - ... and 2 more: https://git.openjdk.org/jdk/compare/93b141e6...ebbcc405
FYI: `BoolTest::negate` already does what you want: `mask negate( ) const {
return mask(_test^4); }`
I think you should use that instead :)
-
PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2943422494
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v7]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Use offer method - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/8259ff41..7f26f172 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=05-06 Stats: 570 lines in 10 files changed: 455 ins; 88 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v8]
On Thu, 5 Jun 2025 09:41:10 GMT, Erik Gahlin wrote:
>> Could I have review of an enhancement that adds rate-limited sampling to
>> Java events, including five events in the JDK (SocketRead, SocketWrite,
>> FileRead, FileWrite, and JavaExceptionThrow).
>>
>> Testing: test/jdk/jdk/jfr
>>
>> Thanks
>> Erik
>
> Erik Gahlin has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove the mistakenly added file.
> - Fix whitespace
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1252:
> 1250: } finally {
> 1251: long bytes = bytesWritten > 0 ? bytesWritten : 0;
> 1252: FileWriteEvent.offer(start, path, bytes);
The number of bytes written is >=0, it can't be < 0, so I'm not sure why the
existing code checks bytesWritten > 0.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128429546
Re: RFR: 8352565: Add native method implementation of Reference.get() [v9]
> Please review this change which adds a native method providing the > implementation of Reference::get. Referece::get is an intrinsic candidate, so > this native method implementation is only used when the intrinsic is not. > > Currently there is intrinsic support by the interpreter, C1, C2, and graal, > which are always used. With this change we can later remove all the > per-platform interpreter intrinsic implementations, and might also remove the > C1 intrinsic implementation. > > Testing: > (1) mach5 tier1-6 normal (so using all the existing intrinsics). > (2) mach5 tier1-6 with interpreter and C1 Reference::get intrinsics disabled. Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: fix comment alignment - Changes: - all: https://git.openjdk.org/jdk/pull/24315/files - new: https://git.openjdk.org/jdk/pull/24315/files/98056a8b..edd4dec2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24315&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24315&range=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24315/head:pull/24315 PR: https://git.openjdk.org/jdk/pull/24315
Re: RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens
On Mon, 2 Jun 2025 11:54:10 GMT, Alice Pellegrini wrote:
> The implemented solution modifies the `OutputBuffer` implementation instead
> of the `OutputAnalyzer` implementation.
> This is because the **OutputBuffer implementation which handles processes**
> (LazyOutputBuffer) starts a thread in its constructor, so we would need to
> add a strange additional constructor parameter to the
> `OutputBuffer.of(Process, Charset)` static method, while the printing through
> to stdout (and stderr) only makes sense for LazyOutputBuffer.
>
> I believe changing the config option from `outputanalyzer.verbose` to `output
> buffer.verbose` would make it cleaner, and avoid referencing the
> OutputAnalyzer in the OutputBuffer implementation.
test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
> 148: this.p = p;
> 149: logProgress("Gathering output");
> 150: boolean verbose =
> Boolean.valueOf(System.getProperty("outputanalyzer.verbose", "false"));
Suggestion:
boolean verbose = Boolean.getBoolean("outputanalyzer.verbose");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2121483594
RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens
The implemented solution modifies the `OutputBuffer` implementation instead of the `OutputAnalyzer` implementation. This is because the **OutputBuffer implementation which handles processes** (LazyOutputBuffer) starts a thread in its constructor, so we would need to add a strange additional constructor parameter to the `OutputBuffer.of(Process, Charset)` static method, while the printing through to stdout (and stderr) only makes sense for LazyOutputBuffer. I believe changing the config option from `outputanalyzer.verbose` to `output buffer.verbose` would make it cleaner, and avoid referencing the OutputAnalyzer in the OutputBuffer implementation. - Commit messages: - Update test/lib/jdk/test/lib/process/OutputBuffer.java - Initial working solution Changes: https://git.openjdk.org/jdk/pull/25587/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25587&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8356438 Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/25587.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25587/head:pull/25587 PR: https://git.openjdk.org/jdk/pull/25587
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Wed, 28 May 2025 12:28:20 GMT, Emanuel Peter wrote:
>> test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 237:
>>
>>> 235: // Byte tests
>>> 236: @Test
>>> 237: @IR(counts = { IRNode.XOR_V_MASK, "= 0", IRNode.XOR_VB, "= 0" },
>>
>> Could you still assert the presence of some other vectors, just to make sure
>> we are indeed getting vectors here?
>
> Not testing for any present vectors makes me a little nervous: what if we
> just don't get any vectors because inlining fails or something else silly
> happens?
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128379987
Re: RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]
> The implemented solution modifies the `OutputBuffer` implementation instead > of the `OutputAnalyzer` implementation. > This is because the **OutputBuffer implementation which handles processes** > (LazyOutputBuffer) starts a thread in its constructor, so we would need to > add a strange additional constructor parameter to the > `OutputBuffer.of(Process, Charset)` static method, while the printing through > to stdout (and stderr) only makes sense for LazyOutputBuffer. > > I believe changing the config option from `outputanalyzer.verbose` to `output > buffer.verbose` would make it cleaner, and avoid referencing the > OutputAnalyzer in the OutputBuffer implementation. Alice Pellegrini 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 three additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into 8356438-outputanalyzer-optional-print - Update test/lib/jdk/test/lib/process/OutputBuffer.java Co-authored-by: Chen Liang - Initial working solution - Changes: - all: https://git.openjdk.org/jdk/pull/25587/files - new: https://git.openjdk.org/jdk/pull/25587/files/db559d20..7cfbec22 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25587&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25587&range=00-01 Stats: 20245 lines in 457 files changed: 17183 ins; 1318 del; 1744 mod Patch: https://git.openjdk.org/jdk/pull/25587.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25587/head:pull/25587 PR: https://git.openjdk.org/jdk/pull/25587
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Wed, 28 May 2025 12:18:15 GMT, Emanuel Peter wrote:
>> erifan has updated the pull request with a new target base due to a merge or
>> a rebase. The incremental webrev excludes the unrelated changes brought in
>> by the merge/rebase. The pull request contains 10 additional commits since
>> the last revision:
>>
>> - Refactor the JTReg tests for compare.xor(maskAll)
>>
>>Also made a bit change to support pattern `VectorMask.fromLong()`.
>> - Merge branch 'master' into JDK-8354242
>> - Refactor code
>>
>>Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this
>>optimization, making the code more modular.
>> - Merge branch 'master' into JDK-8354242
>> - Update the jtreg test
>> - Merge branch 'master' into JDK-8354242
>> - Addressed some review comments
>>
>>1. Call VectorNode::Ideal() only once in XorVNode::Ideal.
>>2. Improve code comments.
>> - Merge branch 'master' into JDK-8354242
>> - Merge branch 'master' into JDK-8354242
>> - 8354242: VectorAPI: combine vector not operation with compare
>>
>>This patch optimizes the following patterns:
>>For integer types:
>>```
>>(XorV (VectorMaskCmp src1 src2 cond) (Replicate -1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>(XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>```
>>cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>>negative comparison of cond.
>>
>>For float and double types:
>>```
>>(XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>(XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>```
>>cond can be eq or ne.
>>
>>Benchmarks on Nvidia Grace machine with 128-bit SVE2:
>>With option `-XX:UseSVE=2`:
>>```
>>Benchmark UnitBefore Score Error After
>> Score Error Uplift
>>testCompareEQMaskNotByte ops/s 7912127.225 2677.289518
>> 10266136.26 8955.008548 1.29
>>testCompareEQMaskNotDoubleops/s 884737.6799 446.963779
>> 1179760.772 448.031844 1.33
>>testCompareEQMaskNotFloat ops/s 1765045.787 682.332214
>> 2359520.803 896.305743 1.33
>>testCompareEQMaskNotInt ops/s 1787221.411 977.743935
>> 2353952.519 960.069976 1.31
>>testCompareEQMaskNotLong ops/s 895297.1974 673.44808
>> 1178449.02 323.804205 1.31
>>testCompareEQMaskNotShort ops/s 3339987.002 3415.2226
>> 4712761.965 2110.862053 1.41
>>testCompareGEMaskNotByte ops/s 7907615.16 4...
>
> src/hotspot/share/opto/vectornode.cpp line 2251:
>
>> 2249: predicate_node, vt);
>> 2250: if (vmcast_vt != nullptr) {
>> 2251: // We optimized out an VectorMaskCast, and in order to ensure type
>
> Suggestion:
>
> // We optimized out a VectorMaskCast, and in order to ensure type
Done.
> src/hotspot/share/opto/vectornode.cpp line 2253:
>
>> 2251: // We optimized out an VectorMaskCast, and in order to ensure type
>> 2252: // correctness, we need to regenerate one. VectorMaskCast will be
>> encoded as
>> 2253: // empty for types with the same size.
>
> Suggestion:
>
> // a no-op (identity function) for types with the same size.
>
> Or what do you mean by "empty"? `TOP`? All zeros?
I mean `no-op`. Done, thanks.
> test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 96:
>
>> 94: Generator lGen = RD.uniformLongs(Long.MIN_VALUE,
>> Long.MAX_VALUE);
>> 95: Generator fGen = RD.uniformFloats(Float.MIN_VALUE,
>> Float.MAX_VALUE);
>> 96: Generator dGen = RD.uniformDoubles(Double.MIN_VALUE,
>> Double.MAX_VALUE);
>
> Are you sure you only want to draw from the uniform distribution?
> If you don't super care about the distribution, please just take
> `RD.ints/longs/floats/doubles()`.
> That way, you get all sorts of distributions, and also some that include NaN
> values etc. I think that would be important for your float cmp cases, no?
For float and double, we have to use the uniform distribution, because we have
to make sure `NAN` is not generated. I added some comments about the reasons.
For other types, changed to use `RD.ints/longs`.
We have covered the special cases like +/- Inf, NaN.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128376851
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r212837
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128375032
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
On Thu, 5 Jun 2025 10:10:41 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move the timestamp to before the try block, change bytes to bytesWritten > and remove unnecessary code src/java.base/share/classes/java/lang/Throwable.java line 124: > 122: * exceptions should be traced by JFR. > 123: */ > 124: static boolean jfrTracing; Are you sure this is okay? - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128525269
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v8]
On Thu, 5 Jun 2025 09:51:51 GMT, Alan Bateman wrote: >> Erik Gahlin has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Remove the mistakenly added file. >> - Fix whitespace > > src/jdk.jfr/share/conf/jfr/default.jfc line 845: > >> 843: true >> 844: true >> 845: > control="exceptions-throttle-rate">100/s > > What was used to propose 100/s as the default and 300/s in profile.jfc? Just > curious if there was a benchmark or test to see the overhead. It's hard to benchmark since it depends on the application, but we have used 150 events / second for the Object Allocation Sample event without complaints. If you divide one second by 20 ms, the previous threshold, you end up with at most 50 events / second per thread. That said, I think few applications actually have socket read/write around 20-25 ms, so I don't think we should draw too many conclusions from that. Another way of thinking about it is that the cost of walking maximum 64 stack frames is roughly 5000 ns, so we end up with 100 * 5000 = 500 000 ns or 0.05% of CPU usage. There is also the cost of sampling, and that's why I put in the 1 ms threshold, which puts an upper bound of 1000 sampling attempts per thread. For profile, I used the same value as Object Allocation Sample. - PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128549389
Re: RFR: 8358426: Improve lazy computation in Locale
On Thu, 5 Jun 2025 00:18:34 GMT, Chen Liang wrote: >> Please review this PR which improves occurrences of lazy computation in >> `Locale` and `BaseLocale`. >> >> Existing lazy initialization strategies such as CHM, static nested class, >> and local inner class are replaced with Stable Values. >> >> Lambda usage is intentionally avoided in this change during `Locale` >> creation and in static fields due to potential startup performance >> degradation as noted by >> [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932). >> >> Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166 >> resource is represented as a SV. Also, I did not think it was necessary to >> maintain a SV for _both_ the array and set of ISO3166-1 alpha-2 codes. > > Looks correct. Note we have a separate effort #25040 to avoid anonymous > supplier classes, don't know if @minborg wishes locale to migrate to that one > directly or remain on the more high-level SV. Looks good! As @liach mentioned, there is an even better way coming, but I think it's good to do this update anyway. Here is a similar one that I am working on: https://github.com/openjdk/jdk/pull/25630 So, once we have stable field updaters, it will be almost trivial to update the code in this PR. - PR Comment: https://git.openjdk.org/jdk/pull/25646#issuecomment-2943716752
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v9]
On Thu, 5 Jun 2025 10:10:41 GMT, Erik Gahlin wrote: >> Could I have review of an enhancement that adds rate-limited sampling to >> Java events, including five events in the JDK (SocketRead, SocketWrite, >> FileRead, FileWrite, and JavaExceptionThrow). >> >> Testing: test/jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move the timestamp to before the try block, change bytes to bytesWritten > and remove unnecessary code Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25559#pullrequestreview-2899934064
Integrated: 8351594: JFR: Rate-limited sampling of Java events
On Fri, 30 May 2025 22:30:25 GMT, Erik Gahlin wrote: > Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik This pull request has now been integrated. Changeset: eb770a06 Author:Erik Gahlin URL: https://git.openjdk.org/jdk/commit/eb770a060ad86d69b38df7d11622e9e25a528e1d Stats: 1346 lines in 37 files changed: 1056 ins; 174 del; 116 mod 8351594: JFR: Rate-limited sampling of Java events Reviewed-by: mgronlun, alanb - PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]
On Thu, 5 Jun 2025 13:11:39 GMT, David Holmes wrote: > To where is the output going "as it happens"? If the `outputanalyzer.verbose` flag is set, the output is going to be printed to the console, in addition to being stored in the buffer > If you use OutputAnalyzer correctly then on failure all output to stdout and > stderr is printed via reportDiagnosticSummary. Could you give a concrete > example of a test with a problem and how this change fixes it? As far as I understand, the creator of the issue would like debugging to be easier; they'd like, while debugging (so having paused) the execution of a test, to be able to see the output of whatever tool is wrapper by `OutputBuffer` in the console, **before** hitting the failure, looking at the output so far in the console, while being able to examine the state of the program being debugged. As far as an example, I defer to the poster @mpdonova in case they have a more concrete example. - PR Comment: https://git.openjdk.org/jdk/pull/25587#issuecomment-2944467386
Re: RFR: 8358626: Emit UTF-8 CLDR resources [v2]
> Changes to generate CLDR resource bundles in UTF-8 encoding. The resource > files in `java.base` are supposed to be US English only, but they also need > to use UTF-8 as some of the names are non-ASCII (e.g., Türkiye) Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: java.base/Gensrc.gmk comment - Changes: - all: https://git.openjdk.org/jdk/pull/25648/files - new: https://git.openjdk.org/jdk/pull/25648/files/a5b1c576..5f87c71e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25648&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25648&range=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25648.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25648/head:pull/25648 PR: https://git.openjdk.org/jdk/pull/25648
Re: RFR: 8358626: Emit UTF-8 CLDR resources
On Thu, 5 Jun 2025 01:42:00 GMT, Naoto Sato wrote: >> Changes to generate CLDR resource bundles in UTF-8 encoding. The resource >> files in `java.base` are supposed to be US English only, but they also need >> to use UTF-8 as some of the names are non-ASCII (e.g., Türkiye) > > Looks like this PR did not send out the email in the first place, I am > leaving a comment to kick the bot > @naotoj, shall we add this note as a comment to places where `-utf8` is added? Good point. Added. - PR Comment: https://git.openjdk.org/jdk/pull/25648#issuecomment-2945607754
Re: RFR: 8352565: Add native method implementation of Reference.get() [v9]
On Thu, 5 Jun 2025 08:42:38 GMT, Kim Barrett wrote: >> Please review this change which adds a native method providing the >> implementation of Reference::get. Referece::get is an intrinsic candidate, >> so >> this native method implementation is only used when the intrinsic is not. >> >> Currently there is intrinsic support by the interpreter, C1, C2, and graal, >> which are always used. With this change we can later remove all the >> per-platform interpreter intrinsic implementations, and might also remove the >> C1 intrinsic implementation. >> >> Testing: >> (1) mach5 tier1-6 normal (so using all the existing intrinsics). >> (2) mach5 tier1-6 with interpreter and C1 Reference::get intrinsics disabled. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment alignment Marked as reviewed by vlivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24315#pullrequestreview-2901636995
Re: RFR: 8358426: Improve lazy computation in Locale
On Thu, 5 Jun 2025 18:32:34 GMT, Justin Lu wrote: >> src/java.base/share/classes/java/util/Locale.java line 2578: >> >>> 2576: return Set.of(LocaleISOData.ISO3166_3); >>> 2577: } >>> 2578: }); >> >> What about moving these four stable suppliers and `getISO2Table` to >> LocaleISOData to shrink size of Locale.java? > > I like this suggestion, we can also move `getISO3Code` in addition to those > you mentioned, and have all ISO resources and methods come from > LocaleISOData. @naotoj, would you be in favor of this? I agree, that would be more straightforward. Thanks for the suggestion! - PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2129770483
Re: RFR: 8352565: Add native method implementation of Reference.get() [v10]
On Wed, 4 Jun 2025 17:39:55 GMT, Chen Liang wrote:
>> Kim Barrett has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - add pseudo-native entry for Reference.get0
>> - tidy CallGenerator lookup in Compile ctor
>
> src/hotspot/share/opto/compile.cpp line 786:
>
>> 784: initial_gvn()->set_type_bottom(s);
>> 785: verify_start(s);
>> 786: if (method()->intrinsic_id() == vmIntrinsics::_Reference_get) {
>
> Should we remove this now or as part of the redundant intrinsic cleanup for
> interpreter and c1? I see the interpreter is now kept intact.
I'm planning to remove the interpreter intrinsic as a followup. I couldn't
figure out how to hit this after making Reference::get0 the intrinsic.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2131561433
Re: RFR: 8352565: Add native method implementation of Reference.get() [v10]
On Fri, 6 Jun 2025 05:57:16 GMT, Kim Barrett wrote: >> Please review this change which adds a native method providing the >> implementation of Reference::get. Referece::get is an intrinsic candidate, >> so >> this native method implementation is only used when the intrinsic is not. >> >> Currently there is intrinsic support by the interpreter, C1, C2, and graal, >> which are always used. With this change we can later remove all the >> per-platform interpreter intrinsic implementations, and might also remove the >> C1 intrinsic implementation. >> >> Testing: >> (1) mach5 tier1-6 normal (so using all the existing intrinsics). >> (2) mach5 tier1-6 with interpreter and C1 Reference::get intrinsics disabled. > > Kim Barrett has updated the pull request incrementally with two additional > commits since the last revision: > > - add pseudo-native entry for Reference.get0 > - tidy CallGenerator lookup in Compile ctor src/hotspot/share/interpreter/templateInterpreterGenerator.cpp line 231: > 229: // intrinsic is disabled. > 230: native_method_entry(java_lang_Thread_currentThread) > 231: native_method_entry(java_lang_ref_reference_get0) It turned out there was a bug lurking in the change to move the intrinsic to Reference::get0. I had tested it with the interpreter intrinsic made inoperative, but nearly forgot to test the normal case. It turned out that if the interpreter intrinsic was operational but disabled then the interpreter would hit an assert "tried to execute native method as non-native". This line is the fix for that. - PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2131558170
Re: RFR: 8352565: Add native method implementation of Reference.get() [v10]
> Please review this change which adds a native method providing the > implementation of Reference::get. Referece::get is an intrinsic candidate, so > this native method implementation is only used when the intrinsic is not. > > Currently there is intrinsic support by the interpreter, C1, C2, and graal, > which are always used. With this change we can later remove all the > per-platform interpreter intrinsic implementations, and might also remove the > C1 intrinsic implementation. > > Testing: > (1) mach5 tier1-6 normal (so using all the existing intrinsics). > (2) mach5 tier1-6 with interpreter and C1 Reference::get intrinsics disabled. Kim Barrett has updated the pull request incrementally with two additional commits since the last revision: - add pseudo-native entry for Reference.get0 - tidy CallGenerator lookup in Compile ctor - Changes: - all: https://git.openjdk.org/jdk/pull/24315/files - new: https://git.openjdk.org/jdk/pull/24315/files/edd4dec2..46ba079f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24315&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24315&range=08-09 Stats: 6 lines in 2 files changed: 1 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/24315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24315/head:pull/24315 PR: https://git.openjdk.org/jdk/pull/24315
Integrated: 8357821: Revert incorrectly named JavaLangAccess::unchecked* methods
On Fri, 30 May 2025 11:57:39 GMT, Volkan Yazici wrote: > Reverts certain [JDK-8353197](https://bugs.openjdk.org/browse/JDK-8353197) > (which implements JavaDoc improvement and precautionary naming for certain > unsafe methods in `jdk.internal.access.JavaLangAccess`) changes that are > found to be incorrect. See the JBS issue for details on the followed > evaluation process. This pull request has now been integrated. Changeset: e918a59b Author:Volkan Yazici URL: https://git.openjdk.org/jdk/commit/e918a59b1dacf273620aee334517bebfb1fb1a0f Stats: 25 lines in 12 files changed: 0 ins; 0 del; 25 mod 8357821: Revert incorrectly named JavaLangAccess::unchecked* methods Reviewed-by: pminborg - PR: https://git.openjdk.org/jdk/pull/25545
Integrated: 8349914: ZipFile::entries and ZipFile::getInputStream not consistent with each other when there are duplicate entries
On Wed, 4 Jun 2025 09:53:13 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the issue > noted in https://bugs.openjdk.org/browse/JDK-8349914? > > The ZIP specification allows for more than one entry to have the same file > name (and may have different file data). In such situation, as noted in the > linked issue, the `ZipFile.getInputStream(ZipEntry)` may end up picking up an > "incorrect" entry content (and metadata) for the passed entry. > > The commit in this PR addresses that issue by holding on to the LOC offset of > the entry in the `ZipEntry` class. This way, whenever that `ZipEntry` > instance is used, it knows which exact entry it represents. > > A new jtreg test has been introduced to reproduce the issue and verify the > fix. The new test and existing tests in tier1, tier2 and tier3 pass with this > change. This pull request has now been integrated. Changeset: 029e3bf8 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/029e3bf8f582f7399b80c592421b2fd72737e264 Stats: 237 lines in 3 files changed: 233 ins; 0 del; 4 mod 8349914: ZipFile::entries and ZipFile::getInputStream not consistent with each other when there are duplicate entries Co-authored-by: Lance Andersen Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/25635
Re: RFR: 8349914: ZipFile::entries and ZipFile::getInputStream not consistent with each other when there are duplicate entries
On Wed, 4 Jun 2025 09:53:13 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the issue > noted in https://bugs.openjdk.org/browse/JDK-8349914? > > The ZIP specification allows for more than one entry to have the same file > name (and may have different file data). In such situation, as noted in the > linked issue, the `ZipFile.getInputStream(ZipEntry)` may end up picking up an > "incorrect" entry content (and metadata) for the passed entry. > > The commit in this PR addresses that issue by holding on to the LOC offset of > the entry in the `ZipEntry` class. This way, whenever that `ZipEntry` > instance is used, it knows which exact entry it represents. > > A new jtreg test has been introduced to reproduce the issue and verify the > fix. The new test and existing tests in tier1, tier2 and tier3 pass with this > change. Thank you Lance for the review. - PR Comment: https://git.openjdk.org/jdk/pull/25635#issuecomment-2947807108
RFR: 8358520: Improve lazy computation in BreakIteratorResourceBundle and related classes
This PR proposes to simplify lazy computation related to resource bundles. Previously, some objects were computed lazily using a double-checked locking algorithm. StableValues offers a more robust and succinct solution. This PR passes tier1, tier2, and tier3 on multiple platforms. - Commit messages: - Break out logic in separate methods - Revoke the use of immutable collections - Fix concurrency issues Changes: https://git.openjdk.org/jdk/pull/25630/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25630&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8358520 Stats: 138 lines in 3 files changed: 51 ins; 73 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/25630.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25630/head:pull/25630 PR: https://git.openjdk.org/jdk/pull/25630
Re: RFR: 8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Sun, 22 Oct 2023 17:26:52 GMT, Laurent Bourgès wrote: >> * improved mixed insertion sort (makes whole sorting faster) >> * introduced Radix which sort shows several times boost of performance and >> has linear complexity instead of n*ln(n) >> * improved merging sort for almost sorted data >> * optimized parallel sorting >> * improved step for pivot candidates and pivot partitioning >> * extended existing tests >> * added benchmarking JMH tests >> * suggested better buffer allocation: if no memory, it is switched to >> in-place sorting with no OutOfMemoryError, threshold is 1/16th heap >> >> I am going on previous PR by Vladimir Yaroslavskyi: >> https://github.com/openjdk/jdk/pull/3938 > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > add @SuppressWarnings (serial) keep alive, need time to complete - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-2945907790
Re: RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]
On Thu, 5 Jun 2025 09:36:37 GMT, Alice Pellegrini wrote: >> The implemented solution modifies the `OutputBuffer` implementation instead >> of the `OutputAnalyzer` implementation. >> This is because the **OutputBuffer implementation which handles processes** >> (LazyOutputBuffer) starts a thread in its constructor, so we would need to >> add a strange additional constructor parameter to the >> `OutputBuffer.of(Process, Charset)` static method, while the printing >> through to stdout (and stderr) only makes sense for LazyOutputBuffer. >> >> I believe changing the config option from `outputanalyzer.verbose` to >> `output buffer.verbose` would make it cleaner, and avoid referencing the >> OutputAnalyzer in the OutputBuffer implementation. > > Alice Pellegrini 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 three additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into > 8356438-outputanalyzer-optional-print > - Update test/lib/jdk/test/lib/process/OutputBuffer.java > >Co-authored-by: Chen Liang > - Initial working solution I'm not sure I really understand the proposal here. To where is the output going "as it happens"? If you use OutputAnalyzer correctly then on failure all output to stdout and stderr is printed via reportDiagnosticSummary. Could you give a concrete example of a test with a problem and how this change fixes it? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/25587#issuecomment-2944254033
Re: RFR: 8358426: Improve lazy computation in Locale
On Thu, 5 Jun 2025 11:50:01 GMT, Johannes Döbler wrote:
>> Please review this PR which improves occurrences of lazy computation in
>> `Locale` and `BaseLocale`.
>>
>> Existing lazy initialization strategies such as CHM, static nested class,
>> and local inner class are replaced with Stable Values.
>>
>> Lambda usage is intentionally avoided in this change during `Locale`
>> creation and in static fields due to potential startup performance
>> degradation as noted by
>> [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932).
>>
>> Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166
>> resource is represented as a SV. Also, I did not think it was necessary to
>> maintain a SV for _both_ the array and set of ISO3166-1 alpha-2 codes.
>
> src/java.base/share/classes/java/util/Locale.java line 1269:
>
>> 1267: public static String[] getISOCountries() {
>> 1268: String[] countries = ISO_3166_1_ALPHA2.get();
>> 1269: return Arrays.copyOf(countries, countries.length);
>
> what about `return ISO_3166_1_ALPHA2.get().clone();`
`clone` is more concise, but I think I prefer the explicitness of `copyOf` here.
> src/java.base/share/classes/java/util/Locale.java line 2578:
>
>> 2576: return Set.of(LocaleISOData.ISO3166_3);
>> 2577: }
>> 2578: });
>
> What about moving these four stable suppliers and `getISO2Table` to
> LocaleISOData to shrink size of Locale.java?
I like this suggestion, we can also move `getISO3Code` in addition to those you
mentioned, and have all ISO resources and methods come from LocaleISOData.
@naotoj, would you be in favor of this?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2129762095
PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2129762038
Re: RFR: 8358520: Improve lazy computation in BreakIteratorResourceBundle and related classes
On Tue, 3 Jun 2025 20:14:31 GMT, Per Minborg wrote: > This PR proposes to simplify lazy computation related to resource bundles. > Previously, some objects were computed lazily using a double-checked locking > algorithm. StableValues offers a more robust and succinct solution. > > > This PR passes tier1, tier2, and tier3 on multiple platforms. Looks good and clean. Thanks for the refactoring, Per! - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25630#pullrequestreview-2901743240
Re: RFR: 8077587: BigInteger Roots [v15]
On Sat, 17 May 2025 11:25:58 GMT, fabioromano1 wrote: >> The [Rampdown Phase One](https://openjdk.org/projects/jdk/25/) for JDK 25 is >> about 3 weeks from now. >> >> I think it's a bit too late for API additions to be approved in due time. >> And even if we could rush this work for 25, it makes little sense to have >> this in 25 and the future one in 26. IMO, they should be released together. >> So this and the followup PR for `BigDecimal` will probably be integrated >> only in 26. >> >> In other words, take your time ;-) > > @rgiulietti Regarding the tests for _n_-th root, the tests for `sqrt()` could > be extended in order to test also `nthRoot()`. @fabioromano1 Now that the jdk25 stabilization branch has been created, work on mainline (branch master) will no longer affect JDK25 (except for backports of bug fixes). I'll soon start reviewing this PR based on the work of Brent and Zimmermann. - PR Comment: https://git.openjdk.org/jdk/pull/24898#issuecomment-2946510626
Re: RFR: 8358520: Improve lazy computation in BreakIteratorResourceBundle and related classes
On Tue, 3 Jun 2025 20:14:31 GMT, Per Minborg wrote: > This PR proposes to simplify lazy computation related to resource bundles. > Previously, some objects were computed lazily using a double-checked locking > algorithm. StableValues offers a more robust and succinct solution. > > > This PR passes tier1, tier2, and tier3 on multiple platforms. I am thinking about removing javatime supplementary logic altogether, including ParallelListResourceBundle. Might be good to wait for it - PR Comment: https://git.openjdk.org/jdk/pull/25630#issuecomment-2946933327
Re: RFR: 8358520: Improve lazy computation in BreakIteratorResourceBundle and related classes
On Tue, 3 Jun 2025 20:14:31 GMT, Per Minborg wrote: > This PR proposes to simplify lazy computation related to resource bundles. > Previously, some objects were computed lazily using a double-checked locking > algorithm. StableValues offers a more robust and succinct solution. > > > This PR passes tier1, tier2, and tier3 on multiple platforms. Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/25630#pullrequestreview-2903045850
Integrated: 8355746: Start of release updates for JDK 26
On Fri, 2 May 2025 14:48:01 GMT, Nizar Benalla wrote: > Get JDK 26 underway. This pull request has now been integrated. Changeset: af87035b Author:Nizar Benalla Committer: Jesper Wilhelmsson URL: https://git.openjdk.org/jdk/commit/af87035b713f8bfe05a007a4d4670cefc6a6aaf2 Stats: 1830 lines in 60 files changed: 1740 ins; 16 del; 74 mod 8355746: Start of release updates for JDK 26 8355748: Add SourceVersion.RELEASE_26 8355751: Add source 26 and target 26 to javac Co-authored-by: Joe Darcy Reviewed-by: iris, coleenp, darcy - PR: https://git.openjdk.org/jdk/pull/25008
Re: RFR: 8357531: The `SegmentBulkOperations::fill` method can be improved using overlaps [v8]
> This PR builds on a concept John Rose told me about some time ago. Instead of > combining memory operations of various sizes, a single large and skewed > memory operation can be made to clean up the tail of remaining bytes. > > This has the effect of simplifying and shortening the code. The number of > branches to evaluate is reduced. > > It should be noted that the performance of the fill operation affects the > allocation of new segments (as they are zeroed out before being returned to > the client code). > > This PR passes tier1, tier2, and tier3 on multiple platforms. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Use a fixed threashold for fill - Changes: - all: https://git.openjdk.org/jdk/pull/25383/files - new: https://git.openjdk.org/jdk/pull/25383/files/e9e8d15e..0da4c0be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25383&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25383&range=06-07 Stats: 11 lines in 2 files changed: 5 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/25383.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25383/head:pull/25383 PR: https://git.openjdk.org/jdk/pull/25383
Re: RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]
On Thu, 5 Jun 2025 09:36:37 GMT, Alice Pellegrini wrote:
>> The implemented solution modifies the `OutputBuffer` implementation instead
>> of the `OutputAnalyzer` implementation.
>> This is because the **OutputBuffer implementation which handles processes**
>> (LazyOutputBuffer) starts a thread in its constructor, so we would need to
>> add a strange additional constructor parameter to the
>> `OutputBuffer.of(Process, Charset)` static method, while the printing
>> through to stdout (and stderr) only makes sense for LazyOutputBuffer.
>>
>> I believe changing the config option from `outputanalyzer.verbose` to
>> `output buffer.verbose` would make it cleaner, and avoid referencing the
>> OutputAnalyzer in the OutputBuffer implementation.
>
> Alice Pellegrini 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 three additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/master' into
> 8356438-outputanalyzer-optional-print
> - Update test/lib/jdk/test/lib/process/OutputBuffer.java
>
>Co-authored-by: Chen Liang
> - Initial working solution
test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
> 148: this.p = p;
> 149: logProgress("Gathering output");
> 150: boolean verbose = Boolean.getBoolean("outputanalyzer.verbose");
Putting a system property at this level of the code kind of hides the
functionality.
An alternative solution would be to have `OutputAnalyzer` constructor(s) that
takes a boolean argument. That boolean could be set as needed by individual
tests using the `test.debug` property already used by a lot of tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2129066907
Re: RFR: 8353113: Peer supported certificate signature algorithms are not being checked with default SunX509 key manager [v4]
On Wed, 14 May 2025 18:16:13 GMT, Artur Barashev wrote:
>> When the deafult SunX509KeyManagerImpl is being used we are in violation of
>> TLSv1.3 RFC spec because we ignore peer supported certificate signatures
>> sent to us in "signature_algorithms"/"signature_algorithms_cert" extensions:
>> https://datatracker.ietf.org/doc/html/rfc8446#section-4.4.2.2
>> https://datatracker.ietf.org/doc/html/rfc8446#section-4.4.2.3
>>
>> X509KeyManagerImpl on the other hand includes the algorithms sent by the
>> peer in "signature_algorithms_cert" extension (or in "signature_algorithms"
>> extension when "signature_algorithms_cert" extension isn't present) in the
>> algorithm constraints being checked.
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make the test run on TLSv1.3
Some initial comments.
src/java.base/share/classes/sun/security/ssl/SunX509KeyManagerImpl.java line
264:
> 262: * the peer (if any).
> 263: *
> 264: * @since 1.5
I would remove the `@since 1.5` from these methods. It isn't relevant anymore
since this is an internal class and that version is no longer supported. That
version info is in the `X509ExtendedKeyManager` API which is sufficient.
src/java.base/share/classes/sun/security/ssl/SunX509KeyManagerImpl.java line
395:
> 393: if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
> 394: SSLLogger.fine("Ignore alias " + alias +
> 395: ": certificate list does not conform to " +
suggest saying "certificate chain" not "certificate list".
test/lib/jdk/test/lib/security/CertificateBuilder.java line 244:
> 242: * @throws IOException if an encoding error occurs.
> 243: */
> 244: public CertificateBuilder addSubjectAltNameIPExt(List
> IPAddresses)
variables usually start with lower case letter, so suggest `ipAddresses`
-
PR Review: https://git.openjdk.org/jdk/pull/25016#pullrequestreview-2900744862
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2129536065
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2129556680
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2129090967
RFR: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
It's too fragile to depend on generated NPE messages - Commit messages: - Removing exception message check to avoid failures if helpful NPEs are turned off Changes: https://git.openjdk.org/jdk/pull/25655/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25655&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8358633 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25655/head:pull/25655 PR: https://git.openjdk.org/jdk/pull/25655
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v5]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin 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 eight additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Fix adjust boundary - Some reviewer feedback - Consistent annotation - Fix typos - Fix whitespace - Initial - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/59278ca8..535d458a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=03-04 Stats: 2776 lines in 58 files changed: 477 ins; 2217 del; 82 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v6]
On Thu, 5 Jun 2025 08:40:44 GMT, Erik Gahlin wrote:
>> Could I have review of an enhancement that adds rate-limited sampling to
>> Java events, including five events in the JDK (SocketRead, SocketWrite,
>> FileRead, FileWrite, and JavaExceptionThrow).
>>
>> Testing: test/jdk/jdk/jfr
>>
>> Thanks
>> Erik
>
> Erik Gahlin 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 nine additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into throttle-mania-3
> - Merge remote-tracking branch 'upstream/master' into throttle-mania-3
> - Merge remote-tracking branch 'upstream/master' into throttle-mania-3
> - Fix adjust boundary
> - Some reviewer feedback
> - Consistent annotation
> - Fix typos
> - Fix whitespace
> - Initial
src/java.base/share/classes/java/io/RandomAccessFile.java line 594:
> 592: } finally {
> 593: long end = FileWriteEvent.timestamp();
> 594: long duration = end - start;
Suggestion:
long duration = end - start;
src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 65:
> 63: public final class EventInstrumentation {
> 64: public static final long MASK_THROTTLE = 1 << 62;
> 65: public static final long MASK_THROTTLE_CHECK= 1 << 63;
Let's align
Suggestion:
public static final long MASK_THROTTLE = 1 << 62;
public static final long MASK_THROTTLE_CHECK = 1 << 63;
src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 593:
> 591: if (throttled) {
> 592: codeBuilder.aload(0);
> 593: getfield(codeBuilder, eventClassDesc,
> ImplicitFields.FIELD_DURATION);
Suggestion:
getfield(codeBuilder, eventClassDesc,
ImplicitFields.FIELD_DURATION);
test/jdk/jdk/jfr/api/recording/settings/TestSettingsAvailability.java line 93:
> 91: testSetting(EventNames.JVMInformation, "enabled", "period");
> 92: testSetting(EventNames.FileRead, "enabled", "threshold",
> "stackTrace", "throttle");
> 93: testSetting(EventNames.FileWrite, "enabled",
> "threshold","stackTrace", "throttle");
Suggestion:
testSetting(EventNames.FileWrite, "enabled", "threshold", "stackTrace",
"throttle");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128322876
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128321445
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128322474
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128323708
Re: RFR: 8357995: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [core] [v4]
On Tue, 3 Jun 2025 07:55:06 GMT, Volkan Yazici wrote:
>> Passes the `Charset` read from the `stdin.encoding` system property while
>> creating `InputStreamReader` or `Scanner` instances for `System.in`.
>>
>> `stdin.encoding` is a recently added property for Java 25 in
>> [JDK-8350703](https://bugs.openjdk.org/browse/JDK-8350703). Employing it
>> throughout the entire code base is addressed by the parent ticket
>> [JDK-8356893](https://bugs.openjdk.org/browse/JDK-8356893). JDK-8357995 this
>> PR is addressing is a sub-task of JDK-8356893 and is concerned with only
>> areas related to core libraries.
>
> Volkan Yazici has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix missing `java.io.Reader` import in `Ktab`
test/jdk/com/sun/jdi/MultiBreakpointsTest.java line 181:
> 179:
> 180: }
> 181: } catch(Exception e) {
Suggestion:
} catch (Exception e) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25544#discussion_r2128325755
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v4]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Fix adjust boundary - Some reviewer feedback - Consistent annotation - Fix typos - Fix whitespace - Initial - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/7fa2db19..59278ca8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=02-03 Stats: 26802 lines in 553 files changed: 20626 ins; 2170 del; 4006 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8358426: Improve lazy computation in Locale
On Wed, 4 Jun 2025 21:20:46 GMT, Justin Lu wrote: > Please review this PR which improves occurrences of lazy computation in > `Locale` and `BaseLocale`. > > Existing lazy initialization strategies such as CHM, static nested class, and > local inner class are replaced with Stable Values. > > Lambda usage is intentionally avoided in this change during `Locale` creation > and in static fields due to potential startup performance degradation as > noted by [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932). > > Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166 resource > is represented as a SV. Also, I did not think it was necessary to maintain a > SV for _both_ the array and set of ISO3166-1 alpha-2 codes. src/java.base/share/classes/java/util/Locale.java line 2348: > 2346: private static volatile Locale defaultFormatLocale; > 2347: > 2348: private transient final Supplier languageTag = let's use blessed modifiers order Suggestion: private final transient Supplier languageTag = - PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2128278908
Re: RFR: 8351594: JFR: Rate-limited sampling of Java events [v6]
> Could I have review of an enhancement that adds rate-limited sampling to Java > events, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik Erik Gahlin 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 nine additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Merge remote-tracking branch 'upstream/master' into throttle-mania-3 - Fix adjust boundary - Some reviewer feedback - Consistent annotation - Fix typos - Fix whitespace - Initial - Changes: - all: https://git.openjdk.org/jdk/pull/25559/files - new: https://git.openjdk.org/jdk/pull/25559/files/535d458a..8259ff41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25559&range=04-05 Stats: 2319 lines in 41 files changed: 2179 ins; 128 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/25559.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25559/head:pull/25559 PR: https://git.openjdk.org/jdk/pull/25559
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Thu, 29 May 2025 08:00:05 GMT, erifan wrote:
>> src/hotspot/share/opto/vectornode.cpp line 2233:
>>
>>> 2231: if (in2->Opcode() == Op_VectorMaskCast) {
>>> 2232: in2 = in2->in(1);
>>> 2233: }
>>
>> Wow, this seems to be an addition that is not covered in the patterns you
>> mention above, right?
>> But is that even necessary?
>> I suppose here `in2 = VectorMaskCast(all_ones_vector)`.
>> Would we not already want to transform this pattern in
>> `VectorMaskCast::Ideal`, is that not possible and more powerful?
>
> Oh yeah, I forgot to mention it in the above comment and commit message.
>
> Yes, this is for `in2 = VectorMaskCast(all_ones_vector)`. I agree it's
> better to do this transformation in `VectorMaskCast::Ideal`. I'll remove this
> code change and do the `VectorMaskCast` optimization later. Thanks!
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128344233
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v6]
On Wed, 28 May 2025 12:03:48 GMT, Emanuel Peter wrote:
>> erifan has updated the pull request with a new target base due to a merge or
>> a rebase. The incremental webrev excludes the unrelated changes brought in
>> by the merge/rebase. The pull request contains 10 additional commits since
>> the last revision:
>>
>> - Refactor the JTReg tests for compare.xor(maskAll)
>>
>>Also made a bit change to support pattern `VectorMask.fromLong()`.
>> - Merge branch 'master' into JDK-8354242
>> - Refactor code
>>
>>Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this
>>optimization, making the code more modular.
>> - Merge branch 'master' into JDK-8354242
>> - Update the jtreg test
>> - Merge branch 'master' into JDK-8354242
>> - Addressed some review comments
>>
>>1. Call VectorNode::Ideal() only once in XorVNode::Ideal.
>>2. Improve code comments.
>> - Merge branch 'master' into JDK-8354242
>> - Merge branch 'master' into JDK-8354242
>> - 8354242: VectorAPI: combine vector not operation with compare
>>
>>This patch optimizes the following patterns:
>>For integer types:
>>```
>>(XorV (VectorMaskCmp src1 src2 cond) (Replicate -1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>(XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>```
>>cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>>negative comparison of cond.
>>
>>For float and double types:
>>```
>>(XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>(XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>```
>>cond can be eq or ne.
>>
>>Benchmarks on Nvidia Grace machine with 128-bit SVE2:
>>With option `-XX:UseSVE=2`:
>>```
>>Benchmark UnitBefore Score Error After
>> Score Error Uplift
>>testCompareEQMaskNotByte ops/s 7912127.225 2677.289518
>> 10266136.26 8955.008548 1.29
>>testCompareEQMaskNotDoubleops/s 884737.6799 446.963779
>> 1179760.772 448.031844 1.33
>>testCompareEQMaskNotFloat ops/s 1765045.787 682.332214
>> 2359520.803 896.305743 1.33
>>testCompareEQMaskNotInt ops/s 1787221.411 977.743935
>> 2353952.519 960.069976 1.31
>>testCompareEQMaskNotLong ops/s 895297.1974 673.44808
>> 1178449.02 323.804205 1.31
>>testCompareEQMaskNotShort ops/s 3339987.002 3415.2226
>> 4712761.965 2110.862053 1.41
>>testCompareGEMaskNotByte ops/s 7907615.16 4...
>
> src/hotspot/share/opto/vectornode.cpp line 2213:
>
>> 2211: Node* in1 = in(1);
>> 2212: Node* in2 = in(2);
>> 2213: // Transformations for predicated IRs are not supported for now.
>
> Suggestion:
>
> // Transformations for predicated vectors are not supported for now.
Done.
> src/hotspot/share/opto/vectornode.cpp line 2215:
>
>> 2213: // Transformations for predicated IRs are not supported for now.
>> 2214: if (is_predicated_vector() || in1->is_predicated_vector() ||
>> 2215: in2->is_predicated_vector()) {
>
> I would either put all on the same line, or all on separate lines.
Done.
> src/hotspot/share/opto/vectornode.cpp line 2219:
>
>> 2217: }
>> 2218:
>> 2219: // XorV/XorVMask is commutative, swap
>> VectorMaskCmp/Op_VectorMaskCast to in1.
>
> Suggestion:
>
> // XorV/XorVMask is commutative, swap VectorMaskCmp/VectorMaskCast to in1.
>
> Would look a little cleaner, and you did also not write `Op_VectorMaskCmp`
> either ;)
Done, thanks!
> src/hotspot/share/opto/vectornode.cpp line 2225:
>
>> 2223: }
>> 2224:
>> 2225: const TypeVect* vmcast_vt = nullptr;
>
> Suggestion:
>
> const TypeVect* vector_mask_cast_vt = nullptr;
>
> I think it would not hurt to write it out. Otherwise, the reader always has
> to reconstruct that in their head.
Done.
> src/hotspot/share/opto/vectornode.cpp line 2230:
>
>> 2228: vmcast_vt = in1->as_Vector()->vect_type();
>> 2229: in1 = in1->in(1);
>> 2230: }
>
> Add a comment why you check `in1->outcnt() == 1`.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128341063
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128340484
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128341959
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128342468
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2128342908
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v7]
> This patch optimizes the following patterns: > For integer types: > > (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) > => (VectorMaskCmp src1 src2 ncond) > (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) > => (VectorMaskCmp src1 src2 ncond) > > cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the > negative comparison of cond. > > For float and double types: > > (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) > => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) > (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) > => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) > > cond can be eq or ne. > > Benchmarks on Nvidia Grace machine with 128-bit SVE2: With option > `-XX:UseSVE=2`: > > Benchmark UnitBefore Score Error After > Score Error Uplift > testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 > 10266136.26 8955.008548 1.29 > testCompareEQMaskNotDoubleops/s 884737.6799 446.963779 > 1179760.772 448.031844 1.33 > testCompareEQMaskNotFloat ops/s 1765045.787 682.332214 > 2359520.803 896.305743 1.33 > testCompareEQMaskNotInt ops/s 1787221.411 977.743935 > 2353952.519 960.069976 1.31 > testCompareEQMaskNotLong ops/s 895297.1974 673.44808 > 1178449.02 323.804205 1.31 > testCompareEQMaskNotShort ops/s 3339987.002 3415.2226 > 4712761.965 2110.862053 1.41 > testCompareGEMaskNotByte ops/s 7907615.16 4094.243652 > 10251646.9 9486.699831 1.29 > testCompareGEMaskNotInt ops/s 1683738.958 4233.813092 > 2352855.205 1251.952546 1.39 > testCompareGEMaskNotLong ops/s 854496.1561 8594.598885 > 1177811.493 521.12291.37 > testCompareGEMaskNotShort ops/s 3341860.309 1578.975338 > 4714008.434 1681.10365 1.41 > testCompareGTMaskNotByte ops/s 7910823.674 2993.367032 > 10245063.58 9774.75138 1.29 > testCompareGTMaskNotInt ops/s 1673393.928 3153.099431 > 2353654.521 1190.848583 1.4 > testCompareGTMaskNotLong ops/s 849405.9159 2432.858159 > 1177952.041 359.96413 1.38 > testCompareGTMaskNotShort ops/s 3339509.141 3339.976585 > 4711442.496 2673.364893 1.41 > testCompareLEMaskNotByte ops/s 7911340.004 3114.69191 > 10231626.5 27134.20035 1.29 > testCompareLEMaskNotInt ops/s 1675812.113 1340.969885 > 2353255.341 1452.4522 1.4 > testCompareLEMaskNotLong ops/s 848862.8036 6564.841731 > 1177763.623 539.290106 1.38 > testCompareLEMaskNotShort ops/s 3324951.54 2380.29473 > 4712116.251 1544.559684 1.41 > testCompareLTMaskNotByte ops/s 7910390.844 2630.861436 > 10239567.69 6487.441672 1.29 > testCompareLTMaskNotInt ops/s 1672180.09 995.238142 > 2353757.863 853.774734 1.4 > testCompareLTMaskNotLong ops/s 856502.26... erifan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Addressed some review comments - Merge branch 'master' into JDK-8354242 - Refactor the JTReg tests for compare.xor(maskAll) Also made a bit change to support pattern `VectorMask.fromLong()`. - Merge branch 'master' into JDK-8354242 - Refactor code Add a new function XorVNode::Ideal_XorV_VectorMaskCmp to do this optimization, making the code more modular. - Merge branch 'master' into JDK-8354242 - Update the jtreg test - Merge branch 'master' into JDK-8354242 - Addressed some review comments 1. Call VectorNode::Ideal() only once in XorVNode::Ideal. 2. Improve code comments. - Merge branch 'master' into JDK-8354242 - ... and 2 more: https://git.openjdk.org/jdk/compare/71938fba...ebbcc405 - Changes: - all: https://git.openjdk.org/jdk/pull/24674/files - new: https://git.openjdk.org/jdk/pull/24674/files/f2f71e34..ebbcc405 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24674&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24674&range=05-06 Stats: 146911 lines in 2345 files changed: 87334 ins; 41007 del; 18570 mod Patch: https://git.openjdk.org/jdk/pull/24674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24674/head:pull/24674 PR: https://git.openjdk.org/jdk/pull/24674
Re: RFR: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
On Thu, 5 Jun 2025 08:12:29 GMT, David Holmes wrote: >> It's too fragile to depend on generated NPE messages > > Sorry Viktor but I strongly disagree. It is far too easy to have a test > "pass" because it catches the wrong instance of a thrown exception and hide > an underlying problem. @dholmes-ora In general I agree with you 100%, in this case it was actually the addition of the exception message which broke test runs when -XX:-OmitStackTraceInFastThrow disables JEP 358: https://github.com/openjdk/jdk/commit/f141674d1619d95053d38a9cd8f93a8959b4a211#diff-17cb67c2df590a137a7c215e9367919cd7224b4eedd9c1b0a7e35ccd0a1ac450R1730 So yeah, we could change the test to accept either a null message, or the JEP 358 message, but a null message would still be not verifying that it is thrown for the intended reason (thus hiding underlying issues, as you noted). - PR Comment: https://git.openjdk.org/jdk/pull/25655#issuecomment-2943361363
Re: RFR: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
On Thu, 5 Jun 2025 08:12:29 GMT, David Holmes wrote: > It is far too easy to have a test "pass" because it catches the wrong > instance of a thrown exception and hide an underlying problem. OmitStackTraceInFastThrow is enabled by default so possible for C2 to compile this to throw a pre-allocated exception with no message detail or stack trace. This is what is happening with these -Xcomp runs so the detail message is null rather than the expected message. An explicit test with Objects.requireNonNull would likely dodge this but does seem fragile. - PR Comment: https://git.openjdk.org/jdk/pull/25655#issuecomment-2943390680
Re: RFR: 8358426: Improve lazy computation in Locale [v2]
> Please review this PR which improves occurrences of lazy computation in > `Locale` and `BaseLocale`. > > Existing lazy initialization strategies such as CHM, static nested class, and > local inner class are replaced with Stable Values. > > Lambda usage is intentionally avoided in this change during `Locale` creation > and in static fields due to potential startup performance degradation as > noted by [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932). > > Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166 resource > is represented as a SV. Also, I did not think it was necessary to maintain a > SV for _both_ the array and set of ISO3166-1 alpha-2 codes. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: review - Moving all ISO resources to LocaleISOData & blessed modifier order for languageTag - Changes: - all: https://git.openjdk.org/jdk/pull/25646/files - new: https://git.openjdk.org/jdk/pull/25646/files/b679594c..e6f3d0de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25646&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25646&range=00-01 Stats: 155 lines in 2 files changed: 78 ins; 63 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/25646.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25646/head:pull/25646 PR: https://git.openjdk.org/jdk/pull/25646
Re: RFR: 8358520: Improve lazy computation in BreakIteratorResourceBundle and related classes
On Tue, 3 Jun 2025 20:14:31 GMT, Per Minborg wrote: > This PR proposes to simplify lazy computation related to resource bundles. > Previously, some objects were computed lazily using a double-checked locking > algorithm. StableValues offers a more robust and succinct solution. > > > This PR passes tier1, tier2, and tier3 on multiple platforms. Should we consider converting `lookup` in ParallelListResourceBundle.java to be a stable supplier of the CHM as well? Then separating the creation of the map from the insertion of parallel data. - PR Comment: https://git.openjdk.org/jdk/pull/25630#issuecomment-2946104521
Re: RFR: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
On Thu, 5 Jun 2025 07:47:17 GMT, Viktor Klang wrote: > It's too fragile to depend on generated NPE messages Dropping checking the message detail okay for now to remove the noise. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25655#pullrequestreview-282391
Integrated: 8358633 : Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491
On Thu, 5 Jun 2025 07:47:17 GMT, Viktor Klang wrote: > It's too fragile to depend on generated NPE messages This pull request has now been integrated. Changeset: 782bbca4 Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/782bbca439cd0d6db9366b4bd8d4861b8f780203 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8358633: Test ThreadPoolExecutorTest::testTimedInvokeAnyNullTimeUnit is broken by JDK-8347491 Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/25655
Re: RFR: 8358426: Improve lazy computation in Locale
On Wed, 4 Jun 2025 21:20:46 GMT, Justin Lu wrote:
> Please review this PR which improves occurrences of lazy computation in
> `Locale` and `BaseLocale`.
>
> Existing lazy initialization strategies such as CHM, static nested class, and
> local inner class are replaced with Stable Values.
>
> Lambda usage is intentionally avoided in this change during `Locale` creation
> and in static fields due to potential startup performance degradation as
> noted by [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932).
>
> Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166 resource
> is represented as a SV. Also, I did not think it was necessary to maintain a
> SV for _both_ the array and set of ISO3166-1 alpha-2 codes.
src/java.base/share/classes/java/util/Locale.java line 1269:
> 1267: public static String[] getISOCountries() {
> 1268: String[] countries = ISO_3166_1_ALPHA2.get();
> 1269: return Arrays.copyOf(countries, countries.length);
what about `return ISO_3166_1_ALPHA2.get().clone();`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2128662123
Re: RFR: 8358426: Improve lazy computation in Locale
On Wed, 4 Jun 2025 21:20:46 GMT, Justin Lu wrote: > Please review this PR which improves occurrences of lazy computation in > `Locale` and `BaseLocale`. > > Existing lazy initialization strategies such as CHM, static nested class, and > local inner class are replaced with Stable Values. > > Lambda usage is intentionally avoided in this change during `Locale` creation > and in static fields due to potential startup performance degradation as > noted by [JDK-8331932](https://bugs.openjdk.org/browse/JDK-8331932). > > Rather than convert `iso3166CodesMap` to a Stable Map, each ISO 3166 resource > is represented as a SV. Also, I did not think it was necessary to maintain a > SV for _both_ the array and set of ISO3166-1 alpha-2 codes. src/java.base/share/classes/java/util/Locale.java line 2578: > 2576: return Set.of(LocaleISOData.ISO3166_3); > 2577: } > 2578: }); What about moving these four stable suppliers and `getISO2Table` to LocaleISOData to shrink size of Locale.java? - PR Review Comment: https://git.openjdk.org/jdk/pull/25646#discussion_r2128672698
Re: RFR: 8357821: Revert incorrectly named JavaLangAccess::unchecked* methods [v2]
On Thu, 5 Jun 2025 06:04:49 GMT, Volkan Yazici wrote: >> Reverts certain [JDK-8353197](https://bugs.openjdk.org/browse/JDK-8353197) >> (which implements JavaDoc improvement and precautionary naming for certain >> unsafe methods in `jdk.internal.access.JavaLangAccess`) changes that are >> found to be incorrect. See the JBS issue for details on the followed >> evaluation process. > > Volkan Yazici has updated the pull request incrementally with one additional > commit since the last revision: > > Add `@implSpec` LGTM. Thank you for these updates! - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25545#pullrequestreview-2899838166
Re: RFR: 8358626: Emit UTF-8 CLDR resources
On Wed, 4 Jun 2025 21:54:15 GMT, Naoto Sato wrote: > Changes to generate CLDR resource bundles in UTF-8 encoding. The resource > files in `java.base` are supposed to be US English only, but they also need > to use UTF-8 as some of the names are non-ASCII (e.g., Türkiye) Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25648#pullrequestreview-2900205154
