Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently [v2]
> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
> TEST=gc/TestAllocHumongousFragment.java
> error info:
>
> Caused by: java.lang.NullPointerException: Cannot invoke
> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
> at
> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
> ... 24 more
>
> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
> (LocaleObjectCache uses SoftReferences, used by printf method called in
> getRandomInstance(Utils.java:511)).
>
> Maybe we have to deal with the case where the getBaseLocale() return value is
> null. the call stack is:
>
> at
> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
> at
> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>
> in LocaleObjectCache.java:64
>
>62 if (key == null || newVal == null) {
>
>63 // subclass must return non-null key/value object
>
>64 return null; // run here
>65 }
SUN Guoyun has updated the pull request incrementally with one additional
commit since the last revision:
8289220: [Shenandoah] TestAllocObjectArrays fails intermittently
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14211/files
- new: https://git.openjdk.org/jdk/pull/14211/files/31137f12..51883706
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14211&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14211&range=00-01
Stats: 20 lines in 3 files changed: 14 ins; 4 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14211.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14211/head:pull/14211
PR: https://git.openjdk.org/jdk/pull/14211
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf wrote: >> Please review these test changes which implement automatic testing of >> container resource updates without JVM restart. Note that this merely tests >> container detection code handling this case. It doesn't do anything special >> for the JVM itself, though it might make sense to add some sanity checks >> should we detect certain limits changing. In another PR, though. >> >> As to the test design, it works similar to the shared temp tests: Interact >> between the two containers by virtue of a shared filesystem `/tmp` and >> creating marker files there in order to make them cooperate. Note that the >> new test needs `podman` version `4.3.0` and better (`4.5` is current). >> >> Testing: >> - [x] GHA >> - [x] Linux x86_64 container tests on cg v1 and cg v2 system >> - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and >> `docker`) > > Severin Gehwolf 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 branch 'master' into jdk-8308090-tests-container-on-fly-updates > - Fix whitespace > - 8308090: Add container tests for on-the-fly resource quota updates These tests didn't fail but I couldn't properly validate the output as we don't seem to save the process stdout file. But I guess these are okay. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14090#pullrequestreview-1454663420
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Tue, 30 May 2023 13:03:27 GMT, Aleksandar Pejović wrote: > The current code for cgroup support in the JDK has large and expensive > dependencies: it uses NIO, streams, and regular expressions. This leads to > unnecessary class loading and slows down startup, especially when the code is > executed early during an application startup. This is especially a problem > for GraalVM, which executes this code during VM startup. > > This PR reduces the dependencies: > - NIO is replaced with regular `java.io` for file access. > - Streams are replaced with loops (a side effect of this is that files are > read in full whereas previously they could be read up to a certain point, > e.g., until a match is found). > - Regular expressions are replaced with manual tokenization (and for usages > of `String.split`, the "regex" is changed to single characters for which > `String.split` has a fast-path implementation that avoids the regular > expression engine). This seems a real backward step. I think some finer grain analysis is needed to work through specific issues, e.g. maybe startup with the regex usage and report back on how much that helps. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1571420444
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
On Fri, 26 May 2023 19:54:07 GMT, Rudi Horn wrote: >> This change prevents the contents of the internal string array from being >> copied back when releasing it. > > Rudi Horn has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains one commit: > > Use JNI_ABORT to release string bytes Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14117#pullrequestreview-1454646625
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
On Wed, 31 May 2023 11:49:00 GMT, Rudi Horn wrote: > I have not checked other uses, but they are probably not as critical yet > given the special status and frequent use of strings. I can possibly revisit > this issue in the future in a further issue / PR if I find time at some stage. Okay, we should create an issue in JBS to follow-up on that, as there may be other opportunities to avoid the copy back. - PR Comment: https://git.openjdk.org/jdk/pull/14117#issuecomment-1571415559
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Wed, 31 May 2023 13:49:46 GMT, Aleksandar Pejović wrote: >>> I guess I'll have to wait for OCA verification. >> >> Yes. >> >>> One failure is due to a lack of reviewers, so would you be able to do a >>> review? >> >> Yes, I'll try to do a review later today or tomorrow. >> >> Thanks! > >> Yes, I'll try to do a review later today or tomorrow. > > Awesome, thanks! @pejovica what testing have you done in relation to these changes? We run our container tests in tier5 - have you tested that? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1571265772
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
On Wed, 31 May 2023 12:07:49 GMT, Severin Gehwolf wrote: >> Severin Gehwolf 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 branch 'master' into jdk-8308090-tests-container-on-fly-updates >> - Fix whitespace >> - 8308090: Add container tests for on-the-fly resource quota updates > > Anyone willing to review this? @jerboaa I can't really review the tests themselves but will run through our CI to see if they cause any problems. If not then they should be okay to add. - PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1571223360
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
On Fri, 26 May 2023 19:54:07 GMT, Rudi Horn wrote: >> This change prevents the contents of the internal string array from being >> copied back when releasing it. > > Rudi Horn has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains one commit: > > Use JNI_ABORT to release string bytes Looks good for core-libs. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14117#pullrequestreview-1454378418
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 23:41:25 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 379:
>
>> 377: * type {@code float} is converted to {@code double}, and each integral
>> type smaller than {@code int} is converted to
>> 378: * {@code int}. As such, the native linker will reject attempts to link
>> function descriptors with certain variadic argument
>> 379: * layouts. Namely, {@linkplain ValueLayout value layouts} that have a
>> carrier type of {@code boolean}, {@code byte},
>
> Is there any reason as to why you decided to say which layouts are **not**
> allowed as variadic layouts? I'm wondering whether, if we add more carriers
> in the future, this list will probably need to be updated? Or do the
> restriction only involve these "small" types, and stuff like `long double` is
> always allowed? (in which case the set of unsupported layouts would remain
> stable over time)
No real reason for the current polarity. The specification explicitly calls out
the float -> double conversion, and then for the integral types it refers to
['integer
promotions'](https://en.cppreference.com/w/c/language/conversion#Integer_promotions),
which is a somewhat complex rule set for assigning ranks to integer types.
So, I think if we add more carriers in the future, we would have to worry about
integral types that have a rank less than (unsigned) int when considering this
list. This is currently only the types I've listed (though, `char` is a bit of
a special case), and this seems relatively stable to me. e.g. `long double`
would not need to be added here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212432229
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 23:36:33 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/Linker.java line 368:
>>
>>> 366: * Variadic functions
>>> 367: *
>>> 368: * Variadic functions (e.g. a C function declared with a trailing
>>> ellipses {@code ...} at the end of the formal parameter
>>
>> Optional suggestion for improvement (maybe now, or maybe later). When
>> reading this great para, I understand that there are two things that fall
>> under the "variadic function" umbrella. Some are declared with `...` and
>> some with `()`. This is a very good definition and I wonder if we should
>> expand a bit more on it - e.g. in a way, we never explain what a variadic
>> function is - we merely define it by saying how it is declared in C. I
>> wonder if we might very very briefly explain that in C, some functions
>> (variadic functions) can take a variable number of parameters. Then we go on
>> to say that these functions can be declared in two ways (e.g. ellipsis and
>> prototype-less). Perhaps if we used a bullet-list to define the two ways in
>> which variadic function can be declared, the definition could stand out even
>> more?
>
> Also, should we say somewhere that, for prototype-less functions,
> `firstVariadicArg` should always have an index of 0 (e.g. any other value is
> illegal, trivially). It's not super important, just one of those little
> things that can reinforce understanding.
> Perhaps if we used a bullet-list to define the two ways in which variadic
> function can be declared, the definition could stand out even more?
Yes, I think this is a good idea. I'll do another pass.
> Also, should we say somewhere that, for prototype-less functions,
> firstVariadicArg should always have an index of 0
I think this is also a good idea. It help hammer the point home.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212426680
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 22:44:52 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
src/java.base/share/classes/java/lang/foreign/Linker.java line 379:
> 377: * type {@code float} is converted to {@code double}, and each integral
> type smaller than {@code int} is converted to
> 378: * {@code int}. As such, the native linker will reject attempts to link
> function descriptors with certain variadic argument
> 379: * layouts. Namely, {@linkplain ValueLayout value layouts} that have a
> carrier type of {@code boolean}, {@code byte},
Is there any reason as to why you decided to say which layouts are **not**
allowed as variadic layouts? I'm wondering whether, if we add more carriers in
the future, this list will probably need to be updated? Or do the restriction
only involve these "small" types, and stuff like `long double` is always
allowed? (in which case the set of unsupported layouts would remain stable over
time)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212419388
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 23:35:08 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 368:
>
>> 366: * Variadic functions
>> 367: *
>> 368: * Variadic functions (e.g. a C function declared with a trailing
>> ellipses {@code ...} at the end of the formal parameter
>
> Optional suggestion for improvement (maybe now, or maybe later). When reading
> this great para, I understand that there are two things that fall under the
> "variadic function" umbrella. Some are declared with `...` and some with
> `()`. This is a very good definition and I wonder if we should expand a bit
> more on it - e.g. in a way, we never explain what a variadic function is - we
> merely define it by saying how it is declared in C. I wonder if we might very
> very briefly explain that in C, some functions (variadic functions) can take
> a variable number of parameters. Then we go on to say that these functions
> can be declared in two ways (e.g. ellipsis and prototype-less). Perhaps if we
> used a bullet-list to define the two ways in which variadic function can be
> declared, the definition could stand out even more?
Also, should we say somewhere that, for prototype-less functions,
`firstVariadicArg` should always have an index of 0 (e.g. any other value is
illegal, trivially). It's not super important, just one of those little things
that can reinforce understanding.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212416524
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 22:44:52 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
src/java.base/share/classes/java/lang/foreign/Linker.java line 368:
> 366: * Variadic functions
> 367: *
> 368: * Variadic functions (e.g. a C function declared with a trailing
> ellipses {@code ...} at the end of the formal parameter
Optional suggestion for improvement (maybe now, or maybe later). When reading
this great para, I understand that there are two things that fall under the
"variadic function" umbrella. Some are declared with `...` and some with `()`.
This is a very good definition and I wonder if we should expand a bit more on
it - e.g. in a way, we never explain what a variadic function is - we merely
define it by saying how it is declared in C. I wonder if we might very very
briefly explain that in C, some functions (variadic functions) can take a
variable number of parameters. Then we go on to say that these functions can be
declared in two ways (e.g. ellipsis and prototype-less). Perhaps if we used a
bullet-list to define the two ways in which variadic function can be declared,
the definition could stand out even more?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212415909
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
On Fri, 26 May 2023 19:54:07 GMT, Rudi Horn wrote: >> This change prevents the contents of the internal string array from being >> copied back when releasing it. > > Rudi Horn has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains one commit: > > Use JNI_ABORT to release string bytes This change looks good to me. Please await a core-libs review before integration though. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14117#pullrequestreview-1454299683
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 15:37:57 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 415:
>
>> 413: * It should be noted that values passed as variadic arguments undergo
>> default argument promotion in C. Each value of
>> 414: * type {@code float} is converted to {@code double}, and each integral
>> type smaller than {@code int} is converted to
>> 415: * {@code int}. As such, the native linker will reject attempts to link
>> a function with variadic parameters which have
>
> maybe "will reject attempts to link a variadic function if one or more
> parameter layouts in the provided function descriptor which correspond to a
> variadic argument is a value layout such that ..."
I did another pass over this. I went back to the first paragraph and defined
the term "variadic argument layout" which I then use when explaining which
layouts can not be used.
I've also moved the note to be directly after the first paragraph so that it
appears closer to where the term is defined.
Hopefully there should be no gaps now.
P.S. I had to re-flow some of the lines in the doc, so they might appear
changed in the diff.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212384221
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and > `float` is promoted to `double`, when being passed as variadic argument (see > e.g. > https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). > This patch restricts the layouts that can be used as variadic layouts to > what is allowed by the C specification. > > The fallback linker is also updated to use to correct function to link > variadic calls (not doing this turned out not to be a problem so far, but it > is problematic for instance on Mac/AArch64 when using the fallback linker). > Adding the restriction on layouts for all linkers is also partly motivated by > the fallback linker rejecting such unsupported variadic layouts already. > > I've added a small paragraph to the Linker javadoc as well that explains the > restriction. Comments on that are welcome, but please explain. > > The tests are updated to no longer try to link variadic functions with the > illegal layouts, and I've added some more negative tests to TestIllegalLink. > > Testing: > - local testing on Windows/x64 > - tier1-3 + jdk-tier5 (ongoing) > - manual test run on mac/aarch64 with the fallback linker to verify the > correctness of the fallback linker changes. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jdk/pull/14225/files - new: https://git.openjdk.org/jdk/pull/14225/files/c21ae49a..9415e2a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14225&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14225&range=00-01 Stats: 25 lines in 2 files changed: 8 ins; 9 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/14225.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14225/head:pull/14225 PR: https://git.openjdk.org/jdk/pull/14225
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters
On Wed, 31 May 2023 15:39:54 GMT, Maurizio Cimadamore
wrote:
>> test/jdk/java/foreign/StdLibTest.java line 386:
>>
>>> 384: return arena.allocateUtf8String("str");
>>> 385: }, "str"),
>>> 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'),
>>> // promote to int, per C spec
>>
>> is it important to retain this, given that there's already an INT case?
>
> Maybe adding a long case would be more useful?
Yes, good idea.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212373879
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Wed, 31 May 2023 12:21:13 GMT, Aleksandar Pejović wrote: > I guess I'll have to wait for OCA verification. Yes. > One failure is due to a lack of reviewers, so would you be able to do a > review? Yes, I'll try to do a review later today or tomorrow. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1570181616
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Wed, 31 May 2023 09:38:08 GMT, Aleksandar Pejović wrote: >> @pejovica Please enable GHA testing on your fork. Once enabled, please merge >> latest master into your branch so as to trigger a GHA run. Thanks! > > @jerboaa I enabled GḪA testing. Other than a couple of Windows errors (which > seem unrelated), everything else seems to be fine. > @pejovica There are some jcheck failures. See: > https://github.com/openjdk/jdk/pull/14216/checks?check_run_id=13870116111 @jerboaa One failure is due to a lack of reviewers, so would you be able to do a review? As for the rest, I've added an issue reference, so that's fixed, and I guess I'll have to wait for OCA verification. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1570124898
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Tue, 30 May 2023 13:03:27 GMT, Aleksandar Pejović wrote: > The current code for cgroup support in the JDK has large and expensive > dependencies: it uses NIO, streams, and regular expressions. This leads to > unnecessary class loading and slows down startup, especially when the code is > executed early during an application startup. This is especially a problem > for GraalVM, which executes this code during VM startup. > > This PR reduces the dependencies: > - NIO is replaced with regular `java.io` for file access. > - Streams are replaced with loops (a side effect of this is that files are > read in full whereas previously they could be read up to a certain point, > e.g., until a match is found). > - Regular expressions are replaced with manual tokenization (and for usages > of `String.split`, the "regex" is changed to single characters for which > `String.split` has a fast-path implementation that avoids the regular > expression engine). @pejovica Please enable GHA testing on your fork. Once enabled, please merge latest master into your branch so as to trigger a GHA run. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1568469816
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Wed, 31 May 2023 12:54:57 GMT, Severin Gehwolf wrote: > Yes, I'll try to do a review later today or tomorrow. Awesome, thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1570276516
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Wed, 31 May 2023 09:38:08 GMT, Aleksandar Pejović wrote: >> @pejovica Please enable GHA testing on your fork. Once enabled, please merge >> latest master into your branch so as to trigger a GHA run. Thanks! > > @jerboaa I enabled GḪA testing. Other than a couple of Windows errors (which > seem unrelated), everything else seems to be fine. @pejovica There are some jcheck failures. See: https://github.com/openjdk/jdk/pull/14216/checks?check_run_id=13870116111 - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1569886416
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Tue, 30 May 2023 13:48:49 GMT, Severin Gehwolf wrote: >> The current code for cgroup support in the JDK has large and expensive >> dependencies: it uses NIO, streams, and regular expressions. This leads to >> unnecessary class loading and slows down startup, especially when the code >> is executed early during an application startup. This is especially a >> problem for GraalVM, which executes this code during VM startup. >> >> This PR reduces the dependencies: >> - NIO is replaced with regular `java.io` for file access. >> - Streams are replaced with loops (a side effect of this is that files are >> read in full whereas previously they could be read up to a certain point, >> e.g., until a match is found). >> - Regular expressions are replaced with manual tokenization (and for usages >> of `String.split`, the "regex" is changed to single characters for which >> `String.split` has a fast-path implementation that avoids the regular >> expression engine). > > @pejovica Please enable GHA testing on your fork. Once enabled, please merge > latest master into your branch so as to trigger a GHA run. Thanks! @jerboaa I enabled GḪA testing. Other than a couple of Windows errors (which seem unrelated), everything else seems to be fine. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1569844601
RFR: 8309191: Reduce JDK dependencies of cgroup support
The current code for cgroup support in the JDK has large and expensive dependencies: it uses NIO, streams, and regular expressions. This leads to unnecessary class loading and slows down startup, especially when the code is executed early during an application startup. This is especially a problem for GraalVM, which executes this code during VM startup. This PR reduces the dependencies: - NIO is replaced with regular `java.io` for file access. - Streams are replaced with loops (a side effect of this is that files are read in full whereas previously they could be read up to a certain point, e.g., until a match is found). - Regular expressions are replaced with manual tokenization (and for usages of `String.split`, the "regex" is changed to single characters for which `String.split` has a fast-path implementation that avoids the regular expression engine). - Commit messages: - Merge branch 'master' into ap/cgroup-tweaks - Use simple loops to process cgroup files - Use java.io for reading cgroup files - Reimplement mountinfo parsing without using regex - Use simple patterns to parse cgroup files - Factor out logging from CgroupSubsystemFactory.create Changes: https://git.openjdk.org/jdk/pull/14216/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14216&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309191 Stats: 163 lines in 6 files changed: 73 ins; 55 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/14216.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14216/head:pull/14216 PR: https://git.openjdk.org/jdk/pull/14216
Re: RFR: 8303530: Redefine JAXP Configuration File [v15]
> Add a system property, jdk.xml.config.file, to return the path to a custom > JAXP configuration file. The current configuration file, jaxp.properties, > that the JDK supports will become the default configuration file. > > CSR: https://bugs.openjdk.org/browse/JDK-8303531 > > Tests: XML SQE and JCK tests passed. Joe Wang has updated the pull request incrementally with one additional commit since the last revision: adjust javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/12985/files - new: https://git.openjdk.org/jdk/pull/12985/files/277c49d0..115e62ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12985&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12985&range=13-14 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/12985.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12985/head:pull/12985 PR: https://git.openjdk.org/jdk/pull/12985
Re: RFR: 8303530: Redefine JAXP Configuration File [v14]
On Wed, 31 May 2023 21:19:42 GMT, Lance Andersen wrote:
>> Joe Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> additional javadoc update
>
> src/java.xml/share/classes/module-info.java line 153:
>
>> 151: * User-defined Configuration File
>> 152: * In addition to the {@code jaxp.properties} file, the system property
>> 153: * {@systemProperty java.xml.config.file} can be set on the command
>> line or at run-time
>
> I think we can simplify to "...the system property {@systemProperty
> java.xml.config.file} can be set to specify..."
Thanks Lance. Adjusted the javadoc accordingly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1212355544
Re: RFR: 8303530: Redefine JAXP Configuration File [v14]
On Wed, 31 May 2023 21:09:57 GMT, Joe Wang wrote:
>> Add a system property, jdk.xml.config.file, to return the path to a custom
>> JAXP configuration file. The current configuration file, jaxp.properties,
>> that the JDK supports will become the default configuration file.
>>
>> CSR: https://bugs.openjdk.org/browse/JDK-8303531
>>
>> Tests: XML SQE and JCK tests passed.
>
> Joe Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> additional javadoc update
Marked as reviewed by lancea (Reviewer).
src/java.xml/share/classes/module-info.java line 153:
> 151: * User-defined Configuration File
> 152: * In addition to the {@code jaxp.properties} file, the system property
> 153: * {@systemProperty java.xml.config.file} can be set on the command line
> or at run-time
I think we can simplify to "...the system property {@systemProperty
java.xml.config.file} can be set to specify..."
src/java.xml/share/classes/module-info.java line 182:
> 180: *
> 181: *
> 182: * With the APIs for factories or processors
I do not think "With" makes sense here. perhaps just remove it completely
src/java.xml/share/classes/module-info.java line 224:
> 222: *
> 223: *
> 224: * If the property is not set on the factory, or with its system
> property,
perhaps. "or using a system property"
-
PR Review: https://git.openjdk.org/jdk/pull/12985#pullrequestreview-1454179699
PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1212327827
PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1212323773
PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1212324452
Re: RFR: 8303530: Redefine JAXP Configuration File [v14]
> Add a system property, jdk.xml.config.file, to return the path to a custom > JAXP configuration file. The current configuration file, jaxp.properties, > that the JDK supports will become the default configuration file. > > CSR: https://bugs.openjdk.org/browse/JDK-8303531 > > Tests: XML SQE and JCK tests passed. Joe Wang has updated the pull request incrementally with one additional commit since the last revision: additional javadoc update - Changes: - all: https://git.openjdk.org/jdk/pull/12985/files - new: https://git.openjdk.org/jdk/pull/12985/files/98d9417b..277c49d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12985&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12985&range=12-13 Stats: 9 lines in 1 file changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/12985.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12985/head:pull/12985 PR: https://git.openjdk.org/jdk/pull/12985
Integrated: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
On Wed, 31 May 2023 20:34:07 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > with ZGC and Generational ZGC again This pull request has now been integrated. Changeset: e42a4b65 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/e42a4b659a78721567e4e882a26fe2972975bc80 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again Reviewed-by: bpb, azvegint - PR: https://git.openjdk.org/jdk/pull/14253
Re: Integrated: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
On Wed, 31 May 2023 20:38:23 GMT, Brian Burkhalter wrote: >> A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java >> with ZGC and Generational ZGC again > > Marked as reviewed by bpb (Reviewer). @bplb and @azvegint - Thanks for the fast reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14253#issuecomment-1570917645
Re: Integrated: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
On Wed, 31 May 2023 20:34:07 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > with ZGC and Generational ZGC again Marked as reviewed by azvegint (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14253#pullrequestreview-1454120005
Re: Integrated: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
On Wed, 31 May 2023 20:34:07 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > with ZGC and Generational ZGC again Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14253#pullrequestreview-1454119784
Integrated: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again - Commit messages: - 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again Changes: https://git.openjdk.org/jdk/pull/14253/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14253&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309236 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14253.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14253/head:pull/14253 PR: https://git.openjdk.org/jdk/pull/14253
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v3]
On Wed, 31 May 2023 18:02:09 GMT, Jan Lahoda wrote: >> Joe Darcy has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains 24 commits: >> >> - Merge branch 'master' into JDK-8306584 >> - Merge branch 'master' into JDK-8306584 >> - Merge branch 'master' into JDK-8306584 >> - Sync in symbol changes for JDK 21 build 24. >> - Merge branch 'master' into JDK-8306584 >> - Merge branch 'master' into JDK-8306584 >> - Merge branch 'master' into JDK-8306584 >> - Minor test fixes. >> - Merge branch 'master' into JDK-8306584 >> - Update symbol files to JDK 21 b23. >> - ... and 14 more: https://git.openjdk.org/jdk/compare/cb40db05...376dbe26 > > src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassFile.java line > 127: > >> 125: V64(64, 0), // JDK 20 >> 126: V65(65, 0), // JDK 21 >> 127: V66(66, 0); // JDK 22 > > A very small nit/suggestion - it should be possible to do: > > V66(66, 0), //JDK 22 > ; > > > (i.e. ending the enum constant with `,`, and putting the semicolon on a new > line.) This way adding a new constant would mean just one line addition, no > modification. (The same could be done for other enums.) Updated. > test/langtools/tools/javac/versions/Versions.java line 93: > >> 91: TWENTY(false,"64.0", "20", Versions::checksrc20), >> 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc21), >> 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc21); > > Should there be `checksrc22` instead of `checksrc21`? Or is that done later? Good catch. I have a refactoring of the test planned which will eliminate the explicit "checksrc$N" methods. - PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212284124 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212283880
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v4]
> Time to get JDK 22 underway... Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Respond to review comments. - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Sync in symbol changes for JDK 21 build 24. - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Minor test fixes. - ... and 16 more: https://git.openjdk.org/jdk/compare/12649025...7b446793 - Changes: https://git.openjdk.org/jdk/pull/13567/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13567&range=03 Stats: 5843 lines in 69 files changed: 5789 ins; 0 del; 54 mod Patch: https://git.openjdk.org/jdk/pull/13567.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13567/head:pull/13567 PR: https://git.openjdk.org/jdk/pull/13567
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ I am Ok with the client-libs changes here. I did not look at anything else. So you'll need more approvals before its ready to push. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1454101589
Re: RFR: 8301569: list mode of jmod and jimage cannot be used normally in turkish locale
On Sun, 29 Jan 2023 23:54:21 GMT, Glavo wrote: >> When the default Locale is `tr`, the jmod and jimage commands have the >> following problems: >> >> * The jmod command does not correctly recognize the `list` mode typed in >> lowercase; >> * The jimage command cannot obtain the help information of the `list` mode. > > There are some similar hidden dangers in OpenJDK. I hope to clean them up. > > These hidden dangers are scattered in many modules. Should I solve them in > multiple PRs? > Hello @Glavo, I see you reopened this PR, so I'm guessing you are still > interested in pursuing this further. Are you considering updating this PR to > implement Alan's suggestion to do similar changes in `JImageTask.java` and > `JlinkTask.java` to use `Locale.ROOT` or update this proposed change to use > `Locale.ENGLISH`? @jaikiran Hi Jaikiran, as my previous reply, I do not agree that `Locale.ENGLISH` should be used here, I think `Locale.ROOT` is more reasonable. - PR Comment: https://git.openjdk.org/jdk/pull/12281#issuecomment-1570842199
Integrated: 8304914: Use OperatingSystem, Architecture, and Version in jpackage
On Fri, 21 Apr 2023 17:28:54 GMT, Roger Riggs wrote: > Refactor the Platform class in jdk.jpackage to use the internal > OperatingSystem, Architecture, and Version classes. > The OperatingSystem.isXXX() and Architecture.isYYY() methods replace > comparisons in the Platform class. > The checks of the os.version are replaced but may not be needed if OpenJDK no > longer supports them. > > It is recommended to remove os version checks that apply only to Mac versions > before 10.15. > Mac OS X 10.15 is the oldest version supported. This pull request has now been integrated. Changeset: c3cd481a Author:Roger Riggs URL: https://git.openjdk.org/jdk/commit/c3cd481a9a51a55649ae4ffb2b98cb9eee8b3bbb Stats: 236 lines in 23 files changed: 41 ins; 132 del; 63 mod 8304914: Use OperatingSystem, Architecture, and Version in jpackage Reviewed-by: asemenyuk - PR: https://git.openjdk.org/jdk/pull/13586
Re: RFR: JDK-8282797: CompileCommand parsing errors should exit VM [v3]
On Wed, 24 May 2023 11:05:01 GMT, Tobias Holenstein wrote: >> Currently, errors during compile command parsing just print an error but >> don't exit the VM. As a result, issues go unnoticed. >> >> With this PR the behavior is changed to exit the VM when an error occurs. >> >> E.g. `java -XX:CompileCommand=compileonly,HashMap:: -version` will exit the >> VM after a parsing occurred. >> >> CompileCommand: An error occurred during parsing >> Error: Could not parse method pattern >> Line: 'compileonly,HashMap::' >> >> Usage: '-XX:CompileCommand=,' - to set boolean >> option to true >> Usage: '-XX:CompileCommand=,,' >> Use: '-XX:CompileCommand=help' for more information and to list all option. >> >> Error: Could not create the Java Virtual Machine. >> Error: A fatal exception has occurred. Program will exit. >> >> >> ### Updated Tests >> Updated all tests to now expect an error code `1` for wrong `CompileCommand` > > Tobias Holenstein has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Scenario.java > - Update compilerOracle.cpp Update is good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13753#pullrequestreview-1453939326
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Wed, 31 May 2023 14:46:59 GMT, Aleksey Shipilev wrote: >> Two thoughts here. >> The random number source (SecureRandom) should have its own tests, UUID has >> a simple dependency on a generator. >> The test for UUID is that it composes the bits into the UUID correctly. The >> randomness of the generator should be factored out. >> >> Second, to raise concern about collisions, then the test should not throw on >> the first detected collision but complete the cycle and provide the simple >> stats on the number of collisions per COUNT (1,000,000). >> >> All those 5 second test runs add up. > >> Two thoughts here. The random number source (SecureRandom) should have its >> own tests, UUID has a simple dependency on a generator. The test for UUID is >> that it composes the bits into the UUID correctly. The randomness of the >> generator should be factored out. > > This test verifies the way UUID uses the SecureRandom. Think about this as > the less of a unit, and more of the integration test. This would be even more > important once things like https://bugs.openjdk.org/browse/JDK-8308804 show > up. > >> Second, to raise concern about collisions, then the test should not throw on >> the first detected collision but complete the cycle and provide the simple >> stats on the number of collisions per COUNT (1,000,000). > > All right, that we can do. See new commit. > >> All those 5 second test runs add up. > > Yes, and it would be sad to waste those 5 seconds on something irrelevant. I > would argue that `UUID.randomUUID` breakage would be very unfortunate for the > real world systems. Spending 5 seconds per test run on it is a good > investment here. We're into the area of diminishing returns. I'd expect the implementation bug mentioned above would be found by far fewer iterations. Thanks for the test change. - PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1212139442
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v3]
On Wed, 31 May 2023 14:53:09 GMT, Aleksey Shipilev wrote: >> UUID is very important class that is used to track identities of objects in >> large scale systems. Yet, the coverage in JDK test is disappointing: it >> tests only 100 of UUID instances per test, which is way too small to detect >> collisions due to the bad randomness for example. >> >> I have some pending work to improve UUID performance, and tests should be >> improved. >> >> The improved test still runs in less than 5 seconds on my laptop. > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Collect collisions and report them once at the end Looks good, thanks for the updates. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14134#pullrequestreview-1453903860
Re: RFR: 8301569: list mode of jmod and jimage cannot be used normally in turkish locale
On Sun, 29 Jan 2023 15:37:28 GMT, Glavo wrote: > When the default Locale is `tr`, the jmod and jimage commands have the > following problems: > > * The jmod command does not correctly recognize the `list` mode typed in > lowercase; > * The jimage command cannot obtain the help information of the `list` mode. `jdk.jlink` uses `Locale.ROOT` in most calls. I suggest to change the existing references of `Locale.ENGLISH` to use `Locale.ROOT` (one in JImageTask and one in JlinkTask). Should consider also change `VersionPropsPlugin` although not strictly necessary. - PR Comment: https://git.openjdk.org/jdk/pull/12281#issuecomment-1570709944
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v4]
> The internal enum jdk.internal.util.Architecture does not provide information > about the big or little endianness or the address size (64 or 32 bits). The > endian-ness and address size are intrinsic to the architecture. > > The values of the enum are extended to separately identify the big endian and > little-endian uses of the ISA. > For example, `PPC64` and `PPC64LE` for the big and little-endian versions. > The enum values directly reflect the build-time artifacts and resulting > executables. > > This information about an architecture will make the enum more useful > especially to identify a target platform in a cross-platform use case. A > method is added to map well known aliases for the platforms to the > Architecture enum. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove duplicate import - Changes: - all: https://git.openjdk.org/jdk/pull/14063/files - new: https://git.openjdk.org/jdk/pull/14063/files/a6abcdfe..aafcc7de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14063&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14063&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14063.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14063/head:pull/14063 PR: https://git.openjdk.org/jdk/pull/14063
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v3]
On Tue, 30 May 2023 22:16:08 GMT, Joe Darcy wrote: >> Time to get JDK 22 underway... > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 24 commits: > > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Sync in symbol changes for JDK 21 build 24. > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Minor test fixes. > - Merge branch 'master' into JDK-8306584 > - Update symbol files to JDK 21 b23. > - ... and 14 more: https://git.openjdk.org/jdk/compare/cb40db05...376dbe26 javac and symbol related changes seem reasonable to me. Two minor comments added for consideration. src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassFile.java line 127: > 125: V64(64, 0), // JDK 20 > 126: V65(65, 0), // JDK 21 > 127: V66(66, 0); // JDK 22 A very small nit/suggestion - it should be possible to do: V66(66, 0), //JDK 22 ; (i.e. ending the enum constant with `,`, and putting the semicolon on a new line.) This way adding a new constant would mean just one line addition, no modification. (The same could be done for other enums.) test/langtools/tools/javac/versions/Versions.java line 93: > 91: TWENTY(false,"64.0", "20", Versions::checksrc20), > 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc21), > 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc21); Should there be `checksrc22` instead of `checksrc21`? Or is that done later? - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13567#pullrequestreview-1453854784 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212101629 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1212114188
Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v6]
On Wed, 31 May 2023 14:32:25 GMT, Roger Riggs wrote: >> Refactor the Platform class in jdk.jpackage to use the internal >> OperatingSystem, Architecture, and Version classes. >> The OperatingSystem.isXXX() and Architecture.isYYY() methods replace >> comparisons in the Platform class. >> The checks of the os.version are replaced but may not be needed if OpenJDK >> no longer supports them. >> >> It is recommended to remove os version checks that apply only to Mac >> versions before 10.15. >> Mac OS X 10.15 is the oldest version supported. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing import of OSVersion Marked as reviewed by asemenyuk (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13586#pullrequestreview-1453868736
Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
On Wed, 31 May 2023 16:40:54 GMT, Daniel D. Daugherty wrote: > A couple of trivial ProblemListings: > [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList > jdk/incubator/vector/Float64VectorTests.java on aarch64 > [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java This pull request has now been integrated. Changeset: 45473ef2 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/45473ef23520271954fa7196a5be588f88337aaf Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64 8309231: ProblemList vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java Reviewed-by: darcy - PR: https://git.openjdk.org/jdk/pull/14250
Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
A couple of trivial ProblemListings: [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64 [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java - Commit messages: - 8309231: ProblemList vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java - 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64 Changes: https://git.openjdk.org/jdk/pull/14250/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14250&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309230 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14250.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14250/head:pull/14250 PR: https://git.openjdk.org/jdk/pull/14250
Re: Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
On Wed, 31 May 2023 16:40:54 GMT, Daniel D. Daugherty wrote: > A couple of trivial ProblemListings: > [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList > jdk/incubator/vector/Float64VectorTests.java on aarch64 > [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14250#pullrequestreview-1453713285
Re: Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
On Wed, 31 May 2023 16:48:06 GMT, Joe Darcy wrote: >> A couple of trivial ProblemListings: >> [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList >> jdk/incubator/vector/Float64VectorTests.java on aarch64 >> [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList >> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java > > Marked as reviewed by darcy (Reviewer). @jddarcy - Thanks for the fast review! - PR Comment: https://git.openjdk.org/jdk/pull/14250#issuecomment-1570578322
Integrated: 8299505: findVirtual on array classes incorrectly restricts the receiver type
On Sat, 6 May 2023 18:15:56 GMT, Chen Liang wrote: > The access hack for array class clone is only applied to `checkAccess` but > missing before call to `restrictProtectedReceiver`, causing the array > receiver type to be incorrectly replaced by the lookupClass type. This patch > fixes that and adds a test to ensure an original lookup resolves `clone` for > both array classes (public) and Object (inherited protected) correctly, and > restores the old MethodHandlesGeneralTest from > [JDK-8001105](https://bugs.openjdk.org/browse/JDK-8001105) which ensures > correctness for publicLookup (which is already correct). This pull request has now been integrated. Changeset: 78aa5f3f Author:Chen Liang Committer: Mandy Chung URL: https://git.openjdk.org/jdk/commit/78aa5f3fc1c7fc7929e0d2b5d94da0827483b7c5 Stats: 104 lines in 3 files changed: 88 ins; 9 del; 7 mod 8299505: findVirtual on array classes incorrectly restricts the receiver type Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/13855
Integrated: 8308022: update for deprecated sprintf for java.base
On Fri, 12 May 2023 17:57:43 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this update reviewed? > > The sprintf is deprecated in Xcode 14, and Microsoft Virtual Studio, because > of security concerns. The issue was addressed in > [JDK-8296812](https://bugs.openjdk.org/browse/JDK-8296812) for building > failure, and > [JDK-8299378](https://bugs.openjdk.org/browse/JDK-8299378)/[JDK-8299635](https://bugs.openjdk.org/browse/JDK-8299635)/[JDK-8301132](https://bugs.openjdk.org/browse/JDK-8301132) > for testing issues . This is a break-down update for sprintf uses in the > java.base module. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 42ca6e69 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.org/jdk/commit/42ca6e69420e090cdec16f3bd1e5c70506511663 Stats: 37 lines in 6 files changed: 5 ins; 0 del; 32 mod 8308022: update for deprecated sprintf for java.base Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/13963
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently
On Wed, 31 May 2023 10:00:18 GMT, SUN Guoyun wrote: > Jtreg tier1 can trigger the same error with vmoptions:"-Xcomp > -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive > -XX:+ShenandoahOOMDuringEvacALot I found the GC occurs between when the soft > reference is assigned and when it is used. Yes, this seems to be a bug here so I think move the issue to core-libs/java.util:i18n as it looks like the caching done by BaseLocale needs to be re-examined. - PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1570534648
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v3]
On Wed, 31 May 2023 14:08:09 GMT, Roger Riggs wrote: >> The internal enum jdk.internal.util.Architecture does not provide >> information about the big or little endianness or the address size (64 or 32 >> bits). The endian-ness and address size are intrinsic to the architecture. >> >> The values of the enum are extended to separately identify the big endian >> and little-endian uses of the ISA. >> For example, `PPC64` and `PPC64LE` for the big and little-endian versions. >> The enum values directly reflect the build-time artifacts and resulting >> executables. >> >> This information about an architecture will make the enum more useful >> especially to identify a target platform in a cross-platform use case. A >> method is added to map well known aliases for the platforms to the >> Architecture enum. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Don't map ppc64le to ppc64 when processing the PlatformProps.java.template > - Merge branch 'master' into 8308452-cross-platform-arch > - Correct handling of ppc64le/ppc64 using non-canonical architecture. >Keep canonical architecture names for s390, amd/x86_64. > - Merge branch 'master' into 8308452-cross-platform-arch > - Merge branch 'master' into 8308452-cross-platform-arch > - Review suggestions > - Correct address size for ARM (32-bit) > - 8308452: Extend internal Architecture enum with byte order and address size src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 29: > 27: import jdk.internal.vm.annotation.ForceInline; > 28: > 29: import java.util.Locale; Nit: Stray import? - PR Review Comment: https://git.openjdk.org/jdk/pull/14063#discussion_r1211959219
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 12:38:14 GMT, Johannes Kuhn wrote: >>> Then we still need to obtain the implemented interface and original method >>> handle information every time they are queried. Having these information >>> (or the method handle providing access) computed early is more convenient. >> >> I was thinking the wrapper instance class still implements some private >> methods to get the implemented interface and original method handle which >> can be accessed only `java.base` via deep reflection. > >> It should not trigger `checkPackageAccess` if it does not implement the >> interface AFAICT. > > `checkPackageAccess` is also called by the application class loader: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java#L174-L189 > > It should make it impossible to reference for example `sun.misc.Unsafe`. I'm referring to the private `ClassLoader.checkPackageAccess(Class cls, ProtectionDomain pd)` method that is only invoked by the VM. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1211956537
Integrated: 8308316: Default decomposition mode in Collator
On Thu, 18 May 2023 18:02:32 GMT, Naoto Sato wrote: > Amending the description about the default decomposition mode in > `Collator.NO_DECOMPOSITION` javadoc. This pull request has now been integrated. Changeset: 12649025 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/126490251721d131098a0bc2def8fd02f97cd5af Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod 8308316: Default decomposition mode in Collator Reviewed-by: rriggs - PR: https://git.openjdk.org/jdk/pull/14049
Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v6]
On Wed, 31 May 2023 14:32:25 GMT, Roger Riggs wrote: >> Refactor the Platform class in jdk.jpackage to use the internal >> OperatingSystem, Architecture, and Version classes. >> The OperatingSystem.isXXX() and Architecture.isYYY() methods replace >> comparisons in the Platform class. >> The checks of the os.version are replaced but may not be needed if OpenJDK >> no longer supports them. >> >> It is recommended to remove os version checks that apply only to Mac >> versions before 10.15. >> Mac OS X 10.15 is the oldest version supported. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing import of OSVersion Merged with current head and retested. Any other concerns? - PR Comment: https://git.openjdk.org/jdk/pull/13586#issuecomment-1570484204
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters
On Tue, 30 May 2023 17:25:35 GMT, Jorn Vernee wrote:
> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
> `float` is promoted to `double`, when being passed as variadic argument (see
> e.g.
> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
> This patch restricts the layouts that can be used as variadic layouts to
> what is allowed by the C specification.
>
> The fallback linker is also updated to use to correct function to link
> variadic calls (not doing this turned out not to be a problem so far, but it
> is problematic for instance on Mac/AArch64 when using the fallback linker).
> Adding the restriction on layouts for all linkers is also partly motivated by
> the fallback linker rejecting such unsupported variadic layouts already.
>
> I've added a small paragraph to the Linker javadoc as well that explains the
> restriction. Comments on that are welcome, but please explain.
>
> The tests are updated to no longer try to link variadic functions with the
> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>
> Testing:
> - local testing on Windows/x64
> - tier1-3 + jdk-tier5 (ongoing)
> - manual test run on mac/aarch64 with the fallback linker to verify the
> correctness of the fallback linker changes.
Looks good
src/java.base/share/classes/java/lang/foreign/Linker.java line 415:
> 413: * It should be noted that values passed as variadic arguments undergo
> default argument promotion in C. Each value of
> 414: * type {@code float} is converted to {@code double}, and each integral
> type smaller than {@code int} is converted to
> 415: * {@code int}. As such, the native linker will reject attempts to link
> a function with variadic parameters which have
maybe "will reject attempts to link a variadic function if one or more
parameter layouts in the provided function descriptor which correspond to a
variadic argument is a value layout such that ..."
test/jdk/java/foreign/StdLibTest.java line 386:
> 384: return arena.allocateUtf8String("str");
> 385: }, "str"),
> 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'), //
> promote to int, per C spec
is it important to retain this, given that there's already an INT case?
-
Marked as reviewed by mcimadamore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14225#pullrequestreview-1453564448
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211924259
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211927081
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters
On Wed, 31 May 2023 15:39:31 GMT, Maurizio Cimadamore
wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> test/jdk/java/foreign/StdLibTest.java line 386:
>
>> 384: return arena.allocateUtf8String("str");
>> 385: }, "str"),
>> 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'), //
>> promote to int, per C spec
>
> is it important to retain this, given that there's already an INT case?
Maybe adding a long case would be more useful?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211927710
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v3]
> UUID is very important class that is used to track identities of objects in > large scale systems. Yet, the coverage in JDK test is disappointing: it tests > only 100 of UUID instances per test, which is way too small to detect > collisions due to the bad randomness for example. > > I have some pending work to improve UUID performance, and tests should be > improved. > > The improved test still runs in less than 5 seconds on my laptop. Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Collect collisions and report them once at the end - Changes: - all: https://git.openjdk.org/jdk/pull/14134/files - new: https://git.openjdk.org/jdk/pull/14134/files/48d6bc5c..40aa3118 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14134&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14134&range=01-02 Stats: 30 lines in 1 file changed: 27 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14134/head:pull/14134 PR: https://git.openjdk.org/jdk/pull/14134
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Wed, 31 May 2023 14:17:40 GMT, Roger Riggs wrote: > Two thoughts here. The random number source (SecureRandom) should have its > own tests, UUID has a simple dependency on a generator. The test for UUID is > that it composes the bits into the UUID correctly. The randomness of the > generator should be factored out. This test verifies the way UUID uses the SecureRandom. Think about this as the less of a unit, and more of the integration test. This would be even more important once things like https://bugs.openjdk.org/browse/JDK-8308804 show up. > Second, to raise concern about collisions, then the test should not throw on > the first detected collision but complete the cycle and provide the simple > stats on the number of collisions per COUNT (1,000,000). All right, that we can do. See new commit. > All those 5 second test runs add up. Yes, and it would be sad to waste those 5 seconds on something irrelevant. I would argue that `UUID.randomUUID` breakage would be very unfortunate for the real world systems. Spending 5 seconds per test run on it is a good investment here. - PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1211842661
Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v6]
> Refactor the Platform class in jdk.jpackage to use the internal > OperatingSystem, Architecture, and Version classes. > The OperatingSystem.isXXX() and Architecture.isYYY() methods replace > comparisons in the Platform class. > The checks of the os.version are replaced but may not be needed if OpenJDK no > longer supports them. > > It is recommended to remove os version checks that apply only to Mac versions > before 10.15. > Mac OS X 10.15 is the oldest version supported. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Add missing import of OSVersion - Changes: - all: https://git.openjdk.org/jdk/pull/13586/files - new: https://git.openjdk.org/jdk/pull/13586/files/ab6e20ce..14bf4776 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13586&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13586&range=04-05 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13586.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13586/head:pull/13586 PR: https://git.openjdk.org/jdk/pull/13586
Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v5]
> Refactor the Platform class in jdk.jpackage to use the internal > OperatingSystem, Architecture, and Version classes. > The OperatingSystem.isXXX() and Architecture.isYYY() methods replace > comparisons in the Platform class. > The checks of the os.version are replaced but may not be needed if OpenJDK no > longer supports them. > > It is recommended to remove os version checks that apply only to Mac versions > before 10.15. > Mac OS X 10.15 is the oldest version supported. Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 38 commits: - Update OperatingSystem.version() to OSVersion.current() - Merge branch 'master' into 8304914-os-jpackage - Merge branch 'master' into 8304914-os-jpackage - Revert "The OperatingSystem enum treats AIX separately from Linux." AIX is NOT an alias for Linux. This reverts commit 802456d52a3c85a2abd8830ea80147b212d2a5b0. - Merge branch 'master' into 8304914-os-jpackage - The OperatingSystem enum treats AIX separately from Linux. The original Platform used the same enum for both. Whereever OperatingSystem.isLinux is used in jpackage it should include isAix as well. - Minor source code style cleanup - Merge branch 'master' into 8304914-os-jpackage - Merge branch '8306678-os-version' into 8304914-os-jpackage - Merge branch '8304915-arch-enum' into 8306678-os-version - ... and 28 more: https://git.openjdk.org/jdk/compare/25b98030...ab6e20ce - Changes: https://git.openjdk.org/jdk/pull/13586/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13586&range=04 Stats: 235 lines in 23 files changed: 40 ins; 132 del; 63 mod Patch: https://git.openjdk.org/jdk/pull/13586.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13586/head:pull/13586 PR: https://git.openjdk.org/jdk/pull/13586
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Wed, 31 May 2023 14:05:20 GMT, Aleksey Shipilev wrote: >> My point was that its probably not practical to test (more than once). >> If it fails, it will be considered just as you propose and disregarded and >> in the meantime consumes test cycles in each of the test contexts. Either >> provide more information about the conditions under which it failed or >> remove it. > > Sorry, I have trouble following the argument here. > > Let me re-iterate: the probability for bona-fide collision is so vanishingly > low, the test failure here is a strong signal that something is wrong with > the implementation. We can put more guidance in the test comments there, like > "This is extremely unlikely to happen. If you see this failing, this highly > likely points to the implementation bug, rather than the odd chance." > > What I expect to happen when that test fails, is that it prompts the > investigation with multiple stress tests to get a better estimate of the > actual collision rate. Assuming we actually see a collision, it is likely to > be caused by much higher probability error somewhere in the code. In fact, if > this test is _actually noisy_ to the point it becomes a testing problem, this > already gives us the signal that actual collision rate is many orders of > magnitude higher than math predicts, and this becomes an even _stronger_ > signal that random UUIDs are seriously broken for practical use. Two thoughts here. The random number source (SecureRandom) should have its own tests, UUID has a simple dependency on a generator. The test for UUID is that it composes the bits into the UUID correctly. The randomness of the generator should be factored out. Second, to raise concern about collisions, then the test should not throw on the first detected collision but complete the cycle and provide the simple stats on the number of collisions per COUNT (1,000,000). All those 5 second test runs add up. - PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1211795871
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v3]
On Mon, 29 May 2023 07:25:26 GMT, Jan Lahoda wrote:
>> The pattern matching switches are using a bootstrap method
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch.
>> Basically, for a switch like:
>>
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>>
>>
>> this method will produce a MethodHandle that will be analyze the provided
>> selector value (`obj` in the example), and will return the case index to
>> which the switch should jump. This method also accepts a (re)start index for
>> the search, which is used to handle guards. For example, if the
>> `s.isEmpty()` guard in the above sample returns false, the matching is
>> restarted on the next case.
>>
>> The current implementation is fairly slow, it basically goes through the
>> labels in a loop. The proposal here is to replace that with a MethodHandle
>> structure like this:
>>
>> obj == null ? -1
>> : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>>
>>
>> This appear to run faster than the current implementation, using testcase
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these
>> are the results
>>
>> PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1515989.562
>> ± 32047.918 ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 2630707.585
>> ± 37202.210 ops/s
>>
>> PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 6789310.900
>> ± 61921.636 ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 10771729.464
>> ± 69607.467 ops/s
>>
>>
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one
>> proposed here. The translation in javac used is the one from #9746 in all
>> cases.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains six commits:
>
> - Reflecting review feedback.
> - Merge branch 'master' into JDK-8291966
> - Adding comments
> - Improving performance
> - Merge branch 'master' into JDK-8291966
> - 8291966: SwitchBootstrap.typeSwitch could be faster
This patch is intended to eliminate some consecutive unnecessary tests like in
case like:
switch (o) {
case Runnable r when ... -> {}
case Runnable r when ... -> {}
case Runnable r when ... -> {}
case Object o -> {}
}
If `o` is not a `Runnable`, the `instanceof` will only happen for the first
case, and the rest will be skipped, as these tests could not pass. But (as a
current limitation), if it is not a consecutive run, the duplicate `instanceof`
checks will still happen.
I am quite sure there are ways to improve the bootstrap further, but might be
better to have some (more) real-world examples to know what to optimize for.
-
PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1570306708
RFR: 8308031: Linkers should reject unpromoted variadic parameters
In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and `float` is promoted to `double`, when being passed as variadic argument (see e.g. https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). This patch restricts the layouts that can be used as variadic layouts to what is allowed by the C specification. The fallback linker is also updated to use to correct function to link variadic calls (not doing this turned out not to be a problem so far, but it is problematic for instance on Mac/AArch64 when using the fallback linker). Adding the restriction on layouts for all linkers is also partly motivated by the fallback linker rejecting such unsupported variadic layouts already. I've added a small paragraph to the Linker javadoc as well that explains the restriction. Comments on that are welcome, but please explain. The tests are updated to no longer try to link variadic functions with the illegal layouts, and I've added some more negative tests to TestIllegalLink. Testing: - local testing on Windows/x64 - tier1-3 + jdk-tier5 (ongoing) - manual test run on mac/aarch64 with the fallback linker to verify the correctness of the fallback linker changes. - Commit messages: - fix word order - adjust whitespace - simplify test changes - reject invalid variadic layouts - VA Fixes Changes: https://git.openjdk.org/jdk/pull/14225/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14225&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308031 Stats: 108 lines in 11 files changed: 95 ins; 3 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/14225.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14225/head:pull/14225 PR: https://git.openjdk.org/jdk/pull/14225
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Wed, 31 May 2023 13:11:08 GMT, Roger Riggs wrote: >> This is a non-practical concern, IMO. By spec, `UUID.randomUUID` is >> generated from the cryptographically secure random, with >120 bits of >> randomness, so the collision is extremely unlikely. Collision math involves >> birthday paradox, but Wikipedia article on UUIDs fortunately gives us the >> approximated solutions already: >> https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions >> >> Quote: "Thus, the probability to find a duplicate within 103 trillion >> version-4 UUIDs is one in a billion." >> >> In other words, finding a collision in this test with 1M UUIDs points to the >> implementation issue, not a test bug, with a very high probability. In yet >> another words, if a unit test with 1M UUIDs is able to find a collision, >> then this is a strong signal that many production systems that assume >> extremely low collision probability are up for subtle misbehavior. > > My point was that its probably not practical to test (more than once). > If it fails, it will be considered just as you propose and disregarded and in > the meantime consumes test cycles in each of the test contexts. Either > provide more information about the conditions under which it failed or remove > it. Sorry, I have trouble following the argument here. Let me re-iterate: the probability for bona-fide collision is so vanishingly low, the test failure here is a strong signal that something is wrong with the implementation. We can put more guidance in the test comments there, like "This is extremely unlikely to happen. If you see this failing, this highly likely points to the implementation bug, rather than the odd chance." What I expect to happen when that test fails, is that it prompts the investigation with multiple stress tests to get a better estimate of the actual collision rate. Assuming we actually see a collision, it is likely to be caused by much higher probability error somewhere in the code. In fact, if this test is _actually noisy_ to the point it becomes a testing problem, this already gives us the signal that actual collision rate is many orders of magnitude higher than math predicts, and this becomes an even _stronger_ signal that random UUIDs are seriously broken for practical use. - PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1211776452
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v3]
> The internal enum jdk.internal.util.Architecture does not provide information > about the big or little endianness or the address size (64 or 32 bits). The > endian-ness and address size are intrinsic to the architecture. > > The values of the enum are extended to separately identify the big endian and > little-endian uses of the ISA. > For example, `PPC64` and `PPC64LE` for the big and little-endian versions. > The enum values directly reflect the build-time artifacts and resulting > executables. > > This information about an architecture will make the enum more useful > especially to identify a target platform in a cross-platform use case. A > method is added to map well known aliases for the platforms to the > Architecture enum. Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Don't map ppc64le to ppc64 when processing the PlatformProps.java.template - Merge branch 'master' into 8308452-cross-platform-arch - Correct handling of ppc64le/ppc64 using non-canonical architecture. Keep canonical architecture names for s390, amd/x86_64. - Merge branch 'master' into 8308452-cross-platform-arch - Merge branch 'master' into 8308452-cross-platform-arch - Review suggestions - Correct address size for ARM (32-bit) - 8308452: Extend internal Architecture enum with byte order and address size - Changes: https://git.openjdk.org/jdk/pull/14063/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14063&range=02 Stats: 149 lines in 6 files changed: 116 ins; 19 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/14063.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14063/head:pull/14063 PR: https://git.openjdk.org/jdk/pull/14063
Re: RFR: 8308286 Fix clang warnings in linux code [v3]
On Wed, 31 May 2023 13:37:06 GMT, Artem Semenov wrote: >> When using the clang compiler to build OpenJDk on Linux, we encounter >> various "warnings as errors". >> They can be fixed with small changes. > > Artem Semenov has updated the pull request incrementally with one additional > commit since the last revision: > > update src/java.security.jgss/share/native/libj2gss/gssapi.h line 47: > 45: > 46: // Condition was copied from > 47: // > Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h In the current version of the file above, it's written as #if defined(__APPLE__) && (defined(__ppc__) ||... Is it better? - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211757380
Re: RFR: 8308286 Fix clang warnings in linux code [v2]
On Tue, 30 May 2023 08:14:59 GMT, Alexey Ushakov wrote: >> Artem Semenov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update > > src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 655: > >> 653: // linker loaded it. We use "base diff" in read_lib_segments call >> below. >> 654: >> 655: if (ps_pdread(ph, (psaddr_t) link_map_addr + >> LINK_MAP_ADDR_OFFSET, > > Extra white spaces before 'if' done > src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 662: > >> 660: >> 661: // read address of the name >> 662: if (ps_pdread(ph, (psaddr_t) link_map_addr + >> LINK_MAP_NAME_OFFSET, > > Extra white-spaces before 'if' done > src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 717: > >> 715: >> 716: // read next link_map address >> 717: if (ps_pdread(ph, (psaddr_t) link_map_addr + >> LINK_MAP_NEXT_OFFSET, > > Extra white-spaces before 'if' done > src/jdk.jpackage/share/native/common/tstrings.cpp line 60: > >> 58: #endif >> 59: // With g++ this compiles only with '-std=gnu++0x' option >> 60: ret = vsnprintf(&*fmtout.begin(), fmtout.size(), format, >> args); > > Extra white-spaces before 'ret' done - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211732345 PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211734132 PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211734711 PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211735111
Re: RFR: 8308286 Fix clang warnings in linux code [v3]
On Sun, 28 May 2023 03:57:40 GMT, Kim Barrett wrote:
>> Artem Semenov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update
>
> src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c line 1163:
>
>> 1161: #if defined(__clang__)
>> 1162: #pragma clang diagnostic push
>> 1163: #pragma clang diagnostic ignored "-Wparentheses"
>
> I think this warning is because of the several `if (init_result = ...)`?
> Better would be to just add the extra parens.
done
> src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 575:
>
>> 573: if (ps_pdread(ph, (char *)link_map_addr + LINK_MAP_LD_OFFSET,
>> 574:&lib_ld, sizeof(uintptr_t)) != PS_OK) {
>> 575: #else
>
> What problem is being "fixed" by these? I'm dubious that this is the best
> solution to whatever the problem
> is, but can't evaluate or suggest alternatives without knowing what it is.
done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211733228
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211732832
Re: RFR: 8308286 Fix clang warnings in linux code [v3]
> When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. Artem Semenov has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.org/jdk/pull/14033/files - new: https://git.openjdk.org/jdk/pull/14033/files/3f343c26..d5b70cb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14033&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14033&range=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14033/head:pull/14033 PR: https://git.openjdk.org/jdk/pull/14033
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview) [v2]
On Tue, 30 May 2023 17:31:54 GMT, Rémi Forax wrote: > One surprising thing is that Subtask.get() give less leeway to the owner > thread than to the other virtual threads so in onComplete() storing a Subtask > to use it later by the owner thread does not work well if join() is not > called yet. This is perhaps a bit too much. Maybe but it would be a bug if the owner were to call Subtask::get before joining. This is easy to detect for subtasks that are forked by the owner. Time will tell how common it will be for subtasks to fork additional subtasks. - PR Comment: https://git.openjdk.org/jdk/pull/13932#issuecomment-1570217584
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]
On Tue, 30 May 2023 09:32:02 GMT, Jan Lahoda wrote: >> @forax >> >> Hi! Sorry for this sudden message, but this one captured my attention >> >>> and subtype checks are usually fast. >> >> And I hope this PR to be the right place to raise this. >> >> I was looking this PR to better understand how it applies to secondary >> supers and https://bugs.openjdk.org/browse/JDK-8180450 (that's still WIP) >> doesn't look like subtype checks are yet fast nor scalable (with multiple >> interfaces and > 1 receiver types observed) - see >> https://ionutbalosin.com/2023/03/jvm-performance-comparison-for-jdk-17/#typecheckscalabilitybenchmark >> that can be modified to use type switch too. >> >> In addition, at least for secondary types, a missed `IsInstance` (ie which >> type isn't implemented) can cost O(n) over the secondary supers types (see >> https://ionutbalosin.com/2023/03/jvm-performance-comparison-for-jdk-17/#typecheckslowpathbenchmark) >> that's still not ideal, due to the high bootstrapping cost of `prnz scas`. >> >> Hence, the implementation of type switch is likely to account for the >> existing (performance) deficiencies of the secondary supers type check or is >> relying on the fix https://bugs.openjdk.org/browse/JDK-8180450 that will >> appear at some point? >> >> Hope to have interacted in the right way with this with the JDK dev >> community, and thanks again for your hard work on this wonderful piece of >> engineering! > > @franz1981, thanks for your comment. I am afraid I don't know much about > JDK-8180450, but I suspect that it will affect the (type) switch lookup. > Please correct me if I am wrong, but it seems this patch is still an > improvement over the current state, and when JDK-8180450 is resolved, it > should automatically improve the type switch lookup as well. So, this patch > still seems useful to me, with a potential for future improvements, both > inside and outside type switch bootstrap itself. @lahodaj > So, this patch still seems useful to me, with a potential for future > improvements, both inside and outside type switch bootstrap itself. Yep, it is indeed still valuable: sorry if I didn't make it clear. My point is more related the need of the amount of type checks in some cases (reducing the impact of JDK-8180450); if a typecheck-like op is still required to route to some case (and which cost depends by many factors really), any target `checkcast` could be maybe saved. I'm currently unaware if the current state of this PR bring any improvement on this front too (or if is even possible), but this would be welcome regardless JDK-8180450 and much more if considering the mentioned issue too. This can be left for a follow-up PR too, obviously. I'm bringing the attention to this exactly because regardless JDK-8180450, secondary supers negative type check still have costs that would be awesome if could be addressed while exposing this feature to final users. - PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1570214110
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Wed, 31 May 2023 07:38:39 GMT, Aleksey Shipilev wrote:
>> test/jdk/java/util/UUID/UUIDTest.java line 79:
>>
>>> 77: }
>>> 78: if (!set.add(u)) {
>>> 79: throw new Exception("UUID collision: " + u);
>>
>> I would be concerned that if this failure was reported, it would be
>> intermittent, hard to track down, and not reproducible. Without a hook for
>> the generator and the seed, its just going to be noise in the testing.
>
> This is a non-practical concern, IMO. By spec, `UUID.randomUUID` is generated
> from the cryptographically secure random, with >120 bits of randomness, so
> the collision is extremely unlikely. Collision math involves birthday
> paradox, but Wikipedia article on UUIDs fortunately gives us the approximated
> solutions already:
> https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions
>
> Quote: "Thus, the probability to find a duplicate within 103 trillion
> version-4 UUIDs is one in a billion."
>
> In other words, finding a collision in this test with 1M UUIDs points to the
> implementation issue, not a test bug, with a very high probability. In yet
> another words, if a unit test with 1M UUIDs is able to find a collision, then
> this is a strong signal that many production systems that assume extremely
> low collision probability are up for subtle misbehavior.
My point was that its probably not practical to test (more than once).
If it fails, it will be considered just as you propose and disregarded and in
the meantime consumes test cycles in each of the test contexts. Either provide
more information about the conditions under which it failed or remove it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1211696030
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v24]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits: - Merge branch 'master' into 8306112 - Remove mandated flag - Remove trailing whitespace - Add main tests for inferface/enum/record - Improving error recovery in presence of less important syntactic errors in top-level methods and fields. Author: Jan Lahoda - Ignore SKIPs (semicolon class declarations) - Allow unqualified access to unnamed class (internally visible) - Fix missing constructor error messages and handle inner class launching - Merge branch 'master' into 8306112 - Issue warning if traditional main not used. - ... and 30 more: https://git.openjdk.org/jdk/compare/4aea7dab...d0189fc2 - Changes: https://git.openjdk.org/jdk/pull/13689/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13689&range=23 Stats: 1299 lines in 31 files changed: 1134 ins; 32 del; 133 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v23]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Remove mandated flag - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/06aa43ec..a8b31010 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13689&range=22 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13689&range=21-22 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 00:49:22 GMT, Mandy Chung wrote: >>> Apparently, calling utility methods from sun.invoke, like the >>> ensureOriginalLookup in this patch, also triggers this check. >> >> It should not trigger `checkPackageAccess` if it does not implement the >> interface AFAICT. >> >> It would trigger module-access check. `java.base` can export `sun.invoke` >> to the dynamic module (it may be worth considering putting the internal >> interface in a specific package for the dynamic module' use - `sun.invoke` >> is ok since it currently has one interface but hard to catch when someone >> adds a new class/interface in `sun.invoke` unintentionally and leak out >> sensitive API. > >> Then we still need to obtain the implemented interface and original method >> handle information every time they are queried. Having these information (or >> the method handle providing access) computed early is more convenient. > > I was thinking the wrapper instance class still implements some private > methods to get the implemented interface and original method handle which can > be accessed only `java.base` via deep reflection. > It should not trigger `checkPackageAccess` if it does not implement the > interface AFAICT. `checkPackageAccess` is also called by the application class loader: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java#L174-L189 It should make it impossible to reference for example `sun.misc.Unsafe`. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1211652282
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf wrote: >> Please review these test changes which implement automatic testing of >> container resource updates without JVM restart. Note that this merely tests >> container detection code handling this case. It doesn't do anything special >> for the JVM itself, though it might make sense to add some sanity checks >> should we detect certain limits changing. In another PR, though. >> >> As to the test design, it works similar to the shared temp tests: Interact >> between the two containers by virtue of a shared filesystem `/tmp` and >> creating marker files there in order to make them cooperate. Note that the >> new test needs `podman` version `4.3.0` and better (`4.5` is current). >> >> Testing: >> - [x] GHA >> - [x] Linux x86_64 container tests on cg v1 and cg v2 system >> - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and >> `docker`) > > Severin Gehwolf 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 branch 'master' into jdk-8308090-tests-container-on-fly-updates > - Fix whitespace > - 8308090: Add container tests for on-the-fly resource quota updates Anyone willing to review this? - PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1570090062
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded double streams to RandomGenerator [v10]
> The `default` method `nextDouble(double origin, double bound)` in > `java.util.random.RandomGenerator` aims at generating a uniformly and > spatially equidistributed random `double` in the left-closed and right-open > range [`origin`, `bound`). It does so by applying the affine transform > `origin + (bound - origin) * r` to a uniformly and spatially equidistributed > random `double` `r` in the range [0, 1). > > Since floating-point arithmetic suffers from small but noticeable rounding > errors, this ends up slightly deforming the distribution of `r` when applying > the affine transform. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Correction in comment. - Changes: - all: https://git.openjdk.org/jdk/pull/12719/files - new: https://git.openjdk.org/jdk/pull/12719/files/b3667a95..80e49d41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12719&range=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12719/head:pull/12719 PR: https://git.openjdk.org/jdk/pull/12719
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
On Fri, 26 May 2023 19:54:07 GMT, Rudi Horn wrote: >> This change prevents the contents of the internal string array from being >> copied back when releasing it. > > Rudi Horn has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains one commit: > > Use JNI_ABORT to release string bytes I have not checked other uses, but they are probably not as critical yet given the special status and frequent use of strings. I can possibly revisit this issue in the future in a further issue / PR if I find time at some stage. - PR Comment: https://git.openjdk.org/jdk/pull/14117#issuecomment-1570043153
Re: RFR: JDK-8282797: CompileCommand parsing errors should exit VM [v3]
On Wed, 24 May 2023 11:05:01 GMT, Tobias Holenstein wrote: >> Currently, errors during compile command parsing just print an error but >> don't exit the VM. As a result, issues go unnoticed. >> >> With this PR the behavior is changed to exit the VM when an error occurs. >> >> E.g. `java -XX:CompileCommand=compileonly,HashMap:: -version` will exit the >> VM after a parsing occurred. >> >> CompileCommand: An error occurred during parsing >> Error: Could not parse method pattern >> Line: 'compileonly,HashMap::' >> >> Usage: '-XX:CompileCommand=,' - to set boolean >> option to true >> Usage: '-XX:CompileCommand=,,' >> Use: '-XX:CompileCommand=help' for more information and to list all option. >> >> Error: Could not create the Java Virtual Machine. >> Error: A fatal exception has occurred. Program will exit. >> >> >> ### Updated Tests >> Updated all tests to now expect an error code `1` for wrong `CompileCommand` > > Tobias Holenstein has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Scenario.java > - Update compilerOracle.cpp Thanks for doing the updates, looks good! - Marked as reviewed by chagedorn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13753#pullrequestreview-1452870393
Re: RFR: JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly [v2]
On Tue, 23 May 2023 09:08:20 GMT, Tobias Holenstein wrote: >> At the moment `CompileCommand` and `CompileOnly` use different syntax for >> matching methods. >> >> ### Old CompileOnly format >> - matching a **method name** with **class name** and **package name**: >> `-XX:CompileOnly=package/path/Class.method` >> `-XX:CompileOnly=package/path/Class::method` >> `-XX:CompileOnly=package.path.Class::method` >> BUT NOT `-XX:CompileOnly=package.path.Class.method` >> >> - just matching a **single method name**: >> `-XX:CompileOnly=.hashCode` >> `-XX:CompileOnly=::hashCode` >> BUT NOT `-XX:CompileOnly=hashCode` >> >> - Matching **all method names** in a **class name** with **package name** >> `-XX:CompileOnly=java/lang/String` >> BUT NOT `-XX:CompileOnly=java/lang/String.` >> BUT NOT `-XX:CompileOnly=java.lang.String` >> BUT NOT `-XX:CompileOnly=java.lang.String::` (This is actually a bug) >> BUT NOT `-XX:CompileOnly=String` >> BUT NOT `-XX:CompileOnly=String.` >> BUT NOT `-XX:CompileOnly=String::` >> >> - Matching **all method names** in a **class name** with **NO package name** >> `-XX:CompileOnly=String` >> BUT NOT `-XX:CompileOnly=String.` >> BUT NOT `-XX:CompileOnly=String::` >> >> - There is a bug when `CompileOnly` ends with `::` where the `CompileOnly` >> is just ignored >> e.g. `-XX:CompileOnly=String::` compiles as many methods as when omitting >> the `-XX:CompileOnly=` command >> >> ### CompileCommand=compileonly format >> `CompileCommand` allows two different forms for paths: >> - `package/path/Class.method` >> - `package.path.Class::method` >> >> In contrary to `CompileOnly` `CompileCommand` supports wildcard matching >> using `*`. `*` can appear at the beginning and/or end of a >> `package.path.Class` and `method` name. >> >> Valid forms: >> `-XX:CompileCommand=compileonly,*.lang.*::*shCo*` >> `-XX:CompileCommand=compileonly,*/lang/*.*shCo*` >> `-XX:CompileCommand=compileonly,java.lang.String::*` >> `-XX:CompileCommand=compileonly,*::hashCode` >> `-XX:CompileCommand=compileonly,*ng.String::hashC*` >> `-XX:CompileCommand=compileonly,*String::hash*` >> >> Invalid forms (Error: Embedded * not allowed): >> `-XX:CompileCommand=compileonly,java.*.String::has*Code` >> >> ### Use CompileCommand syntax for CompileOnly >> At the moment, in some cases it is not possible to just take pattern used >> with `CompileOnly` and plug it into compile command file. Syntax used by >> CompileOnly is also not very intuitive. >> >> `CompileOnly` is convenient because it's shorter to write and takes lists of >> patterns, whereas `CompileCommand` only takes one pattern per command. >> >> W... > > Tobias Holenstein has updated the pull request incrementally with three > additional commits since the last revision: > > - Update Test8211698.java > - Update src/hotspot/share/compiler/compilerOracle.cpp > >Co-authored-by: Christian Hagedorn > - Update src/hotspot/share/compiler/compilerOracle.cpp > >Co-authored-by: Christian Hagedorn Update looks good! - Marked as reviewed by chagedorn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13802#pullrequestreview-1452864244
RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading
The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to allow deployment to choose whether to allow agents to be loaded/started in the VM. The VM option does the right thing for tools using the Attach API but jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading JVMTI agents when the EnableDynamicAgentLoading is false. Testing: - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java` - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced - Commit messages: - 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading Changes: https://git.openjdk.org/jdk/pull/14244/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14244&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304438 Stats: 78 lines in 2 files changed: 78 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14244.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244 PR: https://git.openjdk.org/jdk/pull/14244
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently
On Tue, 30 May 2023 08:31:08 GMT, SUN Guoyun wrote:
> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
> TEST=gc/TestAllocHumongousFragment.java
> error info:
>
> Caused by: java.lang.NullPointerException: Cannot invoke
> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
> at
> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
> ... 24 more
>
> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
> (LocaleObjectCache uses SoftReferences, used by printf method called in
> getRandomInstance(Utils.java:511)).
>
> Maybe we have to deal with the case where the getBaseLocale() return value is
> null. the call stack is:
>
> at
> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
> at
> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>
> in LocaleObjectCache.java:64
>
>62 if (key == null || newVal == null) {
>
>63 // subclass must return non-null key/value object
>
>64 return null; // run here
>65 }
Jtreg tier1 can trigger the same error with vmoptions:"-Xcomp
-XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive
-XX:+ShenandoahOOMDuringEvacALot
I found the GC occurs between when the soft reference is assigned and when it
is used.
private BaseLocale getBaseLocale() {
-return (holder == null) ? holderRef.get() : holder;
+//return (holder == null) ? holderRef.get() : holder;
+if (holder == null) {
+ System.out.println("getBaseLocale this=" + this + "
SoftReference=" + holderRef.get()); // null
+ return holderRef.get();
+} else {
+ System.out.println("getBaseLocale return holder");
+ return holder;
+}
}
The following modification verifies this.
--- a/src/java.base/share/classes/sun/util/locale/BaseLocale.java
+++ b/src/java.base/share/classes/sun/util/locale/BaseLocale.java
@@ -257,19 +261,21 @@ public final class BaseLocale {
private final boolean normalized;
private final int hash;
-
+private BaseLocale locale;// make locale to a member variable
private Key(String language, String script, String region,
String variant, boolean normalize) {
-BaseLocale locale = new BaseLocale(language, script, region,
variant, normalize);
+locale = new BaseLocale(language, script, region, variant,
normalize);
this.normalized = normalize;
But this should not be a reasonable solution. I think I should to find a better
solution.
-
PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1569881913
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Tue, 30 May 2023 21:44:17 GMT, JoKern65 wrote: >> src/hotspot/share/runtime/javaThread.cpp line 115: >> >>> 113: #include >>> 114: #endif >>> 115: >> >> Could these conditionals be included in globalDefinitions_xlc.hpp instead? > > In principle the `#include ` could be included in > globalDefinitions_xlc.hpp. But alloca.h implements it with a > `#undef alloca` > `#define alloca(size) __builtin_alloca (size)` > If this new global define does not introduce new trouble (even in future > coding) when it is seen in every source, we can go this way. > Are there any obstacles from anyone else? At least the whole jdk actually builds with `#include ` in globalDefinitions_xlc.hpp - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1211272616
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v3]
On Tue, 30 May 2023 22:16:08 GMT, Joe Darcy wrote: >> Time to get JDK 22 underway... > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 24 commits: > > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Sync in symbol changes for JDK 21 build 24. > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Merge branch 'master' into JDK-8306584 > - Minor test fixes. > - Merge branch 'master' into JDK-8306584 > - Update symbol files to JDK 21 b23. > - ... and 14 more: https://git.openjdk.org/jdk/compare/cb40db05...376dbe26 Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13567#pullrequestreview-1452455740
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v2]
On Tue, 30 May 2023 20:55:39 GMT, Roger Riggs wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments
>
> test/jdk/java/util/UUID/UUIDTest.java line 79:
>
>> 77: }
>> 78: if (!set.add(u)) {
>> 79: throw new Exception("UUID collision: " + u);
>
> I would be concerned that if this failure was reported, it would be
> intermittent, hard to track down, and not reproducible. Without a hook for
> the generator and the seed, its just going to be noise in the testing.
This is a non-practical concern, IMO. By spec, `UUID.randomUUID` is generated
from the cryptographically secure random, with >120 bits of randomness, so the
collision is extremely unlikely. Collision math involves birthday paradox, but
Wikipedia article on UUIDs fortunately gives us the approximated solutions
already:
https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions
Quote: "Thus, the probability to find a duplicate within 103 trillion version-4
UUIDs is one in a billion."
In other words, finding a collision in this test with 1M UUIDs points to the
implementation issue, not a test bug, with a very high probability. In yet
another words, if a unit test with 1M UUIDs is able to find a collision, then
this is a strong signal that many production systems that assume extremely low
collision probability are up for subtle misbehavior.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1211223450
Re: RFR: 8308780: Fix the Java Integer types on Windows
On Wed, 24 May 2023 13:56:05 GMT, Julian Waters wrote: > On Windows, the basic Java Integer types are defined as long and __int64 > respectively. In particular, the former is rather problematic since it breaks > compilation as the Visual C++ becomes stricter and more compliant with every > release, which means the way Windows code treats long as a typedef for int is > no longer correct, especially with -permissive- enabled. Instead of changing > every piece of broken code to match the jint = long typedef, which is far too > time consuming, we can instead change jint to an int (which is still the same > 32 bit number type as long), as there are far fewer problems caused by this > definition. It's better to get this over and done with sooner than later when > a future version of Visual C++ finally starts to break on existing code Bumping :( - PR Comment: https://git.openjdk.org/jdk/pull/14125#issuecomment-1569611758
