date:20240513

On Mon, 13 May 2024 09:56:35 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 17 additional 
> commits since the last revision:
> 
>  - Escape prompt
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Clarify input charset
>  - Make IO final
>  - Fix System.console().readln(null) in jshell
>
>Without it, jshell hangs on me. Will think of a test.
>  - Fix typo
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Simplify output.exp
>  - Cover null prompt in input tests
>  - Make input test parametric
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/8b9c719f...17100ab8

Marked as reviewed by smarks (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2054056729

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]

On Mon, 13 May 2024 21:32:18 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addresses a review comment

Marked as reviewed by smarks (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19184#pullrequestreview-2054044199

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

2024-05-13 Thread ExE Boss

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

src/hotspot/share/prims/nativeLookup.cpp line 275:

> 273: 
> 274:   // Otherwise call static method findNative in ClassLoader
> 275: 

Suggestion:

src/hotspot/share/prims/nativeLookup.cpp line 419:

> 417:   if (entry != nullptr) return entry;
> 418: 
> 419: 

Suggestion:

src/hotspot/share/prims/nativeLookup.cpp line 426:

> 424:   return nullptr;
> 425: }
> 426:   }

Suggestion:

  }

src/java.base/share/classes/java/lang/Module.java line 331:

> 329: String modflag = isNamed() ? getName() : "ALL-UNNAMED";
> 330: String caller = currentClass != null ? 
> currentClass.getName() : "code";
> 331: System.err.printf("""

This message should probably be different when linking native methods, since 
otherwise it’ll be:

WARNING: A restricted method in foo has been called
WARNING: bar has been called by Baz in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for callers in this 
module
WARNING: Restricted methods will be blocked in a future release unless native 
access is enabled


when it should really be something like:

WARNING: A JNI native method in foo has been linked
WARNING: bar has been linked in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for native methods 
in this module
WARNING: Native methods will be blocked in a future release unless native 
access is enabled

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248442
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248501
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248577
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599253428

Re: RFR: 8331907: BigInteger and BigDecimal should use optimized division [v4]

2024-05-13 Thread Brian Burkhalter

On Fri, 10 May 2024 16:16:18 GMT, Daniel Jeliński  wrote:

>> Replace the custom unsigned divide / remainder functions with the 
>> compiler-optimized Long.divideUnsigned / remainderUnsigned.
>> 
>> No new tests. Existing tier1-3 tests continue to pass.
>> 
>> I added a new set of divide benchmarks. Results in thread.
>> 
>> I also removed the BigDecimal.toString benchmarks. They no longer serve 
>> their purpose - toString caches the returned value, so we were only 
>> benchmarking the cache access time.
>
> Daniel Jeliński has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Integer divide

I have nothing to add to the comments already resolved. +1

-

Marked as reviewed by bpb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19134#pullrequestreview-2054040985

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

2024-05-13 Thread Sandhya Viswanathan

On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rearrange; add lambdas for clarity

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1083:

> 1081: // haystack - the address of the first byte of the haystack
> 1082: // hsLen - the sizeof the haystack
> 1083: // isU - true if argument encoding is either UU or UL

We need to list needleLen here as well?

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1096:

> 1094:MacroAssembler *_masm) {
> 1095: 
> 1096:   assert_different_registers(eq_mask, haystack, needleLen, rTmp, hsLen, 
> r10);

r10 kind of stands out here. You could say nMinusK in this assert.
The assert following to this one is checking for nMinusK==r10 so that should 
suffice.
BTW, didn't see anything in the code below that needs nMinuxK to be r10.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1120:

> 1118: #define cmp_0 XMM_TMP3
> 1119: #undef cmp_k
> 1120: #define cmp_k XMM_TMP4

XMM_TMP4 is not reused so cmp_k could be declared as const. In general limiting 
undef/define pair only to reused registers would make the review easier.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1125:

> 1123: #undef lastMask
> 1124: 
> 1125:   int sizeIncr = isU ? 2 : 1;

sizeIncr and scale seems to be same, we could just use one of them in this 
function.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1178:

> 1176: __ andq(eq_mask, lastMask);
> 1177: if (needToSaveRCX) {
> 1178:   __ movdq(rcx, saveRCX);

movdq is an expensive instruction (about 3 cycle). If we have another gpr 
temporary available here for shiftVal, then we dont need to do save/restore rcx.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1183:

> 1181: 
> 1182: if (bytesToCompare > 2) {
> 1183:   if (size > (isU ? 4 : 2)) {

this and other usages could be simplified to: size > 2 * scale

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599201163
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599203881
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599211645
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599202848
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599242323
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599228299

Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]

2024-05-13 Thread Julian Waters

On Thu, 9 May 2024 07:50:00 GMT, Julian Waters  wrote:

>> WIP
>> 
>> This changeset contains hsdis for Windows/gcc Port. It supports both the 
>> binutils and capstone backends, though the LLVM backend is left out due to 
>> compatibility issues encountered during the build. Currently, which gcc 
>> distributions are supported is still to be clarified, as several, ranging 
>> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, 
>> the build system hack in place at the moment to compile the binutils backend 
>> on Windows is still left in place, for now.
>
> Julian Waters has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Add check for compiler in configure
>  - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis

Hmm, it seems Magnus isn't available at the moment. @erikj79 might you be able 
to answer my questions regarding hsdis?

-

PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2109049333

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException [v2]

> This change adds wrapping of the CancellationException produced by 
> CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
> information and align better with FutureTask.
> 
> Running the sample code from the JBS issue in JShell will produce the 
> following:
> 
> 
> jshell> java.util.concurrent.CancellationException: 
>   at 
> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>   at 
> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>   at REPL.$JShell$18.m2($JShell$18.java:10)
>   at REPL.$JShell$17.m1($JShell$17.java:8)
>   at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>   at java.base/java.lang.Thread.run(Thread.java:1575)
> Caused by: java.util.concurrent.CancellationException
>   at 
> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>   at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>   ... 1 more

Viktor Klang has updated the pull request incrementally with one additional 
commit since the last revision:

  Adding CancellationException detail message to CompletableFuture

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19219/files
  - new: https://git.openjdk.org/jdk/pull/19219/files/641bbfe3..078930ab

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19219&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19219&range=00-01

  Stats: 14 lines in 2 files changed: 0 ins; 0 del; 14 mod
  Patch: https://git.openjdk.org/jdk/pull/19219.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19219/head:pull/19219

PR: https://git.openjdk.org/jdk/pull/19219

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException [v2]

On Mon, 13 May 2024 18:16:55 GMT, Doug Lea  wrote:

>> src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 
>> 392:
>> 
>>> 390: return null;
>>> 391: if (x instanceof CancellationException)
>>> 392: throw new CancellationException("", 
>>> (CancellationException)x);
>> 
>> One option here would be to put "CompletableFuture.get()" or "get()" as a 
>> message.
>
> Given the serviceability motivation, the overkill of adding "get" and "join" 
> strings seems reasonable.

@DougLea Yeah, I think that's fair enough. I pushed a commit which allows for 
customization of the details message.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1599221161

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

2024-05-13 Thread Volodymyr Paprotski

On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rearrange; add lambdas for clarity

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4492:

> 4490: 
> 4491: // Compare char[] or byte[] arrays aligned to 4 bytes or substrings.
> 4492: void C2_MacroAssembler::arrays_equals(bool is_array_equ, Register ary1,

I liked the old style better, fewer longer lines.. same for rest of the changes 
in this file.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4594:

> 4592: #endif //_LP64
> 4593: bind(COMPARE_WIDE_VECTORS);
> 4594: vmovdqu(vec1, Address(ary1, limit,

create a local scale variable instead of ternary operators. Used several times.

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4250:

> 4248:   generate_chacha_stubs();
> 4249: 
> 4250:   if ((UseAVX == 2) && EnableX86ECoreOpts && 
> VM_Version::supports_avx2()) {

Just `if (EnableX86ECoreOpts)`?

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 391:

> 389:   }
> 390: 
> 391:   __ cmpq(needle_len, isU ? 2 : 1);

Can we remove this comparison? i.e.
- broadcast first and last character unconditionally (same character). Or
- move broadcasts 'down' into individual cases..
There is already specialized code to handle needle of size 1.. This adds extra 
pathlength. (Will we actually call this intrinsic for needle_size==1? Assume 
length>=2?)

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1365:

> 1363:   // Compare first byte of needle to haystack
> 1364:  vpcmpeq(cmp_0, byte_0, Address(haystack, 0), 
> Assembler::AVX_256bit);
> 1365:   if (size != (isU ? 2 : 1)) {

`if (size != scale)`

Though in this case, `elem_size` might hold more meaning.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1372:

> 1370: 
> 1371: if (bytesToCompare > 2) {
> 1372:   if (size > (isU ? 4 : 2)) {

`if (size > 2*scale)`?

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1373:

> 1371: if (bytesToCompare > 2) {
> 1372:   if (size > (isU ? 4 : 2)) {
> 1373: if (doEarlyBailout) {

Is there a big perf difference when `doEarlyBailout` is enabled? And/or just 
for this function?

(i.e. removing `doEarlyBailout` in this function will mean less pathlength. 
Feels like a few extra vpands should be cheap enough.)

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1469:

> 1467: 
> 1468:   if (isU && (size & 1)) {
> 1469: __ emit_int8(0xcc);

This should also be an `assert()` to catch this at compile-time.

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1633:

> 1631:   if (isU) {
> 1632: if ((size & 1) != 0) {
> 1633:   __ emit_int8(0xcc);

Compile-time assert to ensure this code is never called instead?

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1889:

> 1887: //  r13 = (needle length - 1)
> 1888: //  r14 =

Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

2024-05-13 Thread Sandhya Viswanathan

On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rearrange; add lambdas for clarity

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1054:

> 1052: } else if (isUL) {
> 1053:   __ movzbl(rTmp, Address(needle, 2));
> 1054:   __ movdl(byte_1, rTmp);

Should be: __ movdl(byte_2, rTmp);

src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1056:

> 1054:   __ movdl(byte_1, rTmp);
> 1055:   // 1st byte of needle in words
> 1056:   __ vpbroadcastw(byte_1, byte_1, Assembler::AVX_256bit);

Should be:
__ vpbroadcastw(byte_2, byte_2, Assembler::AVX_256bit);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599194092
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1599194375

Moving core reflection's Signature parsing to Class-File API

2024-05-13 Thread -

Hello,
Class-File API has a convenient Signature model API, which can replace the
Generic Tree internal API in the current core reflection implementation.
The old internal Tree API + visitor pattern was bloated with a lot of
unnecessary classes, which can be removed with the new CF API.

A working implementation with all tier 1 tests passing is available at
https://github.com/openjdk/jdk/compare/master...liachmodded:jdk:feature/new-generic-info?expand=1
.

In my implementation:
 - reflectiveObjects (classes directly implementing ParameterizedType,
etc.) remain; they are exported, and their implementation is sufficient
as-is.
 - all other old generic classes are removed
 - few existing usages of old generic code moved to BytecodeDescriptor, use
sites converted to throw GenericSignatureFormatError (preexisting behavior)
instead of IllegalArgumentException
 - Just 5 new classes are added to replaced all removed old classes.

Given the size of this patch, I decide to start a discussion on the mailing
lists on this proposal. Is there any shortcomings with this patch, or is
there anything that I should be cautious of for this patch?

Best regards,
Chen Liang

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replaced another unused exception with _

Right, making the field volatile is a much safer solution.

-

PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2108832766

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]

> Making sure `restoreEcho` correctly reflects the state in the shutdown 
> thread, which differs from the application's thread that issues the 
> `readPassword()` method.

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Addresses a review comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19184/files
  - new: https://git.openjdk.org/jdk/pull/19184/files/3e7d2e0c..fb5beb11

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=02-03

  Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/19184.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184

PR: https://git.openjdk.org/jdk/pull/19184

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replaced another unused exception with _

Hm, I don't think we want to add any synchronized blocks within a shutdown 
hook. If a thread is blocked reading from the console, it will hold readLock; 
if the JVM is then shut down using a signal, it will run shutdown hooks, and 
this hook will block awaiting readLock. This can deadlock the shutdown 
completely.

To ensure proper memory visibility, maybe consider making `restoreEcho` be 
volatile.

-

PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2108811013

Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]

On Sun, 12 May 2024 07:27:26 GMT, Alan Bateman  wrote:

>> If an event class is loaded before JFR is started, the event class needs to 
>> be retransformed, but if it is loaded later, we can add instrumentation on 
>> class load and avoid the retransformation. More happens when an event class 
>> is loaded compared to ordinary class load, for example, a startTime field is 
>> added.
>> 
>> I did a JMH run and the difference between Event::enabled() and a boolean 
>> flag was a fraction of nanosecond.
>
>> If an event class is loaded before JFR is started, the event class needs to 
>> be retransformed, but if it is loaded later, we can add instrumentation on 
>> class load and avoid the retransformation. More happens when an event class 
>> is loaded compared to ordinary class load, for example, a startTime field is 
>> added.
>> 
>> I did a JMH run and the difference between Event::enabled() and a boolean 
>> flag was a fraction of nanosecond.
> 
> There are instances of FIS/FOS created in initPhase1 for the standard streams 
> so loading as few classes and executing as minimal as possible is good. RAF 
> will typically be used early too as the zip code opens zip files with a RAF. 
> So doing as little as possible is good.

My main concern here is the addition of clutter (checking two flags, and 
creating two levels of nested "impl" methods) at every call site. We may need 
to rearrange our internal API for JFR (jdk.internal.events) in order to clean 
up the call sites without loading additional classes unnecessarily.

I think the main performance comparison to make is the impact on startup time 
of loading the additional FileReadEvent class. That is, to compare the startup 
time of the baseline with code that loads the FileReadEvent class, with JFR 
disabled in both cases. I don't know how to do this. Anyway, if loading of 
additional classes imposes unacceptable overhead, then that justifies doing 
more work to avoid it. That's separate from whether adding the `jfrTracing` 
flag as done in this PR is an effective way to do it.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1599077056

Re: RFR: 8326951: Missing `@since` tags [v9]

> I added `@since` tags for methods/constructors that do not match the `@since` 
> of the enclosing class.
> 
> The `write` method already existed in `PrintStream` in earlier versions and 
> instances of it could always call this method, since it extends 
> `FilterOutputStream` [which has the 
> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
> 
> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
> And the checker tool differentiates between them because of that.
> 
> This is similar to #18032 and #18373 
> 
> For context, I am writing tests to check for accurate use of `@since` tags in 
> documentation comments in source code.
> We're following these rules for now:
> 
> ### Rule 1: Introduction of New Elements
> 
> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
> include `@since N`.
>   - Exception: Member elements (fields, methods, nested classes) may omit 
> `@since` if their version matches the value specified for the enclosing class 
> or interface.
> 
> ### Rule 2: Existing Elements in Subsequent JDK Versions
> 
> - If an element exists in JDK N, with an equivalent in JDK N-1, it should not 
> include `@since N`.
> 
> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
> 
> - When inspecting methods, prioritize the `@since` annotation of the 
> supertype's overridden method.
> - If unavailable or if the enclosing class's `@since` is newer, use the 
> enclosing element's `@since`.
> 
>   I.e. if A extends B, and we add a method to B in JDK N, and add an override 
> of the method to A in JDK M (M > N), we will use N as the effective `@since` 
> for the method.

Nizar Benalla has updated the pull request incrementally with one additional 
commit since the last revision:

  empty commit and merge

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18055/files
  - new: https://git.openjdk.org/jdk/pull/18055/files/d3b8e64c..e66fbf5a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18055&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18055&range=07-08

  Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/18055.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18055/head:pull/18055

PR: https://git.openjdk.org/jdk/pull/18055

Re: RFR: 8326951: Missing `@since` tags [v8]

> I added `@since` tags for methods/constructors that do not match the `@since` 
> of the enclosing class.
> 
> The `write` method already existed in `PrintStream` in earlier versions and 
> instances of it could always call this method, since it extends 
> `FilterOutputStream` [which has the 
> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
> 
> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
> And the checker tool differentiates between them because of that.
> 
> This is similar to #18032 and #18373 
> 
> For context, I am writing tests to check for accurate use of `@since` tags in 
> documentation comments in source code.
> We're following these rules for now:
> 
> ### Rule 1: Introduction of New Elements
> 
> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
> include `@since N`.
>   - Exception: Member elements (fields, methods, nested classes) may omit 
> `@since` if their version matches the value specified for the enclosing class 
> or interface.
> 
> ### Rule 2: Existing Elements in Subsequent JDK Versions
> 
> - If an element exists in JDK N, with an equivalent in JDK N-1, it should not 
> include `@since N`.
> 
> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
> 
> - When inspecting methods, prioritize the `@since` annotation of the 
> supertype's overridden method.
> - If unavailable or if the enclosing class's `@since` is newer, use the 
> enclosing element's `@since`.
> 
>   I.e. if A extends B, and we add a method to B in JDK N, and add an override 
> of the method to A in JDK M (M > N), we will use N as the effective `@since` 
> for the method.

Nizar Benalla 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 15 additional commits 
since the last revision:

 - Merge remote-tracking branch 'upstream/master' into add-missing-since-tags
 - deal with methods with different return types - added some spaces for 
readability
 - removed change
 - Update full name
 - update the copyright year to 2024
 - Revert "update the copyright year to 2024"
   
   This reverts commit 9ddd230dcf88bedade76a8e2804db6e120a200f8.
 - update the copyright year to 2024
 - added since tag
 - Revert "fix rest of private/public since tags"
   
   This reverts commit 2c04a9d8e773616b7b6239335d4e5eb955944ad1.
 - Revert "removed unnecessary @ since tags"
   
   This reverts commit 5da3f0d83d19393eeb3a9da68aac40dd999de660.
 - ... and 5 more: https://git.openjdk.org/jdk/compare/893cd607...d3b8e64c

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18055/files
  - new: https://git.openjdk.org/jdk/pull/18055/files/670acaec..d3b8e64c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18055&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18055&range=06-07

  Stats: 592766 lines in 7064 files changed: 99933 ins; 147099 del; 345734 mod
  Patch: https://git.openjdk.org/jdk/pull/18055.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18055/head:pull/18055

PR: https://git.openjdk.org/jdk/pull/18055

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v4]

> Please review this PR that aims to add all the remaining needed `@since` tags 
> in `java.base`, and group them into a single fix.
> This is related to #18934 and my work around the `@since` checker feature.
> Explicit `@since` tags are needed for some overriding methods for the purpose 
> of the checker.
> 
> I will only give the example with the `CompletableFuture` class but here is 
> the before where the methods only appeared in "Methods declared in interface 
> N"
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";>
> 
> 
> 
> and after where the method has it's own javadoc, the main description is 
> added and the `@since` tags are added as intended.
> 
> I don't have an account on `https://cr.openjdk.org/` but I could host the 
> generated docs somewhere if that is needed.
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";>
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";>
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";>
> 
> 
> TIA

Nizar Benalla 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 11 additional commits 
since the last revision:

 - Merge remote-tracking branch 'upstream/master' into 8330954
 - (C)
 - (C)
 - Move security classes changes to pr18373
 - remove couple extra lines
 - Pull request is now only about `@since` tags, might add an other commit
 - add one more `{inheritDoc}` to `CompletableFuture.state`
 - add `@throws` declarations to javadoc
 - add ``{@inheritDoc}`` to new javadoc comments
 - remove two extra spaces
 - ... and 1 more: https://git.openjdk.org/jdk/compare/14267b99...b3574b97

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18954/files
  - new: https://git.openjdk.org/jdk/pull/18954/files/be91ab80..b3574b97

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18954&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18954&range=02-03

  Stats: 52850 lines in 2204 files changed: 24705 ins; 18722 del; 9423 mod
  Patch: https://git.openjdk.org/jdk/pull/18954.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18954/head:pull/18954

PR: https://git.openjdk.org/jdk/pull/18954

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v31]

2024-05-13 Thread Hans Boehm

On Mon, May 13, 2024 at 12:16 PM Brent Christian 
wrote:

> On 5/10/24 10:54 AM, Hans Boehm wrote:
> > I'm not convinced this helps.
> >
> > The isAlive() spec says:
> >
> > "A thread is alive if it has been started and has not yet terminated."
> >
> > Clearly an object is reachable if it can be accessed by a thread that
> will be started in the
> > future. Is that part of a "potential continuing computation from any
> live thread"?
>
> I would think; one "computation" a live thread can perform is to start
> another thread, which in
> turn might access the object.
>
> > I think the JLS wording is a bit sloppy about what "live thread" means
> here. And that sloppiness
> > is probably better than pointing at a precise definition that I'm not
> sure really applies.
> >
> > "in any potential continuing computation from any live thread" really
> seems to mean something
> > like "in any continuing computation in which no finalizers are run"?
>
> Once an object becomes finalizer-reachable, it can only be reached via a
> running finalizer. (JLS
> 12.6.1: "A finalizer-reachable object can be reached from some finalizable
> object through some chain
> of references, but not from any live thread.")
>
> So maybe finalizer threads should not be considered "live" threads.
>
> > Even if the object is later accessed from an existing "live" thread, it
> should not be considered
> >  reachable if that happens only after a finalizer later makes it
> reachable again.
>
> Agreed - if an object can (*and only can*) be accessed again after a
> finalizer resurrects it, that
> doesn't count as "reachable". In fact, at that point, the object must have
> transitioned from
> reachable to finalizer-reachable.
>
> If an object gets resurrected by a finalizer, it does become reachable
> again and can again be
> accessed by the program. (And of course if the object's own finalizer ran,
> it won't run again if the
> object again stops being reachable.)
>
> > So I don't see why the thread from which the access happens matters at
> all.
>
> I think it would only matter for accesses from a finalizer thread.
>
I'm arguing that even that doesn't matter.  Let's say B is reachable from
A, and A has a finalizer. Whether B is accessed directly by the finalizer
from the finalizer thread, or the finalizer invokes some parallel algorithm
on A, so that B is accessed by a helper "live thread" shouldn't matter.
What does matter is that neither A nor B are reachable before the finalizer
runs, because they can only be accessed as the result of a finalizer
running.

I now wonder whether "A reachable object is any object that can be
accessed in any potential continuing computation before any additional
finalizers are started." wouldn't actually be a much better way to state
this. (Officially, this wording is presumably nearly obsolete, since I
don't think Cleaners or References have this particular issue. OTOH, that
would also make it much clearer that this is so, and post finalizers, there
is no issue here.)

Hans

>
> -Brent
>

Withdrawn: 8330755: ProblemList files have entries referring to non-existent tests

2024-05-13 Thread Doug Simon

On Sun, 21 Apr 2024 22:00:52 GMT, Doug Simon  wrote:

> This PR adds a check for the format of ProblemList files and ensures they 
> only have entries referring to existing tests.
> 
> The cleanups in the second commit of this PR were done based on the output of 
> `CheckProblemLists`:
> 
>> make test TEST=build/problemLists/CheckProblemLists.java
> ...
> STDOUT:
> Checking 
> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt
> Checking 
> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt
> Checking 
> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt
> Checking 
> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt
> Checking 
> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt
> Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt
> Checked 13 problem list files
> Test roots:
>   /Users/dnsimon/dev/jdk-jdk/open/test/jdk
>   /Users/dnsimon/dev/jdk-jdk/open/test/lib-test
>   /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test
>   /Users/dnsimon/dev/jdk-jdk/open/test/jaxp
>   /Users/dnsimon/dev/jdk-jdk/open/test/langtools
>   /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg
> Following errors found:
> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: 
> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under 
> any test root
> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all
> 
> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: 
> TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, 
> issueId=000] duplicates 
> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76
> java/util/Properties/StoreReproducibilityTest.java 000 generic-all
> 
> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: 
> java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any 
> test root
> java/lang/management/MemoryMXBean/PendingAllGC.sh   8158837 
> generic-all
> 
> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:667: 
> javax/swing/JFi...

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jdk/pull/18879

Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]

2024-05-13 Thread Doug Simon

On Wed, 24 Apr 2024 10:50:44 GMT, Doug Simon  wrote:

>> This PR adds a check for the format of ProblemList files and ensures they 
>> only have entries referring to existing tests.
>> 
>> The cleanups in the second commit of this PR were done based on the output 
>> of `CheckProblemLists`:
>> 
>>> make test TEST=build/problemLists/CheckProblemLists.java
>> ...
>> STDOUT:
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt
>> Checked 13 problem list files
>> Test roots:
>>   /Users/dnsimon/dev/jdk-jdk/open/test/jdk
>>   /Users/dnsimon/dev/jdk-jdk/open/test/lib-test
>>   /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test
>>   /Users/dnsimon/dev/jdk-jdk/open/test/jaxp
>>   /Users/dnsimon/dev/jdk-jdk/open/test/langtools
>>   /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg
>> Following errors found:
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: 
>> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under 
>> any test root
>> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all
>> 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: 
>> TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, 
>> issueId=000] duplicates 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76
>> java/util/Properties/StoreReproducibilityTest.java 000 generic-all
>> 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: 
>> java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any 
>> test root
>> java/lang/management/MemoryMXBean/PendingAllGC.sh   8158837 
>> generic-all
>> 
>> ...
>
> Doug Simon has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   removed CheckProblemLists.java

The issue was resolved when I merged the PR to clean up the closed problem 
lists.

I'll just close this PR and leave it as documentation for future open 
ProblemList cleanup if someone wants to take it on.

-

PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2108657466

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v31]

2024-05-13 Thread Brent Christian

On 5/10/24 10:54 AM, Hans Boehm wrote:

I'm not convinced this helps.

The isAlive() spec says:

"A thread is alive if it has been started and has not yet terminated."

Clearly an object is reachable if it can be accessed by a thread that will be started in the
future. Is that part of a "potential continuing computation from any live thread"?

I would think; one "computation" a live thread can perform is to start another
thread, which in
turn might access the object.

I think the JLS wording is a bit sloppy about what "live thread" means here. And that sloppiness
is probably better than pointing at a precise definition that I'm not sure really applies.

"in any potential continuing computation from any live thread" really seems to mean something
like "in any continuing computation in which no finalizers are run"?

Once an object becomes finalizer-reachable, it can only be reached via a
running finalizer. (JLS
12.6.1: "A finalizer-reachable object can be reached from some finalizable
object through some chain
of references, but not from any live thread.")

So maybe finalizer threads should not be considered "live" threads.

Even if the object is later accessed from an existing "live" thread, it should
not be considered
reachable if that happens only after a finalizer later makes it reachable
again.

Agreed - if an object can (*and only can*) be accessed again after a finalizer
resurrects it, that
doesn't count as "reachable". In fact, at that point, the object must have transitioned from
reachable to finalizer-reachable.

If an object gets resurrected by a finalizer, it does become reachable again and can again be
accessed by the program. (And of course if the object's own finalizer ran, it won't run again if the
object again stops being reachable.)

So I don't see why the thread from which the access happens matters at all.

I think it would only matter for accesses from a finalizer thread.

-Brent

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v3]

2024-05-13 Thread Kevin Rushforth

On Mon, 13 May 2024 18:00:27 GMT, Chen Liang  wrote:

>> I don't want to merge or rebase on an active PR. It should get fixed once 
>> this is integrated.
>
> Sure, this comment serves as a note to reviewers that these 2 header changes 
> have been committed in other changes and thus can be safely ignored.

There is no harm in merging the upstream master, using `git merge`, to bring 
your branch more up-to-date. And it can be helpful in cases like this where it 
might help reviewers avoid extraneous information. It is also recommended in 
other cases (e.g., a long-running PR where there is a lot of drift between the 
PR source branch and the upstream target branch).

Skara will squash all commits anyway when integrating so the results will be 
identical.

Btw, one thing you should _not_ do is rebase a source branch of an active PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1598896706

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

2024-05-13 Thread Doug Lea

On Mon, 13 May 2024 17:06:10 GMT, Viktor Klang  wrote:

>> This change adds wrapping of the CancellationException produced by 
>> CompletableFuture::get() and CompletableFuture::join() to add more 
>> diagnostic information and align better with FutureTask.
>> 
>> Running the sample code from the JBS issue in JShell will produce the 
>> following:
>> 
>> 
>> jshell> java.util.concurrent.CancellationException: 
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>>  at REPL.$JShell$18.m2($JShell$18.java:10)
>>  at REPL.$JShell$17.m1($JShell$17.java:8)
>>  at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>>  at java.base/java.lang.Thread.run(Thread.java:1575)
>> Caused by: java.util.concurrent.CancellationException
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>>  at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>>  ... 1 more
>
> src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 
> 392:
> 
>> 390: return null;
>> 391: if (x instanceof CancellationException)
>> 392: throw new CancellationException("", 
>> (CancellationException)x);
> 
> One option here would be to put "CompletableFuture.get()" or "get()" as a 
> message.

Given the serviceability motivation, the overkill of adding "get" and "join" 
strings seems reasonable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1598868469

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

2024-05-13 Thread Joe Wang

On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replaced another unused exception with _

Marked as reviewed by joehw (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19184#pullrequestreview-2053385714

Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]

2024-05-13 Thread Sonia Zaldana Calles

On Sun, 12 May 2024 18:52:30 GMT, Alan Bateman  wrote:

> This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI 
> functions GetStaticMethodID, GetMethodID and NewObject return NULL with a 
> pending exception when they fail. So I would expect CHECK_EXCEPTION_NULL_FAIL 
> to just check if obj is NULL rather check for an exception first. It's not 
> wrong to check for an exception, just curious when looking at this macro.

Hi @AlanBateman, thanks for taking a look. That's a good point - would it be 
worthwhile to delete the exception check in this case?

-

PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2108464274

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v3]

On Mon, 13 May 2024 17:52:23 GMT, Chen Liang  wrote:

>> Nizar Benalla has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - (C)
>>  - (C)
>
> src/java.base/share/classes/java/security/interfaces/RSAPrivateKey.java line 
> 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights 
>> reserved.
> 
> This diff is redundant but no-op. You should merge openjdk/jdk's master 
> branch to your PR branch, so the diff displayed by GitHub is up-to-date and 
> this will go away.

I don't want to merge or rebase on an active PR. It should get fixed once this 
is integrated.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1598847527

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v3]

On Mon, 13 May 2024 17:57:42 GMT, Nizar Benalla  wrote:

>> src/java.base/share/classes/java/security/interfaces/RSAPrivateKey.java line 
>> 2:
>> 
>>> 1: /*
>>> 2:  * Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights 
>>> reserved.
>> 
>> This diff is redundant but no-op. You should merge openjdk/jdk's master 
>> branch to your PR branch, so the diff displayed by GitHub is up-to-date and 
>> this will go away.
>
> I don't want to merge or rebase on an active PR. It should get fixed once 
> this is integrated.

Sure, this comment serves as a note to reviewers that these 2 header changes 
have been committed in other changes and thus can be safely ignored.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1598850816

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v3]

On Mon, 13 May 2024 17:44:52 GMT, Nizar Benalla  wrote:

>> Please review this PR that aims to add all the remaining needed `@since` 
>> tags in `java.base`, and group them into a single fix.
>> This is related to #18934 and my work around the `@since` checker feature.
>> Explicit `@since` tags are needed for some overriding methods for the 
>> purpose of the checker.
>> 
>> I will only give the example with the `CompletableFuture` class but here is 
>> the before where the methods only appeared in "Methods declared in interface 
>> N"
>> 
>> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";>
>> 
>> 
>> 
>> and after where the method has it's own javadoc, the main description is 
>> added and the `@since` tags are added as intended.
>> 
>> I don't have an account on `https://cr.openjdk.org/` but I could host the 
>> generated docs somewhere if that is needed.
>> 
>> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";>
>> 
>> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";>
>> 
>> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";>
>> 
>> 
>> TIA
>
> Nizar Benalla has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - (C)
>  - (C)

src/java.base/share/classes/java/security/interfaces/RSAPrivateKey.java line 2:

> 1: /*
> 2:  * Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights 
> reserved.

This diff is redundant but no-op. You should merge openjdk/jdk's master branch 
to your PR branch, so the diff displayed by GitHub is up-to-date and this will 
go away.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1598841666

Re: RFR: 8330954: Fix remaining `@since` tags in `java.base` [v3]

> Please review this PR that aims to add all the remaining needed `@since` tags 
> in `java.base`, and group them into a single fix.
> This is related to #18934 and my work around the `@since` checker feature.
> Explicit `@since` tags are needed for some overriding methods for the purpose 
> of the checker.
> 
> I will only give the example with the `CompletableFuture` class but here is 
> the before where the methods only appeared in "Methods declared in interface 
> N"
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";>
> 
> 
> 
> and after where the method has it's own javadoc, the main description is 
> added and the `@since` tags are added as intended.
> 
> I don't have an account on `https://cr.openjdk.org/` but I could host the 
> generated docs somewhere if that is needed.
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";>
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";>
> 
>  src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";>
> 
> 
> TIA

Nizar Benalla has updated the pull request incrementally with two additional 
commits since the last revision:

 - (C)
 - (C)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18954/files
  - new: https://git.openjdk.org/jdk/pull/18954/files/d38a1c7d..be91ab80

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18954&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18954&range=01-02

  Stats: 7 lines in 7 files changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/18954.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18954/head:pull/18954

PR: https://git.openjdk.org/jdk/pull/18954

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v5]

On Thu, 9 May 2024 10:11:22 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - fixed error thrown by VerifierImpl
>  - applied suggested changes

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 233:

> 231: ClassDesc type = ((FieldModel)ae).fieldTypeSymbol();
> 232: ConstantValueEntry cve = cva.constant();
> 233: if (!switch (TypeKind.from(type)) {

Weird-looking switch

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598817747

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v5]

On Mon, 13 May 2024 17:24:39 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>>  line 205:
>> 
>>> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
>>> List errors) {
>>> 204: int payLoad = ((BoundAttribute)a).payloadLen();
>>> 205: if (payLoad != switch (a) {
>> 
>> Please break this up - e.g. with an intermediate `foundPayload` variable - 
>> having a multi-line switch nested as an if condition looks very jarring!
>
> Is this method only supposed to check the attribute size? It would be nice 
> perhaps to enhance this to enforce more structural constraints - I added a 
> couple of comments in that direction, but there's many more (e.g. for 
> instance make sure that any entry that morally points to a class/method is of 
> the right kind)

Some of the checks don't verify the attributes point to valid cp entries; since 
CF API is lazy, those entries much be expanded by calling the accessors on 
Bound attributes.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598822622

Re: In support of Instant.minus(Instant)

Hi,

With Stephen/Roger's comments, as well as Kevin's observation that 
until(end) has a good argument ordering that is easy to understand, I'd 
still propose `until()`. Please post if you have further comments.

Naoto

On 5/3/24 6:39 AM, Roger Riggs wrote:

Hi,

I would also reinforce Stephen's early observation that the pattern for 
"until" methods in java.time includes those of the XXXDate classes, with 
a single Temporal parameter.  Period and Duration are similar values 
holding relative TemporalAmounts.

     public Period until(ChronoLocalDate endDateExclusive)

In addition to Instant, the LocalTime class might also benefit from adding:

 public Duration until(LocalTime endExclusive)`

The API design of java.time included an emphasis on consistent naming 
across the packages.

Regards, Roger

On 5/2/24 4:01 PM, Naoto Sato wrote:
`Temporal` interface is clear that its `minus` methods return objects 
of the same `Temporal` type, and `until` calculates the amount of time 
until another `Temporal` type. Introducing `Instant.minus` that 
returns `Duration` would be confusing to me.

Naoto

On 5/2/24 10:41 AM, Éamonn McManus wrote:
I'd say too that this makes intuitive sense based on algebra. If we 
have:

/instant1/ + /duration/ = /instant2/
then we can subtract /duration/ from both sides:
/instant1 = instant2 - duration/
or we can subtract /instant1/ from both sides:
/duration = instant2 - instant1/

There's no manipulation we can do that would cause us to try to add 
instants together, and it's a bit surprising for the API to allow the 
first subtraction but not the second.
I also think that if I see instant2.minus(instant1) it's immediately 
obvious to me what that means, while instant1.until(instant2) seems 
both less discoverable and less obvious.

On Thu, 2 May 2024 at 10:29, Louis Wasserman > wrote:

    That doesn't follow for me at all.

    The structure formed by Instants and Durations is an affine space
, with
    instants the points and durations the vectors.  (An affine space is
    a vector space without a distinguished origin, which of course
    Instants don't have.)  It is 100% standard to use the minus sign for
    the operation "point - point = vector," even when "point + point" is
    not defined, and to use all the other standard idioms for
    subtraction; the Wikipedia article uses "subtraction" and
    "difference" ubiquitously.

    Personally, I'd be willing to live with a different name for the
    operation, but consider "users keep getting it wrong" a strong
    enough argument all by itself for a version with the swapped
    argument order; it's not obvious to me that another API with the
    same argument order adds enough value over Duration.between to
    bother with.

    On Thu, May 2, 2024 at 10:04 AM Stephen Colebourne
    mailto:scolebou...@joda.org>> wrote:

    On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever mailto:k...@google.com>> wrote:
 > instant − instant = duration // what we're discussing
 > instant + duration = instant // satisfied by
    instant.plus(duration)
 > instant - duration = instant // satisfied by
    instant.minus(duration)
 > duration + duration = duration // satisfied by
    duration.plus(duration)
 > duration - duration = duration // satisfied by
    duration.minus(duration)
 > duration × real number = duration // satisfied by
    duration.multipliedBy(long)
 > duration ÷ real number = duration // satisfied by
    duration.dividedBy(long)
 >
 > All but the first operation have very clear translations from
    conceptual model to code. I'm hoping we can achieve the same
    clarity for instant - instant by using the obvious name:
    instant.minus(instant)

    But you can't have
  instant + instant = ???
    It doesn't make sense.

    This is at the heart of why minus isn't right in this case.
    Stephen

    --     Louis Wasserman (he/they)

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

On Mon, 13 May 2024 16:41:37 GMT, Viktor Klang  wrote:

> This change adds wrapping of the CancellationException produced by 
> CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
> information and align better with FutureTask.
> 
> Running the sample code from the JBS issue in JShell will produce the 
> following:
> 
> 
> jshell> java.util.concurrent.CancellationException: 
>   at 
> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>   at 
> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>   at REPL.$JShell$18.m2($JShell$18.java:10)
>   at REPL.$JShell$17.m1($JShell$17.java:8)
>   at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>   at java.base/java.lang.Thread.run(Thread.java:1575)
> Caused by: java.util.concurrent.CancellationException
>   at 
> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>   at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>   ... 1 more

src/java.base/share/classes/java/util/concurrent/CancellationException.java 
line 72:

> 70:  * @param cause the underlying cancellation exception
> 71:  */
> 72: CancellationException(String message, CancellationException cause) {

Can we remove the message argument, which is always empty?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1598786142

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v5]

On Mon, 13 May 2024 17:15:15 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - fixed error thrown by VerifierImpl
>>  - applied suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>  line 205:
> 
>> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
>> List errors) {
>> 204: int payLoad = ((BoundAttribute)a).payloadLen();
>> 205: if (payLoad != switch (a) {
> 
> Please break this up - e.g. with an intermediate `foundPayload` variable - 
> having a multi-line switch nested as an if condition looks very jarring!

Is this method only supposed to check the attribute size? It would be nice 
perhaps to enhance this to enforce more structural constraints - I added a 
couple of comments in that direction, but there's many more (e.g. for instance 
make sure that any entry that morally points to a class/method is of the right 
kind)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598810642

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v5]

On Thu, 9 May 2024 10:11:22 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - fixed error thrown by VerifierImpl
>  - applied suggested changes

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 205:

> 203: private void verifyAttribute(AttributedElement ae, Attribute a, 
> List errors) {
> 204: int payLoad = ((BoundAttribute)a).payloadLen();
> 205: if (payLoad != switch (a) {

Please break this up - e.g. with an intermediate `foundPayload` variable - 
having a multi-line switch nested as an if condition looks very jarring!

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 246:

> 244: }
> 245: case DeprecatedAttribute da -> 0;
> 246: case EnclosingMethodAttribute ema -> 4;

See 4.7.7:
> Otherwise, the value of the method_index item must be a valid index into the 
> constant_pool table. The constant_pool entry at that index must be a 
> CONSTANT_NameAndType_info structure 
> ([§4.4.6](https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-4.html#jvms-4.4.6))
>  representing the name and type of a method in the class referenced by the 
> class_index attribute above.

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 247:

> 245: case DeprecatedAttribute da -> 0;
> 246: case EnclosingMethodAttribute ema -> 4;
> 247: case ExceptionsAttribute ea -> 2 + 2 * 
> ea.exceptions().size();

See 4.7.5:
> Each value in the exception_index_table array must be a valid index into the 
> constant_pool table. The constant_pool entry at that index must be a 
> CONSTANT_Class_info structure 
> ([§4.4.1](https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-4.html#jvms-4.4.1))
>  representing a class type that this method is declared to throw.

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 283:

> 281: yield l;
> 282: }
> 283: case RuntimeVisibleAnnotationsAttribute aa -> {

I wonder if these 4 cases can be consolidated a bit. They all require an 
annotation accessor, and then something that keeps computes the size of all the 
annotations. E.g.


int annotationSize(List annos) {
long size = 0;
for (var an : annos) {
size += annotationSize(an);
}
return l;
}

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598800394
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598806362
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598807397
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1598803054

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replaced another unused exception with _

Marked as reviewed by prappo (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19184#pullrequestreview-2053274453

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

On Mon, 13 May 2024 16:41:37 GMT, Viktor Klang  wrote:

> This change adds wrapping of the CancellationException produced by 
> CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
> information and align better with FutureTask.
> 
> Running the sample code from the JBS issue in JShell will produce the 
> following:
> 
> 
> jshell> java.util.concurrent.CancellationException: 
>   at 
> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>   at 
> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>   at REPL.$JShell$18.m2($JShell$18.java:10)
>   at REPL.$JShell$17.m1($JShell$17.java:8)
>   at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>   at java.base/java.lang.Thread.run(Thread.java:1575)
> Caused by: java.util.concurrent.CancellationException
>   at 
> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>   at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>   ... 1 more

src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 
392:

> 390: return null;
> 391: if (x instanceof CancellationException)
> 392: throw new CancellationException("", 
> (CancellationException)x);

One option here would be to put "CompletableFuture.get()" or "get()" as a 
message.

src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 
410:

> 408: return null;
> 409: if (x instanceof CancellationException)
> 410: throw new CancellationException("", 
> (CancellationException)x);

One option here would be to put "CompletableFuture.join()" or "join()" as a 
message.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1598789543
PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1598789698

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replaced another unused exception with _

Thanks, Pavel.

> While it's hard to test this change (hence the `noreg-hard` label), the 
> `restoreEcho` functionality does not seem to be tested at all. Should we add 
> a straightforward test for it, perhaps separately?

Filed: https://bugs.openjdk.org/browse/JDK-8332161

-

PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2108282547

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 14:52:45 GMT, Daniel Fuchs  wrote:

>> Fine with me.
>
> Not sure if that's an issue - but if you wanted/needed to delay the loading 
> of those random generator classes that will not be used until something 
> actually wants to use them, you could consider using a `Supplier extends RandomGenerator>>` or a `Supplier` for the 
> `FACTORY_MAP` values?

@dfuch You mean not loading the whole batch but only individual classes, as 
need arises?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598796844

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

On Mon, 13 May 2024 17:03:00 GMT, Chen Liang  wrote:

>> This change adds wrapping of the CancellationException produced by 
>> CompletableFuture::get() and CompletableFuture::join() to add more 
>> diagnostic information and align better with FutureTask.
>> 
>> Running the sample code from the JBS issue in JShell will produce the 
>> following:
>> 
>> 
>> jshell> java.util.concurrent.CancellationException: 
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>>  at REPL.$JShell$18.m2($JShell$18.java:10)
>>  at REPL.$JShell$17.m1($JShell$17.java:8)
>>  at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>>  at java.base/java.lang.Thread.run(Thread.java:1575)
>> Caused by: java.util.concurrent.CancellationException
>>  at 
>> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>>  at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>>  ... 1 more
>
> src/java.base/share/classes/java/util/concurrent/CancellationException.java 
> line 72:
> 
>> 70:  * @param cause the underlying cancellation exception
>> 71:  */
>> 72: CancellationException(String message, CancellationException cause) {
> 
> Can we remove the message argument, which is always empty?

Good question. So what I did was to initially omit it, but then the message 
becomes the type of the cause, which didn't look right. Then I passed in the 
empty-string in the constructor, but that seemed too restrictive, since the 
overload is for in-package use only, I opted for some flexibility in case we 
want to customize the message on `get` vs `join`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19219#discussion_r1598788602

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

> Making sure `restoreEcho` correctly reflects the state in the shutdown 
> thread, which differs from the application's thread that issues the 
> `readPassword()` method.

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Replaced another unused exception with _

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19184/files
  - new: https://git.openjdk.org/jdk/pull/19184/files/9daf6997..3e7d2e0c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19184.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184

PR: https://git.openjdk.org/jdk/pull/19184

Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]

On Fri, 12 Apr 2024 08:55:01 GMT, Viktor Klang  wrote:

>> Keep-alive, maybe people like @viktorklang-ora might look at this simple fix 
>> too
>
> @liach I'm not a Reviewer (yet) so I'll defer to someone like @stuart-marks :)

@viktorklang-ora Can you just leave an approval as a collections engineer to 
ensure this patch isn't "accidentally integrated"?

-

PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-2108242584

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v2]

> Making sure `restoreEcho` correctly reflects the state in the shutdown 
> thread, which differs from the application's thread that issues the 
> `readPassword()` method.

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
  
  Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19184/files
  - new: https://git.openjdk.org/jdk/pull/19184/files/43f555e4..9daf6997

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19184&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19184.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184

PR: https://git.openjdk.org/jdk/pull/19184

Re: RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

On Mon, 13 May 2024 16:41:37 GMT, Viktor Klang  wrote:

> This change adds wrapping of the CancellationException produced by 
> CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
> information and align better with FutureTask.
> 
> Running the sample code from the JBS issue in JShell will produce the 
> following:
> 
> 
> jshell> java.util.concurrent.CancellationException: 
>   at 
> java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
>   at 
> java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
>   at REPL.$JShell$18.m2($JShell$18.java:10)
>   at REPL.$JShell$17.m1($JShell$17.java:8)
>   at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
>   at java.base/java.lang.Thread.run(Thread.java:1575)
> Caused by: java.util.concurrent.CancellationException
>   at 
> java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
>   at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
>   ... 1 more

@DougLea @AlanBateman Submitting this for review as we discussed.

-

PR Comment: https://git.openjdk.org/jdk/pull/19219#issuecomment-2108177405

RFR: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException

This change adds wrapping of the CancellationException produced by 
CompletableFuture::get() and CompletableFuture::join() to add more diagnostic 
information and align better with FutureTask.

Running the sample code from the JBS issue in JShell will produce the following:


jshell> java.util.concurrent.CancellationException: 
at 
java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:392)
at 
java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
at REPL.$JShell$18.m2($JShell$18.java:10)
at REPL.$JShell$17.m1($JShell$17.java:8)
at REPL.$JShell$16B.lambda$main$0($JShell$16B.java:8)
at java.base/java.lang.Thread.run(Thread.java:1575)
Caused by: java.util.concurrent.CancellationException
at 
java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2510)
at REPL.$JShell$16B.lambda$main$1($JShell$16B.java:11)
... 1 more

-

Commit messages:
 - Make it possible to customize the message when wrapping a 
CancellationException
 - Adding wrapping of CompletableFuture CancellationExceptions from get() and 
join() to add diagnostic information

Changes: https://git.openjdk.org/jdk/pull/19219/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19219&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331987
  Stats: 29 lines in 3 files changed: 25 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/19219.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19219/head:pull/19219

PR: https://git.openjdk.org/jdk/pull/19219

Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v9]

2024-05-13 Thread Anthony Scarpino

On Fri, 10 May 2024 00:19:32 GMT, Volodymyr Paprotski  wrote:

>> Performance. Before:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt ScoreError  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6443.934 ±  6.491  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6152.979 ±  4.954  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1895.410 ± 36.979  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1878.955 ± 45.487  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1357.810 ± 26.584  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1352.119 ± 23.547  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply  false  thrpt3  1746.126 ± 
>> 10.970  ops/s
>> 
>> Performance, no intrinsic:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt Score Error  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6529.839 ±  42.420  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6199.747 ± 133.566  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1973.676 ±  54.071  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1932.127 ±  35.920  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1355.788 ± 29.858  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1346.523 ± 28.722  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply   true  thrpt3  1919.57...
>
> Volodymyr Paprotski has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   whitespace

The changes look good and have passed testing

-

Marked as reviewed by ascarpino (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18583#pullrequestreview-2053158639

Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]

2024-05-13 Thread Kevin Walls

On Wed, 24 Apr 2024 10:50:44 GMT, Doug Simon  wrote:

>> This PR adds a check for the format of ProblemList files and ensures they 
>> only have entries referring to existing tests.
>> 
>> The cleanups in the second commit of this PR were done based on the output 
>> of `CheckProblemLists`:
>> 
>>> make test TEST=build/problemLists/CheckProblemLists.java
>> ...
>> STDOUT:
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt
>> Checking 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt
>> Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt
>> Checked 13 problem list files
>> Test roots:
>>   /Users/dnsimon/dev/jdk-jdk/open/test/jdk
>>   /Users/dnsimon/dev/jdk-jdk/open/test/lib-test
>>   /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test
>>   /Users/dnsimon/dev/jdk-jdk/open/test/jaxp
>>   /Users/dnsimon/dev/jdk-jdk/open/test/langtools
>>   /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg
>> Following errors found:
>> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: 
>> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under 
>> any test root
>> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all
>> 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: 
>> TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, 
>> issueId=000] duplicates 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76
>> java/util/Properties/StoreReproducibilityTest.java 000 generic-all
>> 
>> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: 
>> java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any 
>> test root
>> java/lang/management/MemoryMXBean/PendingAllGC.sh   8158837 
>> generic-all
>> 
>> ...
>
> Doug Simon has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   removed CheckProblemLists.java

The tidyup looks good!

I don't understand that this is titled as JDK-8330755 but that's already 
integrated.  So this needs to be done in a separate JBS entry and if the 
suggested CheckProblemLists.java is not going to be in it, we remove that from 
the description.

-

PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2108075635

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

src/hotspot/share/runtime/arguments.cpp line 2271:

> 2269: } else if (match_option(option, "--illegal-native-access=", &tail)) 
> {
> 2270:   if (!create_module_property("jdk.module.illegal.native.access", 
> tail, InternalProperty)) {
> 2271: return JNI_ENOMEM;

I think it would be helpful to get guidance on if this is the right way to add 
this system property, only because this one not a "module property". The 
configuration (WriteableProperty + InternalProperty) look right.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598673962

Re: RFR: 8314891: Additional Zip64 extra header validation [v8]

2024-05-13 Thread Marco N .

On Wed, 8 Nov 2023 17:27:19 GMT, Lance Andersen  wrote:

>> @LanceAndersen 
>> 
>> I noticed that this PR did not update `ZipInputStream.readLOC` to perform 
>> consistency validation between expected and actual extra field size and 
>> values. Any particular reason why processing of LOC headers was not made 
>> consistent with CEN?
>
>> @LanceAndersen
>> 
>> I noticed that this PR did not update `ZipInputStream.readLOC` to perform 
>> consistency validation between expected and actual extra field size and 
>> values. Any particular reason why processing of LOC headers was not made 
>> consistent with CEN?
> 
> Intentional, as this was a follow on to the updates which were done 
> previously to the CEN work in August, this is follow on cleanup.
> 
> Updates to ZipInputStream would be done separately under a separate PR or  
> could be done via your work on 8303866

Hey @LanceAndersen,

It was a common practice in obfuscation, to create zips with invalid headers. 
This change leads to a behavioral change that affects existing work processes.  
Would it be possible to add an system property to restore the old behavior?

-

PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-2107932136

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

2024-05-13 Thread Daniel Fuchs

On Mon, 13 May 2024 13:21:23 GMT, Raffaello Giulietti  
wrote:

>> A followup PR is fine. I think once this PR which addresses the API aspects 
>> (like removal of ServiceLoader and then updates to the create() method 
>> javadoc) is integrated, then the subsequent PR can just be all internal 
>> implementation changes like the proposed removal of reflection.
>
> Fine with me.

Not sure if that's an issue - but if you wanted/needed to delay the loading of 
those random generator classes that will not be used until something actually 
wants to use them, you could consider using a `Supplier>` or a `Supplier` for the `FACTORY_MAP` values?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598616209

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

2024-05-13 Thread Daniel Fuchs

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

Changes to jdk.net and jdk.sctp look ok.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107695217

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 14:08:01 GMT, Jaikiran Pai  wrote:

>> Then I would even remove the double-checking idiom, the `volatile` on `ctor` 
>> and `properties`, and declare methods `getProperties()` and 
>> `ensureConstructors()` as `synchronized`.
>> I'm not sure that the double-checking optimization brings much value on 
>> contemporary JVMs.
>> 
>> But I feel that the followup PR discussed before wouldn't need 
>> `synchronized` at all.
>> 
>> WDYT?
>
>> Then I would even remove the double-checking idiom, the volatile on ctor and 
>> properties, and declare methods getProperties() and ensureConstructors() as 
>> synchronized.
>> I'm not sure that the double-checking optimization brings much value on 
>> contemporary JVMs.
> 
> Making the methods synchronized would bring in a penalty that there will 
> always be a monitor entry at every call site, even after the `properites` and 
> `ctor`(s) are initialized. Ideally, we should just do all of this 
> intialization in the constructor of the `RandomGeneratorFactory`, the one 
> which takes the `Class<>` type of the `RandomGenerator`. We can then make the 
> `properties` and the `ctor`(s) all `final` and not have to worry about any 
> synchronization or volatile semantics. You would of course have to rework the 
> ensureConstructors to not throw an exception at that time.
> 
>> But I feel that the followup PR discussed before wouldn't need synchronized 
>> at all.
> 
> Correct. The more I think about it, I think cleaning up all this in this PR 
> itself might make both reviewing and the implementation a bit more simpler. 
> What's your thoughts?

OK, will do all the work in this PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598548744

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 13:54:18 GMT, Raffaello Giulietti  
wrote:

> Then I would even remove the double-checking idiom, the volatile on ctor and 
> properties, and declare methods getProperties() and ensureConstructors() as 
> synchronized.
> I'm not sure that the double-checking optimization brings much value on 
> contemporary JVMs.

Making the methods synchronized would bring in a penalty that there will always 
be a monitor entry at every call site, even after the `properites` and 
`ctor`(s) are initialized. Ideally, we should just do all of this intialization 
in the constructor of the `RandomGeneratorFactory`, the one which takes the 
`Class<>` type of the `RandomGenerator`. We can then make the `properties` and 
the `ctor`(s) all `final` and not have to worry about any synchronization or 
volatile semantics. You would of course have to rework the ensureConstructors 
to not throw an exception at that time.

> But I feel that the followup PR discussed before wouldn't need synchronized 
> at all.

Correct. The more I think about it, I think cleaning up all this in this PR 
itself might make both reviewing and the implementation a bit more simpler. 
What's your thoughts?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598540564

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 13:41:50 GMT, Jaikiran Pai  wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
> 299:
> 
>> 297: private void ensureConstructors() {
>> 298: if (ctor == null) {  // volatile load
>> 299: synchronized (rgClass) {
> 
> Here too I think we should synchronize on `this` - we would want to allow 
> multiple different instances of a `RandomGeneratorFactory` for the same 
> `RandomGenerator` class type to be able to concurrently instantiate their 
> individual instance fields (like the `ctor`(s) and `properties`).

Then I would even remove the double-checking idiom, the `volatile` on `ctor` 
and `properties`, and declare methods `getProperties()` and 
`ensureConstructors()` as `synchronized`.
I'm not sure that the double-checking optimization brings much value on 
contemporary JVMs.

But I feel that the followup PR discussed before wouldn't need `synchronized` 
at all.

WDYT?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598520001

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

2024-05-13 Thread Weijun Wang

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

security changes (`java.security.jgss`, `jdk.crypto.cryptoki`, 
`jdk.crypto.mscapi`, and `jdk.security.auth`) look good.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107621474

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
299:

> 297: private void ensureConstructors() {
> 298: if (ctor == null) {  // volatile load
> 299: synchronized (rgClass) {

Here too I think we should synchronize on `this` - we would want to allow 
multiple different instances of a `RandomGeneratorFactory` for the same 
`RandomGenerator` class type to be able to concurrently instantiate their 
individual instance fields (like the `ctor`(s) and `properties`).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598500425

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
187:

> 185:  private RandomGeneratorProperties getProperties() {
> 186: if (properties == null) {  // volatile load
> 187: synchronized (rgClass) {

The synchronization on `rgClass` to intialize an instance field `properties` 
appears odd here. I think this should be synchronized on `this`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598485190

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 13:18:24 GMT, Jaikiran Pai  wrote:

>> @jaikiran I agree that we can simplify even more. I just wanted to change as 
>> little as possible in this PR to facilitate reviews.
>> Shall I push your proposed changes in this PR or is a followup PR preferable?
>
> A followup PR is fine. I think once this PR which addresses the API aspects 
> (like removal of ServiceLoader and then updates to the create() method 
> javadoc) is integrated, then the subsequent PR can just be all internal 
> implementation changes like the proposed removal of reflection.

Fine with me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598468638

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

2024-05-13 Thread Erik Joelsson

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

Build changes look good.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107563120

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 13:14:57 GMT, Raffaello Giulietti  
wrote:

>> Thinking a bit more, I think we can even get rid of the reflection used in 
>> `create()` methods implementation, if we wanted to, by doing something like 
>> this:
>> 
>> 
>> private record RandomGenEntry(Class 
>> randomGenClass, int i, int j,
>>   int k, int equiDistribution, boolean 
>> stochastic,
>>   boolean hardware) {
>> 
>> RandomGenerator create() {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random" -> new Random();
>> case "L128X1024MixRandom" -> new L128X1024MixRandom();
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> 
>> RandomGenerator create(long seed) {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random", "SplittableRandom", "SecureRandom" -> {
>> throw new UnsupportedOperationException("cannot 
>> construct with a long seed");
>> }
>> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> 
>> RandomGenerator create(byte[] seed) {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random", "SplittableRandom", "SecureRandom" -> {
>> throw new UnsupportedOperationException("cannot 
>> construct with a byte[] seed");
>> }
>> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> }
>> 
>> 
>> private static final Map FACTORY_MAP = ... // 
>> construct the map
>
> @jaikiran I agree that we can simplify even more. I just wanted to change as 
> little as possible in this PR to facilitate reviews.
> Shall I push your proposed changes in this PR or is a followup PR preferable?

A followup PR is fine. I think once this PR which addresses the API aspects 
(like removal of ServiceLoader and then updates to the create() method javadoc) 
is integrated, then the subsequent PR can just be all internal implementation 
changes like the proposed removal of reflection.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598464086

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 13:06:04 GMT, Jaikiran Pai  wrote:

>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java 
>> line 190:
>> 
>>> 188: if (properties == null) {  // double-checking idiom
>>> 189: RandomGeneratorProperties props = 
>>> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
>>> 190: Objects.requireNonNull(props, rgClass + " missing 
>>> annotation");
>> 
>> Hello Raffaello, with the `RandomGenerator` implementations now all residing 
>> within the java.base module, I think an additional advantage of that is 
>> that, it will now allow us to remove this internal 
>> `RandomGeneratorProperties` annotation and thus this reflection code.
>> 
>> I think one way to do that would be something like this within this 
>> `RandomGeneratorFactory` class itself: 
>> 
>> 
>> private record RandomGenEntry(Class randomGenClass, int i, int j,
>>   int k, int equiDistribution, boolean 
>> stochastic,
>>   boolean hardware) {
>> 
>> }
>> 
>> private static final Map FACTORY_MAP = ... // 
>> construct the map
>> 
>> 
>> where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be 
>> a record which holds these additional details about the `RandomGenerator`. 
>> This current PR is about getting rid of ServiceLoader usage. So if you want 
>> to remove the usage of this annotation and reflection is a separate PR 
>> that's fine with me. Furthermore, although I don't see the necessity of an 
>> annotation for what we are doing here, if you think that the removal of the 
>> annotation and reflection isn't worth doing, that is OK too.
>
> Thinking a bit more, I think we can even get rid of the reflection used in 
> `create()` methods implementation, if we wanted to, by doing something like 
> this:
> 
> 
> private record RandomGenEntry(Class 
> randomGenClass, int i, int j,
>   int k, int equiDistribution, boolean 
> stochastic,
>   boolean hardware) {
> 
> RandomGenerator create() {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random" -> new Random();
> case "L128X1024MixRandom" -> new L128X1024MixRandom();
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
> 
> RandomGenerator create(long seed) {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random", "SplittableRandom", "SecureRandom" -> {
> throw new UnsupportedOperationException("cannot construct 
> with a long seed");
> }
> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
> 
> RandomGenerator create(byte[] seed) {
> String algo = randomGenClass.getSimpleName();
> return switch (algo) {
> case "Random", "SplittableRandom", "SecureRandom" -> {
> throw new UnsupportedOperationException("cannot construct 
> with a byte[] seed");
> }
> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
> // ... so on for the rest
> default -> throw new InternalError("should not happen");
> };
> }
> }
> 
> 
> private static final Map FACTORY_MAP = ... // 
> construct the map

@jaikiran I agree that we can simplify even more. I just wanted to change as 
little as possible in this PR to facilitate reviews.
Shall I push your proposed changes in this PR or is a followup PR preferable?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598459168

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 12:45:59 GMT, Jaikiran Pai  wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
> 190:
> 
>> 188: if (properties == null) {  // double-checking idiom
>> 189: RandomGeneratorProperties props = 
>> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
>> 190: Objects.requireNonNull(props, rgClass + " missing 
>> annotation");
> 
> Hello Raffaello, with the `RandomGenerator` implementations now all residing 
> within the java.base module, I think an additional advantage of that is that, 
> it will now allow us to remove this internal `RandomGeneratorProperties` 
> annotation and thus this reflection code.
> 
> I think one way to do that would be something like this within this 
> `RandomGeneratorFactory` class itself: 
> 
> 
> private record RandomGenEntry(Class randomGenClass, int i, int j,
>   int k, int equiDistribution, boolean stochastic,
>   boolean hardware) {
> 
> }
> 
> private static final Map FACTORY_MAP = ... // 
> construct the map
> 
> 
> where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be 
> a record which holds these additional details about the `RandomGenerator`. 
> This current PR is about getting rid of ServiceLoader usage. So if you want 
> to remove the usage of this annotation and reflection is a separate PR that's 
> fine with me. Furthermore, although I don't see the necessity of an 
> annotation for what we are doing here, if you think that the removal of the 
> annotation and reflection isn't worth doing, that is OK too.

Thinking a bit more, I think we can even get rid of the reflection used in 
`create()` methods implementation, if we wanted to, by doing something like 
this:

private record RandomGenEntry(Class randomGenClass, 
int i, int j,
  int k, int equiDistribution, boolean 
stochastic,
  boolean hardware) {

RandomGenerator create() {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random" -> new Random();
case "L128X1024MixRandom" -> new L128X1024MixRandom();
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}

RandomGenerator create(long seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct 
with a long seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}

RandomGenerator create(byte[] seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct 
with a byte[] seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}
}

private static final Map FACTORY_MAP = ... // 
construct the map

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598446741

Re: RFR: 8330205: Initial troff manpage generation for JDK 24

On Thu, 9 May 2024 08:18:41 GMT, David Holmes  wrote:

>> Please review this mechanical change to man pages. This PR should be 
>> integrated after https://github.com/openjdk/jdk/pull/18787.
>
> src/java.base/share/man/java.1 line 3856:
> 
>> 3854: .SH REMOVED JAVA OPTIONS
>> 3855: .PP
>> 3856: These \f[V]java\f[R] options have been removed in JDK 24 and using them
> 
> This is incorrect. You can't just change 23 to 24 here as the actual set of 
> flags listed below will be will be different.

David, my understanding is that you agree that this is okay (per our 
out-of-band, follow-up discussion).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19119#discussion_r1598432769

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
190:

> 188: if (properties == null) {  // double-checking idiom
> 189: RandomGeneratorProperties props = 
> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
> 190: Objects.requireNonNull(props, rgClass + " missing 
> annotation");

Hello Raffaello, with the `RandomGenerator` implementations now all residing 
within the java.base module, I think an additional advantage of that is that, 
it will now allow us to remove this internal `RandomGeneratorProperties` 
annotation and thus this reflection code.

I think one way to do that would be something like this within this 
`RandomGeneratorFactory` class itself: 

private record RandomGenEntry(Class randomGenClass, int i, int j,
  int k, int equiDistribution, boolean stochastic,
  boolean hardware) {

}

private static final Map FACTORY_MAP = ... // construct 
the map

where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be a 
record which holds these additional details about the `RandomGenerator`. 
This current PR is about getting rid of ServiceLoader usage. So if you want to 
remove the usage of this annotation and reflection is a separate PR that's fine 
with me. Furthermore, although I don't see the necessity of an annotation for 
what we are doing here, if you think that the removal of the annotation and 
reflection isn't worth doing, that is OK too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598417990

Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v8]

On Mon, 13 May 2024 07:51:19 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/java/lang/classfile/Attributes.java line 153:
>> 
>>> 151: 
>>> 152: /**
>>> 153:  * {@return Attribute mapper for the {@code AnnotationDefault} 
>>> attribute}
>> 
>> Just wondering, can we change `{@code AnnotationDefault}` to `{@value 
>> #NAME_ANNOTATION_DEFAULT}`, etc? This way, the names are still rendered as 
>> code in Javadoc HTML, but they are generated with links to the constants, 
>> and programmers will see these constants and prefer them over hardcoded 
>> values.
>
> On the other side it is questionable if the attribute names should be exposed 
> in the API. We provide corresponding mappers and attribute models. I don't 
> see a case where user would need to use the attribute names directly.

Makes sense, we can always add these literals back if we do need them.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1598368707

Re: RFR: 8332003: Clarify javadoc for MemoryLayout::offsetHandle [v6]

> Consider this layout:
> 
> 
> MemoryLayout SEQ = MemoryLayout.sequenceLayout(5,
>  MemoryLayout.sequenceLayout(10, JAVA_INT));
> 
> 
> And the corresponding offset handle:
> 
> 
> MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(), 
> PathElement.sequenceLayout());
> 
> 
> The resulting method handle takes two additional `long` indices. The 
> implementation checks that the dynamically provided indices conform to the 
> bound available at construction: that is, the first index must be < 5, while 
> the second must be < 10. If this is not true, an `IndexOutOfBoundException` 
> is thrown.
> 
> However, the javadoc for `MemoryLayout::byteOffsetHandle` doesn't claim 
> anything in this direction. There are only some vague claims in the javadoc 
> for `PathElement::sequenceElement()` and `PathElement::sequenceElement(long, 
> long, long)` which make some claims on which indices are actually allowed, 
> but the text seems more in the tone of a discussion, rather than actual 
> normative text.
> 
> I've tweaked the javadoc for `MemoryLayout::byteOffsetHandle` to actually 
> state that the indices will be checked against the "size" of the 
> corresponding open path element (this is a new concept that I also have 
> defined in the javadoc).
> 
> I also added a test for `byteOffsetHandle` as I don't think we had a test for 
> that specifically (although this method is tested indirectly, via 
> `MemoryLayout::varHandle`).

Maurizio Cimadamore has updated the pull request with a new target base due to 
a merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains seven additional 
commits since the last revision:

 - Merge branch 'master' into layout_docs_fixes
 - Fix another index check
 - Replace > 0 with >= 0
 - Address review comments
 - Update copyright
 - Add javadoc to other MemoryLayout methods returning VarHandle/MethodHandle 
to describe which exception can be thrown by returned handle
 - Initial push

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19158/files
  - new: https://git.openjdk.org/jdk/pull/19158/files/629000d1..38a9924f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19158&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19158&range=04-05

  Stats: 19721 lines in 372 files changed: 10283 ins; 6458 del; 2980 mod
  Patch: https://git.openjdk.org/jdk/pull/19158.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19158/head:pull/19158

PR: https://git.openjdk.org/jdk/pull/19158

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
> the use of JNI in the following ways:
> 
> * `System::load` and `System::loadLibrary` are now restricted methods
> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
> * binding a JNI `native` method declaration to a native implementation is now 
> considered a restricted operation
> 
> This PR slightly changes the way in which the JDK deals with restricted 
> methods, even for FFM API calls. In Java 22, the single 
> `--enable-native-access` was used both to specify a set of modules for which 
> native access should be allowed *and* to specify whether illegal native 
> access (that is, native access occurring from a module not specified by 
> `--enable-native-access`) should be treated as an error or a warning. More 
> specifically, an error is only issued if the `--enable-native-access flag` is 
> used at least once.
> 
> Here, a new flag is introduced, namely 
> `illegal-native-access=allow/warn/deny`, which is used to specify what should 
> happen when access to a restricted method and/or functionality is found 
> outside the set of modules specified with `--enable-native-access`. The 
> default policy is `warn`, but users can select `allow` to suppress the 
> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
> aligns the treatment of restricted methods with other mechanisms, such as 
> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
> 
> Some changes were required in the package-info javadoc for 
> `java.lang.foreign`, to reflect the changes in the command line flags 
> described above.

Maurizio Cimadamore has updated the pull request incrementally with three 
additional commits since the last revision:

 - Fix another typo
 - Fix typo
 - Add more comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/c4938dc7..bad10942

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=01-02

  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19213.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213

PR: https://git.openjdk.org/jdk/pull/19213

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v2]

On Mon, 13 May 2024 11:38:40 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Avoid call to VM::isModuleSystemInited
>   Use initial error stream

src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 124:

> 122: if (module != null) {
> 123: // not in init phase
> 124: Holder.JLA.ensureNativeAccess(module, owner, methodName, 
> currentClass);

In an earlier iteration I had a call to `VM::isModuleSystemInited`, but I 
discovered that caused a performance regression, since that method involves a 
volatile access. Perhaps we should rethink that part of the init code to use 
stable fields, but it's probably better done separately.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598328283

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v2]

> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
> the use of JNI in the following ways:
> 
> * `System::load` and `System::loadLibrary` are now restricted methods
> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
> * binding a JNI `native` method declaration to a native implementation is now 
> considered a restricted operation
> 
> This PR slightly changes the way in which the JDK deals with restricted 
> methods, even for FFM API calls. In Java 22, the single 
> `--enable-native-access` was used both to specify a set of modules for which 
> native access should be allowed *and* to specify whether illegal native 
> access (that is, native access occurring from a module not specified by 
> `--enable-native-access`) should be treated as an error or a warning. More 
> specifically, an error is only issued if the `--enable-native-access flag` is 
> used at least once.
> 
> Here, a new flag is introduced, namely 
> `illegal-native-access=allow/warn/deny`, which is used to specify what should 
> happen when access to a restricted method and/or functionality is found 
> outside the set of modules specified with `--enable-native-access`. The 
> default policy is `warn`, but users can select `allow` to suppress the 
> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
> aligns the treatment of restricted methods with other mechanisms, such as 
> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
> 
> Some changes were required in the package-info javadoc for 
> `java.lang.foreign`, to reflect the changes in the command line flags 
> described above.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Avoid call to VM::isModuleSystemInited
  Use initial error stream

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/d9fe9a71..c4938dc7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=00-01

  Stats: 11 lines in 2 files changed: 3 ins; 0 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/19213.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213

PR: https://git.openjdk.org/jdk/pull/19213

Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v2]

2024-05-13 Thread Doug Lea

On Sun, 12 May 2024 19:46:42 GMT, Viktor Klang  wrote:

>> Doug Lea has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1242:
> 
>> 1240: INITIAL_QUEUE_CAPACITY];
>> 1241: this.owner = owner;
>> 1242: //top = base = 1;
> 
> Leftover?

Yes, sorry for mis-committing incomplete-undo that needs another compensating 
change to avoid small performance glitch. Will fix.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1598301076

RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI

This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting the 
use of JNI in the following ways:

* `System::load` and `System::loadLibrary` are now restricted methods
* `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
* binding a JNI `native` method declaration to a native implementation is now 
considered a restricted operation

This PR slightly changes the way in which the JDK deals with restricted 
methods, even for FFM API calls. In Java 22, the single 
`--enable-native-access` was used both to specify a set of modules for which 
native access should be allowed *and* to specify whether illegal native access 
(that is, native access occurring from a module not specified by 
`--enable-native-access`) should be treated as an error or a warning. More 
specifically, an error is only issued if the `--enable-native-access flag` is 
used at least once.

Here, a new flag is introduced, namely `illegal-native-access=allow/warn/deny`, 
which is used to specify what should happen when access to a restricted method 
and/or functionality is found outside the set of modules specified with 
`--enable-native-access`. The default policy is `warn`, but users can select 
`allow` to suppress the warnings, or `deny` to cause `IllegalCallerException` 
to be thrown. This aligns the treatment of restricted methods with other 
mechanisms, such as `--illegal-access` and the more recent 
`--sun-misc-unsafe-memory-access`.

Some changes were required in the package-info javadoc for `java.lang.foreign`, 
to reflect the changes in the command line flags described above.

-

Commit messages:
 - Initial push

Changes: https://git.openjdk.org/jdk/pull/19213/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331671
  Stats: 466 lines in 99 files changed: 301 ins; 53 del; 112 mod
  Patch: https://git.openjdk.org/jdk/pull/19213.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213

PR: https://git.openjdk.org/jdk/pull/19213

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI

On Mon, 13 May 2024 10:42:26 GMT, Maurizio Cimadamore  
wrote:

> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
> the use of JNI in the following ways:
> 
> * `System::load` and `System::loadLibrary` are now restricted methods
> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
> * binding a JNI `native` method declaration to a native implementation is now 
> considered a restricted operation
> 
> This PR slightly changes the way in which the JDK deals with restricted 
> methods, even for FFM API calls. In Java 22, the single 
> `--enable-native-access` was used both to specify a set of modules for which 
> native access should be allowed *and* to specify whether illegal native 
> access (that is, native access occurring from a module not specified by 
> `--enable-native-access`) should be treated as an error or a warning. More 
> specifically, an error is only issued if the `--enable-native-access flag` is 
> used at least once.
> 
> Here, a new flag is introduced, namely 
> `illegal-native-access=allow/warn/deny`, which is used to specify what should 
> happen when access to a restricted method and/or functionality is found 
> outside the set of modules specified with `--enable-native-access`. The 
> default policy is `warn`, but users can select `allow` to suppress the 
> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
> aligns the treatment of restricted methods with other mechanisms, such as 
> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
> 
> Some changes were required in the package-info javadoc for 
> `java.lang.foreign`, to reflect the changes in the command line flags 
> described above.

Javadoc: 
https://cr.openjdk.org/~mcimadamore/jdk/8331671/v1/javadoc/api/index.html
Specdiff: 
https://cr.openjdk.org/~mcimadamore/jdk/8331671/v1/specdiff_out/overview-summary.html

make/conf/module-loader-map.conf line 105:

> 103: java.smartcardio \
> 104: jdk.accessibility \
> 105: jdk.attach \

The list of allowed modules has been rewritten from scratch, by looking at the 
set of modules containing at least one `native` method declaration.

src/hotspot/share/prims/nativeLookup.cpp line 277:

> 275: 
> 276:   Klass*   klass = vmClasses::ClassLoader_klass();
> 277:   Handle jni_class(THREAD, method->method_holder()->java_mirror());

This is the biggest change in this PR. That is, we need to pass enough 
arguments to `ClassLoader::findNative` so that the method can start a 
restricted check accordingly.

src/java.base/share/classes/java/lang/Module.java line 311:

> 309: Module target = moduleForNativeAccess();
> 310: ModuleBootstrap.IllegalNativeAccess illegalNativeAccess = 
> ModuleBootstrap.illegalNativeAccess();
> 311: if (illegalNativeAccess != 
> ModuleBootstrap.IllegalNativeAccess.ALLOW &&

There are some changes in this code:
* this code is no-op if `--illegal-native-access` is set to `allow`
* we also attach the location of the problematic class to the warning message, 
using `CodeSource`
* we use the "initial error stream" to emit the warning, similarly to what is 
done for other runtime warnings

src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 115:

> 113: @ForceInline
> 114: public static void ensureNativeAccess(Class currentClass, 
> Class owner, String methodName) {
> 115: if (VM.isModuleSystemInited()) {

If we call this code too early, we can see cases where `module` is `null`.

src/java.desktop/macosx/classes/com/apple/eio/FileManager.java line 61:

> 59: }
> 60: 
> 61: @SuppressWarnings({"removal", "restricted"})

There are several of these changes. One option might have been to just disable 
restricted warnings when building. But on a deeper look, I realized that in all 
these places we already disabled deprecation warnings for the use of security 
manager, so I also added a new suppression instead.

test/jdk/java/foreign/enablenativeaccess/panama_jni_load_module/module-info.java
 line 24:

> 22:  */
> 23: 
> 24: module panama_jni_load_module {

This module setup is a bit convoluted, but I wanted to make sure that we got 
separate warnings for `System.loadLibrary` and binding of the `native` method, 
and that warning on the _use_ of the native method was not generated 
(typically, all three operations occur in the same module).

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107272261
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598269825
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598271285
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598274987
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598276455
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598277853
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598279827

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

On Mon, 13 May 2024 10:30:43 GMT, Per Minborg  wrote:

> Some of the deprecated methods are very likely to be run in hot loops (e.g. 
> read/store operations). Unless we set 
> `--sun-misc-unsafe-memory-access=allow`, what would be the performance impact 
> on various platforms for these operations?

The intention is that there isn't any impact for the initial default ("allow"), 
and in the future "warn" after the first waning. If the "debug" value is used 
then there is a stack trace printed but that's not something for production use.

-

PR Comment: https://git.openjdk.org/jdk/pull/19174#issuecomment-2107279163

Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

On Fri, 10 May 2024 21:55:26 GMT, Naoto Sato  wrote:

> Making sure `restoreEcho` correctly reflects the state in the shutdown 
> thread, which differs from the application's thread that issues the 
> `readPassword()` method.

Generally looks good.

It's hard to imagine that this shutdown hook might be reading `restoreEcho` 
concurrently with some other thread writing to it. So, our primary goal here is 
to ensure that whatever writes have been done to `restoreEcho` before the hook 
starts are seen by the hook.

While it's hard to test this change (hence the `noreg-hard` label), the 
`restoreEcho` functionality does not seem to be tested at all. Should we add a 
straightforward test for it, perhaps separately?

-

PR Review: https://git.openjdk.org/jdk/pull/19184#pullrequestreview-2052349894

Re: RFR: 8305457: Implement java.io.IO [v7]

On Fri, 10 May 2024 16:41:28 GMT, Naoto Sato  wrote:

>> When implementing, I asked myself if I must use any of those monitors and 
>> decided that I don't have to. My reasoning is below.
>> 
>> `readLock`:
>> 
>> - is used inside the object that `Reader reader` is initialised with, and
>> 
>> - it also guards fields such as `char[] rcb`, `boolean restoreEcho` and 
>> `boolean shutdownHookInstalled`.
>> 
>> Since `println` and `print` don't call methods on `reader` or access the 
>> above fields, they don't require `readLock`.
>> 
>> `writeLock`:
>> 
>> - is used inside objects that `Writer out` and `PrintWriter pw` are 
>> initialised with, and
>> - also in compound operations that first write and then immediately read. (I 
>> assume, it's so that no concurrent write could sneak in between writing and 
>> reading parts of such a compound.)
>> 
>> `println` or `print` don't call methods on `out` and certainly don't do any 
>> write-read compounds. That said, they call methods on `pw`. But `pw` uses 
>> `writeLock` internally. So in that sense we're covered. 
>> 
>> One potential concern is a write-write compound in `print`:
>> 
>> 
>> @Override
>> public JdkConsole print(Object obj) {
>> pw.print(obj);
>> pw.flush(); // automatic flushing does not cover print
>> return this;
>> }
>> 
>> 
>> I'm saying write-_write_, not write-_flush_, because as far as 
>> synchronisation is concerned,  `pw.flush()` should behave the same as 
>> `pw.print("")`.
>> 
>> While not using `writeLock` is not strictly correct, I believe the potential 
>> execution interleavings with other writes are benign. What's the worst that 
>> could happen? You flush more than you expected? Big deal!
>> 
>> Since we exhausted all the reasons to use `writeLock`, I don't think we need 
>> one.
>> 
>> 
>> 
>> Naoto has already reviewed this PR with only minor comments. While that 
>> increases my confidence in that the implementation is correct, it doesn't 
>> hurt to request re-review of this specific part: @naotoj, do you think I 
>> should use any of those monitors?
>
> I think your investigation is correct. As to the write-write case, there 
> already is the same pattern in (`formatter` basically utilizes `pw` 
> underneath) 
> 
> public JdkConsole format(String fmt, Object ... args) {
> formatter.format(fmt, args).flush();
> return this;
> }
> 
> So I think it is acceptable.

Thank you for that explanation, Pavel. I think the crucial detail happens to be:

> But pw uses writeLock internally. So in that sense we're covered.

As you note, the same instance of `writeLock` will get used internally by the 
`PrintWriter`, so I think the current version of this code is good and won't 
require additionally locking in the outer code.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1598268588

Re: RFR: 8305457: Implement java.io.IO [v9]

On Mon, 13 May 2024 09:56:35 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 17 additional 
> commits since the last revision:
> 
>  - Escape prompt
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Clarify input charset
>  - Make IO final
>  - Fix System.console().readln(null) in jshell
>
>Without it, jshell hangs on me. Will think of a test.
>  - Fix typo
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Simplify output.exp
>  - Cover null prompt in input tests
>  - Make input test parametric
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/ea3ed865...17100ab8

The latest updated state of this PR looks good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2052345767

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

2024-05-13 Thread Per Minborg

On Fri, 10 May 2024 10:06:55 GMT, Alan Bateman  wrote:

> This is the implementation changes for JEP 471.
> 
> The methods in sun.misc.Unsafe for on-heap and off-heap access are deprecated 
> for removal. This means a removal warning at compile time. No methods have 
> been removed. A deprecated message is added to each of the methods but 
> unlikely to be seen as the JDK does not generate or publish the API docs for 
> this class.
> 
> A new command line option --sun-misc-unsafe-memory-access=$value is 
> introduced to allow or deny access to these methods. The default proposed for 
> JDK 23 is "allow" so no change in behavior compared to JDK 22 or previous 
> releases.
> 
> A new test is added to test the command line option settings. The existing 
> micros for FFM that use Unsafe are updated to suppress the removal warning at 
> compile time. A new micro is introduced with a small sample of methods to 
> ensure the changes doesn't cause any perf regressions.
> 
> For now, the changes include the update to the man page for the "java" 
> command. It might be that this has to be separated out so that it goes with 
> other updates in the release.

Some of the deprecated methods are very likely to be run in hot loops (e.g. 
read/store operations). Unless we set `--sun-misc-unsafe-memory-access=allow`, 
what would be the performance impact on various platforms for these operations?

-

PR Comment: https://git.openjdk.org/jdk/pull/19174#issuecomment-2107216975

Re: RFR: 8331855: Convert jdk.jdeps jdeprscan and jdeps to use the Classfile API [v2]

On Sun, 12 May 2024 02:42:32 GMT, Chen Liang  wrote:

>> Summary of the changes:
>>  - Moved `com.sun.tools.classfile.Dependency` and `Dependencies` to jdeps; 
>> they are exclusively used by jdeps in sources, and they are not used in any 
>> tests too. This will ease the removal of `com.sun.tools.classfile` later.
>>  - A few visitor patterns have been rewritten with pattern matching, notably 
>> in:
>>- `CPEntries`/`CPSelector` (removed)
>>- `Dependencies.BasicDependencyFinder.Visitor` has been rewritten to use 
>> pattern matching to capture dependencies.
>>  - `MethodSig` and its tests have been removed in favor of `MethodTypeDesc`.
>>  - Otherwise, the changes are mostly mechanical replacements.
>> 
>> All tests in `test/langtools/tools/jdeprscan` and 
>> `test/langtools/tools/jdeps` pass.
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Migrate omitted getdeps test in langtools

@asotona Requesting your review for this and the other test migration patch.

-

PR Comment: https://git.openjdk.org/jdk/pull/19193#issuecomment-2107175526

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v31]

On Thu, 9 May 2024 18:44:10 GMT, Brent Christian  wrote:

>> Classes in the `java.lang.ref` package would benefit from an update to bring 
>> the spec in line with how the VM already behaves. The changes would focus on 
>> _happens-before_ edges at some key points during reference processing.
>> 
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing 
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the 
>> registered cleaning action.
>> 
>> This will bring Cleaner in line (or close) with the memory visibility 
>> guarantees made for finalizers in [JLS 
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object 
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   add link to Thread.isAlive()

You've addressed my comments, I don't have anything else.

-

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16644#pullrequestreview-2052249974

Re: RFR: 8305457: Implement java.io.IO [v9]

> Please review this PR which introduces the `java.io.IO` top-level class and 
> three methods to `java.io.Console` for [Implicitly Declared Classes and 
> Instance Main Methods (Third Preview)].
> 
> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
> PR].
> 
> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
> https://bugs.openjdk.org/browse/JDK-8323335
> [draft PR]: https://github.com/openjdk/jdk/pull/18921

Pavel Rappo has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 17 additional commits since the 
last revision:

 - Escape prompt
 - Merge branch 'master' into 8305457-Implement-java.io.IO
 - Clarify input charset
 - Make IO final
 - Fix System.console().readln(null) in jshell
   
   Without it, jshell hangs on me. Will think of a test.
 - Fix typo
 - Merge branch 'master' into 8305457-Implement-java.io.IO
 - Simplify output.exp
 - Cover null prompt in input tests
 - Make input test parametric
 - ... and 7 more: https://git.openjdk.org/jdk/compare/5ac71ae9...17100ab8

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19112/files
  - new: https://git.openjdk.org/jdk/pull/19112/files/43a95732..17100ab8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19112&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19112&range=07-08

  Stats: 3238 lines in 113 files changed: 2069 ins; 612 del; 557 mod
  Patch: https://git.openjdk.org/jdk/pull/19112.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112

PR: https://git.openjdk.org/jdk/pull/19112

Integrated: 8327499: MethodHandleStatics.traceLambdaForm includes methods that cannot be generated

On Fri, 10 May 2024 00:45:31 GMT, Chen Liang  wrote:

> GenerateJLIClassesHelper has been making wrong assumptions about Invoker's 
> LambdaForm method type parameters. Since they are distinct from those of 
> Linkers, they are now tracked and generated separately. It seems that no 
> proper invoker was ever generated before, except it happens that most invoker 
> signatures can be taken as linker signature so we never detected it.
> 
> Requesting @iklam for a review; since I don't know how to deal with CDS, I 
> have to relay to someone else to ensure this fixes the problem from the CDS 
> side as well.

This pull request has now been integrated.

Changeset: adaa509b
Author:Chen Liang 
Committer: Claes Redestad 
URL:   
https://git.openjdk.org/jdk/commit/adaa509b6ed3d12569b8e5f2ec802cef22ab53c7
Stats: 151 lines in 5 files changed: 117 ins; 11 del; 23 mod

8327499: MethodHandleStatics.traceLambdaForm includes methods that cannot be 
generated

Reviewed-by: redestad, iklam

-

PR: https://git.openjdk.org/jdk/pull/19164

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

This is a followup of [18932](https://github.com/openjdk/jdk/pull/18932) in 
which all random number generator algorithms have been moved to module 
`java.base` and module `jdk.random` has been removed.

Reliance on `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
needed and has been replaced by a (lazily populated) map from algorithm names 
to classes.

Moreover, methods `RandomGeneratorFactory.create(long)` and `create(byte[])` 
now throw an `UnsupportedOperationException` rather than silently falling back 
to the no-arg `create()` in case the underlying algorithm does not support a 
`long` resp. `byte[]` seed.

Tests in tier1-tier3 pass after adaptations on the existing 
`RandomTestCoverage`.

-

PR Comment: https://git.openjdk.org/jdk/pull/19212#issuecomment-2107034744

RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

All random number generator algorithms are implemented in module `java.base`. 
The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
needed.

-

Commit messages:
 - 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

Changes: https://git.openjdk.org/jdk/pull/19212/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19212&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332086
  Stats: 204 lines in 4 files changed: 51 ins; 45 del; 108 mod
  Patch: https://git.openjdk.org/jdk/pull/19212.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19212/head:pull/19212

PR: https://git.openjdk.org/jdk/pull/19212

Re: RFR: 8322332: Add API to access ZipEntry.extraAttributes

2024-05-13 Thread Michael Hall

I was thinking of zip api’s other than java’s. The field needs to be at a fixed 
place in the file format whatever the name? Unless a significant api change has 
been made.

A couple of links from googling on “zip extra field chaining"

https://libzip.org/specifications/extrafld.txt
https://www.hackerfactor.com/blog/index.php?/archives/405-Keeping-Zip.html

If this were strictly jar files I would be less concerned but zip is widely 
used outside of java so more chances for conflict where java doesn’t have 
anything like ownership.
At the time I looked at it the provision for “user” additions was very limited 
again making conflict more likely.

> On May 12, 2024, at 9:32 PM, -  wrote:
> 
> Hi Mike,
> I think this particular field has been renamed a few times; and ZipEntry only 
> exposes part of the ZIP format's fields. So most likely there isn't 
> sufficient information for most 3rd-party ZIP processing libraries. I 
> personally don't really use ZipFile so am not quite sure of the recent API 
> changes either.
> 
> On Sun, May 12, 2024 at 6:10 PM Michael Hall  > wrote:
>> I haven’t looked at any of this for sometime. But as I recall there was a 
>> possibility that other 3rd party applications might also be making use of 
>> these fields? IIRC there was some support for chaining multiple uses? Or the 
>> api may of changed and these aren’t the same fields or for some other reason 
>> what I remember is out of date. 
>> 
>> Mike
>>

Re: Enhance proxy support in java.net core libraries



The OpenJDK net-dev mailing list is the best place to bring this. There 
was discussion about SOCKS when the HTTP client was developed, I thought 
JEP 321 had a summary on this but it seems not. I'm sure others on 
net-dev can say more on this.


-Alan


On 12/05/2024 22:58, Alessandro Autiero wrote:
Hello, my name is Alessandro Autiero and I'd like to propose three 
enhancements for the java core libraries to better support proxies in 
network components of the JDK.


There are three classes in the java.net  package that 
have proxy support:


  * java.net.Socket
Introduced in Java 1.0, supports HTTP(S)/SOCKS proxies modelled by
java.net.Proxy through the java.net.Socket(java.net.Proxy) constructor
  * java.net.HttpURLConnection
Introduced in Java 1.1, supports HTTP(S)/SOCKS proxies modelled by
java.net.Proxy through the
java.net.URL#openConnection(java.net.Proxy) public method
  * java.net.HttpClient (Introduced in Java 11)
Introduced in Java 11, supports HTTP(S) proxies modelled by
java.net.ProxySelector through the public
proxy(java.net.ProxySelector) method in its builder or the default
java.net.ProxySelector, which can be set by calling
java.net.ProxySelector#setDefault(java.net.ProxySelector)

While most proxies provide support for both the HTTP and SOCKS scheme, 
considering that the older HTTP client API had support for both, 
developers might choose to use the older api, or to use an external 
one, if they need or want to provide support for this feature. A quick 
Google search for a recommendation on which Http Client to use on a 
modern Java version yields many results that list SOCKS support as a 
feature to keep in mind when making a choice. While this is not 
necessarily indicative of the average Java developer sentiment about 
the feature, I think that it should be considered, alongside a couple 
of issues that were opened on StackOverFlow 
 
asking about support for this feature. Accordingly, I propose adding 
support for SOCKS proxies in java.net.HttpClient. If the change is 
allowed, consider that the default java.net.ProxySelector is an 
instance of sun.net.spi.DefaultProxySelector, which supports SOCKS 
proxies, but this implementation cannot be initialized by the user as 
it's not exposed by the module system. Starting from Java 9, 
ProxySelector#of(InetSocketAddress) was introduced, which returns an 
instance of java.net.ProxySelector$StaticProxySelector 
, 
a static inner class of ProxySelector introduced in Java 9 which only 
implements support for HTTP(S) proxies. StaticProxySelector's 
constructor could be modified from 
StaticProxySelector(java.net.InetSocketAddress) to 
StaticProxySelector(java.net.Proxy$Type, java.net.InetSocketAddress) 
to initialize the java.net.Proxy instance with a specified proxy type 
instead of hard coding HTTP. Then we could overload the method 
ProxySelector#of(InetSocketAddress) with 
ProxySelector#of(java.net.Proxy$Type, InetSocketAddress) method which 
would invoke the constructor we defined earlier. This change would not 
be breaking as StaticProxySelector is package private, no public 
methods would be deleted and the default scheme would still be HTTP. 
jdk.internal.net.http.HttpRequestImpl uses the ProxySelector in its 
retrieveProxy method, but checks if the proxy is an HTTP proxy: this 
would need to be changed as well. Finally, considering that unlike 
HttpURLConnection, HttpClient doesn't delegate the underlying 
connection to java.net.Socket, the java.net.http module would need to 
be enhanced to support SOCKS authentication, which could take more effort.


Another observation that I've made is about authentication. If a proxy 
requires basic authentication, that is authentication through a 
username and optionally a password, a developer can implement the 
java.net.Authenticator class and override the 
getPasswordAuthentication method. While basic authentication is still 
the norm for most proxies, it's disabled by default in the JDK since 
Java 8. Though, it's possible to enable it by overriding the net 
properties jdk.http.auth.proxying.disabledSchemes and 
jdk.http.auth.tunneling.disabledSchemes using System.setProperty. I 
couldn't find an explanation about why this change was implemented, so 
I can only speculate that it was done to incentivize Java developers 
to use an IP whitelist instead of basic auth to improve security, 
assuming that the connection isn't secure(HTTP). The problem though is 
that the net properties that need to be changed to allow basic proxy 
authentication are only read only one time in the static initializer 
of sun.net.www.protocol.http.HttpURLConnection class, the underlying 
implementation of java.net.HttpURLConnecti

Integrated: 8331535: Incorrect prompt for Console.readLine

2024-05-13 Thread Jan Lahoda

On Fri, 3 May 2024 10:11:02 GMT, Jan Lahoda  wrote:

> When JLine reads a line, there may be a prompt provided. However, JLine will 
> not interpret the prompt literally, it will handle `%` specially. As a 
> consequence, doing:
> 
> System.console().readLine("%%s");
> 
> 
> will not print `%s`, as first `String.format` is used, which will convert 
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution 
> is to duplicate the `%`, so that JLine will print it.

This pull request has now been integrated.

Changeset: 5a8df410
Author:Jan Lahoda 
URL:   
https://git.openjdk.org/jdk/commit/5a8df4106ac5386eb72e874dcadf2b18defe27d8
Stats: 296 lines in 6 files changed: 291 ins; 0 del; 5 mod

8331535: Incorrect prompt for Console.readLine
8331681: Test that jdk.internal.io.JdkConsole does not interpret prompts

Reviewed-by: naoto, asotona

-

PR: https://git.openjdk.org/jdk/pull/19081

Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v8]

2024-05-13 Thread Adam Sotona

On Sun, 12 May 2024 15:11:17 GMT, Chen Liang  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fixed tests
>
> src/java.base/share/classes/java/lang/classfile/Attributes.java line 153:
> 
>> 151: 
>> 152: /**
>> 153:  * {@return Attribute mapper for the {@code AnnotationDefault} 
>> attribute}
> 
> Just wondering, can we change `{@code AnnotationDefault}` to `{@value 
> #NAME_ANNOTATION_DEFAULT}`, etc? This way, the names are still rendered as 
> code in Javadoc HTML, but they are generated with links to the constants, and 
> programmers will see these constants and prefer them over hardcoded values.

On the other side it is questionable if the attribute names should be exposed 
in the API. We provide corresponding mappers and attribute models. I don't see 
a case where user would need to use the attribute names directly.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1598026518

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal