Re: RFR: 8319386: Migrate Class::getEnclosingMethod/Constructor away from old generic utilities

[Chen Liang]

On Fri, 3 Nov 2023 14:03:02 GMT, Chen Liang  wrote:

> Please review a patch that migrates `Class::getEnclosingMethod` and 
> `Class::getEnclosingConstructor`'s descriptor parsing from old generic 
> utilities to more simple utilities from java.lang.invoke implementation. This 
> will help migrate away from the old generic repositories in the future.
> 
> The `getClassLoader()` call plus 
> https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java#L93
> should be functionally equivalent to the previous `getDeclsLoader()`
> https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java#L60-L68
> plus
> https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java#L113-L114

Superseded by #19281

-

PR Comment: https://git.openjdk.org/jdk/pull/16496#issuecomment-2118545901

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

[Scott Gibbons]

On Wed, 15 May 2024 19:18:02 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 40:
> 
>> 38:   private static boolean failure = false;
>> 39:   public static void main(String[] args) throws Exception {
>> 40: String testName = "IndexOf";
> 
> intentation

Fixed

-

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

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

[Scott Gibbons]

On Tue, 14 May 2024 00:38:30 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 1178:
> 
>> 1176: __ andq(eq_mask, lastMask);
>> 1177: if (needToSaveRCX) {
>> 1178:   __ movdq(rcx, saveRCX);
> 
> movdq is an expensive instruction (about 3 cycle). If we have another gpr 
> temporary available here for shiftVal, then we dont need to do save/restore 
> rcx.

No longer need to use rcx.  Refactored.

-

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

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

[Scott Gibbons]

On Tue, 14 May 2024 18:38:38 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 1784:
> 
>> 1782:   __ subq(tmp, haystack_len);
>> 1783: }
>> 1784: __ leaq(haystack, Address(rsp, tmp, Address::times_1));
> 
> This whole code is repeated in two places. Could be made into a function and 
> used at both places.

This is the only place now.

-

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

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

[Scott Gibbons]

On Fri, 17 May 2024 22:40:50 GMT, Sandhya Viswanathan 
 wrote:

>> The entry code switches Windows calling convention into Linux calling 
>> convention by moving/saving registers, which are properly restored on 
>> function exit.  This makes register tracking easier.
>
> I don't see the place where the switch is happening before this initial piece 
> of code.  You also have windows tests failing in the GHA. Could you please 
> double check?

Fixed to use c_rargX

-

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

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

[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:

  Addressing lots of comments.  Interim commit.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/fb4da92a..9a861979

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=16753=19
 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=18-19

  Stats: 1639 lines in 9 files changed: 429 ins; 683 del; 527 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: 8331865: Consolidate size and alignment checks in LayoutPath [v2]

[Paul Sandoz]

On Thu, 16 May 2024 14:37:15 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 copyrights

Took me a few passes to work it all out. Looks good. All the bounds checking 
action now consistently passes through the call to `checkEnclosingLayout` in 
adaption of the raw (and unsafe) segment accessing VarHandle.

Separately, we might be missing a few long argument accepting guard methods for 
simpler cases as I suspect they are still focused more on int index types.

-

PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2064493620

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v9]

[Erik Joelsson]

On Fri, 17 May 2024 21:57:00 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 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.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   modernize make copy; update module summary and jaxp-strict.

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2064478337

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

[Sandhya Viswanathan]

On Thu, 16 May 2024 17:08:21 GMT, Scott Gibbons  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 238:
>> 
>>> 236: const Register needle   = rdx;
>>> 237: const Register needle_len   = rcx;
>>> 238: 
>> 
>> This is the calling convention on Linux. How is windows platform handled?
>
> The entry code switches Windows calling convention into Linux calling 
> convention by moving/saving registers, which are properly restored on 
> function exit.  This makes register tracking easier.

I don't see the place where the switch is happening before this initial piece 
of code.  You also have windows tests failing in the GHA. Could you please 
double check?

-

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

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

[Sandhya Viswanathan]

On Thu, 16 May 2024 20:22:40 GMT, Scott Gibbons  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1510:
>> 
>>> 1508:   compare_big_haystack_to_needle(sizeKnown, size, 
>>> NUMBER_OF_NEEDLE_BYTES_TO_COMPARE, loop_top, hsPtrRet, hsLength,
>>> 1509:  needleLen, isU, DO_EARLY_BAILOUT, 
>>> eq_mask, temp2, r10, _masm);
>>> 1510: 
>> 
>> At this point hsLength is not the remaining length from hsPtrRet, would that 
>> cause a problem? If not, all the special paths in 
>> compare_big_haystack_to_needle need not be generated on this call.
>
> 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.

-

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

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

[Volodymyr Paprotski]

On Fri, 17 May 2024 21:16:47 GMT, Volodymyr Paprotski  wrote:

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

Thanks Sandhya!

Now that I have @ascarpino approval as well, I plan to integrate next Tuesday.

-

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

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v6]

[Justin Lu]

> Please review this PR which corrects an edge case bug for 
> java.text.DecimalFormat that causes incorrect parsing results for strings 
> with very large exponent values.
> 
> When parsing values with large exponents, if the value of the exponent 
> exceeds `Integer.MAX_VALUE`, the parsed value  is equal to 0. If the value of 
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the 
> mantissa. Both results are confusing and incorrect.
> 
> For example,
> 
> 
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
> 
> 
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.

Justin Lu has updated the pull request incrementally with two additional 
commits since the last revision:

 - change impl to support whitebox test
 - adjust impl to increase accuracy when decimalAt/exponent value are both 
around Integer.MAX/MIN

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19075/files
  - new: https://git.openjdk.org/jdk/pull/19075/files/2c167493..7fe44600

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19075=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=19075=04-05

  Stats: 90 lines in 2 files changed: 65 ins; 12 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/19075.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075

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

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

[Sandhya Viswanathan]

On Fri, 17 May 2024 21:16:47 GMT, Volodymyr Paprotski  wrote:

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

Marked as reviewed by sviswanathan (Reviewer).

The intrinsics and the C2 changes look good to me.

-

PR Review: https://git.openjdk.org/jdk/pull/18583#pullrequestreview-2064439617
PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2118426661

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v9]

[Joe Wang]

> 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 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.

Joe Wang has updated the pull request incrementally with one additional commit 
since the last revision:

  modernize make copy; update module summary and jaxp-strict.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18831/files
  - new: https://git.openjdk.org/jdk/pull/18831/files/cf4df792..2ee2c7ca

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18831=08
 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=07-08

  Stats: 24 lines in 3 files changed: 1 ins; 12 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/18831.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18831/head:pull/18831

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

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Joe Wang]

On Thu, 16 May 2024 22:20:39 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 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.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

Thanks Alan, Erik! Updated accordingly.

-

PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2118424649

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

[Volodymyr Paprotski]

> 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*...

Volodymyr Paprotski has updated the pull request incrementally with one 
additional commit since the last revision:

  shenandoah verifier

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18583/files
  - new: https://git.openjdk.org/jdk/pull/18583/files/5c360e35..df4fe6fa

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18583=10
 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=09-10

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

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

Re: RFR: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16

[Iris Clark]

On Fri, 17 May 2024 18:04:02 GMT, Justin Lu  wrote:

> Please review this PR which is a trivial update to the IANA sub tag registry 
> to version 2024-05-16. Locale tests pass as expected after update.
> 
> Associated announcement -> 
> https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html

Marked as reviewed by iris (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19286#pullrequestreview-2064365331

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

[Chen Liang]

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).

Will we incorporate the changes for JDK-8 target code as in #19151?

-

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

Re: RFR: 8331851: Add specific regression leap year tests for Calendar.roll()

[Naoto Sato]

On Wed, 15 May 2024 09:39:16 GMT, serhiysachkov  wrote:

> Add specific regression tests for Calendar.roll() method to explicitly 
> various leap year test scenarios. This is inspired by the ambiguity which 
> occurred in leap year unaware test creation as in case with Calendar.add() in 
> swing component test case as detailed in 
> (https://bugs.openjdk.org/browse/JDK-8327088).

LGTM. I might want to remove those test case names as they are apparent from 
the parameters, but it's OK.

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19247#pullrequestreview-2064330497

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

[ExE Boss]

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/reflect/ClassFileAssembler.java line 
206:

> 204: //-
> 205: // Constants //
> 206: //-

Suggestion:

//---//
// Constants //
//---//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
227:

> 225: //---
> 226: // Local variable loads and stores //
> 227: //---

Suggestion:

//-//
// Local variable loads and stores //
//-//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
411:

> 409: //-
> 410: // Widening conversions only //
> 411: //-

Suggestion:

//---//
// Widening conversions only //
//---//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
439:

> 437: //
> 438: // Control flow //
> 439: //

Suggestion:

//--//
// Control flow //
//--//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
519:

> 517: //---
> 518: // Return instructions //
> 519: //---

Suggestion:

//-//
// Return instructions //
//-//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
553:

> 551: //
> 552: // Field operations //
> 553: //

Suggestion:

//--//
// Field operations //
//--//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
581:

> 579: //--
> 580: // Method invocations //
> 581: //--

Suggestion:

////
// Method invocations //
////

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
632:

> 630: //
> 631: // Array length //
> 632: //

Suggestion:

//--//
// Array length //
//--//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
640:

> 638: //---
> 639: // New //
> 640: //---

Suggestion:

//-//
// New //
//-//

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
650:

> 648: //--
> 649: // Athrow //
> 650: //--

Suggestion:

////
// Athrow //
////

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
659:

> 657: //
> 658: // Checkcast and instanceof //
> 659: //

Suggestion:

//--//
// Checkcast and instanceof //
//--//

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593971885
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972168
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972556
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972931
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593973196
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593973651
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974149
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974540
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974850
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593975284
PR Review Comment: 

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

[Jonathan Gibbons]

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).

-

Commit messages:
 - incorporate review comments
 - Merge remote-tracking branch 'upstream/master' into 
8330177.dangling-java.base
 - JDK-8331879: Clean up non-standard use of /// comments in `java.base`

Changes: https://git.openjdk.org/jdk/pull/19130/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19130=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331879
  Stats: 117 lines in 25 files changed: 0 ins; 0 del; 117 mod
  Patch: https://git.openjdk.org/jdk/pull/19130.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130

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

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

[Calvin Cheung]

On Thu, 16 May 2024 17:44:22 GMT, Ioi Lam  wrote:

>> 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:
> 
>   (1) comments from @liach; (2) added javadoc; (3) Use 
> createTestJavaProcessBuilder() instead of 
> createLimitedTestJavaProcessBuilder()

A few minor comments.

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 2:

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

Since this is a new file, should the copyright year be only 2024?
(same for other files)

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 43:

> 41: import jdk.test.lib.cds.CDSAppTester;
> 42: import jdk.test.lib.helpers.ClassFileInstaller;
> 43: import jdk.test.lib.process.OutputAnalyzer;

Is this import needed?

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBenchApp.java line 222:

> 220: }
> 221: long elapsed = System.currentTimeMillis() - started;
> 222: if (System.getProperty("JavacBenchApp.silent") == null) {

Should line 221 be inside the 'if' block?

test/lib/jdk/test/lib/cds/CDSAppTester.java line 218:

> 216: }
> 217: 
> 218: throw new RuntimeException("Must have exactly one command line 
> argument: STATIC or DYNAMIC");

Why not check the number of args at the beginning of the method?

-

PR Review: https://git.openjdk.org/jdk/pull/19256#pullrequestreview-2064242660
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605479322
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605468705
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605474939
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605477874

Re: RFR: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16

[Naoto Sato]

On Fri, 17 May 2024 18:04:02 GMT, Justin Lu  wrote:

> Please review this PR which is a trivial update to the IANA sub tag registry 
> to version 2024-05-16. Locale tests pass as expected after update.
> 
> Associated announcement -> 
> https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html

LGTM

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19286#pullrequestreview-2064215561

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException [v2]

[Roger Riggs]

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Add description of exception behavior when reading components in readRecord()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19043/files
  - new: https://git.openjdk.org/jdk/pull/19043/files/cb9cad62..d2f1db96

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19043=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19043=00-01

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

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

RFR: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16

[Justin Lu]

Please review this PR which is a trivial update to the IANA sub tag registry to 
version 2024-05-16. Locale tests pass as expected after update.

Associated announcement -> 
https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html

-

Commit messages:
 - init

Changes: https://git.openjdk.org/jdk/pull/19286/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19286=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332424
  Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/19286.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19286/head:pull/19286

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

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Roger Riggs]

On Wed, 1 May 2024 18:43:21 GMT, Roger Riggs  wrote:

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

Thanks for the review.
I have some ideas to refactor readOrdinaryObject to split out the various 
subcases and be able to simplify each one.
That's a separate (future) PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19043#issuecomment-2118070421

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Roger Riggs]

On Thu, 16 May 2024 21:18:04 GMT, Stuart Marks  wrote:

>> The issue reported a ClassCastException "cannot assign instance of 
>> java.util.CollSer to field of type java.util.Map"
>> while deserializing an object referring to an immutable Map that contained a 
>> reference to a class that was not available.
>> Immutable Collections such as Map utilize a serialization proxy in their 
>> serialized form.
>> During deserialization the serialization proxy (a private implementation 
>> class) was attempted to be set in a field resulting in the 
>> ClassCastException. The ClassCastException and bug hid the 
>> ClassCastException that should have been thrown.
>> 
>> When reading record fields or fields of a class, the results of 
>> deserialization of individual fields are recorded as dependencies of the 
>> object being constructed.
>> The apparent bug is that the summary of those dependencies is not checked 
>> between reading the fields and invoking the constructor to create the record 
>> or assigning the fields to an object being constructed.
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 2376:
> 
>> 2374: if (handles.lookupException(passHandle) != null) {
>> 2375: return null; // slot marked with exception, don't 
>> create record
>> 2376: }
> 
> It's proper to avoid creating a record instance in this case. However, 
> returning null is new behavior; this initially seemed a bit odd. The calling 
> method `readOrdinaryObject()` does allow a null return if the class couldn't 
> be resolved, and the body of `readOrdinaryObject()` does allow `obj` to be 
> null throughout. So, returning `null` from here is correct, though it took me 
> a while to puzzle out. :-)
> 
> I'd suggest adding some docs to `readRecord()` to indicate that it returns 
> the record instance if it could be created successfully, null if the record 
> class couldn't be resolved, or throws an error or other exception if one 
> occurred when instantiation was attempted.

As you discovered, though it returns null at this point, the return from 
readObject checks for the exception and throws ClassNotFoundException.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19043#discussion_r1605347591

Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]

[Viktor Klang]

On Fri, 17 May 2024 13:49:44 GMT, Doug Lea  wrote:

>> Viktor Klang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Moving the memory leak test for SynchronousQueue into its own test and 
>> runs only for JDK20+, using VirtualThreads
>
> Sure. Seems reasonable.

@DougLea Perfect, thanks for reviewing! 

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2118065006

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

[Volodymyr Paprotski]

On Thu, 16 May 2024 23:21:36 GMT, Sandhya Viswanathan 
 wrote:

>> Volodymyr Paprotski has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   whitespace
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 168:
> 
>> 166:   XMMRegister broadcast5 = xmm24;
>> 167:   KRegister limb0 = k1;
>> 168:   KRegister limb5 = k2;
> 
> limb5 and select are not being used anymore.

Thanks, fixed (and also broadcast5)

> src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 185:
> 
>> 183:   __ evmovdquq(modulus, allLimbs, ExternalAddress(modulus_p256()), 
>> false, Assembler::AVX_512bit, rscratch);
>> 184: 
>> 185:   // A = load(*aLimbs)
> 
> A little bit more description in comments on what the load step involves 
> would be helpful. e.g. Load upper 4 limbs, shift left by 1 limb using perm, 
> or in the lowest limb.

Done

> src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 270:
> 
>> 268:   __ push(r14);
>> 269:   __ push(r15);
>> 270: 
> 
> No need to save/restore  rbx, r12, r14, r15.  Only r13 is used as temp in 
> montgomeryMultiply(aLimbs, bLimbs, rLimbs). That too could be easily changed 
> to r8.

Seems I forgot to completely cleanup, thanks! (Originally copied from poly1305 
stub)

> src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 286:
> 
>> 284:   __ mov(aLimbs, c_rarg0);
>> 285:   __ mov(bLimbs, c_rarg1);
>> 286:   __ mov(rLimbs, c_rarg2);
> 
> We could directly call montgomeryMultiply(c_rarg0, c_rarg1, c_rarg2) then 
> these moves are not necessary.

Gave them symbolic names and passed the gpr temp and parameter. vector register 
map still in the montgomeryMultiply function, but gprs explicitly passed in. 
'close enough'?

> src/hotspot/cpu/x86/vm_version_x86.cpp line 1370:
> 
>> 1368: 
>> 1369: #ifdef _LP64
>> 1370:   if (supports_avx512ifma() && supports_avx512vlbw() && MaxVectorSize 
>> >= 64) {
> 
> No need to tie the intrinsic to MaxVectorSize setting.

Done

> src/hotspot/share/opto/library_call.cpp line 7564:
> 
>> 7562: 
>> 7563:   if (!stubAddr) return false;
>> 7564:   if (stopped())  return true;
> 
> Line 7564 seems redundant here as there is no range check or anything like 
> that  before this.

Oh. That is what that is for...  I thought it was some soft of 'VM quitting' 
short-circuit. Removed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328906
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328960
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328859
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328829
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605329040
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328995

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

[Volodymyr Paprotski]

> 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*...

Volodymyr Paprotski has updated the pull request incrementally with one 
additional commit since the last revision:

  comments from Sandhya

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18583/files
  - new: https://git.openjdk.org/jdk/pull/18583/files/8cd095dd..5c360e35

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18583=09
 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=08-09

  Stats: 82 lines in 4 files changed: 1 ins; 59 del; 22 mod
  Patch: https://git.openjdk.org/jdk/pull/18583.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18583/head:pull/18583

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

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

[Maurizio Cimadamore]

On Fri, 17 May 2024 15:54:04 GMT, Paul Sandoz  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix copyrights
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 630:
> 
>> 628:  * The access operation must fall inside the spatial bounds 
>> of the accessed
>> 629:  * memory segment, or an {@link IndexOutOfBoundsException} is 
>> thrown. This is the case
>> 630:  * when {@code B + A <= S}, where {@code O} is the base offset 
>> (defined above),
> 
> Do you mean `{@code O + A <= S}`?
> (Still working my way through the changes...)

Yes, apologies

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1605237785

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

[Paul Sandoz]

On Thu, 16 May 2024 14:37:15 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 copyrights

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 630:

> 628:  * The access operation must fall inside the spatial bounds 
> of the accessed
> 629:  * memory segment, or an {@link IndexOutOfBoundsException} is 
> thrown. This is the case
> 630:  * when {@code B + A <= S}, where {@code O} is the base offset 
> (defined above),

Do you mean `{@code O + A <= S}`?
(Still working my way through the changes...)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1605231757

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

[Jan Lahoda]

On Sun, 5 May 2024 15:22:15 GMT, Nizar Benalla  wrote:

>> 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:
> 
>   - Not checking elements enclosed within a record, I doubt a record class 
> will change after being created
>   - Added a null check as `toString` can return an exception

Overall seems pretty good, great job!

A few comments inline, to make the code cleaner.

Thanks for working on this!

test/jdk/tools/sincechecker/SinceChecker.java line 224:

> 222: }
> 223: 
> 224: return LEGACY_PREVIEW_METHODS.containsKey(currentVersion)

Nit: could probably be `LEGACY_PREVIEW_METHODS.getOrDefault(currentVersion, 
Map.of()).contains(uniqueId)`

test/jdk/tools/sincechecker/SinceChecker.java line 309:

> 307: error("module-info.java not found or couldn't be opened 
> AND this module has no unqualified exports");
> 308: } catch (NullPointerException e) {
> 309: error("module-info.java does not contain an `@since`");

Catching a NPE like this feels like a code smell to me. I assume it may happen 
when `extractSinceVersionFromText(moduleInfoContent)` returns `null` - so just 
store that in a variable, and check for `null` there.

test/jdk/tools/sincechecker/SinceChecker.java line 331:

> 329: error(ed.getPackage().getQualifiedName() + ": 
> package-info.java not found or couldn't be opened");
> 330: } catch (NullPointerException e) {
> 331: error(ed.getPackage().getQualifiedName() + ": 
> package-info.java  does not contain an `@since`");

Please see the comment on NPE catching in `getModuleVersionFromFile`.

test/jdk/tools/sincechecker/SinceChecker.java line 352:

> 350: }
> 351: checkElement(te.getEnclosingElement(), te, types, javadocHelper, 
> version, elementUtils);
> 352: if( te.getKind() == ElementKind.RECORD){

This seems a bit too broad. Ignoring all members of a record type may lead to 
missed members. `Elements.getOrigin` may be unusable here, but I wonder what 
exactly is the problem which this condition is trying to solve?

-

PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2063646309
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605134919
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605140507
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605143505
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605150261

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

[Jaikiran Pai]

On Fri, 17 May 2024 08:28:15 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   SecureRandom added to the table in package documentation.

Thank you Raffaello for fixing this as well as considering the review 
suggestions. The latest state of this PR in `880138d7` looks good to me. I've 
also reviewed the linked CSR and that too looks fine to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19212#pullrequestreview-2063525156

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

[Jaikiran Pai]

On Thu, 16 May 2024 13:23:14 GMT, Raffaello Giulietti  
wrote:

>> Yes, I thought about this the other day but decided for a bit more 
>> conservative approach, relying on the annotation.
>> 
>> But I agree that, since the meta-information now resides in 
>> `RandomGeneratorProperties`, we might "migrate" the deprecation status here 
>> as well.
>
> Since the legacy generators are public classes, they can be publicly 
> deprecated, so in the last commit the `DEPRECATED` bit for them still relies 
> on the annotation, which IMO is the authoritative "source of truth".
> 
> For the 10 other algorithms, which are accessible only via 
> `RandomFactoryGenerator`, we can rely on the info in `RandomProperties`.

The deprecation process of algorithms is unclear - whether it should be tied 
with the `@Deprecation` of a class that provides that algorithm. We will have 
more clarity if/when we do deprecate these algorithms. The specification in its 
current form doesn't tie it to the `@Deprecation` annotation, which is a good 
thing. So your proposed change in this PR looks fine to me, since it will still 
allow us to move away from the `@Deprecated` annotation check if we wanted to, 
in a subsequent release/change.

-

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

Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]

[Doug Lea]

On Fri, 17 May 2024 13:19:19 GMT, Viktor Klang  wrote:

>> Local testing seems to indicate that this fix (which mirrors what's done in 
>> the FIFO mode) addresses the problem.
>> 
>> But with that said, I haven't come up with a decent way of adding some form 
>> of regression test. Suggestions are most welcome. /cc @DougLea
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Moving the memory leak test for SynchronousQueue into its own test and runs 
> only for JDK20+, using VirtualThreads

Sure. Seems reasonable.

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2117653564

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

[Maurizio Cimadamore]

> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/3a0db276..789bdf48

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19213=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=06-07

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

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

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

[Maurizio Cimadamore]

On Thu, 16 May 2024 18:39:57 GMT, Alan Bateman  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add note on --illegal-native-access default value in the launcher help
>
> src/java.base/share/classes/java/lang/System.java line 2023:
> 
>> 2021:  * @throws NullPointerException if {@code filename} is {@code 
>> null}
>> 2022:  * @throws IllegalCallerException If the caller is in a module 
>> that
>> 2023:  * does not have native access enabled.
> 
> The exception description is fine, just noticed the other exception 
> descriptions start with a lowercase "if", this one is different.

I'll fix this. Note that in `ModuleLayer.Controller`, all `@throws` start with 
capital letter, which is probably where I copied/pasted this from. I'll fix 
all, except for `ModuleLayer` where, for consistency, I think upper case is 
better.

> src/java.base/share/man/java.1 line 587:
> 
>> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
>> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
>> 587: command-line option.
> 
> "This mode disable all illegal native access except for those modules enabled 
> the --enable-native-access command-line option". 
> 
> This can be read to mean that modules granted native access with the command 
> line option is also illegal native access An alternative is to make the 
> second part of the sentence a new sentence, something like "Only modules 
> enabled by the --enable-native-access command line option may perform native 
> access.

I've simplified the text to:


This mode disables illegal native access. That is, any illegal native access 
causes an `IllegalCallerException`.
This mode will become the default in a future release.


I think it's not necessary to state again the dependency on 
`--enable-native-access` as we already defined what "illegal native access" 
means.

-

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

Re: RFR: 8332154: Memory leak in SynchronousQueue [v3]

[Viktor Klang]

> Local testing seems to indicate that this fix (which mirrors what's done in 
> the FIFO mode) addresses the problem.
> 
> But with that said, I haven't come up with a decent way of adding some form 
> of regression test. Suggestions are most welcome. /cc @DougLea

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

  Moving the memory leak test for SynchronousQueue into its own test and runs 
only for JDK20+, using VirtualThreads

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19271/files
  - new: https://git.openjdk.org/jdk/pull/19271/files/f365c5f0..cc0a2014

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19271=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=19271=01-02

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

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

Re: RFR: 8332154: Memory leak in SynchronousQueue [v2]

[Viktor Klang]

On Fri, 17 May 2024 11:52:12 GMT, Doug Lea  wrote:

>> Viktor Klang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Adding a leak detection test for SynchronousQueue
>
> Looks good; thanks. I don't know why I left out this check in a case that is 
> otherwise identical to LTQ.

@DougLea I decided to move to using VirtualThreads and therefor pulling the 
memory leak test out into its own class which only runs for JDK20+.

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2117584999

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Erik Joelsson]

On Fri, 17 May 2024 05:51:31 GMT, Alan Bateman  wrote:

>> Joe Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove jaxp-compat.properties from the list
>
> make/modules/java.xml/Copy.gmk line 37:
> 
>> 35: JAXPPROPFILE_TARGET_FILES := $(subst 
>> $(JAXPPROPFILE_SRC_DIR),$(CONF_DST_DIR),$(JAXPPROPFILE_SRCS))
>> 36: 
>> 37: $(CONF_DST_DIR)/%: $(JAXPPROPFILE_SRC_DIR)/%
> 
> The make file changes to copy the properties files look okay but I'm curious 
> about why the naming changes from "XML" to "JAXPPROFILE".

If we are changing this file, we should modernize it.


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

TARGETS += $(COPY_XML_MODULE_CONF)

-

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

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Erik Joelsson]

On Thu, 16 May 2024 22:20:39 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 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.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

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

> 29: 
> 
> 30: #
> 31: # Copy property files from share/conf to CONF_DST_DIR LIB_DST_DIR

There is no copying to LIB_DST_DIR, so no need to mention it.

-

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

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

[Adam Sotona]

On Fri, 17 May 2024 08:28:51 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>>  line 308:
>> 
>>> 306: 0;
>>> 307: default ->
>>> 308: -1;
>> 
>> I recommend we explicitly return -1 to skip verification only for 
>> UnknownAttribute and CustomAttribute; then our tests can catch missing 
>> verification for new attribute additions.
>
> There are still many attributes missing here (yet). However it is a nice goal.

OK, I've added the missing attributes to the verification.
Now it will throw an AssertionError if we forget to verify newly added 
attribute.

-

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

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

[Adam Sotona]

> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
> bytecode-level class verification.
> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
> additional class checks inspired by 
> `hotspot/share/classfile/classFileParser.cpp`.
> 
> Also new `VerifierSelfTest::testParserVerifier` has been added.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with three additional 
commits since the last revision:

 - added verification of TypeAnnotation attributes
 - added verification of SMT attribute
 - added verification of module-related attributes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16809/files
  - new: https://git.openjdk.org/jdk/pull/16809/files/6f12b3bd..71818488

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=16809=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=16809=06-07

  Stats: 139 lines in 2 files changed: 136 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/16809.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16809/head:pull/16809

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

Integrated: 8331855: Convert jdk.jdeps jdeprscan and jdeps to use the Classfile API

[Chen Liang]

On Sat, 11 May 2024 19:35:33 GMT, Chen Liang  wrote:

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

This pull request has now been integrated.

Changeset: d4c2edf2
Author:Chen Liang 
Committer: Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/d4c2edf2c91a790874c80f1a7bea5bfd4f438bde
Stats: 2156 lines in 20 files changed: 744 ins; 1197 del; 215 mod

8331855: Convert jdk.jdeps jdeprscan and jdeps to use the Classfile API

Reviewed-by: asotona

-

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

Integrated: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Chen Liang]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

This pull request has now been integrated.

Changeset: beeffd46
Author:Chen Liang 
Committer: Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/beeffd4671649e5d8f9c96f0455ac90a82917234
Stats: 1539 lines in 19 files changed: 224 ins; 1098 del; 217 mod

8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

Reviewed-by: asotona

-

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

Re: RFR: 8332154: Memory leak in SynchronousQueue [v2]

[Doug Lea]

On Fri, 17 May 2024 11:39:32 GMT, Viktor Klang  wrote:

>> Local testing seems to indicate that this fix (which mirrors what's done in 
>> the FIFO mode) addresses the problem.
>> 
>> But with that said, I haven't come up with a decent way of adding some form 
>> of regression test. Suggestions are most welcome. /cc @DougLea
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding a leak detection test for SynchronousQueue

Looks good; thanks. I don't know why I left out this check in a case that is 
otherwise identical to LTQ.

-

PR Comment: https://git.openjdk.org/jdk/pull/19271#issuecomment-2117428982

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

[Aman Sharma]

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.

Also, I was a bit confused since I saw somewhere that the names of hidden 
classes are null. But thanks for clarifying here.

> 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 generated with the exact same name and the 
exact same bytecode during runtime as well, it would be easy to verify it. 
However, I do see the names are based on some sort of memory address so and I 
don't know what bytecode it has so I don't have suggestions to make them stable 
as of now. For Proxy classes, I feel it can be addressed unless you disagree or 
some involved in Project Leyden does. :) Thank you for forwarding my mail there.

Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
https://algomaster99.github.io/


From: liangchenb...@gmail.com 
Sent: Friday, May 17, 2024 1:23:58 pm
To: Aman Sharma 
Cc: core-libs-dev@openjdk.org ; 
leyden-...@openjdk.org 
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

Hi Aman,
For `-verbose:class`, it's a JVM argument instead of a program argument; so 
when you run a java program like `java Main`, you should call it as `java 
-verbose:class Main`.
When done correctly, you should see hidden class outputs like:
[0.032s][info][class,load] java.lang.invoke.LambdaForm$MH/0x0200cc000400 
source: __JVM_LookupDefineClass__
The loading of java.lang.invoke hidden classes requires your program to use 
MethodHandle features, like a lambda.

I think the problem you are exploring, that to avoid dynamic class loading and 
effectively turn Java Platform closed for security, is also being accomplished 
by project Leyden (as I've shared initially); Thus, I am forwarding this to 
leyden-dev instead, so you can see what approach Leyden uses to accomplish the 
same goal as yours.

Regards, Chen Liang

On Fri, May 17, 2024 at 4:40 AM Aman Sharma 
mailto:aman...@kth.se>> wrote:

Hi Roger,


Do you have ideas on how to intercept them? My javaagent is not able to nor a 
JVMTI agent passed using `agentpath` option. It also does not seem to show up 
in logs when I pass `-verbose:class`.


Also, what do you think of renaming the proxy classes as suggested below?


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: core-libs-dev 
mailto:core-libs-dev-r...@openjdk.org>> on 
behalf of Roger Riggs mailto:roger.ri...@oracle.com>>
Sent: Friday, May 17, 2024 4:57:46 AM
To: core-libs-dev@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

Hi Aman,

You may also run into hidden classes (JEP 371: Hidden Classes) that allow 
classes to be defined, at runtime, without names.
It has been proposed to use them for generated proxies but that hasn't been 
implemented yet.
There are benefits to having nameless classes, because they can't be referenced 
by name, only as a capability, they can be better encapsulated.

fyi, Roger Riggs


On 5/16/24 8:11 AM, Aman Sharma wrote:

Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant 
CVE-2021-42392.


> Leyden mainly avoids this unstable generation by performing a training run to 
> collect classes loaded


Would love to know the details of Project Leyden and how they worked so far to 
focus on this goal. In our case, the training run is the test suite.


> GeneratedConstructorAccessor is already retired by JEP 416 [2] in Java 18


I did see them not appearing in my allowlist when I ran my study subject 
(Apache PDFBox) with Java 21. Thanks for letting me know about this JEP. I see 
they are re-implemented with method handles.


> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked statically 
to the test suite execution. It gives us all classes that that is loaded post 
the JVM and the javaagent are loaded. So we only check the classes loaded for 
the purpose of running the application. This is also why we did not choose 
-agentlib as it would give classes for the setting up JVM and javaagent and we 
the user of our tool must the classes they load.


Next, we have a `ClassFileTransformer` 

Re: RFR: 8332154: Memory leak in SynchronousQueue [v2]

[Viktor Klang]

On Fri, 17 May 2024 11:35:55 GMT, Viktor Klang  wrote:

>> Local testing seems to indicate that this fix (which mirrors what's done in 
>> the FIFO mode) addresses the problem.
>> 
>> But with that said, I haven't come up with a decent way of adding some form 
>> of regression test. Suggestions are most welcome. /cc @DougLea
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding a leak detection test for SynchronousQueue

test/jdk/java/util/concurrent/tck/SynchronousQueueTest.java line 668:

> 666: }
> 667: 
> 668: private void assertDoesntLeak(SynchronousQueue queue) throws 
> InterruptedException {

@DougLea @AlanBateman Decided to create a memory leak detection test for 
SynchronousQueue. Of course, it will likely take some time to tune the 
parameters of it, but if it completes successfully there was no leak of queue 
items tho...

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19271#discussion_r1604833973

Re: RFR: 8332154: Memory leak in SynchronousQueue [v2]

[Viktor Klang]

> Local testing seems to indicate that this fix (which mirrors what's done in 
> the FIFO mode) addresses the problem.
> 
> But with that said, I haven't come up with a decent way of adding some form 
> of regression test. Suggestions are most welcome. /cc @DougLea

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

  Adding a leak detection test for SynchronousQueue

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19271/files
  - new: https://git.openjdk.org/jdk/pull/19271/files/dfab08e8..f365c5f0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19271=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19271=00-01

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

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

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

[Chen Liang]

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

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

Thanks for the review!

-

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

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Chen Liang]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

Thanks for the review!

-

PR Comment: https://git.openjdk.org/jdk/pull/19206#issuecomment-2117380960

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

[-]

Hi Aman,
For `-verbose:class`, it's a JVM argument instead of a program argument; so
when you run a java program like `java Main`, you should call it as `java
-verbose:class Main`.
When done correctly, you should see hidden class outputs like:
[0.032s][info][class,load]
java.lang.invoke.LambdaForm$MH/0x0200cc000400 source:
__JVM_LookupDefineClass__
The loading of java.lang.invoke hidden classes requires your program to use
MethodHandle features, like a lambda.

I think the problem you are exploring, that to avoid dynamic class loading
and effectively turn Java Platform closed for security, is also being
accomplished by project Leyden (as I've shared initially); Thus, I am
forwarding this to leyden-dev instead, so you can see what approach Leyden
uses to accomplish the same goal as yours.

Regards, Chen Liang

On Fri, May 17, 2024 at 4:40 AM Aman Sharma  wrote:

> Hi Roger,
>
>
> Do you have ideas on how to intercept them? My javaagent is not able to
> nor a JVMTI agent passed using `agentpath` option. It also does not seem to
> show up in logs when I pass `-verbose:class`.
>
>
> Also, what do you think of renaming the proxy classes as suggested below?
>
>
> 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:* core-libs-dev  on behalf of Roger
> Riggs 
> *Sent:* Friday, May 17, 2024 4:57:46 AM
> *To:* core-libs-dev@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
> Hi Aman,
>
> You may also run into hidden classes (JEP 371: Hidden Classes) that allow
> classes to be defined, at runtime, without names.
> It has been proposed to use them for generated proxies but that hasn't
> been implemented yet.
> There are benefits to having nameless classes, because they can't be
> referenced by name, only as a capability, they can be better encapsulated.
>
> fyi, Roger Riggs
>
>
> On 5/16/24 8:11 AM, Aman Sharma wrote:
>
> Hi,
>
>
> Thanks for your response, Liang!
>
>
> > I think you meant CVE-2021-42392 instead of 2022.
>
>
> Sorry of the error. I indeed meant CVE-2021-42392
> .
>
>
> > Leyden mainly avoids this unstable generation by performing a training
> run to collect classes loaded
>
>
> Would love to know the details of Project Leyden and how they worked so
> far to focus on this goal. In our case, the training run is the test suite.
>
>
> > GeneratedConstructorAccessor is already retired by JEP 416 [2] in Java
> 18
>
>
> I did see them not appearing in my allowlist when I ran my study subject
> (Apache PDFBox) with Java 21. Thanks for letting me know about this JEP. I
> see they are re-implemented with method handles.
>
>
> > How are you checking the classes?
>
>
> To detect runtime generated code, we have javaagent that is hooked
> statically to the test suite execution. It gives us all classes that that
> is loaded post the JVM and the javaagent are loaded. So we only check the
> classes loaded for the purpose of running the application. This is also why
> we did not choose -agentlib as it would give classes for the setting up JVM
> and javaagent and we the user of our tool must the classes they load.
>
>
> Next, we have a `ClassFileTransformer` hook in the agent where we produce
> the checksum using the bytecode. And we compare the checksum with the one
> existing in the allowlist. The checksum computation algorithm is same for
> both steps. Let me describe how I compute the checksum.
>
>
>
>1. I get the CONSTANT_Class_info
>
> 
>entry corresponding to `this_class` and rewrite the CONSTANT_Utf8_info
>
> 
>corresponding to a fix String constant, say "foo".
>2. Since, the name of the class is used to refer to its types members
>(fields/method), I get all CONSTANT_Fieldref_info
>
> 
>and if its `class_index` corresponds to the old `this_class`, we rewrite
>the UTF8 value of class_index to the same constant "foo".
>3. Next, since the naming of the fields, in Proxy classes, are also
>suffixed by numbers, for example, `private static Method m4`, we rewrite
>the UTF8 value of name in the CONSTANT_NameAndType_info
>
> .
>
>4. These fields can also have a random order so we simply sort the
>entire byte code using `Arrays.sort(byte[])` to eliminate any differences
>due to ordering of fields/methods.
>5. 

Integrated: 8331724: Refactor j.l.constant implementation to internal package

[Claes Redestad]

On Mon, 6 May 2024 14:39:08 GMT, Claes Redestad  wrote:

> This PR suggests refactoring the implementation classes of java.lang.constant 
> into a new package jdk.internal.constant to enable sharing some trusted 
> static factory methods with users elsewhere in java.base, such as 
> java.lang.invoke and java.lang.classfile. The refactoring also adds some 
> "trusted" methods for use when input is pre-validated, and makes use of them 
> where applicable in the current implementation. There are more changes in the 
> pipeline which will be folded into #17108 or PR'ed once that is integrated. 
> 
> There are two optimizations mixed up here. One in 
> `MethodTypeDesc.ofDescriptor`:
> 
> Name (descString) 
> Cnt Base Error  Test   Error  Unit  Change
> MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I 
>   6  138,371 ±   0,767   136,939 ± 1,126 ns/op   1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor  ()V 
>   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; 
>   6  209,390 ±   4,583   196,175 ± 3,211 ns/op   1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; 
>   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V 
>   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.). 
>   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   1,89x (p = 0,000*)
>   * = significant
> 
> 
> The other in `ClassDesc::nested`, where to get rid of a simple static method 
> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
> method:
> 
> Name  CntBase ErrorTest   
> Error  Unit  Change
> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
> 0,573 ns/op   9,53x (p = 0,000*)
>   * = significant
> 
> 
> The optimizations could be split out and PRd independently, but I think they 
> are simple enough to include in this refactoring.

This pull request has now been integrated.

Changeset: 0b0445be
Author:Claes Redestad 
URL:   
https://git.openjdk.org/jdk/commit/0b0445be2833286b4eace698b91a658de3e7608b
Stats: 1969 lines in 26 files changed: 1028 ins; 856 del; 85 mod

8331724: Refactor j.l.constant implementation to internal package

Reviewed-by: liach, asotona

-

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

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]

[Claes Redestad]

On Wed, 15 May 2024 11:17:42 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

Thanks!

-

PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2117171356

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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:
> 
>   Add note on --illegal-native-access default value in the launcher help

This looks good. Just a few minor comments where future maintainers might 
appreciate comments that describe parameters.

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

> 330: String caller = currentClass != null ? 
> currentClass.getName() : "code";
> 331: if (jni) {
> 332: System.err.printf("""

System.err may change in a running VM. It may be that we will need to change 
this at some point to use its initial setting. Not suggesting we changing it 
now but we might have to re-visit this.

-

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2062832385
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604653749

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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:
> 
>   Add note on --illegal-native-access default value in the launcher help

src/java.base/share/classes/java/lang/ClassLoader.java line 2448:

> 2446:  * Invoked in the VM class linking code.
> 2447:  */
> 2448: static long findNative(ClassLoader loader, Class clazz, String 
> entryName, String javaName) {

I think this is another place where `@param` descriptions would help as it's 
not immediately clear that "javaName" is a method name.

src/java.base/share/classes/java/lang/Runtime.java line 39:

> 37: 
> 38: import jdk.internal.access.SharedSecrets;
> 39: import jdk.internal.javac.Restricted;

Runtime has been touched for a while so you'll need to bump the copyright year.

-

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

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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:
> 
>   Add note on --illegal-native-access default value in the launcher help

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 288:

> 286:  * throw exception depending on the configuration.
> 287:  */
> 288: void ensureNativeAccess(Module m, Class owner, String methodName, 
> Class currentClass, boolean jni);

It might be helpful to future maintainers if we put `@param` descriptions for 
these parameters. I had to re-read Module.enableNativeAccess to remember the 
difference between the owner class and the current class.

-

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

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

[Aman Sharma]

Hi Roger,


Do you have ideas on how to intercept them? My javaagent is not able to nor a 
JVMTI agent passed using `agentpath` option. It also does not seem to show up 
in logs when I pass `-verbose:class`.


Also, what do you think of renaming the proxy classes as suggested below?


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: core-libs-dev  on behalf of Roger Riggs 

Sent: Friday, May 17, 2024 4:57:46 AM
To: core-libs-dev@openjdk.org
Subject: Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

Hi Aman,

You may also run into hidden classes (JEP 371: Hidden Classes) that allow 
classes to be defined, at runtime, without names.
It has been proposed to use them for generated proxies but that hasn't been 
implemented yet.
There are benefits to having nameless classes, because they can't be referenced 
by name, only as a capability, they can be better encapsulated.

fyi, Roger Riggs


On 5/16/24 8:11 AM, Aman Sharma wrote:

Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant 
CVE-2021-42392.


> Leyden mainly avoids this unstable generation by performing a training run to 
> collect classes loaded


Would love to know the details of Project Leyden and how they worked so far to 
focus on this goal. In our case, the training run is the test suite.


> GeneratedConstructorAccessor is already retired by JEP 416 [2] in Java 18


I did see them not appearing in my allowlist when I ran my study subject 
(Apache PDFBox) with Java 21. Thanks for letting me know about this JEP. I see 
they are re-implemented with method handles.


> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked statically 
to the test suite execution. It gives us all classes that that is loaded post 
the JVM and the javaagent are loaded. So we only check the classes loaded for 
the purpose of running the application. This is also why we did not choose 
-agentlib as it would give classes for the setting up JVM and javaagent and we 
the user of our tool must the classes they load.


Next, we have a `ClassFileTransformer` hook in the agent where we produce the 
checksum using the bytecode. And we compare the checksum with the one existing 
in the allowlist. The checksum computation algorithm is same for both steps. 
Let me describe how I compute the checksum.


  1.  I get the 
CONSTANT_Class_info
 entry corresponding to `this_class` and rewrite the 
CONSTANT_Utf8_info
 corresponding to a fix String constant, say "foo".
  2.  Since, the name of the class is used to refer to its types members 
(fields/method), I get all 
CONSTANT_Fieldref_info
 and if its `class_index` corresponds to the old `this_class`, we rewrite the 
UTF8 value of class_index to the same constant "foo".
  3.  Next, since the naming of the fields, in Proxy classes, are also suffixed 
by numbers, for example, `private static Method m4`, we rewrite the UTF8 value 
of name in the 
CONSTANT_NameAndType_info.
  4.  These fields can also have a random order so we simply sort the entire 
byte code using `Arrays.sort(byte[])` to eliminate any differences due to 
ordering of fields/methods.
  5.  Simply sorting the byte array still had minute differences. I could not 
understand why they existed even though values in constant pool of the bytecode 
in allowlist and at runtime were exactly the same after rewriting. The 
differences existed in the bytes of the Code attribute of methods. I concluded 
that the bytes stored some position information. To avoid this, I created a 
subarray where I considered the bytes corresponding to 
`CONSTANT_Utf8_info.bytes` only. Computing a checksum for it resulted in the 
same checksums for both classfiles.

Let's understand the whole approach with an example of Proxy class.

`

public final class $Proxy42 extends Proxy implements 
org.apache.logging.log4j.core.config.plugins.Plugin {

`

The will go in the allowlist as "Proxy_Plugin: ".

When the same class is intercepted at runtime, say "$Proxy10", we look for 
"Proxy_Plugin" in the allowlist and since the checksum algorithm is same in 
both cases, we get a match and let the class load.

This approach has seemed to work well for Proxy classes, Generated Constructor 
Accessor (which is 

Re: RFR: 8330465: Stable Values and Collections (Internal) [v20]

[Per Minborg]

On Fri, 17 May 2024 09:31:33 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using two `AtomicReference` and 
>> two protected by double-checked locking under concurrent access by all 
>> threads:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableBenchmark.atomicthrpt   10259.478 ?   
>> 36.809  ops/us
>> StableBenchmark.dcl   thrpt   10225.710 ?   
>> 26.638  ops/us
>> StableBenchmark.stablethrpt   10   4382.478 ? 
>> 1151.472  ops/us <- StableValue significantly faster
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
>> 385.639  ops/us
>> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
>> 210.610  ops/us
>> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
>> 1426.874  ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
>> 1839.651  ops/us
>> 
>> 
>> Performance for stable lists (thread safe) in both instance and static 
>> contexts whereby we access a single value compared to `ArrayList` instances 
>> (which are not thread-safe) (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
>> 1169.730  ops/us
>> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
>> 704.893  ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Add benchmarks for memoized IntFunction and Function
>  - Add benchmark for memoized supplier

Here are some results of a recently added benchmark that uses a memorized 
function (with 0 and 1 as input values):

![image](https://github.com/openjdk/jdk/assets/7457876/f2fd5b5a-ac89-483b-acb5-bc5de215417a)

See 
[test/micro/org/openjdk/bench/java/lang/stable/MemoizedFunctionBenchmark.java 
for 
details](https://github.com/minborg/jdk/blob/stable-value/test/micro/org/openjdk/bench/java/lang/stable/MemoizedFunctionBenchmark.java)

-

PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2117143526

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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:
> 
>   Add note on --illegal-native-access default value in the launcher help

src/java.base/share/classes/java/lang/foreign/package-info.java line 170:

> 168:  * the special value {@code ALL-UNNAMED} can be used). Access to 
> restricted methods
> 169:  * from modules not listed by that option is deemed illegal. 
> Clients can
> 170:  * control how illegal access to restricted method is handled, using the 
> command line

I assume this should be "to a restricted method is handled" or "to restricted 
methods are handled", either would work here.

-

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

Re: RFR: 8330465: Stable Values and Collections (Internal) [v20]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using two `AtomicReference` and 
> two protected by double-checked locking under concurrent access by all 
> threads:
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableBenchmark.atomicthrpt   10259.478 ?   
> 36.809  ops/us
> StableBenchmark.dcl   thrpt   10225.710 ?   
> 26.638  ops/us
> StableBenchmark.stablethrpt   10   4382.478 ? 
> 1151.472  ops/us <- StableValue significantly faster
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
> 385.639  ops/us
> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
> 210.610  ops/us
> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
> 1426.874  ops/us
> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
> 1839.651  ops/us
> 
> 
> Performance for stable lists (thread safe) in both instance and static 
> contexts whereby we access a single value compared to `ArrayList` instances 
> (which are not thread-safe) (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
> 1169.730  ops/us
> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
> 704.893  ops/us
> StableListElementBenchmark.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with two additional 
commits since the last revision:

 - Add benchmarks for memoized IntFunction and Function
 - Add benchmark for memoized supplier

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/dd0ceaf0..7beab36a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=19
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=18-19

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

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

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

[Alan Bateman]

On Mon, 13 May 2024 21:00:10 GMT, Stuart Marks  wrote:

>>> If an event class is loaded before JFR is started, the event class needs to 
>>> be retransformed, but if it is loaded later, we can add instrumentation on 
>>> class load and avoid the retransformation. More happens when an event class 
>>> is loaded compared to ordinary class load, for example, a startTime field 
>>> is added.
>>> 
>>> I did a JMH run and the difference between Event::enabled() and a boolean 
>>> flag was a fraction of nanosecond.
>> 
>> There are instances of FIS/FOS created in initPhase1 for the standard 
>> streams so loading as few classes and executing as minimal as possible is 
>> good. RAF will typically be used early too as the zip code opens zip files 
>> with a RAF. So doing as little as possible is good.
>
> My main concern here is the addition of clutter (checking two flags, and 
> creating two levels of nested "impl" methods) at every call site. We may need 
> to rearrange our internal API for JFR (jdk.internal.events) in order to clean 
> up the call sites without loading additional classes unnecessarily.
> 
> I think the main performance comparison to make is the impact on startup time 
> of loading the additional FileReadEvent class. That is, to compare the 
> startup time of the baseline with code that loads the FileReadEvent class, 
> with JFR disabled in both cases. I don't know how to do this. Anyway, if 
> loading of additional classes imposes unacceptable overhead, then that 
> justifies doing more work to avoid it. That's separate from whether adding 
> the `jfrTracing` flag as done in this PR is an effective way to do it.

I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable as 
it would avoid calling going through the traceXXX methods when the flag is 
enabled but the specific event is not enabled.

-

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

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

[Alan Bateman]

On Thu, 16 May 2024 12:10:46 GMT, Erik Gahlin  wrote:

>> Hi,
>> 
>> Could I have a review of a change that moves the jdk.FileRead and 
>> jdk.FileWrite events to java.base to remove the use of the ASM 
>> instrumentation.
>> 
>> Testing: jdk/jdk/jfr
>> 
>> Thanks
>> Erik
>
> Erik Gahlin has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove dependency on JFR for retransformation

src/java.base/share/classes/java/io/FileInputStream.java line 65:

> 63: /**
> 64:  * Flag set by jdk.internal.event.JFRTracing to determines if
> 65:  * file reads should be traced by JFR.

s/determines if/indicate if/

-

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

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

[Alan Bateman]

On Sun, 12 May 2024 13:35:42 GMT, Erik Gahlin  wrote:

> I guess it's not 100% safe if the JIT decides to store the read value 
> elsewhere over several event checks, but it seems unlikely. Event settings 
> checks (i.e., Event::isEnabled()) have always used plain reads, so it is not 
> more unreliable than any other event.
> 
> I'm fine with using a volatile too. I used it for the exception events, but I 
> now think a plain write/read of jfrTracing is safe for all practical purposes.

I'd prefer not introduce another volatile read into all these call paths. As 
you say, the issue has always been there so maybe it's okay for now and it can 
be examined later.

-

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

Re: RFR: 8332154: Memory leak in SynchronousQueue

[Vitaly Provodin]

On Thu, 16 May 2024 15:06:45 GMT, Viktor Klang  wrote:

>> Local testing seems to indicate that this fix (which mirrors what's done in 
>> the FIFO mode) addresses the problem.
>> 
>> But with that said, I haven't come up with a decent way of adding some form 
>> of regression test. Suggestions are most welcome. /cc @DougLea
>
> @AlanBateman @DougLea Reviews are most welcome :)

Hi @viktorklang-ora
The changes was verified as follows
1. Create the script launching the test (from bug description) in a loop and 
calculating the number of failures. 

cat 

Integrated: 8329653: JLILaunchTest fails on AIX after JDK-8329131

[Joachim Kern]

On Mon, 29 Apr 2024 14:45:07 GMT, Joachim Kern  wrote:

> Since ~ end of March, after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), 
> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
> 
>  stdout: [];
>  stderr: [Error: could not find libjava.so
> Error: Could not find Java SE Runtime Environment.
> ]
>  exitValue = 2
> 
> java.lang.RuntimeException: Expected to get exit value of [0], exit value is: 
> [2]
> at 
> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
> at JliLaunchTest.main(JliLaunchTest.java:58)
> at 
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at 
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> 
> Maybe we need to do further adjustments to 
> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / 
> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
> Or somehow adjust the coding that attempts to find parts of "JRE" 
> (libjava.so) ? The libjli.so is gone on AIX after 
> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this 
> also the image discovery based on GetApplicationHomeFromDll which uses 
> libjli.so .
> 
> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. 
> There is no other methos available on AIX, because it lacks the $ORIGIN 
> feature of the rpath.

This pull request has now been integrated.

Changeset: 14198f50
Author:Joachim Kern 
Committer: Christoph Langer 
URL:   
https://git.openjdk.org/jdk/commit/14198f502f0a721e479adc754a2c7d94b665fbe6
Stats: 55 lines in 3 files changed: 55 ins; 0 del; 0 mod

8329653: JLILaunchTest fails on AIX after JDK-8329131

Reviewed-by: clanger, mdoerr

-

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

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

[Adam Sotona]

On Wed, 15 May 2024 11:34:37 GMT, Chen Liang  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   applied the suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
>  line 308:
> 
>> 306: 0;
>> 307: default ->
>> 308: -1;
> 
> I recommend we explicitly return -1 to skip verification only for 
> UnknownAttribute and CustomAttribute; then our tests can catch missing 
> verification for new attribute additions.

There are still many attributes missing here (yet). However it is a nice goal.

-

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

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

[Raffaello Giulietti]

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

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  SecureRandom added to the table in package documentation.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19212/files
  - new: https://git.openjdk.org/jdk/pull/19212/files/a77146f5..880138d7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19212=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=19212=05-06

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

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

Re: RFR: 8330465: Stable Values and Collections (Internal) [v19]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using two `AtomicReference` and 
> two protected by double-checked locking under concurrent access by all 
> threads:
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableBenchmark.atomicthrpt   10259.478 ?   
> 36.809  ops/us
> StableBenchmark.dcl   thrpt   10225.710 ?   
> 26.638  ops/us
> StableBenchmark.stablethrpt   10   4382.478 ? 
> 1151.472  ops/us <- StableValue significantly faster
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
> 385.639  ops/us
> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
> 210.610  ops/us
> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
> 1426.874  ops/us
> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
> 1839.651  ops/us
> 
> 
> Performance for stable lists (thread safe) in both instance and static 
> contexts whereby we access a single value compared to `ArrayList` instances 
> (which are not thread-safe) (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
> 1169.730  ops/us
> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
> 704.893  ops/us
> StableListElementBenchmark.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Update ofList and ofMap docs

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/0f798a70..dd0ceaf0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=18
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=17-18

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

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

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]

[Adam Sotona]

On Wed, 15 May 2024 11:17:42 GMT, Claes Redestad  wrote:

>> This PR suggests refactoring the implementation classes of 
>> java.lang.constant into a new package jdk.internal.constant to enable 
>> sharing some trusted static factory methods with users elsewhere in 
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring 
>> also adds some "trusted" methods for use when input is pre-validated, and 
>> makes use of them where applicable in the current implementation. There are 
>> more changes in the pipeline which will be folded into #17108 or PR'ed once 
>> that is integrated. 
>> 
>> There are two optimizations mixed up here. One in 
>> `MethodTypeDesc.ofDescriptor`:
>> 
>> Name 
>> (descString) Cnt Base Error  Test   Error  Unit  Change
>> MethodTypeDescFactories.ofDescriptor  
>> (Ljava/lang/Object;Ljava/lang/String;)I   6  138,371 ±   0,767   136,939 ± 
>> 1,126 ns/op   1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor  
>> ()V   6   10,528 ±   2,495 4,110 ± 0,047 ns/op   2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> ([IJLjava/lang/String;Z)Ljava/util/List;   6  209,390 ±   4,583   196,175 ± 
>> 3,211 ns/op   1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String;   6   36,039 ±   8,68420,794 ± 1,110 ns/op   1,73x 
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (..IIJ)V   6  279,139 ±   6,248   187,934 ± 0,857 ns/op   1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor 
>> (.).   6 2174,387 ± 132,879  1150,652 ± 3,158 ns/op   
>> 1,89x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The other in `ClassDesc::nested`, where to get rid of a simple static method 
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory 
>> method:
>> 
>> Name  CntBase ErrorTest   
>> Error  Unit  Change
>> ClassDescFactories.ReferenceOnly.ofNested   6 209,853 ± 132,525  22,017 ± 
>> 0,573 ns/op   9,53x (p = 0,000*)
>>   * = significant
>> 
>> 
>> The optimizations could be split out and PRd independently, but I think they 
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

It looks good to me.

-

Marked as reviewed by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19105#pullrequestreview-2062597153

Re: RFR: 8330465: Stable Values and Collections (Internal) [v18]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using two `AtomicReference` and 
> two protected by double-checked locking under concurrent access by all 
> threads:
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableBenchmark.atomicthrpt   10259.478 ?   
> 36.809  ops/us
> StableBenchmark.dcl   thrpt   10225.710 ?   
> 26.638  ops/us
> StableBenchmark.stablethrpt   10   4382.478 ? 
> 1151.472  ops/us <- StableValue significantly faster
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
> 385.639  ops/us
> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
> 210.610  ops/us
> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
> 1426.874  ops/us
> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
> 1839.651  ops/us
> 
> 
> Performance for stable lists (thread safe) in both instance and static 
> contexts whereby we access a single value compared to `ArrayList` instances 
> (which are not thread-safe) (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
> 1169.730  ops/us
> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
> 704.893  ops/us
> StableListElementBenchmark.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Simplify StableList

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/35c9252d..0f798a70

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=17
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=16-17

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

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

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

[Adam Sotona]

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

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

Looks good to me, thanks!

-

Marked as reviewed by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19193#pullrequestreview-2062523424

Re: RFR: 8330465: Stable Values and Collections (Internal) [v17]

[Per Minborg]

> # Stable Values & Collections (Internal)
> 
> ## Summary
> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
> which provides immutable value holders where elements are initialized _at 
> most once_. Stable Values & Collections offer the performance and safety 
> benefits of final fields while offering greater flexibility as to the timing 
> of initialization.
> 
> ## Goals
>  * Provide an easy and intuitive API to describe value holders that can 
> change at most once.
>  * Decouple declaration from initialization without significant footprint or 
> performance penalties.
>  * Reduce the amount of static initializer and/or field initialization code.
>  * Uphold integrity and consistency, even in a multi-threaded environment.
>  
> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
> 
> ## Performance 
> Performance compared to instance variables using two `AtomicReference` and 
> two protected by double-checked locking under concurrent access by all 
> threads:
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableBenchmark.atomicthrpt   10259.478 ?   
> 36.809  ops/us
> StableBenchmark.dcl   thrpt   10225.710 ?   
> 26.638  ops/us
> StableBenchmark.stablethrpt   10   4382.478 ? 
> 1151.472  ops/us <- StableValue significantly faster
> 
> 
> Performance compared to static variables protected by `AtomicReference`, 
> class-holder idiom holder, and double-checked locking (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
> 385.639  ops/us
> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
> 210.610  ops/us
> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
> 1426.874  ops/us
> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
> 1839.651  ops/us
> 
> 
> Performance for stable lists (thread safe) in both instance and static 
> contexts whereby we access a single value compared to `ArrayList` instances 
> (which are not thread-safe) (all threads):
> 
> 
> Benchmark  Mode  Cnt  Score  
> Error   Units
> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
> 1169.730  ops/us
> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
> 704.893  ops/us
> StableListElementBenchmark.staticArrayListthrpt   10   7614.741 ?  
> 564.777  ops/us
> StableListElementBe...

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Declare field final

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18794/files
  - new: https://git.openjdk.org/jdk/pull/18794/files/ec7c92cd..35c9252d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18794=16
 - incr: https://webrevs.openjdk.org/?repo=jdk=18794=15-16

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

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

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Alan Bateman]

On Thu, 16 May 2024 22:20:39 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 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.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

src/java.xml/share/classes/module-info.java line 443:

> 441:  * 
> 442:  *
> 443:  * This file allows deployments to test the more secure/strict behavior,

I think it might be better to reduce this paragraph down to just say something 
like "Deploying with this configuation prevents processors from unknowingly 
making outbound network connections to fetch DTDs, or process XML that makes 
use of extension functions."

We could say that a future JDK release may use a strict configuration by 
default but that opens the door to questions as to whether the system property 
is needed, whether jaxp.propeteries is going away, so maybe better to leave 
that out for now.

-

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

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Adam Sotona]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

Looks good to me, great job, thanks!

-

Marked as reviewed by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19206#pullrequestreview-2062430122

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v6]

[Jaikiran Pai]

> Can I please get a review for this patch which proposes to implement the 
> enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?
> 
> The commit in this PR introduces the `-o` and `--output-dir` option to the 
> `jar` command. The option takes a path to a destination directory as a value 
> and extracts the contents of the jar into that directory. This is an optional 
> option and the changes in the commit continue to maintain backward 
> compatibility where the jar is extracted into current directory, if no `-o` 
> or `--output-dir` option has been specified.
> 
> As far as I know, there hasn't been any discussion on what the name of this 
> new option should be. I was initially thinking of using `-d` but that is 
> currently used by the `jar` command for a different purpose. So I decided to 
> use `-o` and `--output-dir`. This is of course open for change depending on 
> any suggestions in this PR.
> 
> The commit in this PR also updates the `jar.properties` file which contains 
> the English version of the jar command's `--help` output. However, no changes 
> have been done to the internationalization files corresponding to this one 
> (for example: `jar_de.properties`), because I don't know what process needs 
> to be followed to have those files updated (if at all they need to be 
> updated).
> 
> The commit also includes a jtreg testcase which verifies the usage of this 
> new option.

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 16 commits:

 - merge latest from master branch
 - cleanup testExtractNoDestDirWithPFlag() test
 - merge latest from master branch
 - merge latest from master branch
 - convert the new test to junit
 - merge latest from master branch
 - Lance's review - include tests for --extract long form option
 - cleanup after each test
 - Adjust test for new error messages
 - Lance's review - add a code comment for xdestDir
 - ... and 6 more: https://git.openjdk.org/jdk/compare/9160ef8b...a8aeff60

-

Changes: https://git.openjdk.org/jdk/pull/2752/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=05
  Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/2752.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752

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

Re: RFR: 8330465: Stable Values and Collections (Internal) [v16]

[Per Minborg]

On Thu, 16 May 2024 12:48:24 GMT, Per Minborg  wrote:

>> # Stable Values & Collections (Internal)
>> 
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, 
>> which provides immutable value holders where elements are initialized _at 
>> most once_. Stable Values & Collections offer the performance and safety 
>> benefits of final fields while offering greater flexibility as to the timing 
>> of initialization.
>> 
>> ## Goals
>>  * Provide an easy and intuitive API to describe value holders that can 
>> change at most once.
>>  * Decouple declaration from initialization without significant footprint or 
>> performance penalties.
>>  * Reduce the amount of static initializer and/or field initialization code.
>>  * Uphold integrity and consistency, even in a multi-threaded environment.
>>  
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>> 
>> ## Performance 
>> Performance compared to instance variables using two `AtomicReference` and 
>> two protected by double-checked locking under concurrent access by all 
>> threads:
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableBenchmark.atomicthrpt   10259.478 ?   
>> 36.809  ops/us
>> StableBenchmark.dcl   thrpt   10225.710 ?   
>> 26.638  ops/us
>> StableBenchmark.stablethrpt   10   4382.478 ? 
>> 1151.472  ops/us <- StableValue significantly faster
>> 
>> 
>> Performance compared to static variables protected by `AtomicReference`, 
>> class-holder idiom holder, and double-checked locking (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableStaticBenchmark.atomic  thrpt   10   6487.835 ?  
>> 385.639  ops/us
>> StableStaticBenchmark.dcl thrpt   10   6605.239 ?  
>> 210.610  ops/us
>> StableStaticBenchmark.stable  thrpt   10  14338.239 ? 
>> 1426.874  ops/us
>> StableStaticBenchmark.staticCHI   thrpt   10  13780.341 ? 
>> 1839.651  ops/us
>> 
>> 
>> Performance for stable lists (thread safe) in both instance and static 
>> contexts whereby we access a single value compared to `ArrayList` instances 
>> (which are not thread-safe) (all threads):
>> 
>> 
>> Benchmark  Mode  Cnt  Score  
>> Error   Units
>> StableListElementBenchmark.instanceArrayList  thrpt   10   5812.992 ? 
>> 1169.730  ops/us
>> StableListElementBenchmark.instanceList   thrpt   10   4818.643 ?  
>> 704.893  ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix copyringht issues

> _Mailing list message from [Olexandr 
> Rotan](mailto:rotanolexandr...@gmail.com) on 
> [compiler-dev](mailto:compiler-...@mail.openjdk.org):_
> 
> Is it possible to make stable values and collections Serializable? I see 
> various applications for this feature in entity classes as a way to preserve 
> immutability of entity fields and at the same time not break current JPA 
> specifications. It is a *very* common task in commercial development. Current 
> workarounds lack either thread safety or performance, and this looks like a 
> really good solution for both of those problems. However, unless StableValue 
> is serializable, it is really unlikely that JPA will adopt them (we have 
> precedent with Optional).
> 
> On Thu, May 16, 2024 at 5:07?PM Per Minborg  wrote:
> 
> -- next part -- An HTML attachment was scrubbed... 
> URL: 
> 

`Serializable` is on the list to explore.

-

PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2116840097

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v5]

[Jaikiran Pai]

On Wed, 8 May 2024 05:37:24 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this patch which proposes to implement the 
>> enhancement request noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>> 
>> The commit in this PR introduces the `-o` and `--output-dir` option to the 
>> `jar` command. The option takes a path to a destination directory as a value 
>> and extracts the contents of the jar into that directory. This is an 
>> optional option and the changes in the commit continue to maintain backward 
>> compatibility where the jar is extracted into current directory, if no `-o` 
>> or `--output-dir` option has been specified.
>> 
>> As far as I know, there hasn't been any discussion on what the name of this 
>> new option should be. I was initially thinking of using `-d` but that is 
>> currently used by the `jar` command for a different purpose. So I decided to 
>> use `-o` and `--output-dir`. This is of course open for change depending on 
>> any suggestions in this PR.
>> 
>> The commit in this PR also updates the `jar.properties` file which contains 
>> the English version of the jar command's `--help` output. However, no 
>> changes have been done to the internationalization files corresponding to 
>> this one (for example: `jar_de.properties`), because I don't know what 
>> process needs to be followed to have those files updated (if at all they 
>> need to be updated).
>> 
>> The commit also includes a jtreg testcase which verifies the usage of this 
>> new option.
>
> Jaikiran Pai has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - cleanup testExtractNoDestDirWithPFlag() test
>  - merge latest from master branch
>  - merge latest from master branch
>  - convert the new test to junit
>  - merge latest from master branch
>  - Lance's review - include tests for --extract long form option
>  - cleanup after each test
>  - Adjust test for new error messages
>  - Lance's review - add a code comment for xdestDir
>  - Lance's review - updates to the help messages in jar.properties
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/3b8227ba...46fb5a8e

I've also updated the CSR https://bugs.openjdk.org/browse/JDK-8264510 for 
review. That CSR would be the best place to get up to speed with this proposed 
enhancement and the semantics of the new option.

-

PR Comment: https://git.openjdk.org/jdk/pull/2752#issuecomment-2116836829

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Alan Bateman]

On Thu, 16 May 2024 22:20:39 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 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.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

src/java.xml/share/conf/jaxp-strict.properties line 9:

> 7: # test the more secure/strict behavior, identify issues such as a processor
> 8: # unknowingly makes outbound network connections to fetch DTD, or 
> processes XML
> 9: # that relies on extension functions.

There isn't a JEP to propose that XML processing be secure by default on the 
technical roadmap right now so I think this paragraph will need to be tweaked 
to avoid making any assumptions. I think just say that the file provides the 
settings for more secure XML processing and drop the text about testing (and 
"and create your own configuration file for the experiment" from the paragraph 
below).

-

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

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Chen Liang]

On Fri, 17 May 2024 06:03:00 GMT, Adam Sotona  wrote:

>> Some tests are not migrated to the ClassFile API in previous migrations.
>> 
>>  - Some are simple oversights that didn't remove usages of 
>> com.sun.tools.classfile;
>>  - The CallerSensitive ones used an old utility, replaced by CF API-based 
>> new code;
>>  - many in javac are because the files are compiled with older source 
>> compatibility. Those patches are converted to have the source code stored in 
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
>> `CompilerUtils.compile`;
>>  - As described in the JBS issue, there are a few other tests not covered; 
>> one is in #19193 while the others are blocked by CreateSymbols migration or 
>> bugs.
>> 
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:
> 
>> 149: 
>> 150: boolean needsCsm = false;
>> 151: for (var element : code) {
> 
> Scanning the instructions is a bit different approach than in the original 
> test.

Same as the other method name check, it was manually extracted ReferenceFinder 
logic, we should retire ReferenceFinder with the old class file library.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604405594

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Chen Liang]

On Fri, 17 May 2024 06:07:47 GMT, Adam Sotona  wrote:

>> Some tests are not migrated to the ClassFile API in previous migrations.
>> 
>>  - Some are simple oversights that didn't remove usages of 
>> com.sun.tools.classfile;
>>  - The CallerSensitive ones used an old utility, replaced by CF API-based 
>> new code;
>>  - many in javac are because the files are compiled with older source 
>> compatibility. Those patches are converted to have the source code stored in 
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
>> `CompilerUtils.compile`;
>>  - As described in the JBS issue, there are a few other tests not covered; 
>> one is in #19193 while the others are blocked by CreateSymbols migration or 
>> bugs.
>> 
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line 
> 125:
> 
>> 123: && invoke.method() instanceof MethodRefEntry ref
>> 124: && ref.owner().name().equalsString(className)
>> 125: && ref.name().equalsString(methodName)) {
> 
> Is this additional test necessary, I don't see it in the original test.

This was part of the now-removed ReferenceFinder logic; it scans cp as a fast 
path and then does instruction scan for all methods to find relevant methods to 
pass to visitor.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604402631

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Adam Sotona]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

test/langtools/tools/javac/7166455/CheckACC_STRICTFlagOnclinitTest.java line 81:

> 79: ToolBox toolBox = new ToolBox();
> 80: toolBox.writeJavaFiles(in, SOURCE);
> 81: CompilerUtils.compile(in, out, "--release", "16");

Smart move 

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604399988

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Adam Sotona]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line 
125:

> 123: && invoke.method() instanceof MethodRefEntry ref
> 124: && ref.owner().name().equalsString(className)
> 125: && ref.name().equalsString(methodName)) {

Is this additional test necessary, I don't see it in the original test.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604394473

Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API

[Adam Sotona]

On Sun, 12 May 2024 08:36:44 GMT, Chen Liang  wrote:

> Some tests are not migrated to the ClassFile API in previous migrations.
> 
>  - Some are simple oversights that didn't remove usages of 
> com.sun.tools.classfile;
>  - The CallerSensitive ones used an old utility, replaced by CF API-based new 
> code;
>  - many in javac are because the files are compiled with older source 
> compatibility. Those patches are converted to have the source code stored in 
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and 
> `CompilerUtils.compile`;
>  - As described in the JBS issue, there are a few other tests not covered; 
> one is in #19193 while the others are blocked by CreateSymbols migration or 
> bugs.
> 
> Testing: all modified tests pass.

test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:

> 149: 
> 150: boolean needsCsm = false;
> 151: for (var element : code) {

Scanning the instructions is a bit different approach than in the original test.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604390936

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v5]

[Jaikiran Pai]

On Wed, 8 May 2024 05:37:24 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this patch which proposes to implement the 
>> enhancement request noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>> 
>> The commit in this PR introduces the `-o` and `--output-dir` option to the 
>> `jar` command. The option takes a path to a destination directory as a value 
>> and extracts the contents of the jar into that directory. This is an 
>> optional option and the changes in the commit continue to maintain backward 
>> compatibility where the jar is extracted into current directory, if no `-o` 
>> or `--output-dir` option has been specified.
>> 
>> As far as I know, there hasn't been any discussion on what the name of this 
>> new option should be. I was initially thinking of using `-d` but that is 
>> currently used by the `jar` command for a different purpose. So I decided to 
>> use `-o` and `--output-dir`. This is of course open for change depending on 
>> any suggestions in this PR.
>> 
>> The commit in this PR also updates the `jar.properties` file which contains 
>> the English version of the jar command's `--help` output. However, no 
>> changes have been done to the internationalization files corresponding to 
>> this one (for example: `jar_de.properties`), because I don't know what 
>> process needs to be followed to have those files updated (if at all they 
>> need to be updated).
>> 
>> The commit also includes a jtreg testcase which verifies the usage of this 
>> new option.
>
> Jaikiran Pai has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - cleanup testExtractNoDestDirWithPFlag() test
>  - merge latest from master branch
>  - merge latest from master branch
>  - convert the new test to junit
>  - merge latest from master branch
>  - Lance's review - include tests for --extract long form option
>  - cleanup after each test
>  - Adjust test for new error messages
>  - Lance's review - add a code comment for xdestDir
>  - Lance's review - updates to the help messages in jar.properties
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/3b8227ba...46fb5a8e

Hello everyone, this was a PR that I had opened some years back to introduce an 
option for the `jar` command to extract the contents to a directory of choice. 
After evaluating the option name (discussion within this PR), we had settled on 
`--dir` as the option. For some reason (I don't recollect) I never took this PR 
to completion. I've now reopened this PR and merged with the latest master 
branch and run all the tests including the new ones introduced in this PR. I 
would like to take this to completion, so I request reviews on this.

-

PR Comment: https://git.openjdk.org/jdk/pull/2752#issuecomment-2116762815