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

[Alan Bateman]

On Wed, 22 May 2024 21:42:14 GMT, Kevin Rushforth  wrote:

> Further, I confirm that if I pass that option to jlink or jpackage when 
> creating a custom runtime, there is no warning.

Great! What about jpackage without a custom runtime, wondering if 
--java-options can be tested.

-

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

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

[Jaikiran Pai]

On Tue, 21 May 2024 07:26:17 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 ([sample 
>> results](https://cr.openjdk.org/~alanb/8331670-results.txt)).
>> 
>> 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.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2072956262

Re: RFR: 8242888: Convert dynamic proxy to hidden classes

[Alan Bateman]

On Thu, 23 May 2024 03:28:30 GMT, Chen Liang  wrote:

> Please review this change that convert dynamic proxies implementations to 
> hidden classes, intended to target JDK 24.
> 
> Summary:
> 1. Adds new implementation while preserving the old implementation behind 
> `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility 
> issues.
> 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in 
> native code; I updated native code to reuse that ClassLoader for Proxy 
> support.
> 3. ProxyGenerator changes mainly involve using Class data to pass Method list 
> (accessed in a single condy) and removal of obsolete setup code generation.
> 
> Testing: tier1 and tier2 have no related failures.
> 
> Comment: Since #8278, Proxy has been converted to ClassFile API, and 
> infrastructure has changed; now, the migration to hidden classes is much 
> cleaner and has less impact, such as preserving ProtectionDomain and dynamic 
> module without "anchor classes", and avoiding java.lang.invoke package.

There are compatibility concerns and behavioural differences that will require 
significant effort to consider before doing this. This is the reason that this 
one has been kicked down the road several times.

-

PR Comment: https://git.openjdk.org/jdk/pull/19356#issuecomment-2126310091

Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`

[Alan Bateman]

On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons  wrote:

> A long vertical series of lines beginning /// is replaced by lines beginning 
> //|.

This one looks unusual when it's just one line, I could imagine deleting the 
"|" in these cases.

-

PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2126289283

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Joseph D. Darcy]

On 5/22/2024 11:19 AM, Aman Sharma wrote:


Hi,


[snip]


Notice the difference in the order of fields and `helpCommand` method 
is mapped to a different field name in both classes. This happens 
because the method array returned by `getMethods` is not sorted in any 
particular order 
 
when generating a proxy class. What dictates this order? And why is it 
not deterministic?



Regards,
Aman Sharma



As a general comment, it is _not_ the goal of the API specification to 
(over) specify exact behavior in cases like this.


See as an example the discussion concerning behavioral compatibility 
starting around slide 46 of


"Contributing to OpenJDK: Participating in stewardship for the long-term,"
https://jcp.org/aboutJava/communityprocess/ec-public/materials/2023-06-13/Contributing_to_OpenJDK_2023_04_12.pdf


This approach has evolved over the years and releases.

In this case semantically, the array returned by getMethod is a set and 
the no particular meaning should be read into the order of the elements.


HTH,


-Joe

RFR: 8242888: Convert dynamic proxy to hidden classes

[Chen Liang]

Please review this change that convert dynamic proxies implementations to 
hidden classes, intended to target JDK 24.

Summary:
1. Adds new implementation while preserving the old implementation behind 
`-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility issues.
2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in 
native code; I updated native code to reuse that ClassLoader for Proxy support.
3. ProxyGenerator changes mainly involve using Class data to pass Method list 
(accessed in a single condy) and removal of obsolete setup code generation.

Testing: tier1 and tier2 have no related failures.

Comment: Since #8278, Proxy has been converted to ClassFile API, and 
infrastructure has changed; now, the migration to hidden classes is much 
cleaner and has less impact, such as preserving ProtectionDomain and dynamic 
module without "anchor classes", and avoiding java.lang.invoke package.

-

Commit messages:
 - Fixes
 - Merge branch 'master' of https://github.com/openjdk/jdk into 
feature/hidden-proxy
 - First draft

Changes: https://git.openjdk.org/jdk/pull/19356/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19356&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8242888
  Stats: 303 lines in 8 files changed: 70 ins; 153 del; 80 mod
  Patch: https://git.openjdk.org/jdk/pull/19356.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19356/head:pull/19356

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

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Check macos build

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/42af0b50..40a1e628

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=29
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=28-29

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

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

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Check macos build

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/027daf73..42af0b50

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=28
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=27-28

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

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

Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v7]

[Joe Darcy]

On Thu, 23 May 2024 01:05:07 GMT, Chen Liang  wrote:

> > As a general comment, please update all the links to "mandated" so that the 
> > text "implicitly declared" get linked to the MANDATED enum constant.
> 
> Should we update the API specification for `Parameter::isImplicit`, which 
> checks the "mandated" status but does not refer to this concept at all?

I think it would be a reasonable separate RFE for Parameter:isImplicit to have 
an `@see` or `@link` tag to MANDATED.

-

PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2126025587

Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v7]

[Chen Liang]

On Thu, 23 May 2024 00:58:14 GMT, Joe Darcy  wrote:

> As a general comment, please update all the links to "mandated" so that the 
> text "implicitly declared" get linked to the MANDATED enum constant.

Should we update the API specification for `Parameter::isImplicit`, which 
checks the "mandated" status but does not refer to this concept at all?

-

PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2126011251

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change for addressing 
>> https://bugs.openjdk.org/browse/JDK-8332490?
>> 
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
>> then doesn't close the stream. This can lead to resource leak which can then 
>> cause OOM as noted in that JBS issue.
>> 
>> The commit in this PR closes the `InflaterInputStream` when the reads 
>> complete.
>> 
>> 
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>> 
>> and
>> 
>> 
>> ./build/macosx-aarch64/images/jdk/bin/java -jar 
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
>> -t max -f 1 -wi 2 -foe true
>> 
>> pass after this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a comment

Thank you Alan and Claes for the reviews.

-

PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2126009374

Integrated: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Jaikiran Pai]

On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

This pull request has now been integrated.

Changeset: 98f6a808
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/98f6a80852383dcbdad7292b7d269a8547d54d45
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod

8332490: JMH 
org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

Reviewed-by: aturbanov, redestad

-

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

Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v7]

[Joe Darcy]

On Tue, 12 Dec 2023 22:21:29 GMT, Joe Darcy  wrote:

>> Vicente Romero has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fixing comment
>
> As the core reflection code will encounter record classes compiled before and 
> after the javac code generation change, if the old behavior can be triggered 
> in javac using `--release $OLD`/`--source $OLD`, that would be helpful to 
> include as part of the testing.

> @jddarcy I have uploaded a new commit addressing your comments, thanks

Thanks @vicente-romero-oracle .

As a general comment, please update all the links to "mandated" so that the 
text "implicitly declared" get linked to the MANDATED enum constant. The term 
"mandated" may not familiar to core reflection readers, but is defined 
elsewhere in the platform so I don't think having the term "mandated" appears 
in the text in core reflection is necessarily helpful to readers. Concretely, 
replace javadoc like
`
implicitly ({@linkplain 
java.compiler/javax.lang.model.util.Elements.Origin#MANDATED mandated}) 
declared`

with

{@linkplain java.compiler/javax.lang.model.util.Elements.Origin#MANDATED 
implicitly declared}

Also, before this PR is pushed, please reflow the updated paragraphs so avoid 
introducing long lines of text.

-

PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2126006488

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

[Doug Lea]

On Wed, 22 May 2024 21:23:04 GMT, Sunmisc Unsafe  wrote:

>> After recheckiing, the best policy is to leave internal queues the same, but 
>> initialize external queues larger.
>
> Probably a misplaced post again, but why can't you allocate an array of arrays
> where the outer array is 30
> and the inner array will be doubled, 
> then the segment can be calculated by log(index)
> with: resize O(1), read/write O(1).
> 
> This approach can also be used in ConcurrentHashMap
> sample implementation 
> https://github.com/sunmisc/wormhole/blob/main/main/src/main/java/sunmisc/utils/concurrent/memory/ReferenceSegmentMemory.java
> 
> I don't know if this method exists anywhere else, I invented it, maybe it was 
> invented before me.

Thanks for link to sample code link. I had tried something like this in an 
analogous case in SubmissionPublisher, but couldn't reduce overhead enough to 
be worthwhile. I may try again there first,m since resizing there is more 
common and disruptive.

-

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

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

[Doug Lea]

On Thu, 16 May 2024 10:29:48 GMT, Sunmisc Unsafe  wrote:

>> Doug Lea has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments
>
> Maybe I don't quite understand, or I don't have proof, But wouldn't it be 
> better if invokeAll in FIFO mode (asyncMode) traverses the Future list as a 
> FIFO (currently it traverses in LIFO order)

@sunmisc invokeAll doesn't specify an exec or join order; forward exec snf 
inverse normally uses fewest resources.

-

PR Comment: https://git.openjdk.org/jdk/pull/19131#issuecomment-2125944250

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

[Doug Lea]

On Wed, 22 May 2024 15:51:05 GMT, Viktor Klang  wrote:

>> Doug Lea 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 36 additional commits 
>> since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - More performance tradoffs
>>  - Address review comments
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Repack some fields; adjust control flow
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Next version
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Reduce unneeded signals
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - ... and 26 more: https://git.openjdk.org/jdk/compare/44041895...f1fc4f3e
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1884:
> 
>> 1882: else
>> 1883: nc = (v.stackPred & LMASK) | (c & TC_MASK);
>> 1884: if (c == (c = compareAndExchangeCtl(c, nc | ac))) {
> 
> So the TTAS wasn't worth it on some architectures? 🤔

Right. so just added to code path

-

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

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

[Doug Lea]

On Wed, 22 May 2024 15:45:43 GMT, Viktor Klang  wrote:

>> Doug Lea 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 36 additional commits 
>> since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - More performance tradoffs
>>  - Address review comments
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Repack some fields; adjust control flow
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Next version
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - Reduce unneeded signals
>>  - Merge branch 'openjdk:master' into JDK-8322732
>>  - ... and 26 more: https://git.openjdk.org/jdk/compare/709b497f...f1fc4f3e
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 586:
> 
>> 584:  * term. We use Marsaglia XorShifts, seeded with the Weyl sequence
>> 585:  * from ThreadLocalRandom probes, which are cheap and
>> 586:  * suffice. Each queue's polling attempt o avoid becoming stuck
> 
> Suggestion:
> 
>  * suffice. Each queue's polling attempt to avoid becoming stuck

thanks; fixed.

-

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

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]

[Martin Doerr]

On Wed, 22 May 2024 14:47:43 GMT, Yudi Zheng  wrote:

>> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
>> candidate to its caller simplifies the intrinsic implementation in JIT 
>> compiler.
>
> Yudi Zheng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address comments.

PPC64 part and shared code looks correct. "java/math/BigInteger" tests have 
passed on PPC64. I didn't review the other platforms.

-

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18226#pullrequestreview-2072434441

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove DO_EARLY_BAILOUT

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/ed4451d1..027daf73

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=27
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=26-27

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

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

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

[Kevin Rushforth]

On Fri, 17 May 2024 13:38:25 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:
> 
>   Address review comments

I tested this with JavaFX and everything is working as I would expect. Without 
any options, I get the expected warnings, one time per modules for the three 
`javafx.*` modules that use JNI. If I pass the `--enable-native-access` options 
at runtime, listing those three modules, there is no warning. Further, I 
confirm that if I pass that option to jlink or jpackage when creating a custom 
runtime, there is no warning.

-

Marked as reviewed by kcr (Author).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2072430338

Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v3]

[Jorn Vernee]

On Tue, 21 May 2024 10:20:27 GMT, Maurizio Cimadamore  
wrote:

>> When creating a nested memory access var handle, we ensure that the segment 
>> is accessed at the correct alignment for the root layout being accessed. But 
>> we do not ensure that the segment has at least the size of the accessed root 
>> layout. Example:
>> 
>> 
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), 
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), 
>> PathElement.groupElement("x"));
>> 
>> 
>> If I have a memory segment whose size is 12, I can successfully call 
>> X_HANDLE on it with index 1, like so:
>> 
>> 
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>> 
>> 
>> This seems incorrect: after all, the provided segment doesn't have enough 
>> space to fit _two_ elements of the nested structs. 
>> 
>> This PR adds checks to detect this kind of scenario earlier, thus improving 
>> usability. To achieve this we could, in principle, just tweak 
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>> 
>> But doing so will add yet another redundant check on top of the other 
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). 
>> Instead, this PR rethinks how memory segment var handles are created, in 
>> order to eliminate redundant checks.
>> 
>> The main observation is that size and alignment checks depend on an 
>> _enclosing_ layout. This layout *might* be the very same value layout being 
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in 
>> the general case, the accessed value layout and the enclosing layout might 
>> differ (e.g. think about accessing a struct field).
>> 
>> Furthermore, the enclosing layout check depends on the _base_ offset at 
>> which the segment is accessed, _prior_ to any index computation that occurs 
>> if the accessed layout path has any open elements. It is important to notice 
>> that this base offset is only available when looking at the var handle that 
>> is returned to the user. For instance, an indexed var handle with 
>> coordinates:
>> 
>> 
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, 
>> long /* index 3 */)
>> 
>> 
>> Is obtained through adaptation, by taking a more basic var handle of the 
>> form:
>> 
>> 
>> (MemorySegment, long /* offset */)
>> 
>> 
>> And then injecting the result of the index multiplication into `offset`. As 
>> such, we can't add an enclosing layout check inside the var handle returned 
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the 
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typo in javadoc

Marked as reviewed by jvernee (Reviewer).

src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
 line 123:

> 121: static $type$ get(VarHandle ob, Object obb, long base) {
> 122: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
> 123: AbstractMemorySegmentImpl bb = checkReadOnly(obb, true);

For getter methods, which pass a constant `true` here, `checkReadOnly` 
essentially just does a null check and cast on the segment. Not sure if it's 
worth simplifying... (I'm happy if you want to leave it like this as well)

-

PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2072366007
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1610647133

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

[Sunmisc Unsafe]

On Sun, 12 May 2024 13:05:48 GMT, Doug Lea  wrote:

>> The goal is to reduce the worst form of contention: when queue arrays are 
>> laid out adjacently in memory. Increasing sizes has some impact but with 
>> diminishing returns. Thanks for the comment as a reminder that I haven't 
>> rechecked this lately in light of other layout changes. Will do.
>
> After recheckiing, the best policy is to leave internal queues the same, but 
> initialize external queues larger.

Probably a misplaced post again, but why can't you allocate an array of arrays
where the outer array is 30
and the inner array will be doubled, 
then the segment can be calculated by log(index)
with: resize O(1), read/write O(1).

This approach can also be used in ConcurrentHashMap
sample implementation 
https://github.com/sunmisc/wormhole/blob/main/main/src/main/java/sunmisc/utils/concurrent/memory/ReferenceSegmentMemory.java

I don't know if this method exists anywhere else, I invented it, maybe it was 
invented before me.

-

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

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

[Naoto Sato]

On Wed, 22 May 2024 21:06:11 GMT, Pavel Rappo  wrote:

> Okay, but can we call it a best-effort attempt to restore the echo state? I 
> guess, it is a judgement call.

That would be fair, and exactly what I am aiming for, considering we can do 
nothing for the `halt()` case.

-

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

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Claes Redestad]

On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change for addressing 
>> https://bugs.openjdk.org/browse/JDK-8332490?
>> 
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
>> then doesn't close the stream. This can lead to resource leak which can then 
>> cause OOM as noted in that JBS issue.
>> 
>> The commit in this PR closes the `InflaterInputStream` when the reads 
>> complete.
>> 
>> 
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>> 
>> and
>> 
>> 
>> ./build/macosx-aarch64/images/jdk/bin/java -jar 
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
>> -t max -f 1 -wi 2 -foe true
>> 
>> pass after this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a comment

Sorry, thought I had already approved. Comment is fine.

-

Marked as reviewed by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2072382952

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

[Pavel Rappo]

On Wed, 22 May 2024 19:32:07 GMT, Naoto Sato  wrote:

> > I might be confused, but what if the shutdown hook completes and then some 
> > application thread enters `readPassword`. If that thread manages to turn 
> > off echo before all other shutdown hooks complete, it might never execute 
> > `finally`, hence a race.
> 
> Yes, that is possible. However I would say that it is equally unlikely as an 
> app installs a shutdown hook with readPassword(), so I think this is OK too. 
> BTW, if an app issues `Runtime.halt()` while `readPassword()` is waiting, 
> then those shutdown hooks aren't even executed, thus ends up in not restoring 
> the echo without a race, which I think is OK as well.

Okay, but can we call it a best-effort attempt to restore the echo state? I 
guess, it is a judgement call.

-

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

Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`

[Naoto Sato]

On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons  wrote:

> With the advent of JEP 467, `///` comments may be treated as documentation 
> comments, and may be subject to the recently new `javac` warning about 
> "dangling doc comments" in unexpected places.
> 
> In keeping with the policy to keep the `java.base` module free of all `javac` 
> warnings, this patch proposes edits to existing uses of `///`.
> 
> There are two dominant policies in the proposed changes. 
> 1. A long horizontal line of `/` is replaced by `//-`
> 2. A long vertical series of lines beginning `///` is replaced by lines 
> beginning `//|`.
> 
> As with all style changes, I have also tried to honor local usage, for 
> consistency.
> 
> In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, 
> `CLOVER:OFF`).  I investigated the use of such comments to determine that the 
> exact form of the comment prefix was not significant. (Phew!)
> 
> 
> (This PR is informally blocked by JEP 467).

src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java 
line 122:

> 120:  * see store.c of gennorm for more information and values
> 121:  */
> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50  */

This source file is coming from the upstream ICU4J project. Even if this is a 
`non-standard` comment, I would keep it as it is to minimize the merge effort.

src/java.base/share/classes/jdk/internal/icu/lang/UCharacterDirection.java line 
61:

> 59: {
> 60: }
> 61: // CLOVER:ON

Same here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610588637
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610586405

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]

[Dean Long]

On Wed, 22 May 2024 14:28:41 GMT, Yudi Zheng  wrote:

>> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4693:
>> 
>>> 4691: const Register xlen  = r1;
>>> 4692: const Register z = r2;
>>> 4693: const Register zlen  = r3;
>> 
>> LibraryCallKit::inline_squareToLen() is still computing zlen and passing it 
>> as the 4th arg, even though the value is unused.
>
> ppc x86 are not using `multiply_to_len` for `generate_squareToLen`. I think 
> we still need to pass zlen for these platforms.

OK.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1610580527

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Chen Liang]

Hi Aman,
Even though the specification says "not in any particular order," the
getInterfaces and getMethods actually return an ordered array, in the order
these methods/interfaces are declared in their class files.

I believe you are decompiling the proxy classes generated by an older
version of the JDK; for example, back in JDK 8, the proxy methods were not
ordered because they were tracked in a HashMap:
https://github.com/openjdk/jdk8u/blob/6b53212ef78ad50f9eede829c5ff87cadcdb434b/jdk/src/share/classes/sun/misc/ProxyGenerator.java#L405
Which is no longer the case:
https://github.com/openjdk/jdk/blob/d59c12fe1041a1f61f68408241a9aa4d96ac4fd2/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L241

- Chen

On Wed, May 22, 2024 at 1:19 PM Aman Sharma  wrote:

> Hi,
>
>
> Another thing I wanted to look into in this thread was the order of fields
> in the Proxy classes generated. They are also based on the a number. The
> same proxy classes across different executions can have random order of
> `Method` fields and the methods could be mapped to different field names.
>
>
> For example, consider the proxy class based on `picocli.CommandLine
> `
> in two different executions.
>
> // fields and method are truncated for brevity
> public final class $Proxy9 extends Proxy implements CommandLine.Command {
> private static Method m1;
> private static Method m32;
> private static Method m21;
> private static Method m43;
> private static Method m36;
> private static Method m27;
>
> public final boolean helpCommand() throws  {
> try {
> return (Boolean)super.h.invoke(this, m32, (Object[])null);
> } catch (RuntimeException | Error var2) {
> throw var2;
> } catch (Throwable var3) {
> throw new UndeclaredThrowableException(var3);
> }
>  }
>
> // fields and method are truncated for brevity
> public final class $Proxy13 extends Proxy implements CommandLine.Command {
> private static Method m1;
> private static Method m29;
> private static Method m16;
> private static Method m40;
> private static Method m38;
> private static Method m12;
>
> public final boolean helpCommand() throws  {
> try {
> return (Boolean)super.h.invoke(this, m29, (Object[])null);
> } catch (RuntimeException | Error var2) {
> throw var2;
> } catch (Throwable var3) {
> throw new UndeclaredThrowableException(var3);
> }
> }
>
>
> Notice the difference in the order of fields and `helpCommand` method is
> mapped to a different field name in both classes. This happens because
> the method array returned by `getMethods` is not sorted in any particular
> order
> 
> when generating a proxy class. What dictates this order? And why is it
> not deterministic?
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
>  
> 
> https://algomaster99.github.io/
> --
> *From:* Aman Sharma
> *Sent:* Wednesday, May 22, 2024 4:12:19 PM
> *To:* Chen Liang
> *Cc:* David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
>
> Hi Chen,
>
>
> That's clear. Thanks for letting me know. I guess then Project Leyden is
> working on naming the hidden classes deterministically to achieve their
> goals .
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
>  
> 
> https://algomaster99.github.io/
> --
> *From:* Chen Liang 
> *Sent:* Wednesday, May 22, 2024 1:35:46 PM
> *To:* Aman Sharma
> *Cc:* David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
> Hi Aman,
> We have tried defining Proxy as hidden classes; a previous attempt was on
> hold because of issues with serialization. Otherwise, Proxies work great as
> hidden classes.
>
> Chen
>
> On Mon, May 20, 2024 at 7:56 AM Aman Sharma  wrote:
>
>> Hi David,
>>
>>
>> > I would not expect any class load
>> events.
>>
>>
>> I understand. I also haven't tried to intercept them but I see only one
>

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

[Naoto Sato]

On Wed, 22 May 2024 18:35:40 GMT, Pavel Rappo  wrote:

> I might be confused, but what if the shutdown hook completes and then some 
> application thread enters `readPassword`. If that thread manages to turn off 
> echo before all other shutdown hooks complete, it might never execute 
> `finally`, hence a race.

Yes, that is possible. However I would say that it is equally unlikely as an 
app installs a shutdown hook with readPassword(), so I think this is OK too. 
BTW, if an app issues `Runtime.halt()` while `readPassword()` is waiting, then 
those shutdown hooks aren't even executed, thus ends up in not restoring the 
echo without a race, which I think is OK as well.

-

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

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert last change to IndexOf.java

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/f4eefe1a..ed4451d1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=26
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=25-26

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

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

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 58 commits:

 - Merge branch 'openjdk:master' into indexof
 - un-helper-ize preload_needle_helper; try fix for macos build
 - Added header file
 - Adding exhaustive test
 - Added comments; move n-k<32 code up a level
 - Fixed CI compiles; re-factor UL processing
 - Addressing lots of comments.  Interim commit.
 - Rearrange; add lambdas for clarity
 - Merge remote-tracking branch 'origin/master' into indexof
 - Move arrays_equals back to c2_MacroAssembler
 - ... and 48 more: https://git.openjdk.org/jdk/compare/37c47785...f4eefe1a

-

Changes: https://git.openjdk.org/jdk/pull/16753/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=25
  Stats: 4303 lines in 16 files changed: 4140 ins; 26 del; 137 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

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

RE: stack overflow in regex engine

[mark.yagnatinsky]

Makes sense, thanks!

From: Philip Race 
Sent: Wednesday, May 22, 2024 2:41 PM
To: Yagnatinsky, Mark : IT (NYK) ; 
core-libs-dev@openjdk.org
Subject: Re: stack overflow in regex engine


CAUTION: This email originated from outside our organisation - 
philip.r...@oracle.com Do not click on links, 
open attachments, or respond unless you recognize the sender and can validate 
the content is safe.

On 5/22/24 10:51 AM, 
mark.yagnatin...@barclays.com wrote:
Ah, didn't realize P4 is default; that makes sense.
So I should not even be trying to derive omens from that.
So I guess only the assignee would know whether or not the status is closer to
"I was going to work on that next week" versus
"I totally forgot about that thing, and am about to forget about it again"
I'm quite sure he's on this list and will hopefully read the advocacy section 
of my email.

Um.  I feel awkward writing this paragraph because you know how OpenJDK works 
much better than I do, so it feels a bit silly to argue with you about it.  
But.  Um.
When you say "this is not the place to ask for fixes" ...
I was under the impression that "asking for fixes" actually does provide value, 
and not all of that value can be replaced by merely providing fixes.
In particular, asking for fixes gives maintainers a vague sense of how often 
people in the "real world" tend to run into an issue, which in turn informs how 
much "cost" is worth spending on addressing it.
(Where "cost" could mean things like "time" and also things like "this makes 
trickier and hence harder to maintain".)
In fact, I was under the impression that OpenJDK is slightly hostile to "big" 
fixes by "outsiders" because of the worry that there's now a big/complicated 
chunk of code that no one inside the project understands and yet the project is 
responsible for, and the original author might never be heard from again.

I think that is mainly the case for some new feature.
Or if you want to take some existing feature / functionality and re-write it in 
a different way.

True "bug fixes" to existing code are generally welcome, although that isn't 
the same as saying
they are quickly accepted.  They still need review and testing, and if the area 
is sensitive or complex that
can be quite time consuming on all ends of it. Which would all also be the case 
even if an
experienced contributor provided the fix.

-phil.



Anyway, thanks a bunch for responding; I was worried that no one would.

From: Philip Race 
Sent: Wednesday, May 22, 2024 11:54 AM
To: Yagnatinsky, Mark : IT (NYK) 
; 
core-libs-dev@openjdk.org
Subject: Re: stack overflow in regex engine


CAUTION: This email originated from outside our organisation - 
philip.r...@oracle.com Do not click on links, 
open attachments, or respond unless you recognize the sender and can validate 
the content is safe.
P4 is the default JBS priority, so sometimes it just means no one figured out 
the true priority.
But in general P4 bugs could be open for years, or even never get fixed.
The priority is also partially an assessment of where it falls as a priority 
for the JDK developers.
A user of JDK may have an entirely different perspective.
And that's why there are vendors who provide support for JDK. They can also 
arrange the backports you need.
But that's not done here. Here is where you come to participate and contribute 
fixes, not ask for fixes.
So my suggestion is to raise it via your support channel to your particular 
vendor who provided your binary.

-phil
On 5/21/24 8:46 PM, 
mark.yagnatin...@barclays.com wrote:
(Sorry about my previous "do I need to subscribe?" email; in retrospect that 
was needless noise.)
The purpose of this email is twofold: first, inquire about the status of ticket 
filed a few years ago, and second to point out some non-obvious reasons why it 
might be slightly more serious than it seems.
The ticket is this one 
https://bugs.openjdk.org/browse/JDK-8260866
 (stack overflow in regex matching quantified alternation)
The priority is listed as P4, which I guess means something like "medium" (more 
important than p5, but less than p3)
It also has a specific person assigned, which seems vaguely encouraging, but no 
updates at all in the years since it's been created, which seems less 
encouraging.
It was seemingly never once discussed on this mailing list, not even when it 
was first filed.
As an outsider, I'm not quite sure how to interpret all these various omens and 
turn them into guesses about its eventual fate.
Will it remain unfixed for another decade or two?  Will it be fixed in a few 
months, but then never backported to old 

Re: stack overflow in regex engine

[Philip Race]

On 5/22/24 10:51 AM, mark.yagnatin...@barclays.com wrote:


Ah, didn’t realize P4 is default; that makes sense.

So I should not even be trying to derive omens from that.

So I guess only the assignee would know whether or not the status is 
closer to


“I was going to work on that next week” versus

“I totally forgot about that thing, and am about to forget about it again”

I’m quite sure he’s on this list and will hopefully read the advocacy 
section of my email.


Um.  I feel awkward writing this paragraph because you know how 
OpenJDK works much better than I do, so it feels a bit silly to argue 
with you about it.  But.  Um.


When you say “this is not the place to ask for fixes” …

I was under the impression that “asking for fixes” actually does 
provide value, and not all of that value can be replaced by merely 
providing fixes.


In particular, asking for fixes gives maintainers a vague sense of how 
often people in the “real world” tend to run into an issue, which in 
turn informs how much “cost” is worth spending on addressing it.


(Where “cost” could mean things like “time” and also things like “this 
makes trickier and hence harder to maintain”.)


In fact, I was under the impression that OpenJDK is slightly hostile 
to “big” fixes by “outsiders” because of the worry that there’s now a 
big/complicated chunk of code that no one inside the project 
understands and yet the project is responsible for, and the original 
author might never be heard from again.




I think that is mainly the case for some new feature.
Or if you want to take some existing feature / functionality and 
re-write it in a different way.


True "bug fixes" to existing code are generally welcome, although that 
isn't the same as saying
they are quickly accepted.  They still need review and testing, and if 
the area is sensitive or complex that
can be quite time consuming on all ends of it. Which would all also be 
the case even if an

experienced contributor provided the fix.

-phil.


Anyway, thanks a bunch for responding; I was worried that no one would.

*From:*Philip Race 
*Sent:* Wednesday, May 22, 2024 11:54 AM
*To:* Yagnatinsky, Mark : IT (NYK) ; 
core-libs-dev@openjdk.org

*Subject:* Re: stack overflow in regex engine

CAUTION: This email originated from outside our organisation - 
philip.r...@oracle.com Do not click on links, open attachments, or 
respond unless you recognize the sender and can validate the content 
is safe.


P4 is the default JBS priority, so sometimes it just means no one 
figured out the true priority.

But in general P4 bugs could be open for years, or even never get fixed.
The priority is also partially an assessment of where it falls as a 
priority for the JDK developers.

A user of JDK may have an entirely different perspective.
And that's why there are vendors who provide support for JDK. They can 
also arrange the backports you need.
But that's not done here. Here is where you come to participate and 
contribute fixes, not ask for fixes.
So my suggestion is to raise it via your support channel to your 
particular vendor who provided your binary.


-phil

On 5/21/24 8:46 PM, mark.yagnatin...@barclays.com wrote:

(Sorry about my previous “do I need to subscribe?” email; in
retrospect that was needless noise.)

The purpose of this email is twofold: first, inquire about the
status of ticket filed a few years ago, and second to point out
some non-obvious reasons why it might be slightly more serious
than it seems.

The ticket is this one https://bugs.openjdk.org/browse/JDK-8260866


(stack overflow in regex matching quantified alternation)

The priority is listed as P4, which I guess means something like
“medium” (more important than p5, but less than p3)

It also has a specific person assigned, which seems vaguely
encouraging, but no updates at all in the years since it’s been
created, which seems less encouraging.

It was seemingly never once discussed on this mailing list, not
even when it was first filed.

As an outsider, I’m not quite sure how to interpret all these
various omens and turn them into guesses about its eventual fate.

Will it remain unfixed for another decade or two?  Will it be
fixed in a few months, but then never backported to old versions? 
Something else?  No one knows?

That concludes the status inquiry.  Now on to the advocacy.  Some
bugs are annoying, but once you hit them, you can work around them
by changing your code so it does not trigger the bug.

Note the phrase “your code” above.  This is much more awkward to
do if the bug triggered by third-party code you got from maven
central or something.

At that point your options are to either ask the third party
library to work around it, or else fork the de

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

[Pavel Rappo]

On Wed, 22 May 2024 17:20:02 GMT, Naoto Sato  wrote:

> Hi Pavel,
> 
> > If I read 
> > [this](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#shutdown)
> >  correctly, due to the mechanics of JVM exit, we simply don't know which 
> > thread finishes first: a thread that calls `readPassword` or the shutdown 
> > hook.
> 
> IIUC, the thread that waits on `readPassword()` is not a shutdown hook, 
> right? Then I think it is guaranteed that it is still waiting when all the 
> shutdown hooks are executed.
> 
> > If the shutdown hook finishes first, then a `readPassword` thread can 
> > corrupt the state: unlike the shutdown hook, which JVM _normally has to 
> > wait to complete_, the `readPassword` thread can be terminated at any 
> > moment. It might as well be terminated before `finally` but after 
> > `echo(false)`, in which case we end up with echo turned off.
> 
> After the shutdown hooks finish, then the VM is terminated 
> (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination).
>  In there:
> 
> ```
> finally clauses are not executed;
> ```
> 
> So I think the shutdown hook's restoreEcho has the final say, sans the 
> situation if some app installs a shutdown hook with `readPassword` but I 
> don't think we can guarantee that case, and I believe it is OK.

I might be confused, but what if the shutdown hook completes and then some 
application thread enters `readPassword`. If that thread manages to turn off 
echo before all other shutdown hooks complete, it might never execute 
`finally`, hence a race.

-

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

Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template

[Paul Sandoz]

On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li  wrote:

> Hi,
> Can you help to review this simple patch?
> Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not 
> necessary, could be removed.
> Thanks

The intrinsic implementation will not perform bounds checks. I think what you 
may be observing is the result of bounds checks when Java code is interpreted 
(or perhaps from compiled C1 code). You need to first ensure the code is 
compiled to C2 before executing with out of bounds values.

-

PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2125465612

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Aman Sharma]

Hi,


Another thing I wanted to look into in this thread was the order of fields in 
the Proxy classes generated. They are also based on the a number. The same 
proxy classes across different executions can have random order of `Method` 
fields and the methods could be mapped to different field names.


For example, consider the proxy class based on 
`picocli.CommandLine`
 in two different executions.

// fields and method are truncated for brevity
public final class $Proxy9 extends Proxy implements CommandLine.Command {
private static Method m1;
private static Method m32;
private static Method m21;
private static Method m43;
private static Method m36;
private static Method m27;

public final boolean helpCommand() throws  {
try {
return (Boolean)super.h.invoke(this, m32, (Object[])null);
} catch (RuntimeException | Error var2) {
throw var2;
} catch (Throwable var3) {
throw new UndeclaredThrowableException(var3);
}
 }

// fields and method are truncated for brevity
public final class $Proxy13 extends Proxy implements CommandLine.Command {
private static Method m1;
private static Method m29;
private static Method m16;
private static Method m40;
private static Method m38;
private static Method m12;

public final boolean helpCommand() throws  {
try {
return (Boolean)super.h.invoke(this, m29, (Object[])null);
} catch (RuntimeException | Error var2) {
throw var2;
} catch (Throwable var3) {
throw new UndeclaredThrowableException(var3);
}
}


Notice the difference in the order of fields and `helpCommand` method is mapped 
to a different field name in both classes. This happens because the method 
array returned by `getMethods` is not sorted in any particular 
order
 when generating a proxy class. What dictates this order? And why is it not 
deterministic?


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: Aman Sharma
Sent: Wednesday, May 22, 2024 4:12:19 PM
To: Chen Liang
Cc: David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`


Hi Chen,


That's clear. Thanks for letting me know. I guess then Project Leyden is 
working on naming the hidden classes deterministically to achieve their 
goals.


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: Chen Liang 
Sent: Wednesday, May 22, 2024 1:35:46 PM
To: Aman Sharma
Cc: David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

Hi Aman,
We have tried defining Proxy as hidden classes; a previous attempt was on hold 
because of issues with serialization. Otherwise, Proxies work great as hidden 
classes.

Chen

On Mon, May 20, 2024 at 7:56 AM Aman Sharma 
mailto:aman...@kth.se>> wrote:

Hi David,


> I would not expect any class load
events.


I understand. I also haven't tried to intercept them but I see only one 
approach right now to include them in an allowlist - 1) statically look for 
invocations of "Lookup::defineHiddenClass". 2) Instrument them so that its 
first argument "bytes" can be looked into upon. I haven't looked into it much 
because I did not have much idea about it. And they are hidden so it made it 
worse. 😅 Thanks for sharing the JEP!


>

java.lang.reflect.Proxy could define hidden classes to act as the proxy classes 
which implement proxy interfaces; from JEP 317


It says that Proxy classes will also become hidden classes. Is it underway? 
Right now one can intercept, transform them, and include them in an allowlist. 
What do you think of naming them independent of AtomicLong so that a proxy 
class generated at runtime is easy to lookup in the allowlist?



Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

RE: stack overflow in regex engine

[mark.yagnatinsky]

Ah, didn't realize P4 is default; that makes sense.
So I should not even be trying to derive omens from that.
So I guess only the assignee would know whether or not the status is closer to
"I was going to work on that next week" versus
"I totally forgot about that thing, and am about to forget about it again"
I'm quite sure he's on this list and will hopefully read the advocacy section 
of my email.

Um.  I feel awkward writing this paragraph because you know how OpenJDK works 
much better than I do, so it feels a bit silly to argue with you about it.  
But.  Um.
When you say "this is not the place to ask for fixes" ...
I was under the impression that "asking for fixes" actually does provide value, 
and not all of that value can be replaced by merely providing fixes.
In particular, asking for fixes gives maintainers a vague sense of how often 
people in the "real world" tend to run into an issue, which in turn informs how 
much "cost" is worth spending on addressing it.
(Where "cost" could mean things like "time" and also things like "this makes 
trickier and hence harder to maintain".)
In fact, I was under the impression that OpenJDK is slightly hostile to "big" 
fixes by "outsiders" because of the worry that there's now a big/complicated 
chunk of code that no one inside the project understands and yet the project is 
responsible for, and the original author might never be heard from again.

Anyway, thanks a bunch for responding; I was worried that no one would.

From: Philip Race 
Sent: Wednesday, May 22, 2024 11:54 AM
To: Yagnatinsky, Mark : IT (NYK) ; 
core-libs-dev@openjdk.org
Subject: Re: stack overflow in regex engine


CAUTION: This email originated from outside our organisation - 
philip.r...@oracle.com Do not click on links, 
open attachments, or respond unless you recognize the sender and can validate 
the content is safe.
P4 is the default JBS priority, so sometimes it just means no one figured out 
the true priority.
But in general P4 bugs could be open for years, or even never get fixed.
The priority is also partially an assessment of where it falls as a priority 
for the JDK developers.
A user of JDK may have an entirely different perspective.
And that's why there are vendors who provide support for JDK. They can also 
arrange the backports you need.
But that's not done here. Here is where you come to participate and contribute 
fixes, not ask for fixes.
So my suggestion is to raise it via your support channel to your particular 
vendor who provided your binary.

-phil
On 5/21/24 8:46 PM, 
mark.yagnatin...@barclays.com wrote:
(Sorry about my previous "do I need to subscribe?" email; in retrospect that 
was needless noise.)
The purpose of this email is twofold: first, inquire about the status of ticket 
filed a few years ago, and second to point out some non-obvious reasons why it 
might be slightly more serious than it seems.
The ticket is this one 
https://bugs.openjdk.org/browse/JDK-8260866
 (stack overflow in regex matching quantified alternation)
The priority is listed as P4, which I guess means something like "medium" (more 
important than p5, but less than p3)
It also has a specific person assigned, which seems vaguely encouraging, but no 
updates at all in the years since it's been created, which seems less 
encouraging.
It was seemingly never once discussed on this mailing list, not even when it 
was first filed.
As an outsider, I'm not quite sure how to interpret all these various omens and 
turn them into guesses about its eventual fate.
Will it remain unfixed for another decade or two?  Will it be fixed in a few 
months, but then never backported to old versions?  Something else?  No one 
knows?

That concludes the status inquiry.  Now on to the advocacy.  Some bugs are 
annoying, but once you hit them, you can work around them by changing your code 
so it does not trigger the bug.
Note the phrase "your code" above.  This is much more awkward to do if the bug 
triggered by third-party code you got from maven central or something.
At that point your options are to either ask the third party library to work 
around it, or else fork the dependency (which is not well supported by 
mainstream build tools (or maybe I'm just using them wrong)).
In this case, regular expressions are so ubiquitous that the bug is quite 
plausibly more likely to be triggered by some third party dependency than by 
code you own.
That was the case for me today: after spending hours trying to track down a 
stack overflow error I found the offending regex in a third party library.
The good news is that for the kinds of inputs we need to handle, it is indeed 
easy to substitute a much simpler regex that would avoid the issue.
The bad news is that it's not my code, so I can't.  I could petition the 

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  un-helper-ize preload_needle_helper; try fix for macos build

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/f002fd54..b0ef5e6f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=24
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=23-24

  Stats: 102 lines in 1 file changed: 5 ins; 91 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

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

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

[Naoto Sato]

On Tue, 21 May 2024 22:51:14 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:
> 
>   Used a dedicate lock for restoreEcho

Hi Pavel,

> If I read 
> [this](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#shutdown)
>  correctly, due to the mechanics of JVM exit, we simply don't know which 
> thread finishes first: a thread that calls `readPassword` or the shutdown 
> hook.

IIUC, the thread that waits on `readPassword()` is not a shutdown hook, right? 
Then I think it is guaranteed that it is still waiting when all the shutdown 
hooks are executed.

> If the shutdown hook finishes first, then a `readPassword` thread can corrupt 
> the state: unlike the shutdown hook, which JVM _normally has to wait to 
> complete_, the `readPassword` thread can be terminated at any moment. It 
> might as well be terminated before `finally` but after `echo(false)`, in 
> which case we end up with echo turned off.

After the shutdown hooks finish, then the VM is terminated 
(https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination).
 In there:


finally clauses are not executed;

So I think the shutdown hook's restoreEcho has the final say, sans the 
situation if some app installs a shutdown hook with `readPassword` but I don't 
think we can guarantee that case, and I believe it is OK.

-

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

Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template

[Hamlin Li]

On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li  wrote:

> Hi,
> Can you help to review this simple patch?
> Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not 
> necessary, could be removed.
> Thanks

In one side, yes, there is a gap in the tests.

In another side, I wonder if the check here is necessary (i.e. 
`VectorIntrinsics.checkIndex(vix, a.length)`). 
* For default java implementation, it's not necessary, as java code will check 
it anyway; 
* for intrinsic implementation, I saw relative information (array, offset, 
index map, offset in the map) are wrapped in LoadVectorGatherNode, I wonder it 
will also check IOOBE?

As I totally removed `VectorIntrinsics.checkIndex(vix, a.length);`, and in 
either conditions it still throws IOOBE when it should throw IOOBE.

-

PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2125320559

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Added header file

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/b6d77fe0..f002fd54

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=23
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=22-23

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

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

Re: RFR: 8332340: Add JavacBench as a test case for CDS [v3]

[Ioi Lam]

On Tue, 21 May 2024 21:03:20 GMT, Matias Saavedra Silva  
wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   @calvinccheung comments
>
> test/lib/jdk/test/lib/cds/CDSAppTester.java line 147:
> 
>> 145: }
>> 146: 
>> 147: private OutputAnalyzer dumpStaticArchive() throws Exception {
> 
> The code from 156 to 162 is repeated 3 times here, is it worth making another 
> function for this?

Thanks for the suggestion. I've refactored the code a bit to make it easier to 
write new execution modes (which will be needed in Leyden).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1610325214

Re: RFR: 8332340: Add JavacBench as a test case for CDS [v4]

[Ioi Lam]

> JavacBench is a test program that compiles 90 Java source files. It uses a 
> fair amount of invokedynamic callsites, so it's good for testing CDS support 
> for indy and lambda expressions.
> 
> This test was first integrated into the 
> [leyden](https://github.com/openjdk/leyden/tree/premain) repo. Hence some of 
> the files have copyrights in 2023.

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  @matias9927 comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19256/files
  - new: https://git.openjdk.org/jdk/pull/19256/files/ad97efa1..be84efcc

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

  Stats: 108 lines in 1 file changed: 43 ins; 36 del; 29 mod
  Patch: https://git.openjdk.org/jdk/pull/19256.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19256/head:pull/19256

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

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

[Scott Gibbons]

On Wed, 22 May 2024 16:25:24 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:
> 
>   Added comments; move n-k<32 code up a level

By her request

-

PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2125255793

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Adding exhaustive test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/f4ca4a5e..b6d77fe0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=22
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=21-22

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

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

Integrated: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic

[Volodymyr Paprotski]

On Tue, 2 Apr 2024 15:42:05 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.574 ± 
> 10.591  ops/s
> 
> Performance, **with intrinsics*...

This pull request has now been integrated.

Changeset: afed7d0b
Author:Volodymyr Paprotski 
Committer: Sandhya Viswanathan 
URL:   
https://git.openjdk.org/jdk/commit/afed7d0b0593864e5595840a6b645c210ff28c7c
Stats: 2409 lines in 36 files changed: 2093 ins; 156 del; 160 mod

8329538: Accelerate P256 on x86_64 using Montgomery intrinsic

Reviewed-by: ihse, ascarpino, sviswanathan

-

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

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

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Added comments; move n-k<32 code up a level

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/38868a35..f4ca4a5e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=21
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=20-21

  Stats: 214 lines in 4 files changed: 100 ins; 72 del; 42 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

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

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

[Viktor Klang]

On Wed, 22 May 2024 15:32:42 GMT, Doug Lea  wrote:

>> This set of changes address causes of poor utilization with small numbers of 
>> cores due to overly aggressive contention avoidance. A number of further 
>> adjustments were needed to still avoid most contention effects in 
>> deployments with large numbers of cores
>
> Doug Lea 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 36 additional commits since 
> the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - More performance tradoffs
>  - Address review comments
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Repack some fields; adjust control flow
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Next version
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Reduce unneeded signals
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - ... and 26 more: https://git.openjdk.org/jdk/compare/bdb1c0d3...f1fc4f3e

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2053:

> 2051: WorkQueue[] qs;
> 2052: int spins = ((short)(qc >>> TC_SHIFT) << 1) + 
> SPIN_WAITS + 1;
> 2053: while (((p = w.phase) & IDLE) != 0 && --spins > 0)

Nice catch on the spins :)

-

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

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

[Pavel Rappo]

On Tue, 21 May 2024 22:51:14 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:
> 
>   Used a dedicate lock for restoreEcho

I looked at the most recent commit, 
https://github.com/openjdk/jdk/pull/19184/commits/f69f747a8669647d6f924369fd98b945f9d0ae63.

You are right, the [race][] that we hypothesised previously when `restoreEcho` 
was `volatile` is no longer present. However, there's another race. If it's of 
any consolation, that new race isn't new at all: it was there before. (I know 
it's frustrating to be discussing the same problem over and over again, but 
we're making progress.)

What we want to do is restore the echo state immediately before JVM exits. We 
achieve this by installing a shutdown hook which restores the state. However, 
there's no coordination between the shutdown hook and any thread that also 
modifies the state.

If I read [this][shutdown-sequence] correctly, due to the mechanics of JVM 
exit, we simply don't know which thread finishes first: a thread that calls 
`readPassword` or the shutdown hook. If the shutdown hook finishes first, then 
a `readPassword` thread can corrupt the state: unlike the shutdown hook, which 
JVM _normally has to wait to complete_, the `readPassword` thread can be 
terminated at any moment. It might as well be terminated before `finally` but 
after `echo(false)`, in which case we end up with echo turned off.

What I'm saying is that we should ensure that the shutdown hook has the final 
say: once completed, no one should modify echo status. I understand that this 
means more involved communication between threads.

Does it make sense?

[shutdown-sequence]: 
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#shutdown
[race]: https://github.com/openjdk/jdk/pull/19184#issuecomment-2109779741

-

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

Re: stack overflow in regex engine

[Philip Race]

P4 is the default JBS priority, so sometimes it just means no one 
figured out the true priority.

But in general P4 bugs could be open for years, or even never get fixed.
The priority is also partially an assessment of where it falls as a 
priority for the JDK developers.

A user of JDK may have an entirely different perspective.
And that's why there are vendors who provide support for JDK. They can 
also arrange the backports you need.
But that's not done here. Here is where you come to participate and 
contribute fixes, not ask for fixes.
So my suggestion is to raise it via your support channel to your 
particular vendor who provided your binary.


-phil


On 5/21/24 8:46 PM, mark.yagnatin...@barclays.com wrote:


(Sorry about my previous “do I need to subscribe?” email; in 
retrospect that was needless noise.)


The purpose of this email is twofold: first, inquire about the status 
of ticket filed a few years ago, and second to point out some 
non-obvious reasons why it might be slightly more serious than it seems.


The ticket is this one https://bugs.openjdk.org/browse/JDK-8260866 
(stack overflow in regex matching quantified alternation)


The priority is listed as P4, which I guess means something like 
“medium” (more important than p5, but less than p3)


It also has a specific person assigned, which seems vaguely 
encouraging, but no updates at all in the years since it’s been 
created, which seems less encouraging.


It was seemingly never once discussed on this mailing list, not even 
when it was first filed.


As an outsider, I’m not quite sure how to interpret all these various 
omens and turn them into guesses about its eventual fate.


Will it remain unfixed for another decade or two?  Will it be fixed in 
a few months, but then never backported to old versions?  Something 
else?  No one knows?


That concludes the status inquiry.  Now on to the advocacy.  Some bugs 
are annoying, but once you hit them, you can work around them by 
changing your code so it does not trigger the bug.


Note the phrase “your code” above.  This is much more awkward to do if 
the bug triggered by third-party code you got from maven central or 
something.


At that point your options are to either ask the third party library 
to work around it, or else fork the dependency (which is not well 
supported by mainstream build tools (or maybe I’m just using them wrong)).


In this case, regular expressions are so ubiquitous that the bug is 
quite plausibly more likely to be triggered by some third party 
dependency than by code you own.


That was the case for me today: after spending hours trying to track 
down a stack overflow error I found the offending regex in a third 
party library.


The good news is that for the kinds of inputs we need to handle, it is 
indeed easy to substitute a much simpler regex that would avoid the issue.


The bad news is that it’s not my code, so I can’t.  I could petition 
the maintainers of the library, but this is not great because:


First, maybe the version I’m on is not longer even supported, and 
newer versions are not compatible,


Second, it may take them a while to fix it, and third, it is wasteful 
(and inelegant) to have workarounds slowly percolate throughout the 
Java ecosystem instead of fixing the problem at the root.


The other annoying thing here is that even when you have “enough” 
stack space to avoid crashing, using it may not be quite “free”.


For instance, project loom’s foundational premise seems to be that 
“most threads have oversized stacks; we can have more threads if we 
start off with small stacks and grow them only when needed”.


This would be false when the thread in question uses a regex with 
quantified alternation.


(Since many Loom threads will be based on the same Runnable, it’s a 
pretty safe bet that if one of them uses this feature, many will, so 
you can’t assume it will “average out”.)


There are other reasons besides loom to be low on stack space; maybe 
you’re using some crazy framework(s) that like(s) to have call stacks 
that are crazy deep.


Or maybe you’re running with -Xss set pretty low.  Or you passed a 
small value for stack space to the Thread constructor.


Or maybe none of these things are true, but in most operating systems 
a thread stack costs “real” memory in proportion to its 
high-watermark, so even a SINGLE heavy regex in the lifetime of a 
thread is tantamount to a memory leak of hundreds of kilobytes.


Practicalities aside, I don’t like it when code consumes “surprising” 
types of resources, or surprising amounts of them.


For instance, you wouldn’t expect a sorting function to spawn threads 
behind your back, unless it was called “parallel sort” or something 
like that.


You wouldn’t expect it to allocate multi-gigabyte arrays, nor to 
perform I/O.


Similarly, most functions need only O(1) stack space, so this tends to 
be the default assumption unless the docs explicitly call out “this 
thing might throw stack ov

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

[Viktor Klang]

On Wed, 22 May 2024 15:32:42 GMT, Doug Lea  wrote:

>> This set of changes address causes of poor utilization with small numbers of 
>> cores due to overly aggressive contention avoidance. A number of further 
>> adjustments were needed to still avoid most contention effects in 
>> deployments with large numbers of cores
>
> Doug Lea 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 36 additional commits since 
> the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - More performance tradoffs
>  - Address review comments
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Repack some fields; adjust control flow
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Next version
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Reduce unneeded signals
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - ... and 26 more: https://git.openjdk.org/jdk/compare/d5552864...f1fc4f3e

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1440:

> 1438: while (task != null) {
> 1439: task.doExec();
> 1440: task = nextLocalTask(fifo);

Clean! 👍

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1884:

> 1882: else
> 1883: nc = (v.stackPred & LMASK) | (c & TC_MASK);
> 1884: if (c == (c = compareAndExchangeCtl(c, nc | ac))) {

So the TTAS wasn't worth it on some architectures? 🤔

-

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

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

[Viktor Klang]

On Wed, 22 May 2024 15:32:42 GMT, Doug Lea  wrote:

>> This set of changes address causes of poor utilization with small numbers of 
>> cores due to overly aggressive contention avoidance. A number of further 
>> adjustments were needed to still avoid most contention effects in 
>> deployments with large numbers of cores
>
> Doug Lea 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 36 additional commits since 
> the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - More performance tradoffs
>  - Address review comments
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Repack some fields; adjust control flow
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Next version
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - Reduce unneeded signals
>  - Merge branch 'openjdk:master' into JDK-8322732
>  - ... and 26 more: https://git.openjdk.org/jdk/compare/72365ee9...f1fc4f3e

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 586:

> 584:  * term. We use Marsaglia XorShifts, seeded with the Weyl sequence
> 585:  * from ThreadLocalRandom probes, which are cheap and
> 586:  * suffice. Each queue's polling attempt o avoid becoming stuck

Suggestion:

 * suffice. Each queue's polling attempt to avoid becoming stuck

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 597:

> 595:  * async mode.
> 596:  *
> 597:  * Deactivation. When no tasks are found by a worker in runWorker,

Suggestion:

 * Deactivation: When no tasks are found by a worker in runWorker,

-

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

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

[Doug Lea]

> This set of changes address causes of poor utilization with small numbers of 
> cores due to overly aggressive contention avoidance. A number of further 
> adjustments were needed to still avoid most contention effects in deployments 
> with large numbers of cores

Doug Lea 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 36 additional commits since the 
last revision:

 - Merge branch 'openjdk:master' into JDK-8322732
 - More performance tradoffs
 - Address review comments
 - Merge branch 'openjdk:master' into JDK-8322732
 - Repack some fields; adjust control flow
 - Merge branch 'openjdk:master' into JDK-8322732
 - Next version
 - Merge branch 'openjdk:master' into JDK-8322732
 - Reduce unneeded signals
 - Merge branch 'openjdk:master' into JDK-8322732
 - ... and 26 more: https://git.openjdk.org/jdk/compare/5072e63c...f1fc4f3e

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19131/files
  - new: https://git.openjdk.org/jdk/pull/19131/files/8689ac12..f1fc4f3e

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

  Stats: 64729 lines in 1007 files changed: 46563 ins; 12655 del; 5511 mod
  Patch: https://git.openjdk.org/jdk/pull/19131.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131

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

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]

[Yudi Zheng]

On Wed, 22 May 2024 14:47:43 GMT, Yudi Zheng  wrote:

>> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
>> candidate to its caller simplifies the intrinsic implementation in JIT 
>> compiler.
>
> Yudi Zheng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address comments.

@theRealAph @TheRealMDoerr @RealFYang @offamitkumar could you please help 
review the aarch64/ppc/riscv/s390 changes?

-

PR Comment: https://git.openjdk.org/jdk/pull/18226#issuecomment-2125073511

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null

[Roger Riggs]

On Tue, 21 May 2024 14:28:38 GMT, Matthias Baesken  wrote:

> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing 
> jtreg tests afterwards I run into this error :
> 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: 
> null pointer passed as argument 2, which is declared to never be null
> #0 0x7fd95bec78d8 in spawnChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
> #1 0x7fd95bec78d8 in startChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
> #3 0x7fd93797a06d ()
> 
> this is the memcpy call getting an unexpected null pointer :
> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
> Something similar was discussed and fixed here 
> https://bugs.python.org/issue27570 for Python .
> 
> Similar issue in OpenJDK _ 
> https://bugs.openjdk.org/browse/JDK-8332473
> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed 
> as argument 1, which is declared to never be null

src/java.base/unix/native/libjava/ProcessImpl_md.c line 565:

> 563: memcpy(buf+offset, c->pdir, sp.dirlen);
> 564: }
> 565: offset += sp.dirlen;

I'd be inclined to check sp.dirlen > 0 in the `if` and move the offset += 
inside too. Like:
Suggestion:

if (sp.dirlen > 0 && c->pdir != NULL) {
memcpy(buf+offset, c->pdir, sp.dirlen);
offset += sp.dirlen;
}

The behavior is correct either way.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19329#discussion_r1610170951

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

[Scott Gibbons]

On Fri, 17 May 2024 22:37:13 GMT, Sandhya Viswanathan 
 wrote:

>> Not sure what you mean here.  I *think* you mean that hsLength is not the 
>> length of the remaining bytes in the haystack, but the actual length.  There 
>> may be an issue if that is correct, right?  I'll investigate.
>
> Yes, that is what I meant. Thanks for investigating.

I've moved the code checking for (n-k)<32 to `big_case_loop_helper`, so there's 
no need for this in here any longer.  Removing unneeded parameters from 
`compare_big_haystack_to_needle`.

-

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

Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v11]

[Jan Kratochvil]

> The testcase requires root permissions.
> 
> Designed by  Severin Gehwolf, implemented by Jan Kratochvil.

Jan Kratochvil has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 74 commits:

 - .
 - .
 - Merge branch 'jdk-8331560-cgroup-controller-delegation-merge-diamond' into 
jdk-8331560-cgroup-controller-delegation-merge-cgroup
 - diamond
 - Merge branch 'jerboaarefactor-merge-cgroup' into 
jdk-8331560-cgroup-controller-delegation-merge-cgroup
 - Merge branch 'jerboaarefactor-merge' into jerboaarefactor-merge-cgroup
 - Merge branch 'master' into jerboaarefactor-merge
 - whitespace fix
 - centos7 compat
 - 64a5feb6: 
 - ... and 64 more: https://git.openjdk.org/jdk/compare/3d511ff6...25c0287d

-

Changes: https://git.openjdk.org/jdk/pull/17198/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17198&range=10
  Stats: 2721 lines in 22 files changed: 1298 ins; 1081 del; 342 mod
  Patch: https://git.openjdk.org/jdk/pull/17198.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17198/head:pull/17198

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

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

[Scott Gibbons]

On Mon, 15 Jan 2024 13:30:42 GMT, Andrey Turbanov  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 22 commits:
>> 
>>  - Merge branch 'openjdk:master' into indexof
>>  - Merge branch 'openjdk:master' into indexof
>>  - Addressing review comments.
>>  - Fix for JDK-8321599
>>  - Support UU IndexOf
>>  - Only use optimization when EnableX86ECoreOpts is true
>>  - Fix whitespace
>>  - Merge branch 'openjdk:master' into indexof
>>  - Comments; added exhaustive-ish test
>>  - Subtracting 0x10 twice.
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 220:
> 
>> 218: 
>> 219: for (int x = 0; x < 100; x++) {
>> 220: if(make_new) {
> 
> Suggestion:
> 
> if (make_new) {

Fixed.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 262:
> 
>> 260:   }
>> 261: 
>> 262: if(make_new)
> 
> Suggestion:
> 
> if (make_new)

Fixed.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 295:
> 
>> 293:   }
>> 294: 
>> 295:   if(make_new) testIndex = getRandomIndex(-100, 100);
> 
> Suggestion:
> 
>   if (make_new) testIndex = getRandomIndex(-100, 100);

Fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610093771
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610094790
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610097958

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

[Scott Gibbons]

On Wed, 15 May 2024 19:18:27 GMT, Volodymyr Paprotski  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rearrange; add lambdas for clarity
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 54:
> 
>> 52:   // for (int i = 1; i < 128; i++) {
>> 53:   //   haystack_16[i] = (char) (i);
>> 54:   // }
> 
> dead code

Removed.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 83:
> 
>> 81:   shs = "$&),,18+-!'8)+";
>> 82:   endNeedle = "8)-";
>> 83:   l_offset = 9;
> 
> dead code

Fixed.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 237:
> 
>> 235: + sourceBuffer.toString() + " len Buffer = " + 
>> sourceBuffer.toString().length());
>> 236: System.err.println("  naive = " + 
>> naiveFind(sourceBuffer.toString(), targetString, 0) + ", IndexOf = "
>> 237: + sourceBuffer.indexOf(targetString));
> 
> More tracing left behind here and rest of this function (original just 
> recorded failure and moved along)

I think it's more valuable for a test to print out what it can when a failure 
occurs rather than just saying "failed".

> test/jdk/java/lang/StringBuffer/IndexOf.java line 284:
> 
>> 282: 
>> 283:   // Note: it is possible although highly improbable that failCount will
>> 284:   // be > 0 even if everthing is working ok
> 
> This sounds like either a bug or a testcase bug? Same as line 301, `extremely 
> remote possibility of > 1 match`?

This was there from the original author.  I think they were trying to infer 
that a match could occur in the rare case that the same random string was 
produced.  They're random after all, and there's no reason the same sequence 
could be generated.

> test/micro/org/openjdk/bench/java/lang/StringIndexOfHuge.java line 81:
> 
>> 79: lateMatchString16 = 
>> dataStringHuge16.substring(dataStringHuge16.length() - 31);
>> 80: 
>> 81: searchString = "oscar";
> 
> Would had liked to see a few more small needles (i.e. to test/verify 
> individual switch statement cases)

I'm hoping we can incorporate your test to cover more cases :-).

> test/micro/org/openjdk/bench/java/lang/StringIndexOfHuge.java line 132:
> 
>> 130:   @Benchmark
>> 131:   public int searchHugeLargeSubstring() {
>> 132:   return dataStringHuge.indexOf("B".repeat(30) + "X" + 
>> "A".repeat(30), 74);
> 
> .repeat() call and string concatenation shouldn't be part of the benchmark 
> (here and several other @Benchmark functions in this file) since it will 
> detract from the measurement.
> 
> (String concatenation gets converted (by javac) into 
> StringBuilder().append().append()append().toString())

Since we're only concerned with the delta of performance, does this really 
matter?  Can you suggest an alternative?

> test/micro/org/openjdk/bench/java/lang/StringIndexOfHuge.java line 242:
> 
>> 240:   @Benchmark
>> 241:   public int search16HugeLargeSubstring16() {
>> 242: return dataStringHuge16.indexOf("B".repeat(30) + "X" + 
>> "A".repeat(30), 74);
> 
> `search16HugeLargeSubstring16` implies UU, but with `"B".repeat(30) + "X" + 
> "A".repeat(30)` is UL

Fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610131285
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610134566
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610138116
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610142104
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610130140
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610126743
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610128630

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

[Scott Gibbons]

On Tue, 9 Jan 2024 15:14:41 GMT, Emanuel Peter  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'openjdk:master' into indexof
>>  - Addressing review comments.
>>  - Fix for JDK-8321599
>>  - Support UU IndexOf
>>  - Only use optimization when EnableX86ECoreOpts is true
>>  - Fix whitespace
>>  - Merge branch 'openjdk:master' into indexof
>>  - Comments; added exhaustive-ish test
>>  - Subtracting 0x10 twice.
>>  - Stomped on r13 in switch branch calculation
>>  - ... and 11 more: https://git.openjdk.org/jdk/compare/8a4dc79e...600377b0
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 34:
> 
>> 32: public class IndexOf {
>> 33: 
>> 34:   static Random generator = new Random(1999);
> 
> Would it be an alternative to use the this:
> 
> import jdk.test.lib.Utils;
> ...
> Random random = Utils.getRandomInstance();
> 
> This has a random seed, but it is always printed in the output and can be set 
> via a test-flag.

Changed.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 44:
> 
>> 42: }
>> 43: System.out.println("");
>> 44: generator.setSeed(1999);
> 
> Is there a good reason for a fixed seed?

Nope :-). Needed consistency during testing.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610087089
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610088114

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

[Scott Gibbons]

On Mon, 22 Jan 2024 07:08:31 GMT, Jatin Bhateja  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 505:
>> 
>>> 503: __ cmpb(Address(rbx, r15, Address::times_1, -0xa), rax);
>>> 504: __ jne(L_top_loop_1);
>>> 505: __ jmp(L_0x406019);
>> 
>> Instead of having special handling for each tail size (3 - 31 bytes), can we 
>> directly use 32 bytes VMASKMOVPS with appropriate mask for different tail 
>> sizes and only residual part (0 - 3 bytes) can fall over to scalar tail.
>
> Basically tail size can be rounded to nearest multiple of doubleword.

I have since changed the algorithm due to request from @sviswa7

-

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

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

[Scott Gibbons]

On Mon, 8 Jan 2024 10:32:51 GMT, Jatin Bhateja  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Addressing review comments.
>
> src/hotspot/share/opto/library_call.cpp line 1273:
> 
>> 1271:   Node* result = nullptr;
>> 1272: 
>> 1273:   if ((StubRoutines::string_indexof() != nullptr) && (ae == 
>> StrIntrinsicNode::LL)) {
> 
> Why are we not calling stub for StrIntrinsicNode::UU

Stub being called for LL, UL, and UU now.

-

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

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

[Scott Gibbons]

On Tue, 16 Jan 2024 13:26:15 GMT, Jatin Bhateja  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 22 commits:
>> 
>>  - Merge branch 'openjdk:master' into indexof
>>  - Merge branch 'openjdk:master' into indexof
>>  - Addressing review comments.
>>  - Fix for JDK-8321599
>>  - Support UU IndexOf
>>  - Only use optimization when EnableX86ECoreOpts is true
>>  - Fix whitespace
>>  - Merge branch 'openjdk:master' into indexof
>>  - Comments; added exhaustive-ish test
>>  - Subtracting 0x10 twice.
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 417:
> 
>> 415: __ cmpl(Address(rbx, r15, Address::times_1, -0x14), rax);
>> 416: __ jne(L_top_loop_1);
>> 417: __ jmp(L_0x406019);
> 
> For cases which are multiple of 4 bytes we can use VMASKMOVPS (conditional 
> moves) and VPTEST.

Not sure what you mean here.  Could you elaborate (although it may be moot 
after all the changes)?

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1526:
> 
>> 1524: __ movq(rdx, r8);
>> 1525: __ movq(rcx, r9);
>> 1526: #endif
> 
> Can we spill them into XXMs, to save costly stack operations.

Changed.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1545:
> 
>> 1543: //   return 0;
>> 1544: // }
>> 1545: __ movq(r12, rcx);
> 
> Check for K == 0 should use rsi.

k is needle length, which is rcx.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1545:
> 
>> 1543: //   return 0;
>> 1544: // }
>> 1545: __ movq(r12, rcx);
> 
> Kindly use meaningful variable and label names. It will ease the review 
> process and maintenance.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1551:
> 
>> 1549: __ movq(r15, rsi);
>> 1550: __ movq(r11, rdi);
>> 1551: __ cmpq(rsi, 0x20);
> 
> Comparisons with 32 bit integer length can use cmpl instead of cmpq, this may 
> save emitting REX encoding prefix if index is allocated a GPR from lower 
> register bank (no need for setting REX.W).

I fixed as many as I could find.  Thanks.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1552:
> 
>> 1550: __ movq(r11, rdi);
>> 1551: __ cmpq(rsi, 0x20);
>> 1552: __ jb(L_small_string);
> 
> All the comparisons against needle length are signed integer comparisons, so 
> jb should be replaced by jl

I'm treating everything as unsigned except where intentional negative values 
are used.  It never makes sense for needle length to be negative.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610118449
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610110754
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610105405
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610111320
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610113343
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610116033

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

[Scott Gibbons]

On Mon, 6 May 2024 23:19:07 GMT, Sandhya Viswanathan  
wrote:

>> 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 329:
> 
>> 327: 
>> 
>> 328: 
>> 329: __ bind(L_begin);
> 
> So far we have handled haystack <= 32 and needle_size <= 5 (?) in bytes. A 
> high level algorithm description here is needed in comments to follow the 
> code below.  A description of what are the various paths in terms of haystack 
> and needle sizes and how to reason the assembly code below and make sure that 
> all the paths are taken care of. Also the abstraction level suddenly changes 
> here to detailed code below instead of methods for the various paths.

I added a description.  Can you please check to ensure it meets your objective? 
 Thanks.

-

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

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

[Scott Gibbons]

On Mon, 26 Feb 2024 14:50:30 GMT, Jatin Bhateja  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Addressed some review coments; replaced hard-coded registers with 
>> descriptive names.
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 303:
> 
>> 301:   __ subq(rdi, rax);
>> 302:   __ movq(rdx, rdi);
>> 303:   __ andq(rdx, -16);
> 
> Hi @asgibbons , may I request you to please use meaningful names instead of 
> directly using actual GPR names to ease the review process.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 777:
> 
>> 775:   __ movq(rax, rbx);
>> 776:   __ movq(rbx, r14);
>> 777:   __ leaq(r15, Address(r12, -0x2));
> 
> Kindly use semantically meaningful names instead of direct GPR names.

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610121347
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610121724

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v5]

[Yudi Zheng]

On Wed, 17 Apr 2024 20:04:44 GMT, Dean Long  wrote:

>> Yudi Zheng has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   address comment.
>
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6662:
> 
>> 6660:   push(tmp5);
>> 6661: 
>> 6662:   push(xlen);
> 
> There may be an opportunity here (separate RFE?) to get rid of the 
> save/restore for these.  I don't think it's necessary if this is called as 
> part of a C2 stub.

In the Graal port we did get rid of these save/restore.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1610126799

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]

[Yudi Zheng]

> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
> candidate to its caller simplifies the intrinsic implementation in JIT 
> compiler.

Yudi Zheng has updated the pull request incrementally with one additional 
commit since the last revision:

  address comments.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18226/files
  - new: https://git.openjdk.org/jdk/pull/18226/files/72ba58ce..7c6023f8

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

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

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

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v3]

[Yudi Zheng]

On Mon, 20 May 2024 10:41:36 GMT, Bhavana Kilambi  wrote:

>> @dafedafe @dean-long please take a look and let me know if there are further 
>> issues, thanks!
>
> Hi @mur47x111, do you happen to have any performance results with this patch?

@Bhavana-Kilambi the performance result for x86 is at 
https://github.com/openjdk/jdk/pull/18226#issuecomment-2007922439 . It is 
expected to be negligible.

-

PR Comment: https://git.openjdk.org/jdk/pull/18226#issuecomment-2124984579

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v5]

[Yudi Zheng]

On Wed, 17 Apr 2024 19:33:01 GMT, Dean Long  wrote:

>> Yudi Zheng has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   address comment.
>
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4670:
> 
>> 4668: const Register tmp5  = r15;
>> 4669: const Register tmp6  = r16;
>> 4670: const Register tmp7  = r17;
> 
> Why not minimize changes and continue to use r5 for tmp0?  I see no need for 
> r17 or to reassign all the other tmp registers.

Was attempting to align the suffixes. Will revert.

> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4693:
> 
>> 4691: const Register xlen  = r1;
>> 4692: const Register z = r2;
>> 4693: const Register zlen  = r3;
> 
> LibraryCallKit::inline_squareToLen() is still computing zlen and passing it 
> as the 4th arg, even though the value is unused.

ppc x86 are not using `multiply_to_len` for `generate_squareToLen`. I think we 
still need to pass zlen for these platforms.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1610083476
PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1610088021

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

[Scott Gibbons]

On Tue, 9 Jan 2024 15:06:10 GMT, Emanuel Peter  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'openjdk:master' into indexof
>>  - Addressing review comments.
>>  - Fix for JDK-8321599
>>  - Support UU IndexOf
>>  - Only use optimization when EnableX86ECoreOpts is true
>>  - Fix whitespace
>>  - Merge branch 'openjdk:master' into indexof
>>  - Comments; added exhaustive-ish test
>>  - Subtracting 0x10 twice.
>>  - Stomped on r13 in switch branch calculation
>>  - ... and 11 more: https://git.openjdk.org/jdk/compare/8a4dc79e...600377b0
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1608:
> 
>> 1606: //   vector compares when size is 2 * VEC_SIZE or less. 388. 
>> Use 4
>> 1607: //   vector compares when size is 4 * VEC_SIZE or less. 399. 
>> Use 8
>> 1608: //   vector compares when size is 8 * VEC_SIZE or less.  */
> 
> Is this formatting intended?

Fixed

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1672:
> 
>> 1670: 
>> 1671: // 98  VPCMPEQ VEC_SIZE(%rdi), %ymm2, %ymm2
>> 1672: // 99  vpmovmskb %ymm2, %eax
> 
> It seems that here the comments and code is strangely interleaved / shifted. 
> What is this all for?

All this has been remedied

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 2301:
> 
>> 2299: // 388 setg%dl
>> 2300: // 389 leal-1(%rdx, %rdx), %eax
>> 2301: __ movzbl(rcx, Address(rsi, rax, Address::times_1, -0x20));
> 
> Down here it is even worse

All this has been remedied

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610074501
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610076284
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1610076661

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

[Volodymyr Paprotski]

On Tue, 21 May 2024 17:41:46 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 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:
> 
>  - Merge remote-tracking branch 'origin/master' into ecc-montgomery
>  - shenandoah verifier
>  - comments from Sandhya
>  - whitespace
>  - add message back
>  - whitespace
>  - Use AffinePoint to exit Montgomery domain
>
>Style notes:
>Affine.equals()
>- Mismatched fields only appear to be used from testing, perhaps 
> should be moved there instead
>Affine.getX(boolean)|getY(boolean)
>- "Passing flag is bad design" - cleanest/performant alternative to 
> several instanceof checks
>- needed to convert Affine to Projective (need to stay in montgomery 
> domain)
>ECOperations.PointMultiplier
>   - changes could probably be restored to original (since ProjectivePoint 
> handling no longer required)
>   - consider these changes an improvement? (fewer nested classes)
>   - was an inner-class but not using inner-class features (i.e. ecOps 
> variable should be converted)
>  - whitespace
>  - Comments from Tony and Jatin
>  - Comments from Jatin and Tony
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/c0032e2c...b1a33004

Thanks Tobi!

-

PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2124924526

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

[Pavel Rappo]

> 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 incrementally with one additional 
commit since the last revision:

  Force reasonable terminal size
  
  JLine outputs unexpected stuff if the terminal isn't dumb and small,
  such as that of our CI machines:
  
  if (newLines.size() > displaySize && !isTerminalDumb()) {
  StringBuilder sb = new StringBuilder(">");
  
  This causes the IO test to fail with timeout, because the expected
  prompt is never matched. To avoid that, we reasonably size the
  terminal.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19112/files
  - new: https://git.openjdk.org/jdk/pull/19112/files/719560f6..e252c5d8

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

  Stats: 8 lines in 2 files changed: 1 ins; 6 del; 1 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

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Aman Sharma]

Hi Chen,


That's clear. Thanks for letting me know. I guess then Project Leyden is 
working on naming the hidden classes deterministically to achieve their 
goals.


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: Chen Liang 
Sent: Wednesday, May 22, 2024 1:35:46 PM
To: Aman Sharma
Cc: David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

Hi Aman,
We have tried defining Proxy as hidden classes; a previous attempt was on hold 
because of issues with serialization. Otherwise, Proxies work great as hidden 
classes.

Chen

On Mon, May 20, 2024 at 7:56 AM Aman Sharma 
mailto:aman...@kth.se>> wrote:

Hi David,


> I would not expect any class load
events.


I understand. I also haven't tried to intercept them but I see only one 
approach right now to include them in an allowlist - 1) statically look for 
invocations of "Lookup::defineHiddenClass". 2) Instrument them so that its 
first argument "bytes" can be looked into upon. I haven't looked into it much 
because I did not have much idea about it. And they are hidden so it made it 
worse. 😅 Thanks for sharing the JEP!


>

java.lang.reflect.Proxy could define hidden classes to act as the proxy classes 
which implement proxy interfaces; from JEP 317


It says that Proxy classes will also become hidden classes. Is it underway? 
Right now one can intercept, transform them, and include them in an allowlist. 
What do you think of naming them independent of AtomicLong so that a proxy 
class generated at runtime is easy to lookup in the allowlist?



Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)

https://algomaster99.github.io/

From: David Holmes mailto:david.hol...@oracle.com>>
Sent: Monday, May 20, 2024 2:30:37 PM
To: Aman Sharma; liangchenb...@gmail.com
Cc: core-libs-dev@openjdk.org; 
leyden-...@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

On 20/05/2024 10:12 pm, Aman Sharma wrote:
> Hi David,
>
>
>  > How did you try to intercept them? Hidden classes are not "loaded" in
> the normal sense so won't trigger class load events.
>
>
> I could not intercept them. I only see them when I pass `-verbose:class`
> in the Java CLI.

Yes that is why I asked how you tried to intercept them.

>
> I also couldn't intercept them using JVMTI Class File Load Hook
> 
>  event. However JEP 371 suggests that it should be possible to intercept them 
> using JVMTI Class Load 
>  
> event, but I won't have the bytecode at this stage. So is there no way to get 
> its bytecode before it is linked and initialized in the JVM?

Hidden classes are not loaded so I would not expect any class load
events. However the exact nature of the JVMTI class load event is
unclear as it talks about "class or interface creation" which is neither
loading or defining per se. But a class prepare event sounds like it
should be issued. However neither give you access to the bytecode of the
class AFAICS.

David
-


>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
> 
> https://algomaster99.github.io/
> 
> 
> *From:* David Holmes mailto:david.hol...@oracle.com>>
> *Sent:* Monday, May 20, 2024 2:59:17 AM
> *To:* Aman Sharma; liangchenb...@gmail.com
> *Cc:* core-libs-dev@openjdk.org; 
> leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
> On 17/05/2024 9:43 pm, Aman Sharma wrote:
>> Hi Chen,
>>
>>  > java.lang.invoke.LambdaForm$MH/0x0200cc000400
>>
>> I do see this as output when I pass -verbose:class. However, based on my
>> experiments, I have s

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

[Tobias Hartmann]

On Tue, 21 May 2024 17:41:46 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 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:
> 
>  - Merge remote-tracking branch 'origin/master' into ecc-montgomery
>  - shenandoah verifier
>  - comments from Sandhya
>  - whitespace
>  - add message back
>  - whitespace
>  - Use AffinePoint to exit Montgomery domain
>
>Style notes:
>Affine.equals()
>- Mismatched fields only appear to be used from testing, perhaps 
> should be moved there instead
>Affine.getX(boolean)|getY(boolean)
>- "Passing flag is bad design" - cleanest/performant alternative to 
> several instanceof checks
>- needed to convert Affine to Projective (need to stay in montgomery 
> domain)
>ECOperations.PointMultiplier
>   - changes could probably be restored to original (since ProjectivePoint 
> handling no longer required)
>   - consider these changes an improvement? (fewer nested classes)
>   - was an inner-class but not using inner-class features (i.e. ecOps 
> variable should be converted)
>  - whitespace
>  - Comments from Tony and Jatin
>  - Comments from Jatin and Tony
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/45457761...b1a33004

All tests passed.

-

PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2124892444

Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v7]

[Nizar Benalla]

> This checker checks the values of the `@since` tag found in the documentation 
> comment for an element against the release in which the element first 
> appeared.
> 
> Real since value of an API element is computed as the oldest release in which 
> the given API element was introduced. That is:
> - for modules, classes and interfaces, the release in which the element with 
> the given qualified name was introduced
> - for constructors, the release in which the constructor with the given VM 
> descriptor was introduced
> - for methods and fields, the release in which the given method or field with 
> the given VM descriptor became a member of its enclosing class or interface, 
> whether direct or inherited
> 
> Effective since value of an API element is computed as follows:
> - if the given element has a `@since` tag in its javadoc, it is used
> - in all other cases, return the effective since value of the enclosing 
> element
> 
> The since checker verifies that for every API element, the real since value 
> and the effective since value are the same, and reports an error if they are 
> not.
> 
> Preview method are handled as per JEP 12, if `@PreviewFeature` is used 
> consistently going forward then the checker doesn't need to be updated with 
> every release. The checker has explicit knowledge of preview elements that 
> came before `JDK 14` because they weren't marked in a machine understandable 
> way and preview elements that came before `JDK 17` that didn't have 
> `@PreviewFeature`.
> 
> Important note : We only check code written since `JDK 9` as the releases 
> used to determine the expected value of `@since` tags are taken from the 
> historical data built into `javac` which only goes back that far
> 
> The intial comment at the beginning of `SinceChecker.java` holds more 
> information into the program.
> 
> I already have filed issues and fixed some wrong tags like in #18640, #18032, 
> #18030, #18055, #18373, #18954, #18972.

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

  Add a few more legacy methods, needed a few more after changes to the checker

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18934/files
  - new: https://git.openjdk.org/jdk/pull/18934/files/e82dfbf0..fc10107a

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

  Stats: 16 lines in 1 file changed: 10 ins; 4 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18934.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18934/head:pull/18934

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

Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v2]

[Chen Liang]

> I propose to add type-checked ConstantPool.entryByIndex and 
> ClassReader.readEntryOrNull taking an extra Class parameter, which throws 
> ConstantPoolException instead of ClassCastException on type mismatch, which 
> can happen to malformed ClassFiles.
> 
> Requesting a review from @asotona. Thanks!
> 
> Proposal on mailing list: 
> https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html

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

 - Merge branch 'master' of https://github.com/openjdk/jdk into 
feature/entry-by-type
 - Use constants, beautify code
 - 8332614: Type-checked ConstantPool.entryByIndex and 
ClassReader.readEntryOrNull

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19330/files
  - new: https://git.openjdk.org/jdk/pull/19330/files/5a45e511..c9f6fc18

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

  Stats: 1517 lines in 58 files changed: 777 ins; 404 del; 336 mod
  Patch: https://git.openjdk.org/jdk/pull/19330.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330

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

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]

[Severin Gehwolf]

On Wed, 22 May 2024 12:25:09 GMT, Severin Gehwolf  wrote:

>> I did some testing and it turns out that this is indeed not checked. I 
>> believe this is a miss in the Skara reimplementation of jcheck. I've opened 
>> https://bugs.openjdk.org/browse/SKARA-2265 to track this.
>> 
>> Nevertheless, it would be good if you could fix this. :)
>
> Sure.

Should be fixed now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609943363

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]

[Severin Gehwolf]

> Please review this patch which adds a jlink mode to the JDK which doesn't 
> need the packaged modules being present. A.k.a run-time image based jlink. 
> Fundamentally this patch adds an option to use `jlink` even though your JDK 
> install might not come with the packaged modules (directory `jmods`). This is 
> particularly useful to further reduce the size of a jlinked runtime. After 
> the removal of the concept of a JRE, a common distribution mechanism is still 
> the full JDK with all modules and packaged modules. However, packaged modules 
> can incur an additional size tax. For example in a container scenario it 
> could be useful to have a base JDK container including all modules, but 
> without also delivering the packaged modules. This comes at a size advantage 
> of `~25%`. Such a base JDK container could then be used to `jlink` 
> application specific runtimes, further reducing the size of the application 
> runtime image (App + JDK runtime; as a single image *or* separate bundles, 
> depending on the app b
 eing modularized).
> 
> The basic design of this approach is to add a jlink plugin for tracking 
> non-class and non-resource files of a JDK install. I.e. files which aren't 
> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` 
> class which has all the info of what constitutes the final jlinked runtime.
> 
> Basic usage example:
> 
> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
> --limit-modules java.se)
> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) 
> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
> --limit-modules jdk.jlink)
> $ ls ../linux-x86_64-server-release/images/jdk/jmods
> java.base.jmodjava.net.http.jmod   java.sql.rowset.jmod  
> jdk.crypto.ec.jmod jdk.internal.opt.jmod 
> jdk.jdi.jmod jdk.management.agent.jmod  jdk.security.auth.jmod
> java.compiler.jmodjava.prefs.jmod  java.transaction.xa.jmod  
> jdk.dynalink.jmod  jdk.internal.vm.ci.jmod   
> jdk.jdwp.agent.jmod  jdk.management.jfr.jmodjdk.security.jgss.jmod
> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod  
> jdk.editpad.jmod   jdk.internal.vm.compiler.jmod 
> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
> java.desktop.jmod java.scripting.jmod  java.xml.jmod 
> jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage...

Severin Gehwolf has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 110 commits:

 - Merge branch 'master' into jdk-8311302-jmodless-link
 - Fix new line in jlink.properties
 - Merge branch 'master' into jdk-8311302-jmodless-link
 - Simplify JLINK_JDK_EXTRA_OPTS var handling
 - Only add runtime track files for linkable runtimes
 - Generate the runtime image link diff at jlink time
   
   But only do that once the plugin-pipeline has run. The generation is
   enabled with the hidden option '--generate-linkable-runtime' and
   the resource pools available at jlink time are being used to generate
   the diff.
 - Merge branch 'master' into jdk-8311302-jmodless-link
 - Use shorter name for the build tool
 - Review feedback from Erik J.
 - Remove dependency on CDS which isn't needed anymore
 - ... and 100 more: https://git.openjdk.org/jdk/compare/4f1a10f8...e1e3f02f

-

Changes: https://git.openjdk.org/jdk/pull/14787/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=28
  Stats: 3424 lines in 36 files changed: 3272 ins; 110 del; 42 mod
  Patch: https://git.openjdk.org/jdk/pull/14787.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787

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

RE: do I need to subscribe to this list in order to post?

[mark.yagnatinsky]

Thanks!!

From: David Alayachew 
Sent: Wednesday, May 22, 2024 8:54 AM
To: Yagnatinsky, Mark : IT (NYK) 
Cc: core-libs-dev@openjdk.org
Subject: Re: do I need to subscribe to this list in order to post?


CAUTION: This email originated from outside our organisation - 
davidalayac...@gmail.com Do not click on 
links, open attachments, or respond unless you recognize the sender and can 
validate the content is safe.
Nope. We can see your message. You only subscribe if you want to follow along 
with the general discussions that you are not explicitly invited to.

On Tue, May 21, 2024 at 7:14 PM 
mailto:mark.yagnatin...@barclays.com>> wrote:


This message is for information purposes only. It is not a recommendation, 
advice, offer or solicitation to buy or sell a product or service, nor an 
official confirmation of any transaction. It is directed at persons who are 
professionals and is intended for the recipient(s) only. It is not directed at 
retail customers. This message is subject to the terms at: 
https://www.ib.barclays/disclosures/web-and-email-disclaimer.html.

For important disclosures, please see: 
https://www.ib.barclays/disclosures/sales-and-trading-disclaimer.html
 regarding marketing commentary from Barclays Sales and/or Trading desks, who 
are active market participants; 
https://www.ib.barclays/disclosures/barclays-global-markets-disclosures.html
 regarding our standard terms for Barclays Investment Bank where we trade with 
you in principal-to-principal wholesale markets transactions; and in respect to 
Barclays Research, including disclosures relating to specific issuers, see: 
http://publicresearch.barclays.com.
__
If you are incorporated or operating in Australia, read these important 
disclosures: 
https://www.ib.barclays/disclosures/important-disclosures-asia-pacific.html.
__
For more details about how we use personal information, see our privacy notice: 
https://www.ib.barclays/disclosures/personal-information-use.html.
__

This message is for information purposes only. It is not a recommendation, 
advice, offer or solicitation to buy or sell a product or service, nor an 
official confirmation of any transaction. It is directed at persons who are 
professionals and is intended for the recipient(s) only. It is not directed at 
retail customers. This message is subject to the terms at: 
https://www.ib.barclays/disclosures/web-and-email-disclaimer.html. 

For important disclosures, please see: 
https://www.ib.barclays/disclosures/sales-and-trading-disclaimer.html regarding 
marketing commentary from Barclays Sales and/or Trading desks, who are active 
market participants; 
https://www.ib.barclays/disclosures/barclays-global-markets-disclosures.html 
regarding our standard terms for Barclays Investment Bank where we trade with 
you in principal-to-principal wholesale markets transactions; and in respect to 
Barclays Research, including disclosures relating to specific issuers, see: 
http://publicresearch.barclays.com.
__
 
If you are incorporated or operating in Australia, read these important 
disclosures: 
https://www.ib.barclays/disclosures/important-disclosures-asia-pacific.html.
__
For more details about how we use personal information, see our privacy notice: 
https://www.ib.barclays/disclosures/personal-information-use.html. 
__

Re: do I need to subscribe to this list in order to post?

[David Alayachew]

Nope. We can see your message. You only subscribe if you want to follow
along with the general discussions that you are not explicitly invited to.

On Tue, May 21, 2024 at 7:14 PM  wrote:

>
>
> This message is for information purposes only. It is not a recommendation,
> advice, offer or solicitation to buy or sell a product or service, nor an
> official confirmation of any transaction. It is directed at persons who are
> professionals and is intended for the recipient(s) only. It is not directed
> at retail customers. This message is subject to the terms at:
> https://www.ib.barclays/disclosures/web-and-email-disclaimer.html.
>
> For important disclosures, please see:
> https://www.ib.barclays/disclosures/sales-and-trading-disclaimer.html
> regarding marketing commentary from Barclays Sales and/or Trading desks,
> who are active market participants;
> https://www.ib.barclays/disclosures/barclays-global-markets-disclosures.html
> regarding our standard terms for Barclays Investment Bank where we trade
> with you in principal-to-principal wholesale markets transactions; and in
> respect to Barclays Research, including disclosures relating to specific
> issuers, see: http://publicresearch.barclays.com.
> __
>
> If you are incorporated or operating in Australia, read these important
> disclosures:
> https://www.ib.barclays/disclosures/important-disclosures-asia-pacific.html
> .
>
> __
> For more details about how we use personal information, see our privacy
> notice: https://www.ib.barclays/disclosures/personal-information-use.html.
>
>
> __
>

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

[Pavel Rappo]

> 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 incrementally with one additional 
commit since the last revision:

  Restructure the test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19112/files
  - new: https://git.openjdk.org/jdk/pull/19112/files/809e98e0..719560f6

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

  Stats: 177 lines in 1 file changed: 87 ins; 84 del; 6 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

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]

[Severin Gehwolf]

On Wed, 22 May 2024 08:07:59 GMT, Magnus Ihse Bursie  wrote:

>> Actually, this is a bit strange. I thought jcheck would look for missing 
>> newline at EOF, and that properties files were included in the check 
>> nowadays. I'll need to check this out.
>
> I did some testing and it turns out that this is indeed not checked. I 
> believe this is a miss in the Skara reimplementation of jcheck. I've opened 
> https://bugs.openjdk.org/browse/SKARA-2265 to track this.
> 
> Nevertheless, it would be good if you could fix this. :)

Sure.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609856505

Re: GC triggered before VM initialization completed

[Chen Liang]

Hi Lorris,
This mailing list is for Java's core libraries' development. Your issue is
a support request and is not related to Java's core libraries in any way,
thus I recommend you look for tech support or create/find an issue at
bugs.java.com.

Regards, Chen

On Wed, May 22, 2024 at 7:16 AM Lorris  wrote:

> Hello,
> I get an error when I try to run the java or javac commands on any file.
> The exact message is the following:
>
> *Error occurred during initialization of VM*
> *GC triggered before VM initialization completed. Try increasing NewSize,
> current value 1331K.*
>
> I tried to increase the size up to 6G but it did not solve the problem. I
> am using a modified version of the opendjdk-22.0.1 (3 commits behind the
> master branch as I write this email). This version generates extra fields
> and methods to propagate parameterised types information. I am totally
> aware that the problem comes deep down from the version that I am using,
> but as the error does not provide any other information, I cannot figure
> out the origin of the problem. I also added at the end of this email the
> verbose trace of the java command (it always blocks at the same point,
> regardless of the allocated size), if it can be useful in any way.
>
> Lorris Creantor
>
> *[0.013s][info][class,load] path:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.013s][info][class,load] path:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.014s][info][class,load] java.lang.Object source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.014s][info][class,load] java.io.Serializable source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.015s][info][class,load] java.lang.Comparable source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.015s][info][class,load] java.lang.CharSequence source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.015s][info][class,load] java.lang.constant.Constable source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.015s][info][class,load] java.lang.constant.ConstantDesc source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.015s][info][class,load] java.lang.String source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.016s][info][class,load] java.lang.reflect.AnnotatedElement source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.016s][info][class,load] java.lang.reflect.GenericDeclaration source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.016s][info][class,load] java.lang.reflect.Type source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.017s][info][class,load] java.lang.invoke.TypeDescriptor source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.017s][info][class,load] java.lang.invoke.TypeDescriptor$OfField
> source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.017s][info][class,load] java.lang.Class source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.017s][info][class,load] java.lang.Cloneable source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.018s][info][class,load] java.lang.ClassLoader source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.018s][info][class,load] java.lang.System source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.018s][info][class,load] java.lang.Throwable source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.018s][info][class,load] java.lang.Error source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.019s][info][class,load] java.lang.Exception source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.019s][info][class,load] java.lang.RuntimeException source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.019s][info][class,load] java.lang.SecurityManager source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.019s][info][class,load] java.security.ProtectionDomain source:
> /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base*
> *[0.020s][info][class,load] java.security.AccessControlContext sou

GC triggered before VM initialization completed

[Lorris]

Hello,
I get an error when I try to run the java or javac commands on any file. The 
exact message is the following:

Error occurred during initialization of VM
GC triggered before VM initialization completed. Try increasing NewSize, 
current value 1331K.

I tried to increase the size up to 6G but it did not solve the problem. I am 
using a modified version of the opendjdk-22.0.1 (3 commits behind the master 
branch as I write this email). This version generates extra fields and methods 
to propagate parameterised types information. I am totally aware that the 
problem comes deep down from the version that I am using, but as the error does 
not provide any other information, I cannot figure out the origin of the 
problem. I also added at the end of this email the verbose trace of the java 
command (it always blocks at the same point, regardless of the allocated size), 
if it can be useful in any way.

Lorris Creantor

[0.013s][info][class,load] path: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.013s][info][class,load] path: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.014s][info][class,load] java.lang.Object source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.014s][info][class,load] java.io.Serializable source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.015s][info][class,load] java.lang.Comparable source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.015s][info][class,load] java.lang.CharSequence source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.015s][info][class,load] java.lang.constant.Constable source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.015s][info][class,load] java.lang.constant.ConstantDesc source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.015s][info][class,load] java.lang.String source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.016s][info][class,load] java.lang.reflect.AnnotatedElement source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.016s][info][class,load] java.lang.reflect.GenericDeclaration source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.016s][info][class,load] java.lang.reflect.Type source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.017s][info][class,load] java.lang.invoke.TypeDescriptor source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.017s][info][class,load] java.lang.invoke.TypeDescriptor$OfField source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.017s][info][class,load] java.lang.Class source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.017s][info][class,load] java.lang.Cloneable source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.018s][info][class,load] java.lang.ClassLoader source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.018s][info][class,load] java.lang.System source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.018s][info][class,load] java.lang.Throwable source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.018s][info][class,load] java.lang.Error source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.019s][info][class,load] java.lang.Exception source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.019s][info][class,load] java.lang.RuntimeException source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.019s][info][class,load] java.lang.SecurityManager source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.019s][info][class,load] java.security.ProtectionDomain source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.020s][info][class,load] java.security.AccessControlContext source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.020s][info][class,load] java.security.AccessController source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.020s][info][class,load] java.security.SecureClassLoader source: 
/Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base
[0.020s][info][class,load] java.lang.ReflectiveOperation

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Chen Liang]

Hi Aman,
We have tried defining Proxy as hidden classes; a previous attempt was on
hold because of issues with serialization. Otherwise, Proxies work great as
hidden classes.

Chen

On Mon, May 20, 2024 at 7:56 AM Aman Sharma  wrote:

> Hi David,
>
>
> > I would not expect any class load
> events.
>
>
> I understand. I also haven't tried to intercept them but I see only one
> approach right now to include them in an allowlist - 1) statically look for
> invocations of "Lookup::defineHiddenClass". 2) Instrument them so that
> its first argument "bytes" can be looked into upon. I haven't looked into
> it much because I did not have much idea about it. And they are hidden so
> it made it worse. 😅 Thanks for sharing the JEP!
>
>
> >
> java.lang.reflect.Proxy could define hidden classes to act as the proxy
> classes which implement proxy interfaces; from JEP 317
>
>
> It says that Proxy classes will also become hidden classes. Is it
> underway? Right now one can intercept, transform them, and include them in
> an allowlist. What do you think of naming them independent of AtomicLong so
> that a proxy class generated at runtime is easy to lookup in the allowlist?
>
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
>  
> 
> https://algomaster99.github.io/
> --
> *From:* David Holmes 
> *Sent:* Monday, May 20, 2024 2:30:37 PM
> *To:* Aman Sharma; liangchenb...@gmail.com
> *Cc:* core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
> On 20/05/2024 10:12 pm, Aman Sharma wrote:
> > Hi David,
> >
> >
> >  > How did you try to intercept them? Hidden classes are not "loaded" in
> > the normal sense so won't trigger class load events.
> >
> >
> > I could not intercept them. I only see them when I pass `-verbose:class`
> > in the Java CLI.
>
> Yes that is why I asked how you tried to intercept them.
>
> >
> > I also couldn't intercept them using JVMTI Class File Load Hook
> > <
> https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#ClassFileLoadHook>
> event. However JEP 371 suggests that it should be possible to intercept
> them using JVMTI Class Load <
> https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#ClassLoad>
> event, but I won't have the bytecode at this stage. So is there no way to
> get its bytecode before it is linked and initialized in the JVM?
>
> Hidden classes are not loaded so I would not expect any class load
> events. However the exact nature of the JVMTI class load event is
> unclear as it talks about "class or interface creation" which is neither
> loading or defining per se. But a class prepare event sounds like it
> should be issued. However neither give you access to the bytecode of the
> class AFAICS.
>
> David
> -
>
>
> >
> > Regards,
> > Aman Sharma
> >
> > PhD Student
> > KTH Royal Institute of Technology
> > School of Electrical Engineering and Computer Science (EECS)
> > Department of Theoretical Computer Science (TCS)
> > <
> http://www.kth.se> >
> > https://algomaster99.github.io/
> > 
> > 
> > *From:* David Holmes 
> > *Sent:* Monday, May 20, 2024 2:59:17 AM
> > *To:* Aman Sharma; liangchenb...@gmail.com
> > *Cc:* core-libs-dev@openjdk.org; leyden-...@openjdk.org
> > *Subject:* Re: Deterministic naming of subclasses of
> > `java/lang/reflect/Proxy`
> > On 17/05/2024 9:43 pm, Aman Sharma wrote:
> >> Hi Chen,
> >>
> >>  > java.lang.invoke.LambdaForm$MH/0x0200cc000400
> >>
> >> I do see this as output when I pass -verbose:class. However, based on
> my
> >> experiments, I have seen that neither an agent passed via 'javaagent'
> >> nor an agent passed via 'agentpath' is able to intercept this hidden
> class.
> >
> > How did you try to intercept them? Hidden classes are not "loaded" in
> > the normal sense so won't trigger class load events.
> >
> >> Also, I was a bit confused since I saw somewhere that the names of
> >> hidden classes are null. But thanks for clarifying here.
> >
> > The JEP clearly defines the name format for hidden classes - though the
> > final component is VM specific (and typically a hashcode).
> >
> > https://openjdk.org/jeps/371 
> >
> > Cheers,
> > David
> > -
> >
> >>  > avoid dynamic class loading
> >>
> >> I don't see dynamic class loading as a problem. I only mind some
> >> unstable generation aspects of them which make it hard to verify them
> >> based on an allowlist.
> >>
> >> For example, if this hidden class is gen

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null

[Matthias Baesken]

On Wed, 22 May 2024 07:38:07 GMT, Magnus Ihse Bursie  wrote:

> How come `c->pdir` can be null? Is it if `sp.dirlen` is 0?

I added a bit of tracing output and get 
 Attention c->pdir is null; sp.dirlen is 0 
So yes sp.dirlen is also 0 .

-

PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2124271807

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

[Magnus Ihse Bursie]

On Fri, 17 May 2024 13:38:25 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:
> 
>   Address review comments

Build changes look good. Thanks for trimming down NATIVE_ACCESS_MODULES.

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2070573791

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a stricter default configuration [v12]

[Magnus Ihse Bursie]

On Tue, 21 May 2024 20:28:37 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated on 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>> 
>> Updated on 5/18/2024
>> 
>> Withdraw changes to jaxp.properties. The original idea was to match 
>> jaxp-strict.properties with regard to the properties. However, that change 
>> impact the configuration process, resulting in tests that verify the process 
>> to fail.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a note to module-info

make/modules/java.xml/Copy.gmk line 35:

> 33: $(eval $(call SetupCopyFiles, COPY_XML_MODULE_CONF, \
> 34: DEST := $(CONF_DST_DIR), \
> 35: FILES := $(wildcard 
> $(TOPDIR)/src/java.xml/share/conf/jaxp*.properties*), \

I don't think you need, nor should have, the asterisk after the extension. You 
are only copying `.properties` files.

Suggestion:

FILES := $(wildcard $(TOPDIR)/src/java.xml/share/conf/jaxp*.properties), \

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1609565653

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]

[Magnus Ihse Bursie]

On Wed, 22 May 2024 07:44:08 GMT, Magnus Ihse Bursie  wrote:

>> ... still missing...
>
> Actually, this is a bit strange. I thought jcheck would look for missing 
> newline at EOF, and that properties files were included in the check 
> nowadays. I'll need to check this out.

I did some testing and it turns out that this is indeed not checked. I believe 
this is a miss in the Skara reimplementation of jcheck. I've opened 
https://bugs.openjdk.org/browse/SKARA-2265 to track this.

Nevertheless, it would be good if you could fix this. :)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609491669

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

[Alan Bateman]

On Tue, 21 May 2024 16:59:38 GMT, Brent Christian  wrote:

> Indeed - can't move forward without a CSR. Also wouldn't mind more reviewer 
> ✔️s. 😉

I can do that.  One other thing to do is to rebase the changes, it looks like 
this branch is 6 months behind main line.

-

PR Comment: https://git.openjdk.org/jdk/pull/16644#issuecomment-2124116145

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:28:04 GMT, Claes Redestad  wrote:

> LGTM - feel free to add a comment that closing the InflaterInputStream has no 
> effect on the underlying stream deflated.

Hello Claes, I've updated the PR to include a comment.

-

PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2124098078

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:27:14 GMT, Claes Redestad  wrote:

>> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 
>> 113:
>> 
>>> 111: try (InflaterInputStream iis = new 
>>> InflaterInputStream(deflated)) {
>>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>>> 113: }
>> 
>> Presumably this only works because closing the underlying stream (a BAIS in 
>> this case) is a no-op.
>
> Yes, the underlying BAIS can be repeatedly closed without affecting the 
> benchmark.

Hello Alan, before this change, the memory gets occupied by too many of these:


   1:  10871820  521847360  
jdk.internal.ref.CleanerImpl$PhantomCleanableRef (java.base@23-internal)
   2:  10871758  260922192  java.util.zip.Inflater$InflaterZStreamRef 
(java.base@23-internal)



Closing the `InflaterInputStream` releases those underlying 
`InflaterZStreamRef`s.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609461029

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  add a comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19340/files
  - new: https://git.openjdk.org/jdk/pull/19340/files/f8687c3f..42a2d32a

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

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

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

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]

[Magnus Ihse Bursie]

On Wed, 22 May 2024 07:42:39 GMT, Magnus Ihse Bursie  wrote:

>> src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 
>> 165:
>> 
>>> 163: 
>>> 164: runtime.link.info=Linking based on the current run-time image.
>>> 165: runtime.link.jprt.path.extra=(run-time image)
>> 
>> Missing newline at last line.
>
> ... still missing...

Actually, this is a bit strange. I thought jcheck would look for missing 
newline at EOF, and that properties files were included in the check nowadays. 
I'll need to check this out.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609459326

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]

[Magnus Ihse Bursie]

On Thu, 4 Apr 2024 20:52:59 GMT, Magnus Ihse Bursie  wrote:

>> Severin Gehwolf has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - Use shorter name for the build tool
>>  - Review feedback from Erik J.
>>  - Remove dependency on CDS which isn't needed anymore
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 
> 165:
> 
>> 163: 
>> 164: runtime.link.info=Linking based on the current run-time image.
>> 165: runtime.link.jprt.path.extra=(run-time image)
> 
> Missing newline at last line.

... still missing...

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609457478

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null

[Magnus Ihse Bursie]

On Tue, 21 May 2024 14:28:38 GMT, Matthias Baesken  wrote:

> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing 
> jtreg tests afterwards I run into this error :
> 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: 
> null pointer passed as argument 2, which is declared to never be null
> #0 0x7fd95bec78d8 in spawnChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
> #1 0x7fd95bec78d8 in startChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
> #3 0x7fd93797a06d ()
> 
> this is the memcpy call getting an unexpected null pointer :
> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
> Something similar was discussed and fixed here 
> https://bugs.python.org/issue27570 for Python .
> 
> Similar issue in OpenJDK _ 
> https://bugs.openjdk.org/browse/JDK-8332473
> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed 
> as argument 1, which is declared to never be null

How come `c->pdir` can be null? Is it if `sp.dirlen` is 0?

-

PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2124082247

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Claes Redestad]

On Wed, 22 May 2024 07:20:04 GMT, Alan Bateman  wrote:

>> Can I please get a review of this test-only change for addressing 
>> https://bugs.openjdk.org/browse/JDK-8332490?
>> 
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
>> then doesn't close the stream. This can lead to resource leak which can then 
>> cause OOM as noted in that JBS issue.
>> 
>> The commit in this PR closes the `InflaterInputStream` when the reads 
>> complete.
>> 
>> 
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>> 
>> and
>> 
>> 
>> ./build/macosx-aarch64/images/jdk/bin/java -jar 
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
>> -t max -f 1 -wi 2 -foe true
>> 
>> pass after this change.
>
> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 113:
> 
>> 111: try (InflaterInputStream iis = new 
>> InflaterInputStream(deflated)) {
>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>> 113: }
> 
> Presumably this only works because closing the underlying stream (a BAOS in 
> this case) is a no-op.

Yes, the underlying BAIS can be repeatedly closed without affecting the 
benchmark.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609436837

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Claes Redestad]

On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

LGTM - feel free to add a comment that closing the `InflaterInputStream` has no 
effect on the underlying stream `deflated`.

-

PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2070357036

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Alan Bateman]

On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 113:

> 111: try (InflaterInputStream iis = new 
> InflaterInputStream(deflated)) {
> 112: while (iis.read(inflated, 0, inflated.length) != -1);
> 113: }

Presumably this only works because closing the underlying stream (a BAOS in 
this case) is a no-op.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609427551

Withdrawn: 8323760: clarify specification of Map::putIfAbsent return value

[duke]

On Tue, 16 Jan 2024 07:40:44 GMT, John Hendrikx  wrote:

> Update the documentation for `@return` tag of `putIfAbsent` to match the main 
> description. `putIfAbsent` uses the same wording as `put` for its `@return` 
> tag, but that is incorrect.  `putIfAbsent` never returns the **previous** 
> value, as the whole point of the method is not the replace the value if it 
> was present.  As such, if it returns a value, it is the **current** value, 
> and in all other cases it will return `null`.

This pull request has been closed without being integrated.

-

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