RFR: 8351017: ChronoUnit.MONTHS.between() not giving correct result when date is in February
Clarifying the explanation for `TemporalUnit.between()`. There is already an example for the `HOURS` case where the minutes are not enough to make a full hour. Explaining how smaller units contribute to determining the whole number, along with an explicit `MONTHS` example, which was the case reported by the bug submitter, will help users. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/23937/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23937&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351017 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23937.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23937/head:pull/23937 PR: https://git.openjdk.org/jdk/pull/23937
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451 [v2]
On Tue, 4 Mar 2025 19:37:32 GMT, Vladimir Ivanov wrote: >> test setup was updated to generate data of requested size. > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8350811 [JMH] test foreign.StrLenTest failed with > StringIndexOutOfBoundsException for size=451 @IvaVladimir Your change (at version 6920770118c6533c9e618bad1d6fb883dd6de3da) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/23873#issuecomment-2705186362
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 15:47:58 GMT, Magnus Ihse Bursie wrote: >> What is the intended way of using this? Do you run make with >> `LIBPTHREAD=-pthread` or do you apply a patch on `libraries.m4` for the >> specific way of linking to pthread? > >> What is the intended way of using this? Do you run make with >> LIBPTHREAD=-pthread or do you apply a patch on libraries.m4 for the specific >> way of linking to pthread? > > This is in preparation of the upcoming BSD port, which uses `-pthread` > instead of `-pthread`. It was me who suggested that this is done separately > with the existing code, to minimize the patch of the BSD port. @magicus why can't we just use `-pthread` everywhere? My recollection is that `-pthread` both sets compiler directives needed for pthread programming and links to libpthread, so it seems to be what we should be using. ?? - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2705227213
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451 [v2]
On Tue, 4 Mar 2025 19:37:32 GMT, Vladimir Ivanov wrote: >> test setup was updated to generate data of requested size. > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8350811 [JMH] test foreign.StrLenTest failed with > StringIndexOutOfBoundsException for size=451 Thanks for your review! - PR Comment: https://git.openjdk.org/jdk/pull/23873#issuecomment-2705185511
Re: RFR: 8351017: ChronoUnit.MONTHS.between() not giving correct result when date is in February
On Thu, 6 Mar 2025 23:36:29 GMT, Naoto Sato wrote: > Clarifying the explanation for `TemporalUnit.between()`. There is already an > example for the `HOURS` case where the minutes are not enough to make a full > hour. Explaining how smaller units contribute to determining the whole > number, along with an explicit `MONTHS` example, which was the case reported > by the bug submitter, will help users. Marked as reviewed by scolebourne (Author). - PR Review: https://git.openjdk.org/jdk/pull/23937#pullrequestreview-2666401651
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v4]
On Tue, 17 Dec 2024 16:40:01 GMT, Galder Zamarreño wrote:
>> test/hotspot/jtreg/compiler/intrinsics/math/TestMinMaxInlining.java line 80:
>>
>>> 78: @IR(phase = { CompilePhase.BEFORE_MACRO_EXPANSION }, counts = {
>>> IRNode.MIN_L, "1" })
>>> 79: @IR(phase = { CompilePhase.AFTER_MACRO_EXPANSION }, counts = {
>>> IRNode.MIN_L, "0" })
>>> 80: private static long testLongMin(long a, long b) {
>>
>> Can you add a comment why it disappears after macro expansion?
>
> ~Good question. On non-avx512 machines after macro expansion the min/max
> nodes become cmov nodes, but but that's not the full story because on avx512
> machines, they become minV/maxV nodes. Would you tweak the `@IR` annotations
> to capture this? Or would you leave it just as a comment?~
>
> Scratch that, this is not a test for arrays, so no minV/maxV nodes. I'll just
> add a comment.
I've added a comment
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20098#discussion_r1984510490
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v14]
> This patch intrinsifies `Math.max(long, long)` and `Math.min(long, long)` in
> order to help improve vectorization performance.
>
> Currently vectorization does not kick in for loops containing either of these
> calls because of the following error:
>
>
> VLoop::check_preconditions: failed: control flow in loop not allowed
>
>
> The control flow is due to the java implementation for these methods, e.g.
>
>
> public static long max(long a, long b) {
> return (a >= b) ? a : b;
> }
>
>
> This patch intrinsifies the calls to replace the CmpL + Bool nodes for
> MaxL/MinL nodes respectively.
> By doing this, vectorization no longer finds the control flow and so it can
> carry out the vectorization.
> E.g.
>
>
> SuperWord::transform_loop:
> Loop: N518/N126 counted [int,int),+4 (1025 iters) main has_sfpt
> strip_mined
> 518 CountedLoop === 518 246 126 [[ 513 517 518 242 521 522 422 210 ]]
> inner stride: 4 main of N518 strip mined !orig=[419],[247],[216],[193] !jvms:
> Test::test @ bci:14 (line 21)
>
>
> Applying the same changes to `ReductionPerf` as in
> https://github.com/openjdk/jdk/pull/13056, we can compare the results before
> and after. Before the patch, on darwin/aarch64 (M1):
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
> 1 1 0 0
> ==
> TEST SUCCESS
>
> long min 1155
> long max 1173
>
>
> After the patch, on darwin/aarch64 (M1):
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
> 1 1 0 0
> ==
> TEST SUCCESS
>
> long min 1042
> long max 1042
>
>
> This patch does not add an platform-specific backend implementations for the
> MaxL/MinL nodes.
> Therefore, it still relies on the macro expansion to transform those into
> CMoveL.
>
> I've run tier1 and hotspot compiler tests on darwin/aarch64 and got these
> results:
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg:tier1 2500 2500 0 0
>>> jtreg:test/jdk:tier1 ...
Galder Zamarreño 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 47 additional commits
since the last revision:
- Merge branch 'master' into topic.intrinsify-max-min-long
- Add assertion comments
- Add simple reduction benchmarks on top of multiply ones
- Merge branch 'master' into topic.intrinsify-max-min-long
- Fix typo
- Renaming methods and variables and add docu on algorithms
- Fix copyright years
- Make sure it runs with cpus with either avx512 or asimd
- Test can only run with 256 bit registers or bigger
* Remove platform dependant check
and use platform independent configuration instead.
- Fix license header
- ... and 37 more: https://git.openjdk.org/jdk/compare/a328e466...1aa690d3
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20098/files
- new: https://git.openjdk.org/jdk/pull/20098/files/d0e793a3..1aa690d3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20098&range=13
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20098&range=12-13
Stats: 65249 lines in 2144 files changed: 33401 ins; 21691 del; 10157 mod
Patch: https://git.openjdk.org/jdk/pull/20098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20098/head:pull/20098
PR: https://git.openjdk.org/jdk/pull/20098
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451 [v2]
On Tue, 4 Mar 2025 19:37:32 GMT, Vladimir Ivanov wrote:
>> test setup was updated to generate data of requested size.
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8350811 [JMH] test foreign.StrLenTest failed with
> StringIndexOutOfBoundsException for size=451
test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java line 57:
> 55:
> 56: SegmentAllocator segmentAllocator;
> 57: SegmentAllocator arenaAllocator;
Please update the copyright of this and other modified files.
test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java line 60:
> 58: SlicingPool pool;
> 59:
> 60: @Param({"5", "20", "100"})
Can you also add size 451 in this list as this is what was fixed :-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23873#discussion_r1984488315
PR Review Comment: https://git.openjdk.org/jdk/pull/23873#discussion_r1984497674
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v12]
On Thu, 6 Mar 2025 15:22:18 GMT, Emanuel Peter wrote: >> Also, I've started a [discussion on >> jmh-dev](https://mail.openjdk.org/pipermail/jmh-dev/2025-February/004094.html) >> to see if there's a way to minimise pollution of `Math.min(II)` >> compilation. As a follow to >> https://github.com/openjdk/jdk/pull/20098#issuecomment-2684701935 I looked >> at where the other `Math.min(II)` calls are coming from, and a big chunk >> seem related to the JMH infrastructure. > > @galderz you said you would add some extra comments, then I will review again > :) @eme64 I've added the comment that was pending from your last review. I've also merged latest master. - PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2705620662
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v17]
On Sun, 2 Mar 2025 21:17:04 GMT, Sergey Chernyshev wrote: >> OK for me now. `test_cgroupSubsystem_linux.cpp` needs a copyright update as >> well. > >> OK for me now. `test_cgroupSubsystem_linux.cpp` needs a copyright update as >> well. > > Thanks for your review @jerboaa ! I cheched the > test_cgroupSubsystem_linux.cpp, it's already updated to 2025 in the master > branch. @sercher your new test is failing in our CI: [STDOUT] mkdir: cannot create directory '/sys/fs/cgroup/memory/test': Permission denied sh: /sys/fs/cgroup/memory/test/memory.limit_in_bytes: No such file or directory sh: /sys/fs/cgroup/memory/test/cgroup.procs: No such file or directory I will file a new bug - PR Comment: https://git.openjdk.org/jdk/pull/21808#issuecomment-2705506729
Re: RFR: 8350909: [JMH] test ThreadOnSpinWaitShared failed for 2 threads config
On Fri, 28 Feb 2025 01:20:44 GMT, Vladimir Ivanov wrote: > The scope was updated to support multithread configuration (jmh option '-t > 2') . No other changes needed. Thanks for your review! - PR Comment: https://git.openjdk.org/jdk/pull/23834#issuecomment-2704538159
Integrated: 8349358: [JMH] Cannot access class jdk.internal.vm.ContinuationScope
On Tue, 4 Feb 2025 12:35:50 GMT, SendaoYan wrote: > Hi all, > > Several JMH tests fails "cannot access class > jdk.internal.vm.ContinuationScope (in module java.base) because module > java.base does not export jdk.internal.vm to unnamed module". This PR add VM > option `--add-exports=java.base/jdk.internal.vm=ALL-UNNAMED` to fix the JMH > test bug. > > Change has been verified locally, test-fix only, no risk. This pull request has now been integrated. Changeset: 5c552a9d Author:SendaoYan URL: https://git.openjdk.org/jdk/commit/5c552a9d64c8116161cb9ef4c777e75a2602a75b Stats: 505 lines in 3 files changed: 0 ins; 505 del; 0 mod 8349358: [JMH] Cannot access class jdk.internal.vm.ContinuationScope Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/23437
RFR: 8348986: Improve coverage of enhanced exception messages
Hi, Enhanced exception messages are designed to hide sensitive information such as hostnames, IP addresses from exception message strings, unless the enhanced mode for the specific category has been explicitly enabled. Enhanced exceptions were first introduced in 8204233 in JDK 11 and updated in 8207846. This PR aims to increase the coverage of enhanced exception messages in the networking code. A limited number of exceptions are already hidden (restricted) by default. The new categories and exceptions in this PR will be restricted on an opt-in basis, ie. the default mode will be enhanced (while preserving the existing behavior). The mechanism is controlled by the security/system property "jdk.includeInExceptions" which takes as value a comma separated list of category names, which identify groups of exceptions where the exception message may be enhanced. Any category not listed is "restricted" which means that potentially sensitive information (such as hostnames, IP addresses, user identities) are excluded from the message text. The changes to the java.security conf file describe the exact changes in terms of the categories now supported and any changes in behavior. Thanks, Michael - Commit messages: - remove file added by mistake - whitespace - moved test - Merge branch 'master' into 8348986-exceptions - update - update - Merge branch 'master' into 8348986-exceptions - update - Merge branch 'master' into 8348986-exceptions - Merge branch 'master' into 8348986-exceptions - ... and 3 more: https://git.openjdk.org/jdk/compare/b1a21b56...c4419860 Changes: https://git.openjdk.org/jdk/pull/23929/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8348986 Stats: 1002 lines in 42 files changed: 762 ins; 104 del; 136 mod Patch: https://git.openjdk.org/jdk/pull/23929.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23929/head:pull/23929 PR: https://git.openjdk.org/jdk/pull/23929
Re: RFR: 8351344: Avoid explicit Objects.requireNonNull in String.join
On Thu, 20 Feb 2025 09:30:02 GMT, Andrey Turbanov wrote: > We have helpful NPE messages now - they are more user-friendly. > And shorter methods are more likely to be inlined. Looks nice to me, thanks! src/java.base/share/classes/java/lang/String.java line 3650: > 3648: var delim = delimiter.toString(); > 3649: var elems = new String[8]; > 3650: int size = 0; Can you add a `// implicit null check` before the loop? Otherwise, the null check may be forgotten that future code change may early return before the null check here is triggered. - Marked as reviewed by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23710#pullrequestreview-2638571708 PR Review Comment: https://git.openjdk.org/jdk/pull/23710#discussion_r1964153031
RFR: 8351322: Parameterize link option for pthreads
Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that it's possible to parameterize this for platforms that use different flags for enabling posix threads. This work is a continuation of the work done by Greg Lewis in [1], but generalized for the full JDK, and set at the configure stage. Sponsored by: The FreeBSD Foundation Co-authored-by: Greg Lewis [1]: https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 - Commit messages: - 8351322: Parameterize link option for pthreads Changes: https://git.openjdk.org/jdk/pull/23930/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23930&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351322 Stats: 661 lines in 10 files changed: 9 ins; 0 del; 652 mod Patch: https://git.openjdk.org/jdk/pull/23930.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23930/head:pull/23930 PR: https://git.openjdk.org/jdk/pull/23930
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v12]
On Thu, 27 Feb 2025 16:38:30 GMT, Galder Zamarreño wrote:
>> Galder Zamarreño 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 44 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into topic.intrinsify-max-min-long
>> - Fix typo
>> - Renaming methods and variables and add docu on algorithms
>> - Fix copyright years
>> - Make sure it runs with cpus with either avx512 or asimd
>> - Test can only run with 256 bit registers or bigger
>>
>>* Remove platform dependant check
>>and use platform independent configuration instead.
>> - Fix license header
>> - Tests should also run on aarch64 asimd=true envs
>> - Added comment around the assertions
>> - Adjust min/max identity IR test expectations after changes
>> - ... and 34 more: https://git.openjdk.org/jdk/compare/dfbb2ee6...a190ae68
>
> Also, I've started a [discussion on
> jmh-dev](https://mail.openjdk.org/pipermail/jmh-dev/2025-February/004094.html)
> to see if there's a way to minimise pollution of `Math.min(II)` compilation.
> As a follow to
> https://github.com/openjdk/jdk/pull/20098#issuecomment-2684701935 I looked at
> where the other `Math.min(II)` calls are coming from, and a big chunk seem
> related to the JMH infrastructure.
@galderz about:
> Additional performance improvement: extend backend capabilities for
> vectorization (see Regression 2 + 3). Optional.
I looked at `src/hotspot/cpu/x86/x86.ad`
bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType bt) {
1774 case Op_MaxV:
1775 case Op_MinV:
1776 if (UseSSE < 4 && is_integral_type(bt)) {
1777 return false;
1778 }
...
So it seems that here lanewise min/max are supported for AVX2. But it seems
that's different for reductions:
1818 case Op_MinReductionV:
1819 case Op_MaxReductionV:
1820 if ((bt == T_INT || is_subword_type(bt)) && UseSSE < 4) {
1821 return false;
1822 } else if (bt == T_LONG && (UseAVX < 3 ||
!VM_Version::supports_avx512vlbwdq())) {
1823 return false;
1824 }
...
So it seems maybe we could improve the AVX2 coverage for reductions. But
honestly, I will probably find this issue again once I work on the other
reductions above, and run the benchmarks. I think that will make it easier to
investigate all of this. I will for example adjust the IR rules, and then it
will be apparent where there are cases that are not covered.
@galderz you said you would add some extra comments, then I will review again :)
-
PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2704159992
PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2704161929
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files [v3]
> jpackage app laucnher will expand environment variables in .cfg files.
>
> Previously jpackage app launcher only replaced `$APPDIR`, `$BINDIR`, and
> `$ROOTDIR` tokens with the corresponding path values. With this patch, any
> environment variable can be expanded. The syntax is shell-like
> `$ENV_VAR_NAME` or `${ENV_VAR_NAME}`. If `$ENV_VAR_NAME` syntax is used, the
> variable name stops at the first character outside of `[a-zA-Z0-9_]`
> character range. If `${ENV_VAR_NAME}` syntax is used, the variable name stops
> at the first `}` character after `${` substring. E.g:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Welcome $USER! | USER | USER=John |
> Welcome John! ||
> | Welcome $USER! | USER | not set |
> Welcome $USER! | Unset variables are not expanded. |
> | Welcome $USER2 | USER2 | USER2=John |
> Welcome John! ||
> | Welcome ${USER}2 | USER | USER=John |
> Welcome John2! ||
> | Welcome $USER-2 | USER | USER=John |
> Welcome John-2! ||
> | Welcome ${USER-2} | USER-2 | USER-2=John
> | Welcome John! | `USER-2` is likely to be an invalid env variable
> name, but jpackage launcher is not validating names. |
>
> `$` character combination prevents variable expansion:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \$A! Bye $B | B | B=John | Hello $A! Bye
> John |
> | Hello \${A}! Bye $B | B | B=John | Hello ${A}!
> Bye John |
>
> `\` character combination escapes ``:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \\\$A! Bye $B | A, B | A=AnaB=John
> | Hello \\Ana! Bye John |
>
> If `` character is not followed by another `` character or `$` character, it
> is interpreted as a regular character:
> | String| Expanded string |
> | | --- |
> |a\\b\\c|a\\b\\c|
>
>
> Expansion is non-recursive:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Hello $A! | A | A=$BB=John | Hello
> $B | Variable "B" will ...
Alexey Semenyuk has updated the pull request incrementally with two additional
commits since the last revision:
- Merge branch 'JDK-8341641-master' of
https://github.com/alexeysemenyukoracle/jdk into JDK-8341641-master
- Formatting fixed
-
Changes:
- all: https://git.openjdk.org/jdk/pull/23923/files
- new: https://git.openjdk.org/jdk/pull/23923/files/e5d248a1..05cbac43
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23923&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23923&range=01-02
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/23923.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23923/head:pull/23923
PR: https://git.openjdk.org/jdk/pull/23923
Re: RFR: 8319447: Improve performance of delayed task handling [v6]
On Wed, 5 Mar 2025 23:55:52 GMT, Sunmisc Unsafe wrote:
>> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 3741:
>>
>>> 3739: public ForkJoinTask submitWithTimeout(Callable callable,
>>> 3740: long timeout,
>>> TimeUnit unit,
>>> 3741:
>>> Consumer> timeoutAction) {
>>
>> I wonder if the variance of the Consumer to this method should be more
>> permissible. Something like:
>>
>> public ForkJoinTask submitWithTimeout(Callable callable,
>> long timeout, TimeUnit unit,
>> Consumer> ForkJoinTask> timeoutAction) {
>
> Maybe it's worth using java.time.Duration, although it doesn't fit into the
> API very elegantly.
> I wonder if the variance of the Consumer to this method should be more
> permissible. Something like:
>
> public ForkJoinTask submitWithTimeout(Callable callable, long timeout,
> TimeUnit unit, Consumer> timeoutAction) {
I think this is a good point but it might be more practical to limit it to just
a contravariant Consumer, as in ` Consumer>`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23702#discussion_r1983469247
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files
On Wed, 5 Mar 2025 21:13:44 GMT, Alexey Semenyuk wrote:
> jpackage app laucnher will expand environment variables in .cfg files.
>
> Previously jpackage app launcher only replaced `$APPDIR`, `$BINDIR`, and
> `$ROOTDIR` tokens with the corresponding path values. With this patch, any
> environment variable can be expanded. The syntax is shell-like
> `$ENV_VAR_NAME` or `${ENV_VAR_NAME}`. If `$ENV_VAR_NAME` syntax is used, the
> variable name stops at the first character outside of `[a-zA-Z0-9_]`
> character range. If `${ENV_VAR_NAME}` syntax is used, the variable name stops
> at the first `}` character after `${` substring. E.g:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Welcome $USER! | USER | USER=John |
> Welcome John! ||
> | Welcome $USER! | USER | not set |
> Welcome $USER! | Unset variables are not expanded. |
> | Welcome $USER2 | USER2 | USER2=John |
> Welcome John! ||
> | Welcome ${USER}2 | USER | USER=John |
> Welcome John2! ||
> | Welcome $USER-2 | USER | USER=John |
> Welcome John-2! ||
> | Welcome ${USER-2} | USER-2 | USER-2=John
> | Welcome John! | `USER-2` is likely to be an invalid env variable
> name, but jpackage launcher is not validating names. |
>
> `$` character combination prevents variable expansion:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \$A! Bye $B | B | B=John | Hello $A! Bye
> John |
> | Hello \${A}! Bye $B | B | B=John | Hello ${A}!
> Bye John |
>
> `\` character combination escapes ``:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \\\$A! Bye $B | A, B | A=AnaB=John
> | Hello \\Ana! Bye John |
>
> If `` character is not followed by another `` character or `$` character, it
> is interpreted as a regular character:
> | String| Expanded string |
> | | --- |
> |a\\b\\c|a\\b\\c|
>
>
> Expansion is non-recursive:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Hello $A! | A | A=$BB=John | Hello
> $B | Variable "B" will ...
src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/TokenReplace.java
line 112:
> 110: regexps = new ArrayList<>();
> 111:
> 112: for(;;) {
Suggestion:
for (;;) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23923#discussion_r1983241792
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files [v2]
> jpackage app laucnher will expand environment variables in .cfg files.
>
> Previously jpackage app launcher only replaced `$APPDIR`, `$BINDIR`, and
> `$ROOTDIR` tokens with the corresponding path values. With this patch, any
> environment variable can be expanded. The syntax is shell-like
> `$ENV_VAR_NAME` or `${ENV_VAR_NAME}`. If `$ENV_VAR_NAME` syntax is used, the
> variable name stops at the first character outside of `[a-zA-Z0-9_]`
> character range. If `${ENV_VAR_NAME}` syntax is used, the variable name stops
> at the first `}` character after `${` substring. E.g:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Welcome $USER! | USER | USER=John |
> Welcome John! ||
> | Welcome $USER! | USER | not set |
> Welcome $USER! | Unset variables are not expanded. |
> | Welcome $USER2 | USER2 | USER2=John |
> Welcome John! ||
> | Welcome ${USER}2 | USER | USER=John |
> Welcome John2! ||
> | Welcome $USER-2 | USER | USER=John |
> Welcome John-2! ||
> | Welcome ${USER-2} | USER-2 | USER-2=John
> | Welcome John! | `USER-2` is likely to be an invalid env variable
> name, but jpackage launcher is not validating names. |
>
> `$` character combination prevents variable expansion:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \$A! Bye $B | B | B=John | Hello $A! Bye
> John |
> | Hello \${A}! Bye $B | B | B=John | Hello ${A}!
> Bye John |
>
> `\` character combination escapes ``:
> | String| Variables | Variable Values | Expanded string |
> | | --- |--- |--- |
> | Hello \\\$A! Bye $B | A, B | A=AnaB=John
> | Hello \\Ana! Bye John |
>
> If `` character is not followed by another `` character or `$` character, it
> is interpreted as a regular character:
> | String| Expanded string |
> | | --- |
> |a\\b\\c|a\\b\\c|
>
>
> Expansion is non-recursive:
> | String| Variables | Variable Values | Expanded string | Note |
> | | --- |--- |--- |--- |
> | Hello $A! | A | A=$BB=John | Hello
> $B | Variable "B" will ...
Alexey Semenyuk has updated the pull request incrementally with one additional
commit since the last revision:
Update
src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/TokenReplace.java
Co-authored-by: Andrey Turbanov
-
Changes:
- all: https://git.openjdk.org/jdk/pull/23923/files
- new: https://git.openjdk.org/jdk/pull/23923/files/32f2b529..e5d248a1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23923&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23923&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/23923.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23923/head:pull/23923
PR: https://git.openjdk.org/jdk/pull/23923
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 12:46:25 GMT, David Holmes wrote: > Abstracting this out seems reasonable to me, though I should say I thought we > already used `-pthread` rather than `-lpthread`. I noticed there were a few places that used `-pthread` by default. I left these alone in this PR. - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2703853406
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v12]
On Thu, 27 Feb 2025 16:38:30 GMT, Galder Zamarreño wrote: >> Galder Zamarreño 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 44 additional >> commits since the last revision: >> >> - Merge branch 'master' into topic.intrinsify-max-min-long >> - Fix typo >> - Renaming methods and variables and add docu on algorithms >> - Fix copyright years >> - Make sure it runs with cpus with either avx512 or asimd >> - Test can only run with 256 bit registers or bigger >> >>* Remove platform dependant check >>and use platform independent configuration instead. >> - Fix license header >> - Tests should also run on aarch64 asimd=true envs >> - Added comment around the assertions >> - Adjust min/max identity IR test expectations after changes >> - ... and 34 more: https://git.openjdk.org/jdk/compare/47fdb836...a190ae68 > > Also, I've started a [discussion on > jmh-dev](https://mail.openjdk.org/pipermail/jmh-dev/2025-February/004094.html) > to see if there's a way to minimise pollution of `Math.min(II)` compilation. > As a follow to > https://github.com/openjdk/jdk/pull/20098#issuecomment-2684701935 I looked at > where the other `Math.min(II)` calls are coming from, and a big chunk seem > related to the JMH infrastructure. @galderz about: > Additional performance improvement: make SuperWord recognize more cases as > profitble (see Regression 1). Optional. This should already be covered by these, and I will handle that eventually with the Cost-Model RFE [JDK-8340093](https://bugs.openjdk.org/browse/JDK-8340093): - [JDK-8345044](https://bugs.openjdk.org/browse/JDK-8345044) Sum of array elements not vectorized - (min/max of array) - [JDK-8336000](https://bugs.openjdk.org/browse/JDK-8336000) C2 SuperWord: report that 2-element reductions do not vectorize - You would for example see that on aarch64 machines with only neon/asimd support you can have at most 2 longs per vector, because the max vector length is 128 bits. - PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2704110051
Re: RFR: 8348986: Improve coverage of enhanced exception messages [v2]
> Hi, > > Enhanced exception messages are designed to hide sensitive information such > as hostnames, IP > addresses from exception message strings, unless the enhanced mode for the > specific category > has been explicitly enabled. Enhanced exceptions were first introduced in > 8204233 in JDK 11 and > updated in 8207846. > > This PR aims to increase the coverage of enhanced exception messages in the > networking code. > A limited number of exceptions are already hidden (restricted) by default. > The new categories and > exceptions in this PR will be restricted on an opt-in basis, ie. the default > mode will be enhanced > (while preserving the existing behavior). > > The mechanism is controlled by the security/system property > "jdk.includeInExceptions" which takes as value > a comma separated list of category names, which identify groups of exceptions > where the exception > message may be enhanced. Any category not listed is "restricted" which means > that potentially > sensitive information (such as hostnames, IP addresses, user identities) are > excluded from the message text. > > The changes to the java.security conf file describe the exact changes in > terms of the categories now > supported and any changes in behavior. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: doc + copyright update - Changes: - all: https://git.openjdk.org/jdk/pull/23929/files - new: https://git.openjdk.org/jdk/pull/23929/files/c4419860..074da251 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=00-01 Stats: 23 lines in 3 files changed: 10 ins; 5 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/23929.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23929/head:pull/23929 PR: https://git.openjdk.org/jdk/pull/23929
Re: RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v13]
> This patch intrinsifies `Math.max(long, long)` and `Math.min(long, long)` in
> order to help improve vectorization performance.
>
> Currently vectorization does not kick in for loops containing either of these
> calls because of the following error:
>
>
> VLoop::check_preconditions: failed: control flow in loop not allowed
>
>
> The control flow is due to the java implementation for these methods, e.g.
>
>
> public static long max(long a, long b) {
> return (a >= b) ? a : b;
> }
>
>
> This patch intrinsifies the calls to replace the CmpL + Bool nodes for
> MaxL/MinL nodes respectively.
> By doing this, vectorization no longer finds the control flow and so it can
> carry out the vectorization.
> E.g.
>
>
> SuperWord::transform_loop:
> Loop: N518/N126 counted [int,int),+4 (1025 iters) main has_sfpt
> strip_mined
> 518 CountedLoop === 518 246 126 [[ 513 517 518 242 521 522 422 210 ]]
> inner stride: 4 main of N518 strip mined !orig=[419],[247],[216],[193] !jvms:
> Test::test @ bci:14 (line 21)
>
>
> Applying the same changes to `ReductionPerf` as in
> https://github.com/openjdk/jdk/pull/13056, we can compare the results before
> and after. Before the patch, on darwin/aarch64 (M1):
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
> 1 1 0 0
> ==
> TEST SUCCESS
>
> long min 1155
> long max 1173
>
>
> After the patch, on darwin/aarch64 (M1):
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
> 1 1 0 0
> ==
> TEST SUCCESS
>
> long min 1042
> long max 1042
>
>
> This patch does not add an platform-specific backend implementations for the
> MaxL/MinL nodes.
> Therefore, it still relies on the macro expansion to transform those into
> CMoveL.
>
> I've run tier1 and hotspot compiler tests on darwin/aarch64 and got these
> results:
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>jtreg:test/hotspot/jtreg:tier1 2500 2500 0 0
>>> jtreg:test/jdk:tier1 ...
Galder Zamarreño has updated the pull request incrementally with one additional
commit since the last revision:
Add simple reduction benchmarks on top of multiply ones
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20098/files
- new: https://git.openjdk.org/jdk/pull/20098/files/a190ae68..d0e793a3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20098&range=12
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20098&range=11-12
Stats: 44 lines in 1 file changed: 40 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/20098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20098/head:pull/20098
PR: https://git.openjdk.org/jdk/pull/20098
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 10:39:27 GMT, snake66 wrote: > Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that it's > possible to parameterize this for platforms that use different flags for > enabling posix threads. > > This work is a continuation of the work done by Greg Lewis in [1], but > generalized for the full JDK, and set at the configure stage. > > Sponsored by: The FreeBSD Foundation > Co-authored-by: Greg Lewis > > [1]: > https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 make/test/JtregNativeHotspot.gmk line 886: > 884: BUILD_HOTSPOT_JTREG_LIBRARIES_JDK_LIBS_libnativeStack := > java.base:libjvm > 885: else > 886: BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libbootclssearch_agent += > $(LIBPTREAD) Hi. Should this read `$(LIBPTHREAD)` instead (i.e., missing `H`)? Could be me too, I need new reading glasses... - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983396617
Re: RFR: 8348986: Improve coverage of enhanced exception messages [v2]
On Thu, 6 Mar 2025 10:46:11 GMT, Michael McMahon wrote:
>> Hi,
>>
>> Enhanced exception messages are designed to hide sensitive information such
>> as hostnames, IP
>> addresses from exception message strings, unless the enhanced mode for the
>> specific category
>> has been explicitly enabled. Enhanced exceptions were first introduced in
>> 8204233 in JDK 11 and
>> updated in 8207846.
>>
>> This PR aims to increase the coverage of enhanced exception messages in the
>> networking code.
>> A limited number of exceptions are already hidden (restricted) by default.
>> The new categories and
>> exceptions in this PR will be restricted on an opt-in basis, ie. the default
>> mode will be enhanced
>> (while preserving the existing behavior).
>>
>> The mechanism is controlled by the security/system property
>> "jdk.includeInExceptions" which takes as value
>> a comma separated list of category names, which identify groups of
>> exceptions where the exception
>> message may be enhanced. Any category not listed is "restricted" which means
>> that potentially
>> sensitive information (such as hostnames, IP addresses, user identities) are
>> excluded from the message text.
>>
>> The changes to the java.security conf file describe the exact changes in
>> terms of the categories now
>> supported and any changes in behavior.
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> doc + copyright update
src/java.base/share/classes/jdk/internal/util/Exceptions.java line 103:
> 101: * the output is the replacement string.
> 102: */
> 103: public static abstract class SensitiveInfo {
Let's sort modifiers in blessed order
Suggestion:
public abstract static class SensitiveInfo {
src/java.base/share/classes/jdk/internal/util/Exceptions.java line 338:
> 336: | hostCompatFlag;
> 337: enhancedUserExceptionText =
> SecurityProperties.includedInExceptions("userInfo");
> 338: enhancedJarExceptionText =
> SecurityProperties.INCLUDE_JAR_NAME_IN_EXCEPTIONS;
Suggestion:
enhancedJarExceptionText =
SecurityProperties.INCLUDE_JAR_NAME_IN_EXCEPTIONS;
test/jdk/sun/net/util/ExceptionsTest.java line 47:
> 45:
> 46: static boolean netEnabled() {
> 47: System.out.printf("netEnabled = %b\n", enhancedNetExceptions());
Suggestion:
System.out.printf("netEnabled = %b\n", enhancedNetExceptions());
test/jdk/sun/net/util/ExceptionsTest.java line 52:
> 50:
> 51: static boolean dnsEnabled() {
> 52: System.out.printf("dnsEnabled = %b\n",
> enhancedLookupExceptions());
Suggestion:
System.out.printf("dnsEnabled = %b\n", enhancedLookupExceptions());
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23929#discussion_r1983229570
PR Review Comment: https://git.openjdk.org/jdk/pull/23929#discussion_r1983232405
PR Review Comment: https://git.openjdk.org/jdk/pull/23929#discussion_r1983232786
PR Review Comment: https://git.openjdk.org/jdk/pull/23929#discussion_r1983232968
Re: RFR: 8348986: Improve coverage of enhanced exception messages [v3]
> Hi, > > Enhanced exception messages are designed to hide sensitive information such > as hostnames, IP > addresses from exception message strings, unless the enhanced mode for the > specific category > has been explicitly enabled. Enhanced exceptions were first introduced in > 8204233 in JDK 11 and > updated in 8207846. > > This PR aims to increase the coverage of enhanced exception messages in the > networking code. > A limited number of exceptions are already hidden (restricted) by default. > The new categories and > exceptions in this PR will be restricted on an opt-in basis, ie. the default > mode will be enhanced > (while preserving the existing behavior). > > The mechanism is controlled by the security/system property > "jdk.includeInExceptions" which takes as value > a comma separated list of category names, which identify groups of exceptions > where the exception > message may be enhanced. Any category not listed is "restricted" which means > that potentially > sensitive information (such as hostnames, IP addresses, user identities) are > excluded from the message text. > > The changes to the java.security conf file describe the exact changes in > terms of the categories now > supported and any changes in behavior. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review from turbanoff review Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/23929/files - new: https://git.openjdk.org/jdk/pull/23929/files/074da251..41d1ef82 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/23929.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23929/head:pull/23929 PR: https://git.openjdk.org/jdk/pull/23929
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 15:33:29 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> iwalulya review >> * comments for variables tracking to-collection-set and just dirtied >> cards after GC/refinement >> * predicate for determining whether the refinement has been disabled >> * some other typos/comment improvements >> * renamed _has_xxx_ref to _has_ref_to_xxx to be more consistent with >> naming > > src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 219: > >> 217: // The young gen revising mechanism reads the predictor and the >> values set >> 218: // here. Avoid inconsistencies by locking. >> 219: MutexLocker x(G1RareEvent_lock, Mutex::_no_safepoint_check_flag); > > Who else can be in this critical-section? I don't get what this lock is > protecting us from. Actually further discussion with @albertnetymk showed that this change introduces an unintended behaviorial change where since the refinement control thread is also responsible for updating the current young gen length. It means that the mutex isn't required. However this means that while the refinement is running this is not done any more, because refinement can take seconds, I need to move this work to another thread (probably the `G1ServiceThread´). I will add a separate mutex then. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1983587293
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 14:21:08 GMT, Erik Joelsson wrote: > What is the intended way of using this? Do you run make with > LIBPTHREAD=-pthread or do you apply a patch on libraries.m4 for the specific > way of linking to pthread? This is in preparation of the upcoming BSD port, which uses `-pthread` instead of `-pthread`. It was me who suggested that this is done separately with the existing code, to minimize the patch of the BSD port. > make/autoconf/libraries.m4 line 142: > >> 140: # Threading library >> 141: if test "x$OPENJDK_TARGET_OS" = xlinux || test "x$OPENJDK_TARGET_OS" >> = xaix; then >> 142: BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)" > > If you specifically need this to be resolved in the makefile rather than > here, then please add a comment explaining why, otherwise use a shell script > variable reference. > > Suggestion: > > BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD" Yes, this is incorrect. Remember that m4 are shell scripts so you need to use shell syntax here. (I know it is confusing). - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2704233217 PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983608153
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 10:39:27 GMT, snake66 wrote: > Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that it's > possible to parameterize this for platforms that use different flags for > enabling posix threads. > > This work is a continuation of the work done by Greg Lewis in [1], but > generalized for the full JDK, and set at the configure stage. > > Sponsored by: The FreeBSD Foundation > Co-authored-by: Greg Lewis > > [1]: > https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 make/Hsdis.gmk line 131: > 129: HSDIS_TOOLCHAIN_LIBS := $(MINGW_DLLCRT) -lmingw32 -lgcc -lgcc_eh > -lmoldname \ > 130: -lmingwex -lmsvcrt $(LIBPTHREAD) -ladvapi32 -lshell32 -luser32 > -lkernel32 > 131: else The hsdis build is very weird and outside the normal integrated JDK build. I recommend you leave this instance alone. If you want to port hsdis to BSD you are likely to have to rewrite large parts of the compilation logic here anyway. - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983621784
Re: RFR: 8351344: Avoid explicit Objects.requireNonNull in String.join
On Thu, 20 Feb 2025 09:30:02 GMT, Andrey Turbanov wrote: > We have helpful NPE messages now - they are more user-friendly. > And shorter methods are more likely to be inlined. While having prerequirements checks often is good, I think not having it in String.join can be a good thing. 1. String.join is quite short method. 11 lines. And having 2 lines to have _excessive_ check don't improve reading experience much. 2. There are other overloads which don't have null check. I personally like them more - they are easier to read. 3. `String` is one of core classes, and having a bit shorter and more performant code will never be redundant in it. - PR Comment: https://git.openjdk.org/jdk/pull/23710#issuecomment-2704832981
Re: RFR: 8351344: Avoid explicit Objects.requireNonNull in String.join
On Thu, 20 Feb 2025 09:30:02 GMT, Andrey Turbanov wrote: > We have helpful NPE messages now - they are more user-friendly. > And shorter methods are more likely to be inlined. To confirm your intuition, please show the before/after performance differences if any. - PR Comment: https://git.openjdk.org/jdk/pull/23710#issuecomment-2704887884
Re: RFR: 8351344: Avoid explicit Objects.requireNonNull in String.join
On Thu, 6 Mar 2025 20:01:47 GMT, Andrey Turbanov wrote:
>> src/java.base/share/classes/java/lang/String.java line 3649:
>>
>>> 3647: Iterable elements) {
>>> 3648: Objects.requireNonNull(delimiter);
>>> 3649: Objects.requireNonNull(elements);
>>
>> Hello Andrey, I have a different opinion about this. I think removing
>> existing calls to `Objects.requireNonNull()` in favour of implicit null
>> check (some times too far away in the code) isn't useful.
>>
>> You noted that the implicit `NullPointerException` stacktrace are more user
>> friendly now. I think you can get similar level of useful information from
>> `Objects.requireNonNull()` too. In these two specific cases, what you could
>> perhaps do is change those calls to even pass the `message` so that it's
>> clear what parameter is `null`. Something like:
>>
>> Objects.requireNonNull(delimiter, "delimiter");
>> Objects.requireNonNull(elements, "elements");
>
> Should we add it to other overloads of `join` then?
I agree that the easier to read explicit method calls to validate non-nullness
should be left in place.
> Hello Andrey, I have a different opinion about this. I think removing
> existing calls to `Objects.requireNonNull()` in favour of implicit null check
> (some times too far away in the code) isn't useful.
>
> You noted that the implicit `NullPointerException` stacktrace are more user
> friendly now. I think you can get similar level of useful information from
> `Objects.requireNonNull()` too. In these two specific cases, what you could
> perhaps do is change those calls to even pass the `message` so that it's
> clear what parameter is `null`. Something like:
>
> ```java
> Objects.requireNonNull(delimiter, "delimiter");
> Objects.requireNonNull(elements, "elements");
> ```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23710#discussion_r1984036994
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files [v3]
On Thu, 6 Mar 2025 12:26:50 GMT, Alexey Semenyuk wrote:
>> src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/TokenReplace.java
>> line 112:
>>
>>> 110: regexps = new ArrayList<>();
>>> 111:
>>> 112: for(;;) {
>>
>> Suggestion:
>>
>> for (;;) {
>
>> If you do echo on macOS in terminal for unset variable it will be expanded
>> to empty string, so for "Unset variables are not expanded." should we have
>> Welcome ! instead of Welcome $USER!?
>
> This is true for any shell. It is also true that some shells fail to expand
> undefined variables if configured accordingly. Bash, for example:
>
> bash -c 'FOO=10; echo FOO=$FOO; unset FOO; set -u; echo FOO=$FOO'
>
>
> output:
>
> FOO=10
> bash: line 1: FOO: unbound variable
>
>
> .cfg file is not a script, and the jpackage app launcher is not a shell. The
> substitution performed is less of a variable expansion and more a token
> replacement, and a token is not replaced if it doesn't have a value.
>
> Regardless of the handling of undefined variables, users have to deal with
> the case of undefined variables in .cfg files. Imagine they pass
> `--java-options -DmyAppData=$HOME/.myData` to jpackage and `HOME` happens to
> be undefined on the machine where the app runs. Is `/.myData` any better than
> `$HOME/.myData`?
Agree. Makes sense.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23923#discussion_r1984101788
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files [v3]
On Thu, 6 Mar 2025 21:51:37 GMT, Alexander Matveev wrote: >>> If you do echo on macOS in terminal for unset variable it will be expanded >>> to empty string, so for "Unset variables are not expanded." should we have >>> Welcome ! instead of Welcome $USER!? >> >> This is true for any shell. It is also true that some shells fail to expand >> undefined variables if configured accordingly. Bash, for example: >> >> bash -c 'FOO=10; echo FOO=$FOO; unset FOO; set -u; echo FOO=$FOO' >> >> >> output: >> >> FOO=10 >> bash: line 1: FOO: unbound variable >> >> >> .cfg file is not a script, and the jpackage app launcher is not a shell. The >> substitution performed is less of a variable expansion and more a token >> replacement, and a token is not replaced if it doesn't have a value. >> >> Regardless of the handling of undefined variables, users have to deal with >> the case of undefined variables in .cfg files. Imagine they pass >> `--java-options -DmyAppData=$HOME/.myData` to jpackage and `HOME` happens to >> be undefined on the machine where the app runs. Is `/.myData` any better >> than `$HOME/.myData`? > > Agree. Makes sense. Oh, I thought I posted my reply in the main thread and then didn't find it there, assumed my comment was lost and wrote it again 🤦 - PR Review Comment: https://git.openjdk.org/jdk/pull/23923#discussion_r1984135948
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 13:53:31 GMT, Antonio Vieiro wrote: >> Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that >> it's possible to parameterize this for platforms that use different flags >> for enabling posix threads. >> >> This work is a continuation of the work done by Greg Lewis in [1], but >> generalized for the full JDK, and set at the configure stage. >> >> Sponsored by: The FreeBSD Foundation >> Co-authored-by: Greg Lewis >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > make/test/JtregNativeHotspot.gmk line 886: > >> 884: BUILD_HOTSPOT_JTREG_LIBRARIES_JDK_LIBS_libnativeStack := >> java.base:libjvm >> 885: else >> 886: BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libbootclssearch_agent += >> $(LIBPTREAD) > > Hi. Should this read `$(LIBPTHREAD)` instead (i.e., missing `H`)? > Could be me too, I need new reading glasses... You're absolutely right. @snake66 Making changes to makefiles is tricky, since a misspelled variable do not get any warning but is just silently ignored. For a change like this, that is a pure refactoring that is not supposed to change the output, I highly recommend using the `COMPARE_BUILD` system. Unfortunately, this is severely underdocumented. There is a short paragraph at https://openjdk.org/groups/build/doc/building.html under "Developing the Build System Itself", but in short, what I'd do is to create a diff files of these changes compared to a baseline (e.g. master, a specific build or commit), make sure you have the baseline checked out, and then run `make jdk-image test-image COMPARE_BUILD=PATCH=my_patch.diff`. This will build the targets twice, one without the patch and one with the patch, and then automatically run the `compare` script to analyze any differences. For this particular patch, there should be none. This would likely have caught this typo. (Given that the test-image is actually compared; I suddenly got a bit uncertain about that...) - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983618250
Re: RFR: 8350909: [JMH] test ThreadOnSpinWaitShared failed for 2 threads config
On Fri, 28 Feb 2025 01:20:44 GMT, Vladimir Ivanov wrote: > The scope was updated to support multithread configuration (jmh option '-t > 2') . No other changes needed. LGTM. - Marked as reviewed by jbhateja (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23834#pullrequestreview-2665031670
Re: RFR: 8346118: Improve whitespace normalization in preformatted text [v4]
> Please review an enhancement to make `DocCommentParser` normalize whitespace
> inside `` elements. The normalization is conceptually simple and and
> intended to be minimally invasive. Before parsing, `DocCommentParser` checks
> whether the text is a traditional doc comment and whether every line starts
> with a space character, which is commonly the case in traditional doc
> comments. If so, a single leading space is removed in block content (top
> level text and `{@code}`/`{@literal}` tags) when parsing within HTML ``
> tags.
>
> This fixes the incidental one-space indentation in the vast majority of JDK
> code samples using `` alone or in combination with `` or
> `{@code}`. In fact, I only found one code sample in JDK code that isn't
> solved by this change, for which I included a fix in this PR (it's in
> `String.startsWith(String, int)`, where I replaced the 10 char indentation
> and trailing line with a ``).
>
> The many added `boolean inBlockContent` arguments pased around in
> `DocCommentParser` are to make sure the removal is not applied to multiline
> inline content, which is maybe a bit fussy considering there is not a lot of
> multiline inline content in `` tags and it usually would not mind about
> removal of a non-essential space character, but I wanted to keep the change
> minimal. There are few javadoc tests that had to be adapted, most of the
> testing is done in `test/langtools/tools/javac/doctree`.
>
> If the exact number of leading whitespace in `` tags is important to any
> javadoc user the old output can be restored by increasing the indentation by
> 1. There will be a release note for this of course.
>
> Unfortunately, there is another whitespace problem that can't be solved as
> easily, and that is a leading blank line caused by `\n` open tags.
> Browsers will [ignore a newline immediately following a `` tag][1], but
> not if there is a `` tag in between. There are hundreds of occurrences
> of this in JDK code, including variants with space characters mixed in. The
> fix in javadoc proper would be too complex, so I decided to solve it with 3
> lines of JavaScript and a regex to reverse the order of `\n` at the
> beginning of `` tags while removing any intermediary space. Script
> operation is indiscernible and it solves the problem.
>
> [1]: https://html.spec.whatwg.org/#the-pre-element:the-pre-element
Hannes Wallnöfer has updated the pull request incrementally with one additional
commit since the last revision:
Fix comment
-
Changes:
- all: https://git.openjdk.org/jdk/pull/23868/files
- new: https://git.openjdk.org/jdk/pull/23868/files/8a9036d6..69464173
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23868&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23868&range=02-03
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/23868.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23868/head:pull/23868
PR: https://git.openjdk.org/jdk/pull/23868
Re: RFR: 8346118: Improve whitespace normalization in preformatted text [v3]
> Please review an enhancement to make `DocCommentParser` normalize whitespace
> inside `` elements. The normalization is conceptually simple and and
> intended to be minimally invasive. Before parsing, `DocCommentParser` checks
> whether the text is a traditional doc comment and whether every line starts
> with a space character, which is commonly the case in traditional doc
> comments. If so, a single leading space is removed in block content (top
> level text and `{@code}`/`{@literal}` tags) when parsing within HTML ``
> tags.
>
> This fixes the incidental one-space indentation in the vast majority of JDK
> code samples using `` alone or in combination with `` or
> `{@code}`. In fact, I only found one code sample in JDK code that isn't
> solved by this change, for which I included a fix in this PR (it's in
> `String.startsWith(String, int)`, where I replaced the 10 char indentation
> and trailing line with a ``).
>
> The many added `boolean inBlockContent` arguments pased around in
> `DocCommentParser` are to make sure the removal is not applied to multiline
> inline content, which is maybe a bit fussy considering there is not a lot of
> multiline inline content in `` tags and it usually would not mind about
> removal of a non-essential space character, but I wanted to keep the change
> minimal. There are few javadoc tests that had to be adapted, most of the
> testing is done in `test/langtools/tools/javac/doctree`.
>
> If the exact number of leading whitespace in `` tags is important to any
> javadoc user the old output can be restored by increasing the indentation by
> 1. There will be a release note for this of course.
>
> Unfortunately, there is another whitespace problem that can't be solved as
> easily, and that is a leading blank line caused by `\n` open tags.
> Browsers will [ignore a newline immediately following a `` tag][1], but
> not if there is a `` tag in between. There are hundreds of occurrences
> of this in JDK code, including variants with space characters mixed in. The
> fix in javadoc proper would be too complex, so I decided to solve it with 3
> lines of JavaScript and a regex to reverse the order of `\n` at the
> beginning of `` tags while removing any intermediary space. Script
> operation is indiscernible and it solves the problem.
>
> [1]: https://html.spec.whatwg.org/#the-pre-element:the-pre-element
Hannes Wallnöfer has updated the pull request incrementally with one additional
commit since the last revision:
Switch to post-processing approach that fixes both problems
This implements a visitor-based post-processing method in
DocCommentParser that is able to fix both kinds of whitespace
common in traditional doc comments. Tests have to be adapted
to the additional normalization step. A few more tests should
be added on the javadoc side.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/23868/files
- new: https://git.openjdk.org/jdk/pull/23868/files/d2128b3a..8a9036d6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23868&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23868&range=01-02
Stats: 304 lines in 4 files changed: 200 ins; 63 del; 41 mod
Patch: https://git.openjdk.org/jdk/pull/23868.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23868/head:pull/23868
PR: https://git.openjdk.org/jdk/pull/23868
Re: RFR: 8351344: Avoid explicit Objects.requireNonNull in String.join
On Thu, 20 Feb 2025 09:30:02 GMT, Andrey Turbanov wrote: > We have helpful NPE messages now - they are more user-friendly. > And shorter methods are more likely to be inlined. I second @jaikiran in that the `Objects.requireNonNull()` are there to clearly validate the arguments right at method entry. - PR Comment: https://git.openjdk.org/jdk/pull/23710#issuecomment-2704726863
Re: RFR: 8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files [v3]
On Thu, 6 Mar 2025 15:25:13 GMT, Alexey Semenyuk wrote:
>> jpackage app laucnher will expand environment variables in .cfg files.
>>
>> Previously jpackage app launcher only replaced `$APPDIR`, `$BINDIR`, and
>> `$ROOTDIR` tokens with the corresponding path values. With this patch, any
>> environment variable can be expanded. The syntax is shell-like
>> `$ENV_VAR_NAME` or `${ENV_VAR_NAME}`. If `$ENV_VAR_NAME` syntax is used, the
>> variable name stops at the first character outside of `[a-zA-Z0-9_]`
>> character range. If `${ENV_VAR_NAME}` syntax is used, the variable name
>> stops at the first `}` character after `${` substring. E.g:
>> | String| Variables | Variable Values | Expanded string | Note |
>> | | --- |--- |--- |--- |
>> | Welcome $USER! | USER | USER=John |
>> Welcome John! ||
>> | Welcome $USER! | USER | not set |
>> Welcome $USER! | Unset variables are not expanded. |
>> | Welcome $USER2 | USER2 | USER2=John |
>> Welcome John! ||
>> | Welcome ${USER}2 | USER | USER=John |
>> Welcome John2! ||
>> | Welcome $USER-2 | USER | USER=John |
>> Welcome John-2! ||
>> | Welcome ${USER-2} | USER-2 |
>> USER-2=John | Welcome John! | `USER-2` is likely to be
>> an invalid env variable name, but jpackage launcher is not validating names.
>> |
>>
>> `$` character combination prevents variable expansion:
>> | String| Variables | Variable Values | Expanded string |
>> | | --- |--- |--- |
>> | Hello \$A! Bye $B | B | B=John | Hello $A! Bye
>> John |
>> | Hello \${A}! Bye $B | B | B=John | Hello ${A}!
>> Bye John |
>>
>> `\` character combination escapes ``:
>> | String| Variables | Variable Values | Expanded string |
>> | | --- |--- |--- |
>> | Hello \\\$A! Bye $B | A, B | A=AnaB=John
>> | Hello \\Ana! Bye John |
>>
>> If `` character is not followed by another `` character or `$` character, it
>> is interpreted as a regular character:
>> | String| Expanded string |
>> | | --- |
>> |a\\b\\c|a\\b\\c|
>>
>>
>> Expansion is non-recursive:
>> | String| Variables | Variable Values | Expanded string | Note |
>> | | --- |--- |--- |--- |
>> | Hello $A! | A | A=$B...
>
> Alexey Semenyuk has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Merge branch 'JDK-8341641-master' of
> https://github.com/alexeysemenyukoracle/jdk into JDK-8341641-master
> - Formatting fixed
This is true for any shell behavior. It is also true that some shells fail to
expand undefined variables if configured accordingly. Bash, for example:
bash -c 'FOO=10; echo FOO=$FOO; unset FOO; set -u; echo FOO=$FOO'
Output:
FOO=10
bash: line 1: FOO: unbound variable
.cfg files are not scripts and jpackage app launcher is not an interpreter.
Performed string substitution is less variable expansion and more token
replacement; if token value is not available, it is not replaced.
Regardless of handling undefined variables, users have to deal with the
consequences. Say they pass `--java-options -DmyAppData=$HOME/.myData` option
to jackage and `HOME` variable happens undefined on a machine where installed
app is launched. How would `/.myData` be better than `$HOME/.myData`? At least
it is clear something went wrong in the second case.
-
PR Comment: https://git.openjdk.org/jdk/pull/23923#issuecomment-2704734396
Re: RFR: 8350909: [JMH] test ThreadOnSpinWaitShared failed for 2 threads config
On Fri, 28 Feb 2025 01:20:44 GMT, Vladimir Ivanov wrote: > The scope was updated to support multithread configuration (jmh option '-t > 2') . No other changes needed. @IvaVladimir Your change (at version 2b70cd408597bbfd0a2f490714a7ebab8eceb1f6) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/23834#issuecomment-2704539712
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v13]
On Wed, 5 Mar 2025 10:41:02 GMT, Ivan Walulya wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> * fix whitespace >> * additional whitespace between log tags >> * rename G1ConcurrentRefineWorkTask -> ...SweepTask to conform to the >> other similar rename > > src/hotspot/share/gc/g1/g1ThreadLocalData.hpp line 29: > >> 27: #include "gc/g1/g1BarrierSet.hpp" >> 28: #include "gc/g1/g1CardTable.hpp" >> 29: #include "gc/g1/g1CollectedHeap.hpp" > > probably does not need to be included `g1CardTable.hpp` needed because of `G1CardTable::CardValue` I think. I removed the 'G1CollectedHeap` include though. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1983655594
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 17:28:10 GMT, snake66 wrote: >> make/autoconf/libraries.m4 line 142: >> >>> 140: # Threading library >>> 141: if test "x$OPENJDK_TARGET_OS" = xlinux || test "x$OPENJDK_TARGET_OS" >>> = xaix; then >>> 142: BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)" >> >> If you specifically need this to be resolved in the makefile rather than >> here, then please add a comment explaining why, otherwise use a shell script >> variable reference. >> >> Suggestion: >> >> BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD" > > @erikj79 There will be a later patch to libraries.m4 that sets the variable > based on the target platform, and then populates the `LIBPTHREAD` variable in > spec.gmk. > (https://github.com/openjdk/jdk/pull/23930/files#diff-56172cd2ec5804a5f764a6d0d5970da6144b024a06e008571f9822b2dc83cc36R147) > That means the parenthesis should stay, right? I'm not sure what you mean now. The link is to this very patch, which does what you describe -- sets LIBPTHREAD according to OS and stores it in spec.gmk. And no, the paranthesis should not stay. If you keep them, the variable will be re-evaluated in make every time BASIC_JVM_LIBS is evaluated. That is not needed; by dropping the parenthesis the actual value to be used will be inserted as a string constant. Which is what we want, since we know the value at configure time. - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983853576
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 14:15:38 GMT, Erik Joelsson wrote: >> Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that >> it's possible to parameterize this for platforms that use different flags >> for enabling posix threads. >> >> This work is a continuation of the work done by Greg Lewis in [1], but >> generalized for the full JDK, and set at the configure stage. >> >> Sponsored by: The FreeBSD Foundation >> Co-authored-by: Greg Lewis >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > make/autoconf/libraries.m4 line 142: > >> 140: # Threading library >> 141: if test "x$OPENJDK_TARGET_OS" = xlinux || test "x$OPENJDK_TARGET_OS" >> = xaix; then >> 142: BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)" > > If you specifically need this to be resolved in the makefile rather than > here, then please add a comment explaining why, otherwise use a shell script > variable reference. > > Suggestion: > > BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD" @erikj79 There will be a later patch to libraries.m4 that sets the variable based on the target platform, and then populates the `LIBPTHREAD` variable in spec.gmk. (https://github.com/openjdk/jdk/pull/23930/files#diff-56172cd2ec5804a5f764a6d0d5970da6144b024a06e008571f9822b2dc83cc36R147) That means the parenthesis should stay, right? - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983779126
Re: RFR: 8350460: org.openjdk.bench.vm.floatingpoint.DremFrem JMH fails with -ea [v2]
> The normal SQE process runs all the repo JMH with -ea to get the last bit of > extra testing. This DremFrem JMH contained some asserts that would always > fire on the correct answer, disturbing this normal SQE process. I removed a > lot more asserts from this JMH which seemed to make it more of a SQE test > than a benchmark. We would prefer to keep benchmarks as benchmarks as much as > possible and not creep into tests. Eric Caspole has updated the pull request incrementally with one additional commit since the last revision: Fix header and remove unused Blackhole - Changes: - all: https://git.openjdk.org/jdk/pull/23917/files - new: https://git.openjdk.org/jdk/pull/23917/files/8c882666..649957dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23917&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23917&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/23917.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23917/head:pull/23917 PR: https://git.openjdk.org/jdk/pull/23917
Re: RFR: 8350460: org.openjdk.bench.vm.floatingpoint.DremFrem JMH fails with -ea
On Wed, 5 Mar 2025 16:40:55 GMT, Chen Liang wrote: >> The normal SQE process runs all the repo JMH with -ea to get the last bit of >> extra testing. This DremFrem JMH contained some asserts that would always >> fire on the correct answer, disturbing this normal SQE process. I removed a >> lot more asserts from this JMH which seemed to make it more of a SQE test >> than a benchmark. We would prefer to keep benchmarks as benchmarks as much >> as possible and not creep into tests. > > Also might need to add an 2025 copyright header for this file. Thanks @liach I removed the unused Blackhole and updated the copyright header. - PR Comment: https://git.openjdk.org/jdk/pull/23917#issuecomment-2704796397
Withdrawn: 8345687: Improve the implementation of SegmentFactories::allocateSegment
On Fri, 6 Dec 2024 16:30:47 GMT, Quan Anh Mai wrote: > Hi, > > This patch improves the performance of a typical `Arena::allocate` in several > ways: > > - Delay the creation of the NativeMemorySegmentImpl. This avoids the merge of > the instance with the one obtained from the call in the uncommon path, > increasing the chance the object being scalar replaced. > - Split the allocation of over-aligned memory to a slow-path method. > - Align the memory to 8 bytes, allowing faster zeroing. > - Use a dedicated method to zero the just-allocated native memory, reduce > code size and make it more straightforward. > - Make `VM.pageAlignDirectMemory` a `Boolean` instead of a `boolean` so that > `false` value can be constant folded. > > Please take a look and leave your reviews, thanks a lot. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/22610
