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

[Sandhya Viswanathan]

On Thu, 23 May 2024 23:12:42 GMT, Scott Gibbons  wrote:

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

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1740:

> 1738: //
> 1739: // If a match is found, jump to L_checkRangeAndReturn, which 
> ensures the
> 1740: // matched needle is not past the end of the haystack.

These labels are not in this function.

-

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

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

[Sandhya Viswanathan]

On Fri, 24 May 2024 23:15:26 GMT, Scott Gibbons  wrote:

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

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 316:

> 314:   // Set up jump tables.  Used when needle size <= NUMBER_OF_CASES
> 315:   setup_jump_tables(ae, L_returnRBP, L_checkRangeAndReturn, 
> L_bigCaseFixupAndReturn,
> 316: _jump_table, _jump_table, _masm);

We could directly use L_returnError here instead of L_returnRBP.

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 476:

> 474: // Used to check and return value in rbp - usually error
> 475:   __ bind(L_returnRBP);
> 476:   __ movq(rax, rbp);

This seems spurious as rax is being overwritten at line 489. Did you intend to 
return -1?

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1816:

> 1814: byte_compare_helper(i + 1, L_loopTop, L_fixup, needle, 
> needle_val, hs_ptr, eq_mask, set_bit,
> 1815: rTmp4, ae, _masm);
> 1816:   }

L_checkRange on NoMatch could be L_error instead.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614172379
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614172021
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614175081

Re: RFR: 8330182: Start of release updates for JDK 24 [v7]

[Joe Darcy]

> Get JDK 24 underway.

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

  Add symbol files current with JDK 23 build 24.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18787/files
  - new: https://git.openjdk.org/jdk/pull/18787/files/2a023349..5586ce5d

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

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

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

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

[Chen Liang]

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

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

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

-

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

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

  Stats: 15592 lines in 362 files changed: 10436 ins; 3396 del; 1760 mod
  Patch: https://git.openjdk.org/jdk/pull/19330.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330

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

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]

[Magnus Ihse Bursie]

On Fri, 24 May 2024 16:36:32 GMT, Joe Wang  wrote:

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

LGTM

-

Marked as reviewed by ihse (Reviewer).

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

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

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

  Fix test; review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/be001e2c..b154faee

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

  Stats: 31 lines in 3 files changed: 4 ins; 13 del; 14 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

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

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

[Scott Gibbons]

On Tue, 21 May 2024 22:39:42 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Addressing lots of comments.  Interim commit.
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4737:
> 
>> 4735: bind(COMPARE_BYTE);
>> 4736:   } else {
>> 4737: lea(ary1, Address(ary1, expand_ary2 ? 4 : 2));
> 
> This change is not required. expand_ary2 code doesn't come here.

Right.  Fixed.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1233:
> 
>> 1231:   __ andq(eq_mask, rTmp);
>> 1232: 
>> 1233:   __ testl(eq_mask, eq_mask);
> 
> Mismatch of operation size q vs l: andq and testl.

Fixed.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1623:
> 
>> 1621: 
>> 
>> 1622: //
>> 1623: // Small haystack (<32 bytes) switch
> 
> This should be <= 32 bytes.

Fixed.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1709:
> 
>> 1707: //  XMM_BYTE_K - last element of needle, broadcast
>> 1708: //
>> 1709: //  The haystack is >= 32 bytes
> 
> Should this be > 32 bytes?

Fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614114763
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127986
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127889
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614127781

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

[Scott Gibbons]

On Wed, 22 May 2024 20:36:25 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Revert last change to IndexOf.java
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1218:
> 
>> 1216: // isU - true if argument encoding is either UU or UL
>> 1217: // eq_mask - The bit mask returned that holds the result of the 
>> comparison
>> 1218: // needleLen - a temporary register.  Only used if isUL true
> 
> needleLen is not a temporary register. needleLen is used to read the kThByte 
> from haystack below when !sizeKnown so must hold valid info.

Fixed.

> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1439:
> 
>> 1437:   // back to last valid read position
>> 1438:   __ cmpq(hsPtrRet, last);
>> 1439:   __ jb_b(L_midLoop);
> 
> could be  jbe_b?

Fixed.

-

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

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

[Scott Gibbons]

On Wed, 22 May 2024 18:22:24 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   un-helper-ize preload_needle_helper; try fix for macos build
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 101:
> 
>> 99:   if (isU) { \
>> 100: __ blsrl(tmp, mask); \
>> 101: __ blsrl(mask, tmp); \
> 
> We could do this as 
>   __ blsrl(mask, mask); \
>   __ blsrl(mask, mask); \
> Thereby removing the need for tmp.

Fixed.

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 00:09:38 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments - move stubGen*_string.cpp to c2_stubGen*_string.cpp
>
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1754:
> 
>> 1752: continue;
>> 1753:   } else {
>> 1754: Label L_loopTop;
> 
> L_loopTop label not used in the else block.

Removed.

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 22:26:56 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments.
>
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1122:
> 
>> 1120: // eq_mask - The bit mask returned that holds the result of the 
>> comparison
>> 1121: // rTmp - a temporary register
>> 1122: // rTmp2 - a temporary register
> 
> There is no rtmp, rtmp2 here.

Fixed.

> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1129:
> 
>> 1127: // _masm - Current MacroAssembler instance pointer
>> 1128: //
>> 1129: // If (n - k) < 32, need to handle reading past end of haystack
> 
> Don't see (n-k) < 32 being handled in this function.

Fixed.

-

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

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

[Sandhya Viswanathan]

On Fri, 17 May 2024 23:47:45 GMT, Scott Gibbons  wrote:

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

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

> 4735: bind(COMPARE_BYTE);
> 4736:   } else {
> 4737: lea(ary1, Address(ary1, expand_ary2 ? 4 : 2));

This change is not required. expand_ary2 code doesn't come here.

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

> 1231:   __ andq(eq_mask, rTmp);
> 1232: 
> 1233:   __ testl(eq_mask, eq_mask);

Mismatch of operation size q vs l: andq and testl.

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

> 1621: 
> 
> 1622: //
> 1623: // Small haystack (<32 bytes) switch

This should be <= 32 bytes.

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

> 1707: //  XMM_BYTE_K - last element of needle, broadcast
> 1708: //
> 1709: //  The haystack is >= 32 bytes

Should this be > 32 bytes?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609023624
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609043720
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609160143
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1609163535

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

[Sandhya Viswanathan]

On Wed, 22 May 2024 18:52:27 GMT, Scott Gibbons  wrote:

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

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

> 1216: // isU - true if argument encoding is either UU or UL
> 1217: // eq_mask - The bit mask returned that holds the result of the 
> comparison
> 1218: // needleLen - a temporary register.  Only used if isUL true

needleLen is not a temporary register. needleLen is used to read the kThByte 
from haystack below when !sizeKnown so must hold valid info.

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

> 1437:   // back to last valid read position
> 1438:   __ cmpq(hsPtrRet, last);
> 1439:   __ jb_b(L_midLoop);

could be  jbe_b?

-

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

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

[Sandhya Viswanathan]

On Wed, 22 May 2024 17:40:24 GMT, Scott Gibbons  wrote:

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

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

> 99:   if (isU) { \
> 100: __ blsrl(tmp, mask); \
> 101: __ blsrl(mask, tmp); \

We could do this as 
  __ blsrl(mask, mask); \
  __ blsrl(mask, mask); \
Thereby removing the need for tmp.

-

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

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

[Sandhya Viswanathan]

On Fri, 24 May 2024 20:47:23 GMT, Scott Gibbons  wrote:

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

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1122:

> 1120: // eq_mask - The bit mask returned that holds the result of the 
> comparison
> 1121: // rTmp - a temporary register
> 1122: // rTmp2 - a temporary register

There is no rtmp, rtmp2 here.

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1129:

> 1127: // _masm - Current MacroAssembler instance pointer
> 1128: //
> 1129: // If (n - k) < 32, need to handle reading past end of haystack

Don't see (n-k) < 32 being handled in this function.

-

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

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

[Sandhya Viswanathan]

On Thu, 23 May 2024 23:12:42 GMT, Scott Gibbons  wrote:

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

src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1754:

> 1752: continue;
> 1753:   } else {
> 1754: Label L_loopTop;

L_loopTop label not used in the else block.

-

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

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

[Brent Christian]

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

Brent Christian has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 69 commits:

 - Merge branch 'master' into refDocs2
 - add link to Thread.isAlive()
 - small review tweaks; shorten MemoryConsistency links
 - small grammar fixes
 - new section for finalizer memviz
 - add memviz bullet for finalization
 - remove quotes from dequeue
 - package spec updates, mostly about reference queues and dequeueing
 - move reachability section before notification; update section header
 - add details on use of reference queues; swap reachability/memviz sections
 - ... and 59 more: https://git.openjdk.org/jdk/compare/7bf1989f...d7cbf0d3

-

Changes: https://git.openjdk.org/jdk/pull/16644/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=16644=31
  Stats: 226 lines in 4 files changed: 132 ins; 19 del; 75 mod
  Patch: https://git.openjdk.org/jdk/pull/16644.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644

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

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option

[Alexander Matveev]

Hi Michael,

> Doesn’t this still leave you with an application that isn’t validly signed? 
> And probably won’t run because of that.
Yes, it will leave you with an application that isn’t signed. I was able to run 
such application on same machine as it was generated by jpackage.

> For your example. This almost seems like an Apple bug if you can add a 
> directory to the Contents directory but not a file?
Not sure if it is an Apple bug.

> Would it also generally be a good idea to include a final codesign verify to 
> fail the build if something is wrong with the signature?
Yes, you already suggested it. See https://bugs.openjdk.org/browse/JDK-8318063 
and it was closed as won’t fix because such verification is redundant.

Thanks,
Alexander

From: Michael Hall 
Date: Friday, May 24, 2024 at 1:47 AM
To: Alexander Matveev 
Cc: core-libs-dev 
Subject: Re: RFR: 8332110: [macos] jpackage tries to sign added files without 
the --mac-sign option



On May 24, 2024, at 3:08 AM, Michael Hall  wrote:

On May 23, 2024, at 8:13 PM, Alexander Matveev 
mailto:almat...@openjdk.org>> wrote:

otherwise add additional content as post-processing step.

Doesn’t this still leave you with an application that isn’t validly signed? And 
probably won’t run because of that.

2) jpackage --type app-image -n Test --app-content ReadMe ...

For your example. This almost seems like an Apple bug if you can add a 
directory to the Contents directory but not a file?

Sorry I made my prior off-list.

Would it also generally be a good idea to include a final codesign verify to 
fail the build if something is wrong with the signature?

Something like…

echo '***'
echo 'verifying signature'
echo '***'
codesign -v --verbose=4 outputdir/HalfPipe.app

Expected output…

***
verifying signature
***
outputdir/HalfPipe.app: valid on disk
outputdir/HalfPipe.app: satisfies its Designated Requirement

I think I have suggested this before but don’t remember if I did an enhancement 
request. Maybe you do that and I’m just not aware of it if it doesn’t appear in 
the jpackage output.

Integrated: 8320575: generic type information lost on mandated parameters of record's compact constructors

[Vicente Romero]

On Mon, 11 Dec 2023 23:33:16 GMT, Vicente Romero  wrote:

> Reflection is not retrieving generic type information for mandated 
> parameters. This is a known issue which has been explicitly stated in the API 
> of some reflection methods. Fix for 
> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the 
> parameters of compact constructors of record classes `mandated` as specified 
> in the spec. But this implied that users that previous to this change could 
> retrieve the generic type of parameters of compact constructors now they 
> can't anymore. The proposed fix is to try to retrieve generic type 
> information for mandated parameters if available plus changing the spec of 
> the related reflection methods.
> 
> TIA

This pull request has now been integrated.

Changeset: 7bf1989f
Author:Vicente Romero 
URL:   
https://git.openjdk.org/jdk/commit/7bf1989f59695c3d08b4bd116fb4c022cf9661f4
Stats: 405 lines in 3 files changed: 376 ins; 6 del; 23 mod

8320575: generic type information lost on mandated parameters of record's 
compact constructors

Co-authored-by: Chen Liang 
Reviewed-by: jlahoda

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 19:30:54 GMT, Volodymyr Paprotski  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   mov64 => lea(InternalAddress)
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4633:
> 
>> 4631: andl(result, 0x000f);  //   tail count (in bytes)
>> 4632: andl(limit, 0xfff0);   // vector count (in bytes)
>> 4633: jcc(Assembler::zero, COMPARE_TAIL);
> 
> In the `expand_ary2` case, this is the same andl/compare as line 4549; i.e. I 
> think you can just put `jcc(Assembler::zero, COMPARE_TAIL);` on line 4549, 
> inside the if (and move the other jcc into the else branch)?

OK.  Shortens pathlength by 4 instructions.

> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4639:
> 
>> 4637: negptr(limit);
>> 4638: 
>> 4639: bind(COMPARE_WIDE_VECTORS_16);
> 
> Understanding-check.. this loop will execute at most 2 times, right?
> 
> i.e. process as many 32-byte chunks as possible, then 1-or-2 16-byte chunks 
> then byte-by-byte?
> 
> (Still a good optimization, just trying to understand the scope)

Yes.

> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4718:
> 
>> 4716: jmp(TRUE_LABEL);
>> 4717:   } else {
>> 4718: movl(chr, Address(ary1, limit, scaleFactor));
> 
> scaleFactor is always Address::times_1 here (expand_ary2==false), might be 
> clearer to change it back

*Sigh*.  Changing it back.

> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 57:
> 
>> 55: 
>> 56: generator = new Random();
>> 57: long seed = generator.nextLong();//-5291521104060046276L;
> 
> dead code

Fixed

> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 63:
> 
>> 61: ///  WARM-UP //
>> 62: 
>> 63: for (int i = 0; i < 2; i++) {
> 
> -Xcomp should be more deterministic (and quicker) way to reach the intrinsic 
> (i.e. like the other tests)
> 
> On other hand, perhaps this doesn't matter? @vnkozlov Understanding-check 
> please.. these tests will run as part of every build from this 
> point-till-infinity; Therefore, long test will affect every openjdk 
> developer. But if this test is not run on every build, then the build-time 
> does not matter, then this test can run for as long as it 'wants'.

This test runs in well under 2 minutes.  I'm not sure what is trying to be 
accomplished?

> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 160:
> 
>> 158:   }
>> 159: 
>> 160:   private static String generateTestString(int min, int max) {
> 
> I see you have various `Charset[] charSets` above, but this function still 
> only generates LL. Are those separate tests? Or am I missing some 
> concatenation somewhere that will convert the generated string string to the 
> correct encoding?
> 
> You could had implemented my suggestion from IndexOf.generateTestString here 
> instead, so that the tests that do call this function endup with multiple 
> encodings; i.e. similar to what you already do in the next function.
> 
> I suppose, with addition of String/IndexOf.java that is a moot point.

Yes, I think it's a moot point.  Thanks.

> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 185:
> 
>> 183:   }
>> 184: 
>> 185:   private static int indexOfKernel(String haystack, String needle) {
> 
> Is the intention of kernels not to be inlined so that it would be part of 
> separate compilation?
> 
> If so, you probably want to annotate it with 
> `@CompilerControl(CompilerControl.Mode.DONT_INLINE)`
> 
> i.e. 
> https://github.com/openjdk/jmh/blob/master/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_16_CompilerControl.java

Fixed.

> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 539:
> 
>> 537: failCount = indexOfKernel("", "");
>> 538: 
>> 539: for (int x = 0; x < 100; x++) {
> 
> Should we be concerned about the increased run-time? Or does this execute 
> 'quickly enough'

Runs in well under 2 minutes.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613997645
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613993657
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613998432
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r161481
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614000885
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614001480
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614002801
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1614003072

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

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

  Review comments.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/69ca8d13..be001e2c

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

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

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

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

[Volodymyr Paprotski]

On Wed, 22 May 2024 14:50:40 GMT, Scott Gibbons  wrote:

>> test/jdk/java/lang/StringBuffer/IndexOf.java line 284:
>> 
>>> 282: 
>>> 283:   // Note: it is possible although highly improbable that failCount 
>>> will
>>> 284:   // be > 0 even if everthing is working ok
>> 
>> This sounds like either a bug or a testcase bug? Same as line 301, 
>> `extremely remote possibility of > 1 match`?
>
> This was there from the original author.  I think they were trying to infer 
> that a match could occur in the rare case that the same random string was 
> produced.  They're random after all, and there's no reason the same sequence 
> could be generated.

Makes sense

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 20:12:07 GMT, Vladimir Kozlov  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Test clarifications
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 28:
> 
>> 26:  * @summary Test indexOf and lastIndexOf
>> 27:  * @run main/othervm IndexOf
>> 28:  * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp 
>> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions 
>> -XX:+EnableX86ECoreOpts IndexOf
> 
> I suggest to split it into 2 subtest jobs and use `@requires vm.cpu.features 
> ~= ".*avx2.*"` for second which specified `-XX:UseAVX=2`.
> See `compiler/loopopts/superword/TestDependencyOffsets.java` for example.

Right.  Done.  Also added `@requires vm.compiler2.enabled` since my stub is 
only valid with C2.

-

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

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

[Volodymyr Paprotski]

On Fri, 24 May 2024 15:32:26 GMT, Scott Gibbons  wrote:

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

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

> 4631: andl(result, 0x000f);  //   tail count (in bytes)
> 4632: andl(limit, 0xfff0);   // vector count (in bytes)
> 4633: jcc(Assembler::zero, COMPARE_TAIL);

In the `expand_ary2` case, this is the same andl/compare as line 4549; i.e. I 
think you can just put `jcc(Assembler::zero, COMPARE_TAIL);` on line 4549, 
inside the if (and move the other jcc into the else branch)?

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

> 4637: negptr(limit);
> 4638: 
> 4639: bind(COMPARE_WIDE_VECTORS_16);

Understanding-check.. this loop will execute at most 2 times, right?

i.e. process as many 32-byte chunks as possible, then 1-or-2 16-byte chunks 
then byte-by-byte?

(Still a good optimization, just trying to understand the scope)

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

> 4716: jmp(TRUE_LABEL);
> 4717:   } else {
> 4718: movl(chr, Address(ary1, limit, scaleFactor));

scaleFactor is always Address::times_1 here (expand_ary2==false), might be 
clearer to change it back

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 57:

> 55: 
> 56: generator = new Random();
> 57: long seed = generator.nextLong();//-5291521104060046276L;

dead code

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 63:

> 61: ///  WARM-UP //
> 62: 
> 63: for (int i = 0; i < 2; i++) {

-Xcomp should be more deterministic (and quicker) way to reach the intrinsic 
(i.e. like the other tests)

On other hand, perhaps this doesn't matter? @vnkozlov Understanding-check 
please.. these tests will run as part of every build from this 
point-till-infinity; Therefore, long test will affect every openjdk developer. 
But if this test is not run on every build, then the build-time does not 
matter, then this test can run for as long as it 'wants'.

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 160:

> 158:   }
> 159: 
> 160:   private static String generateTestString(int min, int max) {

I see you have various `Charset[] charSets` above, but this function still only 
generates LL. Are those separate tests? Or am I missing some concatenation 
somewhere that will convert the generated string string to the correct encoding?

You could had implemented my suggestion from IndexOf.generateTestString here 
instead, so that the tests that do call this function endup with multiple 
encodings; i.e. similar to what you already do in the next function.

I suppose, with addition of String/IndexOf.java that is a moot point.

test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 185:

> 183:   }
> 184: 
> 185:   private static int 

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

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

  Split into two subtest jobs

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/485d02fd..69ca8d13

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

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

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

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

[Vladimir Kozlov]

On Fri, 24 May 2024 19:55:40 GMT, Scott Gibbons  wrote:

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

test/jdk/java/lang/StringBuffer/IndexOf.java line 28:

> 26:  * @summary Test indexOf and lastIndexOf
> 27:  * @run main/othervm IndexOf
> 28:  * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp 
> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions 
> -XX:+EnableX86ECoreOpts IndexOf

I suggest to split it into 2 subtest jobs and use `@requires vm.cpu.features ~= 
".*avx2.*"` for second which specified `-XX:UseAVX=2`.
See `compiler/loopopts/superword/TestDependencyOffsets.java` for example.

-

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

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

[Scott Gibbons]

On Wed, 15 May 2024 19:41:58 GMT, Volodymyr Paprotski  wrote:

>> Scott Gibbons has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 50 commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into indexof
>>  - Move arrays_equals back to c2_MacroAssembler
>>  - Merge branch 'openjdk:master' into indexof
>>  - Remove infinite loop (used for debugging)
>>  - Merge branch 'openjdk:master' into indexof
>>  - Cleaned up, ready for review
>>  - Pre-cleanup code
>>  - Add JMH. Add 16-byte compares to arrays_equals
>>  - Better method for mask creation
>>  - Merge branch 'openjdk:master' into indexof
>>  - ... and 40 more: https://git.openjdk.org/jdk/compare/b20fa7b4...f52d281d
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 81:
> 
>> 79:   String shs = (new String((hs_charset == StandardCharsets.UTF_16) ? 
>> haystack_16 : haystack)).substring(0, haystackSize);
>> 80: 
>> 81:   shs = "$&),,18+-!'8)+";
> 
> Should really keep the original test unmodified and add new tests as needed

The test functionality was not changed.  I just added printing of information 
when a failure occurs.

-

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

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

[Scott Gibbons]

On Wed, 15 May 2024 19:34:40 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 90:
> 
>> 88: 
>> 89:   // printStringBytes(shs.getBytes(hs_charset));
>> 90:   for (int i = 0; i < 20; i++) {
> 
> This wont be a deterministic way to reach the intrinsic. I would suggest 
> copying the idea from 
> test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/Poly1305UnitTestDriver.java
> 
> i.e. Have two `@run main` invocations at the top of this file, one with 
> default parameters, one with  `-Xcomp -XX:-TieredCompilation`. You dont need 
> a 'driver' program, that was to handle something else.
> 
> 
> /*
>  * @test
>  * @modules java.base/com.sun.crypto.provider
>  * @run main java.base/com.sun.crypto.provider.Poly1305KAT
>  * @summary Unit test for com.sun.crypto.provider.Poly1305.
>  */
> 
> /*
>  * @test
>  * @modules java.base/com.sun.crypto.provider
>  * @summary Unit test for IntrinsicCandidate in 
> com.sun.crypto.provider.Poly1305.
>  * @run main/othervm -Xcomp -XX:-TieredCompilation 
> -XX:+UnlockDiagnosticVMOptions -XX:+ForceUnreachable 
> java.base/com.sun.crypto.provider.Poly1305KAT
>  */

Done.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 126:
> 
>> 124: int aNewLength = getRandomIndex(min, max);
>> 125: for (int y = 0; y < aNewLength; y++) {
>> 126:   int achar = generator.nextInt(30) + 30;
> 
> This will only ever generate LL cases, i.e. chars from [30,60]. Could be 
> parametrized to also produce utf16 if instead of 30, offset was in the 
> unicode range

Original code.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 199:
> 
>> 197: 
>> System.out.println("Source="+sourceString.substring(hsBegin, hsBegin + 
>> haystackLen));
>> 198: 
>> System.out.println("Target="+targetString.substring(nBegin, nBegin + 
>> needleLen));
>> 199: System.out.println("haystackLen="+haystackLen+" 
>> neeldeLen="+needleLen+" hsBegin="+hsBegin+" nBegin="+nBegin+
> 
> This looks like 'development scaffolding' (i.e. printf debugging) that was 
> meant to be removed

This is additional information printed upon failure instead of just saying 
"failed"

> test/jdk/java/lang/StringBuffer/IndexOf.java line 295:
> 
>> 293: sourceString = generateTestString(99, 100);
>> 294: sourceBuffer = new StringBuffer(sourceString);
>> 295: targetString = generateTestString(10, 11);
> 
> Generate a random int [0,1,2] for LL, UU, UL, pass that as parameter to 
> generateTestString() to test the other paths. Same for other tests in this 
> file using this pattern.
> 
> This test is specific to haystacklen=100, needlelen=10..  what about other 
> haystack/needle sizes to exercise all the paths in the intrinsic assembler 
> (i.e. haystack >=, <=32, needlelen ={1,2,3,4,5..32..}). Elsewhere already?

Original code.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 360:
> 
>> 358: System.err.println("  sAnswer = " + sAnswer + ", sbAnswer = " + 
>> sbAnswer);
>> 359: System.err.println("  testString = '" + testString + "'");
>> 360: System.err.println("  testBuffer = '" + testBuffer + "'");
> 
> tracing left here and further down

Adding more information on failure.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613915508
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613919180
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613920449
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613922554
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613923075

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

[Scott Gibbons]

On Fri, 24 May 2024 18:32:53 GMT, Volodymyr Paprotski  wrote:

>> Fixed
>
> (missed a `git add`? don't see the updates for this file)

Hmmm... Not sure what happened.

>> Since we're only concerned with the delta of performance, does this really 
>> matter?  Can you suggest an alternative?
>
> The needle really should be like the all the other strings, e.g. 
> `dataStringHuge` itself, generated by the setup. 
> 
> As to weather it really matters; the answer is Amdahl's law. You can indeed 
> measure the delta, but you can't measure the speedup of just the indexOf; not 
> with repeat and concatenation obscuring the numbers.

I have to believe that any relatively smart compiler would recognize that as a 
compile-time constant and make the change irrelevant.  I've yielded to your 
desire and changed the code.

-

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

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

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

  Test clarifications

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/5d10a20b..485d02fd

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

  Stats: 69 lines in 2 files changed: 16 ins; 10 del; 43 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

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

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

[Joe Darcy]

On Fri, 10 May 2024 21:06:22 GMT, Vicente Romero  wrote:

>> As the core reflection code will encounter record classes compiled before 
>> and after the javac code generation change, if the old behavior can be 
>> triggered in javac using `--release $OLD`/`--source $OLD`, that would be 
>> helpful to include as part of the testing.
>
> @jddarcy I have uploaded a new commit addressing your comments, thanks

@vicente-romero-oracle , I think the current iteration of the specification 
changes look good.

-

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

Re: RFR: 8330182: Start of release updates for JDK 24 [v6]

[Joe Darcy]

> Get JDK 24 underway.

Joe Darcy has updated the pull request with a new target base due to a merge or 
a rebase. The pull request now contains 13 commits:

 - Merge branch 'master' into JDK-8330188
 - Merge branch 'master' into JDK-8330188
 - Fix-up test.
 - Merge branch 'master' into JDK-8330188
 - Adjust test for deprecated options.
 - Merge branch 'master' into JDK-8330188
 - Update deprecated options test.
 - Merge branch 'master' into JDK-8330188
 - Merge branch 'master' into JDK-8330188
 - Merge branch 'master' into JDK-8330188
 - ... and 3 more: https://git.openjdk.org/jdk/compare/ebc520e8...2a023349

-

Changes: https://git.openjdk.org/jdk/pull/18787/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18787=05
  Stats: 107 lines in 37 files changed: 46 ins; 7 del; 54 mod
  Patch: https://git.openjdk.org/jdk/pull/18787.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18787/head:pull/18787

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

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]

[Lance Andersen]

On Fri, 24 May 2024 16:36:32 GMT, Joe Wang  wrote:

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

Marked as reviewed by lancea (Reviewer).

-

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

Re: RFR: 8332885: Clarify failure_handler self-tests

[Erik Joelsson]

On Fri, 24 May 2024 12:16:25 GMT, Ludvig Janiuk  wrote:

> Adding commetns to clarify that the failure_handler selftests are intended to 
> be run manually. Ideally this would include a more thorough description of 
> the exepcted outputs, but this is what I have time to add right now.

test/failure_handler/README line 115:

> 113: generated artifacts and cannot be run as part of a CI. They are tests 
> which timeout, crash,
> 114: fail in various ways and you can check the failure_handler output 
> yourself. They might also
> 115: leave processes running on your machine so be extra careful about 
> cleaning up.

These lines are longer than the text blocks in the rest of this file. Could you 
adjust formatting to match?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19393#discussion_r1613935457

Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v5]

[Naoto Sato]

> This test intends to verify the behavior of JdkConsole for the java.base 
> module, wrt restoring the echo. The test assumes the internal methods that 
> sets/gets the echo status of the platform.

Naoto Sato has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 21 commits:

 - Merge branch 'master' into JDK-8332161-restoreEcho-Test
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Added comment for restoreEchoLock
 - fixing typo
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - leftover typo
 - get/setEcho() -> echo()
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Used a dedicate lock for restoreEcho
 - ... and 11 more: https://git.openjdk.org/jdk/compare/ebc520e8...36649fbd

-

Changes: https://git.openjdk.org/jdk/pull/19315/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=04
  Stats: 192 lines in 2 files changed: 192 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19315.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315

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

Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v4]

[Naoto Sato]

> This test intends to verify the behavior of JdkConsole for the java.base 
> module, wrt restoring the echo. The test assumes the internal methods that 
> sets/gets the echo status of the platform.

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

 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Added comment for restoreEchoLock
 - fixing typo
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - leftover typo
 - get/setEcho() -> echo()
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Used a dedicate lock for restoreEcho
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - ... and 10 more: https://git.openjdk.org/jdk/compare/8d2a0559...7311487f

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19315/files
  - new: https://git.openjdk.org/jdk/pull/19315/files/1cd88c61..7311487f

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

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

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

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

[Vladimir Kozlov]

On Fri, 24 May 2024 15:33:46 GMT, Scott Gibbons  wrote:

>> Thanks for checking. Well I know that the 
>> `MacroAssembler::movdqu(XMMRegister dst, AddressLiteral src, Register 
>> rscratch)` method actually generates rip-relative addresses. Maybe we could 
>> copy some of that code.
>
> Changed to `lea` with `InternalAddress()`.  Generates the exact same code, 
> but makes more sense.  I looked at `movdqu` and see no code that generates 
> RIP-relative loads.  It merely checks `reachable()` and adds an intermediate 
> `lea` if not reachable.  @djelinski can you clarify please?

I think HotSpot prefer to have full addresses in `lea` for possible patching.

-

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

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

[Volodymyr Paprotski]

On Fri, 17 May 2024 23:59:05 GMT, Scott Gibbons  wrote:

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

(missed a `git add`? don't see the updates for this file)

-

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

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

[Volodymyr Paprotski]

On Wed, 22 May 2024 14:41:36 GMT, Scott Gibbons  wrote:

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

The needle really should be like the all the other strings, e.g. 
`dataStringHuge` itself, generated by the setup. 

As to weather it really matters; the answer is Amdahl's law. You can indeed 
measure the delta, but you can't measure the speedup of just the indexOf; not 
with repeat and concatenation obscuring the numbers.

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 17:59:49 GMT, Vladimir Kozlov  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   mov64 => lea(InternalAddress)
>
> My testing for v34 passed without new failures.

Thank you @vnkozlov .  Waiting for review from @sviswa7 and @jatin-bhateja, 
then I'll integrate.

-

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

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

[Vladimir Kozlov]

On Fri, 24 May 2024 15:32:26 GMT, Scott Gibbons  wrote:

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

I am fine with current version.

-

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16753#pullrequestreview-2077568604

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

[Vladimir Kozlov]

On Fri, 24 May 2024 15:32:26 GMT, Scott Gibbons  wrote:

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

My testing for v34 passed without new failures.

-

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

Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]

[Oussama Louati]

On Wed, 22 May 2024 22:37:35 GMT, Oussama Louati  wrote:

>> test/jdk/java/lang/invoke/CallSiteTest.java line 96:
>> 
>>> 94: }
>>> 95: 
>>> 96: public static class MyCCS extends ConstantCallSite {
>> 
>> Any reason for this change?
>
> Otherwise, it throws an java.lang.IllegalAccessError:
> 
>> Caused by: java.lang.IllegalAccessError: failed to access class 
>> test.java.lang.invoke.CallSiteTest$MyCCS from class 
>> test.java.lang.invoke.CallSiteTest (test.java.lang.invoke.CallSiteTest$MyCCS 
>> is in unnamed module of loader 'app'; test.java.lang.invoke.CallSiteTest is 
>> in unnamed module of loader indify.Indify$Loader @234ba0f8)

removed

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613824020

Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]

[Oussama Louati]

On Wed, 22 May 2024 08:57:23 GMT, Adam Sotona  wrote:

>> Oussama Louati has updated the pull request incrementally with 19 additional 
>> commits since the last revision:
>> 
>>  - fix: fixed whitespaces
>>  - fix: fixed whitespaces
>>  - chore: used Class::descriptorString
>>  - remove: added removed test comments
>>  - remove: added removed test comments
>>  - remove: removed changes in tests
>>  - update: optimize imports and remove unnecessary utils
>>  - update: optimize imports
>>  - update: 5th patch of code review
>>  - update: 5th patch of code review
>>  - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7
>
> test/jdk/java/lang/invoke/CallSiteTest.java line 96:
> 
>> 94: }
>> 95: 
>> 96: public static class MyCCS extends ConstantCallSite {
> 
> Any reason for this change?

Otherwise, it throws an java.lang.IllegalAccessError:

> Caused by: java.lang.IllegalAccessError: failed to access class 
> test.java.lang.invoke.CallSiteTest$MyCCS from class 
> test.java.lang.invoke.CallSiteTest (test.java.lang.invoke.CallSiteTest$MyCCS 
> is in unnamed module of loader 'app'; test.java.lang.invoke.CallSiteTest is 
> in unnamed module of loader indify.Indify$Loader @234ba0f8)

> test/jdk/java/lang/invoke/CallStaticInitOrder.java line 54:
> 
>> 52: 
>> 53: static int Init1Tick;
>> 54: public static class Init1 {
> 
> Is there a reason to change all the classes and methods to public?

the test throws a java.lang.IllegalAccessError

> test/jdk/java/lang/invoke/indify/Indify.java line 61:
> 
>> 59:  * meaning.
>> 60:  * 
>> 61:  *
> 
> Why is the javadoc reformatted?
> It is not possible to review the changes if has changed.

Okay i removed it

> test/jdk/java/lang/invoke/indify/Indify.java line 271:
> 
>> 269: switch (a) {
>> 270: case "--java":
>> 271: return;
> 
> Where this conditional code disappeared?

I wrote it back inside the writeNewClassFile() method.

> test/jdk/java/lang/invoke/indify/Indify.java line 352:
> 
>> 350: if (verbose)  System.err.println("reading " + f);
>> 351: ClassModel model = parseClassFile(f);
>> 352: if (model == null) throw new IOException("Failed to parse class 
>> file: " + f.getName());
> 
> How it suppose to happen the model is null?

I added this check to address the NullPointerException on MicroBenchmarks, it 
will be removed.

> test/jdk/java/lang/invoke/indify/Indify.java line 354:
> 
>> 352: if (model == null) throw new IOException("Failed to parse class 
>> file: " + f.getName());
>> 353: Logic logic = new Logic(model);
>> 354: if(logic.classModel == null) throw new IOException("Failed to 
>> create logic for class file: " + f.getName());
> 
> And null here again?

I added this check to address the NullPointerException on MicroBenchmarks, it 
will be removed.

> test/jdk/java/lang/invoke/indify/Indify.java line 383:
> 
>> 381: if (verbose)  System.err.println("writing "+destFile);
>> 382: Files.write(destFile.toPath(), new_bytes);
>> 383: System.err.println("Wrote New ClassFile to: "+destFile);
> 
> This code assumes `dest` is always specified?
> For me it looks like it diverges from the original behavior.

I added dest verification.

> test/jdk/java/lang/invoke/indify/Indify.java line 438:
> 
>> 436: } catch (Exception ex) {
>> 437: // pass error from reportPatternMethods, etc.
>> 438: throw (RuntimeException) ex;
> 
> Casting all remaining exceptions to `RuntimeException` ?
> I would suggest to keep the original code 
> or include `RuntimeException` in the fall through section above and wrap all 
> remaining exceptions into a `RuntimeException` to replicate the original 
> behavior.

I assumed that the condition 'ex instanceof RuntimeException' is always 'true',
Reverting to the original code to keep the behavior is applied.

> test/jdk/java/lang/invoke/indify/Indify.java line 470:
> 
>> 468: Logic logic = new Logic(model);
>> 469: Boolean isChanged = logic.transform();
>> 470: if(!isChanged)  throw new IOException("No transformation 
>> has been done");
> 
> Throwing an exception when there is not change also significantly changes the 
> original behavior.

Removed, Thank you for pointing that out. it solved the issue of making inner 
classes in tests public

> test/jdk/java/lang/invoke/indify/Indify.java line 503:
> 
>> 501: 
>> 502: Iterator instructionIterator 
>> =getInstructions(m).iterator();
>> 503: final Stack shouldProceedAfterIndyAdded = new 
>> Stack<>();
> 
> What this stack of booleans suppose to do?

We need a boolean value to determine if we should proceed after replacing the 
appropriate "pop" instruction with an "invokedynamic" instruction. However, 
instead of using just a boolean field, we use a stack. The reason for this is 
that within the lambda expression, we can 

Re: RFR: 8307818: Convert Indify tool to Classfile API [v8]

[Oussama Louati]

On Tue, 21 May 2024 11:37:26 GMT, Adam Sotona  wrote:

> Indify invocation on Microbenchmarks as a part of the JDK build (after 
> application of the above patch) still fails with:
> 
> ```
> Failure on 
> /Applications/XcodeJIB.app/dev/github/jdk/build/macosx-aarch64/support/test/micro/classes
> Exception in thread "main" java.lang.NullPointerException: Cannot invoke 
> "java.lang.classfile.ClassModel.thisClass()" because "model" is null
>   at java.base/java.lang.classfile.ClassFile.transform(ClassFile.java:445)
>   at indify.Indify.transformToBytes(Indify.java:369)
>   at indify.Indify.writeNewClassFile(Indify.java:360)
>   at indify.Indify.indifyFile(Indify.java:356)
>   at indify.Indify.indifyTree(Indify.java:391)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indifyTree(Indify.java:393)
>   at indify.Indify.indify(Indify.java:339)
>   at indify.Indify.run(Indify.java:191)
>   at indify.Indify.main(Indify.java:101)
> ```

I applied a patch addressing this issue, the tool invocation should pass now  
on Microbenchmarks.

-

PR Comment: https://git.openjdk.org/jdk/pull/18841#issuecomment-2123566807

Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]

[Oussama Louati]

On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati  wrote:

>> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code 
>> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, 
>> MethodType, and CallSite constants.
>> It currently uses ad-hoc code to process class files and intends to migrate 
>> to ASM; but since we have the Classfile API, we can migrate to Classfile API 
>> instead.
>
> Oussama Louati has updated the pull request incrementally with 19 additional 
> commits since the last revision:
> 
>  - fix: fixed whitespaces
>  - fix: fixed whitespaces
>  - chore: used Class::descriptorString
>  - remove: added removed test comments
>  - remove: added removed test comments
>  - remove: removed changes in tests
>  - update: optimize imports and remove unnecessary utils
>  - update: optimize imports
>  - update: 5th patch of code review
>  - update: 5th patch of code review
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7

Addressing the second round of review from @asotona.

-

PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2076617737

Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]

[Adam Sotona]

On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati  wrote:

>> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code 
>> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, 
>> MethodType, and CallSite constants.
>> It currently uses ad-hoc code to process class files and intends to migrate 
>> to ASM; but since we have the Classfile API, we can migrate to Classfile API 
>> instead.
>
> Oussama Louati has updated the pull request incrementally with 19 additional 
> commits since the last revision:
> 
>  - fix: fixed whitespaces
>  - fix: fixed whitespaces
>  - chore: used Class::descriptorString
>  - remove: added removed test comments
>  - remove: added removed test comments
>  - remove: removed changes in tests
>  - update: optimize imports and remove unnecessary utils
>  - update: optimize imports
>  - update: 5th patch of code review
>  - update: 5th patch of code review
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7

I would recommend to use more high-level models of the Class-File API instead 
of filling the original code from a new source.
There are also some unnecessary reformats of the javadoc, which is very hard to 
review and forgotten debug prints.
For details see my inline comments.

Generally it is a move forward.

Changes requested by asotona (Reviewer).

test/jdk/java/lang/invoke/CallSiteTest.java line 96:

> 94: }
> 95: 
> 96: public static class MyCCS extends ConstantCallSite {

Any reason for this change?

test/jdk/java/lang/invoke/CallStaticInitOrder.java line 54:

> 52: 
> 53: static int Init1Tick;
> 54: public static class Init1 {

Is there a reason to change all the classes and methods to public?

test/jdk/java/lang/invoke/indify/Indify.java line 61:

> 59:  * meaning.
> 60:  * 
> 61:  *

Why is the javadoc reformatted?
It is not possible to review the changes if has changed.

test/jdk/java/lang/invoke/indify/Indify.java line 271:

> 269: switch (a) {
> 270: case "--java":
> 271: return;

Where this conditional code disappeared?

test/jdk/java/lang/invoke/indify/Indify.java line 352:

> 350: if (verbose)  System.err.println("reading " + f);
> 351: ClassModel model = parseClassFile(f);
> 352: if (model == null) throw new IOException("Failed to parse class 
> file: " + f.getName());

How it suppose to happen the model is null?

test/jdk/java/lang/invoke/indify/Indify.java line 354:

> 352: if (model == null) throw new IOException("Failed to parse class 
> file: " + f.getName());
> 353: Logic logic = new Logic(model);
> 354: if(logic.classModel == null) throw new IOException("Failed to 
> create logic for class file: " + f.getName());

And null here again?

test/jdk/java/lang/invoke/indify/Indify.java line 383:

> 381: if (verbose)  System.err.println("writing "+destFile);
> 382: Files.write(destFile.toPath(), new_bytes);
> 383: System.err.println("Wrote New ClassFile to: "+destFile);

This code assumes `dest` is always specified?
For me it looks like it diverges from the original behavior.

test/jdk/java/lang/invoke/indify/Indify.java line 438:

> 436: } catch (Exception ex) {
> 437: // pass error from reportPatternMethods, etc.
> 438: throw (RuntimeException) ex;

Casting all remaining exceptions to `RuntimeException` ?
I would suggest to keep the original code 
or include `RuntimeException` in the fall through section above and wrap all 
remaining exceptions into a `RuntimeException` to replicate the original 
behavior.

test/jdk/java/lang/invoke/indify/Indify.java line 470:

> 468: Logic logic = new Logic(model);
> 469: Boolean isChanged = logic.transform();
> 470: if(!isChanged)  throw new IOException("No transformation has 
> been done");

Throwing an exception when there is not change also significantly changes the 
original behavior.

test/jdk/java/lang/invoke/indify/Indify.java line 503:

> 501: 
> 502: Iterator instructionIterator 
> =getInstructions(m).iterator();
> 503: final Stack shouldProceedAfterIndyAdded = new 
> Stack<>();

What this stack of booleans suppose to do?

test/jdk/java/lang/invoke/indify/Indify.java line 540:

> 538: assert (i.sizeInBytes() == 3 || i2.sizeInBytes() 
> == 3);
> 539: 
> System.err.println("Transforming
>  Method INDY Instructions & Creating New 
> ClassModels--}}}");
> 540: if (!quiet) System.err.println(":::Transfmoring 
> the Method: "+ m.methodName() +" instruction: " + i + " invokedynamic: " + 
> con.index() );

This debug print should be probably removed before integration.

Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]

[Oussama Louati]

> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code 
> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, 
> MethodType, and CallSite constants.
> It currently uses ad-hoc code to process class files and intends to migrate 
> to ASM; but since we have the Classfile API, we can migrate to Classfile API 
> instead.

Oussama Louati has updated the pull request incrementally with 19 additional 
commits since the last revision:

 - fix: fixed whitespaces
 - fix: fixed whitespaces
 - chore: used Class::descriptorString
 - remove: added removed test comments
 - remove: added removed test comments
 - remove: removed changes in tests
 - update: optimize imports and remove unnecessary utils
 - update: optimize imports
 - update: 5th patch of code review
 - update: 5th patch of code review
 - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18841/files
  - new: https://git.openjdk.org/jdk/pull/18841/files/781c951d..2b8c94a7

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

  Stats: 585 lines in 6 files changed: 49 ins; 338 del; 198 mod
  Patch: https://git.openjdk.org/jdk/pull/18841.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18841/head:pull/18841

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

Integrated: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

[Naoto Sato]

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

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

This pull request has now been integrated.

Changeset: 236432db
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/236432dbdb9bab4aece54c2fea08f055e5dbf97e
Stats: 18 lines in 1 file changed: 10 ins; 0 del; 8 mod

8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

Reviewed-by: prappo, joehw, smarks

-

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

Re: RFR: 8332898: failure_handler: log directory of commands

[Leonid Mesnik]

On Fri, 24 May 2024 14:34:39 GMT, Ludvig Janiuk  wrote:

> Also log the directory in which the command used by failure_handler was 
> executed. While often the same, it isn't always, and so it this should 
> simplify troubleshooting for someone looking at this at a glance without 
> being a failure_handler expert.
> 
> Example output after this change:
> 
> 
> [2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2 in 
> //JTwork/scratch
> 
> 
> before:
> 
> 
> [2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2
> 

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19396#pullrequestreview-2077492691

Re: RFR: 8332885: Clarify failure_handler self-tests

[Leonid Mesnik]

On Fri, 24 May 2024 12:16:25 GMT, Ludvig Janiuk  wrote:

> Adding commetns to clarify that the failure_handler selftests are intended to 
> be run manually. Ideally this would include a more thorough description of 
> the exepcted outputs, but this is what I have time to add right now.

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19393#pullrequestreview-2077481241

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

[Naoto Sato]

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

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

  Added comment for restoreEchoLock

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19184/files
  - new: https://git.openjdk.org/jdk/pull/19184/files/69ec27d6..d4b6e695

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

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

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

Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException

[Stuart Marks]

On Wed, 27 Mar 2024 17:36:28 GMT, Liam Miller-Cushon  wrote:

> This change overrides mutator methods in the implementation returned by 
> `Map.of().entrySet()` to throw `UnsupportedOperationException`.

Sorry for the delay, I'm still totally backlogged on other stuff. I can't 
commit to get this into JDK 23.

-

PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2130014264

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]

[Joe Wang]

On Fri, 24 May 2024 16:36:32 GMT, Joe Wang  wrote:

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

Thanks Magnus, Alan. Renamed the template to jaxp-strict.properties.template.

-

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

Re: RFR: 8332597: Remove redundant methods from j.l.classfile.ClassReader API [v2]

[Adam Sotona]

> j.l.classfile.ClassReader instance is exposed in the Class-File API through 
> j.l.classfile.AttributeMapper::readAttribute method only.
> ClassReader only purpose is to serve as a tool for reading content of a 
> custom attribute in a user-provided AttribtueMapper.
> It contains useful set of low-level class reading methods for user to 
> implement a custom attribute content parser.
> 
> However methods ClassReader::thisClassPos and 
> ClassReader::skipAttributeHolder are not necessary for a custom attribute 
> content parsing and so redundant in the API.
> Class-File API implementation internally use these methods, however they 
> should not be exposed in the API.
> 
> This patch removes the methods from the API.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains two commits:

 - Merge branch 'master' into JDK-8332597-classreader-redundancy
   
   # Conflicts:
   #src/java.base/share/classes/jdk/internal/classfile/impl/ClassImpl.java
 - 8332597: Remove redundant methods from j.l.classfile.ClassReader API

-

Changes: https://git.openjdk.org/jdk/pull/19323/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19323=01
  Stats: 23 lines in 5 files changed: 0 ins; 17 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/19323.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19323/head:pull/19323

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

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]

[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 on 5/16/2024
> 
> Design change:
> The design is changed to include in the JDK two configuration files that are 
> the default jaxp.properties and jaxp-strict.properties, instead of three, 
> dropping jaxp-compat.properties.
> 
> Updated on 5/18/2024
> 
> Withdraw changes to jaxp.properties. The original idea was to match 
> jaxp-strict.properties with regard to the properties. However, that change 
> impact the configuration process, resulting in tests that verify the process 
> to fail.
> 
> Updated on 5/23/2024
> 
> Provide a template `jaxp-strict.template` instead of a properties file. This 
> template can be used to create custom configuration files.

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

  rename the template to jaxp-strict.properties.template

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18831/files
  - new: https://git.openjdk.org/jdk/pull/18831/files/0de8ad69..714095d1

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

  Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 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: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v13]

[Adam Sotona]

> java.base java.lang.invoke package heavily uses ASM to generate lambdas and 
> method handles.
> 
> This patch converts ASM calls to Classfile API.
> 
> This PR is continuation of https://github.com/openjdk/jdk/pull/12945
> 
> Any comments and suggestions are welcome.
> 
> Please review.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 23 commits:

 - Merge branch 'master' into JDK-8294960-invoke
   
   # Conflicts:
   #src/java.base/share/classes/java/lang/classfile/Attributes.java
 - fixed CodeBuilder use in j.l.invoke
 - Merge branch 'master' into JDK-8294960-invoke
 - Merge pull request #4 from cl4es/boxunbox_holder
   
   Only create box/unbox MethodRefEntries on request
 - Only create box/unbox MethodRefEntries on request
 - Merge pull request #3 from cl4es/minor_init_improvements
   
   Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator
 - Remove stray MRE_LF_interpretWithArguments
 - Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator
 - Deferred initialization of attributes map by moving into a holder class
   
   Co-authored-by: Claes Redestad 
 - Merge branch 'master' into JDK-8294960-invoke
 - ... and 13 more: https://git.openjdk.org/jdk/compare/cfdc64fc...e1dbabcb

-

Changes: https://git.openjdk.org/jdk/pull/17108/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=17108=12
  Stats: 2113 lines in 10 files changed: 422 ins; 861 del; 830 mod
  Patch: https://git.openjdk.org/jdk/pull/17108.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108

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

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

[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 with a new target base due to a merge 
or a rebase. The pull request now contains 37 commits:

 - fixed ParserVerifier and VerifierSelfTest
 - Merge branch 'master' into JDK-8320396-verifier-extension
 - added verification of TypeAnnotation attributes
 - added verification of SMT attribute
 - added verification of module-related attributes
 - applied the suggested changes
 - applied the suggested changes
 - fixed error thrown by VerifierImpl
 - applied suggested changes
 - Merge branch 'master' into JDK-8320396-verifier-extension
 - ... and 27 more: https://git.openjdk.org/jdk/compare/cfdc64fc...b352b794

-

Changes: https://git.openjdk.org/jdk/pull/16809/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=16809=08
  Stats: 869 lines in 6 files changed: 835 ins; 7 del; 27 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

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

[Stuart Marks]

On Fri, 24 May 2024 15:38:16 GMT, Naoto Sato  wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown 
>> thread, which differs from the application's thread that issues the 
>> `readPassword()` method.
>
> Naoto Sato has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains 11 additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - Used a dedicate lock for restoreEcho
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - copyright year
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - get/setEcho()
>  - Addresses a review comment
>  - Replaced another unused exception with _
>  - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - ... and 1 more: https://git.openjdk.org/jdk/compare/b2ea52b6...69ec27d6

Latest updates look good.

-

Marked as reviewed by smarks (Reviewer).

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

Integrated: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations

[Adam Sotona]

On Mon, 29 Apr 2024 18:48:53 GMT, Adam Sotona  wrote:

> Hi,
> During performance optimization work on Class-File API as JDK lambda 
> generator we found some static initialization killers.
> One of them is `java.lang.classfile.Attributes` with tens of static fields 
> initialized with individual attribute mappers, and common set of all mappers, 
> and static map from attribute names to the mappers.
> 
> I propose to turn all the static fields into lazy-initialized static methods 
> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` 
> static mapping method from the `Attributes` API class.
>  
> Please let me know your comments or objections and please review the 
> [PR](https://github.com/openjdk/jdk/pull/19006) and 
> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23.
> 
> Thank you,
> Adam

This pull request has now been integrated.

Changeset: cfdc64fc
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/cfdc64fcb43e3b261dddc6cc6947235a9e76154e
Stats: 2285 lines in 149 files changed: 961 ins; 615 del; 709 mod

8331291: java.lang.classfile.Attributes class performs a lot of static 
initializations

Reviewed-by: liach, redestad, vromero

-

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

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

[Erik Gahlin]

On Tue, 21 May 2024 22:41:12 GMT, Stuart Marks  wrote:

>> 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.
>
> Collapsing the extra layer of methods and combining the test into
> 
> if (jfrTracing && FileReadEvent.enabled())
> 
> would indeed keep things a little neater.
> 
> I'm still questioning the need for `jfrTracing` at all. There's the matter of 
> how this boolean gets set and unset, and whether this is done in a way that 
> comports with the memory model. Setting this aside, is the only benefit that 
> it avoids loading an extra class at JVM startup time (without JFR)? In this 
> case that would be the `FileReadEvent` class, which is the stub class in 
> `jdk.internal.event`. Wouldn't this class be in the CDS archive, making it 
> not worth the effort of bringing up this new `jfrTracing` mechanism simply to 
> avoid loading it unnecessarily?
> 
> I observe that in JDK 22 some (but not all) of the event classes in 
> `jdk.internal.event` seem to be present in the CDS archive. These include 
> various virtual thread events.

I don't think issue is so much the overhead of loading (one or more) additional 
classes without JFR, even if it causes a extremely small regression, but the 
added overhead added to JFR when trying to fix the regression.

I did an experiment with:

`if (FlightRecorder.enabled && FileReadEvent.enabled())`

and it passes JFR tests and tier1/tier2. I don't think `FlightRecorder.enabled` 
needs to be used on every event class, but it would be good to use on event 
classes loaded during startup, both for safety reasons and to lower overhead of 
JFR startup. The `jfrTracing` flag works as well, but it is perhaps harder to 
comprehend and requires an additional static boolean in every class, which does 
clutter things up.

I can push Alan's suggestion of uniting the checks into one if-statement. It 
may help to see how it  looks.

Virtual thread events are typically loaded in main, after JFR has started, and 
not an issue unless `jcmd JFR.start `is used, which is not that common.

-

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

Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v3]

[Naoto Sato]

> This test intends to verify the behavior of JdkConsole for the java.base 
> module, wrt restoring the echo. The test assumes the internal methods that 
> sets/gets the echo status of the platform.

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

 - fixing typo
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - leftover typo
 - get/setEcho() -> echo()
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - Merge branch 'JDK-8332084-restoreEcho-readLock' into 
JDK-8332161-restoreEcho-Test
 - initial commit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19315/files
  - new: https://git.openjdk.org/jdk/pull/19315/files/ad4a4e47..1cd88c61

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

  Stats: 14345 lines in 305 files changed: 9600 ins; 3407 del; 1338 mod
  Patch: https://git.openjdk.org/jdk/pull/19315.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315

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

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

[Naoto Sato]

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

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

 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - Used a dedicate lock for restoreEcho
 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - copyright year
 - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
 - get/setEcho()
 - Addresses a review comment
 - Replaced another unused exception with _
 - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - ... and 1 more: https://git.openjdk.org/jdk/compare/472fb9f3...69ec27d6

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19184/files
  - new: https://git.openjdk.org/jdk/pull/19184/files/f69f747a..69ec27d6

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

  Stats: 14344 lines in 304 files changed: 9600 ins; 3407 del; 1337 mod
  Patch: https://git.openjdk.org/jdk/pull/19184.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184

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

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

[Scott Gibbons]

On Fri, 24 May 2024 14:49:05 GMT, Daniel Jeliński  wrote:

>> Just did the experiment and it turns out that `mov64(r15, 
>> (int64_t)small_jump_table)` and `lea(r15, 
>> ExternalAddress(small_jump_table))` produce exactly the same code:
>> 
>> `0x7fffe463d68b:  49 bf a0 d5 63 e4 ff 7f 00 00   movabs 
>> r15,0x7fffe463d5a0`
>> 
>> The code in `MacroAssembler` for `lea` calls `mov_literal64` with no check 
>> for whether it can be ip-relative.
>> 
>> I tried doing it myself via `leaq(r15, Address(rip, 
>> (int64_t)small_jump_table - (int64_t)(__ pc(` but there is no definition 
>> in `register_x86.hpp` for register `rip`.  So I'm not sure exactly how to 
>> produce RIP-relative addressing.
>
> Thanks for checking. Well I know that the `MacroAssembler::movdqu(XMMRegister 
> dst, AddressLiteral src, Register rscratch)` method actually generates 
> rip-relative addresses. Maybe we could copy some of that code.

Changed to `lea` with `InternalAddress()`.  Generates the exact same code, but 
makes more sense.  I looked at `movdqu` and see no code that generates 
RIP-relative loads.  It merely checks `reachable()` and adds an intermediate 
`lea` if not reachable.  @djelinski can you clarify please?

-

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

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

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

  mov64 => lea(InternalAddress)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/1a71eb10..5d10a20b

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

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

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

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

[Vladimir Kozlov]

On Fri, 24 May 2024 14:49:05 GMT, Daniel Jeliński  wrote:

>> Just did the experiment and it turns out that `mov64(r15, 
>> (int64_t)small_jump_table)` and `lea(r15, 
>> ExternalAddress(small_jump_table))` produce exactly the same code:
>> 
>> `0x7fffe463d68b:  49 bf a0 d5 63 e4 ff 7f 00 00   movabs 
>> r15,0x7fffe463d5a0`
>> 
>> The code in `MacroAssembler` for `lea` calls `mov_literal64` with no check 
>> for whether it can be ip-relative.
>> 
>> I tried doing it myself via `leaq(r15, Address(rip, 
>> (int64_t)small_jump_table - (int64_t)(__ pc(` but there is no definition 
>> in `register_x86.hpp` for register `rip`.  So I'm not sure exactly how to 
>> produce RIP-relative addressing.
>
> Thanks for checking. Well I know that the `MacroAssembler::movdqu(XMMRegister 
> dst, AddressLiteral src, Register rscratch)` method actually generates 
> rip-relative addresses. Maybe we could copy some of that code.

Use `lea` and `InternalAddress()` for referencing jump tables since the 
addresses are in the same code section.

-

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

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

[Daniel Fuchs]

On Tue, 21 May 2024 07:26:17 GMT, Alan Bateman  wrote:

>> This is the implementation changes for JEP 471.
>> 
>> The methods in sun.misc.Unsafe for on-heap and off-heap access are 
>> deprecated for removal. This means a removal warning at compile time. No 
>> methods have been removed. A deprecated message is added to each of the 
>> methods but unlikely to be seen as the JDK does not generate or publish the 
>> API docs for this class.
>> 
>> A new command line option --sun-misc-unsafe-memory-access=$value is 
>> introduced to allow or deny access to these methods. The default proposed 
>> for JDK 23 is "allow" so no change in behavior compared to JDK 22 or 
>> previous releases.
>> 
>> A new test is added to test the command line option settings. The existing 
>> micros for FFM that use Unsafe are updated to suppress the removal warning 
>> at compile time. A new micro is introduced with a small sample of methods to 
>> ensure the changes doesn't cause any perf regressions ([sample 
>> results](https://cr.openjdk.org/~alanb/8331670-results.txt)).
>> 
>> For now, the changes include the update to the man page for the "java" 
>> command. It might be that this has to be separated out so that it goes with 
>> other updates in the release.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

src/jdk.unsupported/share/classes/sun/misc/Unsafe.java line 1750:

> 1748: }
> 1749: 
> 1750: // Instructure for --sun-misc-unsafe-memory-access= command 
> line option.

Suggestion:

// Infrastructure for --sun-misc-unsafe-memory-access= command line 
option.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1613644783

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

[Yudi Zheng]

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

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

 - Merge remote-tracking branch 'upstream/master' into JDK-8327964
 - address comments.
 - address comments.
 - address comment.
 - address comment.
 - address comment.
 - address comment.
 - Simplify BigInteger.implMultiplyToLen intrinsic

-

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

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

  Stats: 560567 lines in 6784 files changed: 132593 ins; 81763 del; 346211 mod
  Patch: https://git.openjdk.org/jdk/pull/18226.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18226/head:pull/18226

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

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

[Martin Doerr]

On Fri, 24 May 2024 07:24:13 GMT, Matthias Baesken  wrote:

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

I also think that the `sp.dirlen > 0` check is not necessary, but I'm ok with 
this version, too.

-

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19329#pullrequestreview-2077166773

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

[Yudi Zheng]

On Thu, 23 May 2024 10:12:17 GMT, Bhavana Kilambi  wrote:

>> Yudi Zheng has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   address comments.
>
> src/hotspot/share/opto/library_call.cpp line 5925:
> 
>> 5923:   // Set the original stack and the reexecute bit for the interpreter 
>> to reexecute
>> 5924:   // the bytecode that invokes BigInteger.multiplyToLen() if 
>> deoptimization happens
>> 5925:   // on the return from z array allocation in runtime.
> 
> Since we are not allocating z array during runtime anymore, do we still need 
> these comments?

Thanks for pointing it out! Removed.

> src/java.base/share/classes/java/math/BigInteger.java line 1836:
> 
>> 1834: 
>> 1835: if (z == null || z.length < (xlen + ylen))
>> 1836:  z = new int[xlen + ylen];
> 
> Style: only 4 spaces indentation

Done

-

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

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

[Daniel Jeliński]

On Fri, 24 May 2024 14:19:13 GMT, Scott Gibbons  wrote:

>> the RIP-relative lea should have a shorter encoding. I think something like 
>> `lea(r15, ExternalAddress(small_jump_table))` should produce it (untested)
>
> Just did the experiment and it turns out that `mov64(r15, 
> (int64_t)small_jump_table)` and `lea(r15, ExternalAddress(small_jump_table))` 
> produce exactly the same code:
> 
> `0x7fffe463d68b:  49 bf a0 d5 63 e4 ff 7f 00 00   movabs 
> r15,0x7fffe463d5a0`
> 
> The code in `MacroAssembler` for `lea` calls `mov_literal64` with no check 
> for whether it can be ip-relative.
> 
> I tried doing it myself via `leaq(r15, Address(rip, (int64_t)small_jump_table 
> - (int64_t)(__ pc(` but there is no definition in `register_x86.hpp` for 
> register `rip`.  So I'm not sure exactly how to produce RIP-relative 
> addressing.

Thanks for checking. Well I know that the `MacroAssembler::movdqu(XMMRegister 
dst, AddressLiteral src, Register rscratch)` method actually generates 
rip-relative addresses. Maybe we could copy some of that code.

-

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

RFR: 8332898: failure_handler: log directory of commands

[Ludvig Janiuk]

Also log the directory in which the command used by failure_handler was 
executed. While often the same, it isn't always, and so it this should simplify 
troubleshooting for someone looking at this at a glance without being a 
failure_handler expert.

Example output after this change:


[2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2 in 
//JTwork/scratch


before:


[2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2


-

Commit messages:
 - 8332898 failure_handler: log directory of commands

Changes: https://git.openjdk.org/jdk/pull/19396/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19396=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332898
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19396.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19396/head:pull/19396

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

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

[Scott Gibbons]

On Fri, 24 May 2024 06:31:40 GMT, Daniel Jeliński  wrote:

>> It may, but I believe the movq is shorter (although maybe not to r15).  I'll 
>> do some experimentation.
>
> the RIP-relative lea should have a shorter encoding. I think something like 
> `lea(r15, ExternalAddress(small_jump_table))` should produce it (untested)

Just did the experiment and it turns out that `mov64(r15, 
(int64_t)small_jump_table)` and `lea(r15, ExternalAddress(small_jump_table))` 
produce exactly the same code:

`0x7fffe463d68b:  49 bf a0 d5 63 e4 ff 7f 00 00   movabs r15,0x7fffe463d5a0`

The code in `MacroAssembler` for `lea` calls `mov_literal64` with no check for 
whether it can be ip-relative.

I tried doing it myself via `leaq(r15, Address(rip, (int64_t)small_jump_table - 
(int64_t)(__ pc(` but there is no definition in `register_x86.hpp` for 
register `rip`.  So I'm not sure exactly how to produce RIP-relative addressing.

-

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

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

[Roger Riggs]

On Fri, 24 May 2024 07:24:13 GMT, Matthias Baesken  wrote:

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

LGTM  (Freeing the buffer on fail)

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19329#pullrequestreview-2077014965

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

[Jan Lahoda]

On Wed, 22 May 2024 13:58:21 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:
> 
>   Add a few more legacy methods, needed a few more after changes to the 
> checker

Looks good to me, thanks for doing this!

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

> 349: }
> 350: String uniqueId = getElementName(te, element, types);
> 351: boolean isCommonMethod = uniqueId.endsWith("toString()") ||

Nit: `.toString()` instead of `toString()`?

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2076973109
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1613507278

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

[Scott Gibbons]

On Fri, 24 May 2024 00:47:04 GMT, Vladimir Kozlov  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments - move stubGen*_string.cpp to c2_stubGen*_string.cpp
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2000, 2024 Oracle and/or its affiliates. All rights 
>> reserved.
> 
> This copyright header validation failure. Missing comma `,` after 2024.

Fixed.

-

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

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

[Scott Gibbons]

On Fri, 24 May 2024 06:31:36 GMT, Daniel Jeliński  wrote:

>> Thanks for finding this.  It was ignorance on my part as I thought the xorq 
>> would have logic to not emit the REX prefix if not necessary, but it 
>> doesn't.  Fixed.
>
> Right, it seems to surprise people. There's a lot of preexisting uses of xorq 
> / xorptr to zero a register. I think it would make sense to implement this 
> logic in xorq. I can do this if others agree.

Good idea.  I vote yes.

-

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

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

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

  Missing comma

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/c034d3f9..1a71eb10

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

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

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

Integrated: 8305457: Implement java.io.IO

[Pavel Rappo]

On Mon, 6 May 2024 21:45:12 GMT, Pavel Rappo  wrote:

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

This pull request has now been integrated.

Changeset: c099f14f
Author:Pavel Rappo 
URL:   
https://git.openjdk.org/jdk/commit/c099f14f07260713229cffbe7d23aa8305415a67
Stats: 696 lines in 15 files changed: 691 ins; 1 del; 4 mod

8305457: Implement java.io.IO

Reviewed-by: naoto, smarks, jpai, jlahoda

-

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

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

[Jan Lahoda]

On Fri, 24 May 2024 11:30:19 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - Fix test failure
>
>Caused by 4e6d851f3f061b4a9c2b5d2e3fba6a0277ac1f34:
>
>8325324: Implement JEP 477: Implicitly Declared Classes
> and Instance Main Methods (Third Preview)
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Merge remote-tracking branch 'jdk/master' into 8305457-Implement-java.io.IO
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Force reasonable terminal size
>
>JLine outputs unexpected stuff if the terminal isn't dumb and small,
>such as that of our CI machines:
>
>if (newLines.size() > displaySize && !isTerminalDumb()) {
>StringBuilder sb = new StringBuilder(">");
>
>This causes the IO test to fail with timeout, because the expected
>prompt is never matched. To avoid that, we reasonably size the
>terminal.
>  - Restructure the test
>  - Add diagnostic output
>  - Use "expect" that was found
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>
># Conflicts:
>#  src/java.base/share/classes/java/io/ProxyingConsole.java
>#  src/java.base/share/classes/jdk/internal/io/JdkConsole.java
>#  src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>#  
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
>#  src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
>  - Escape prompt
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/239c1b33...5edf686d

Looks good to me.

-

Marked as reviewed by jlahoda (Reviewer).

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

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

[Adam Sotona]

> Hi,
> During performance optimization work on Class-File API as JDK lambda 
> generator we found some static initialization killers.
> One of them is `java.lang.classfile.Attributes` with tens of static fields 
> initialized with individual attribute mappers, and common set of all mappers, 
> and static map from attribute names to the mappers.
> 
> I propose to turn all the static fields into lazy-initialized static methods 
> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` 
> static mapping method from the `Attributes` API class.
>  
> Please let me know your comments or objections and please review the 
> [PR](https://github.com/openjdk/jdk/pull/19006) and 
> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  fixed tests

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19006/files
  - new: https://git.openjdk.org/jdk/pull/19006/files/37f7f63f..db73c2dd

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

  Stats: 8 lines in 4 files changed: 1 ins; 2 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/19006.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006

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

RFR: 8332885: Clarify failure_handler self-tests

[Ludvig Janiuk]

Adding commetns to clarify that the failure_handler selftests are intended to 
be run manually. Ideally this would include a more thorough description of the 
exepcted outputs, but this is what I have time to add right now.

-

Commit messages:
 - 8332885 Clarify failure_handler self-tests

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

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

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v13]

[Alan Bateman]

On Fri, 24 May 2024 05:26:40 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated on 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>> 
>> Updated on 5/18/2024
>> 
>> Withdraw changes to jaxp.properties. The original idea was to match 
>> jaxp-strict.properties with regard to the properties. However, that change 
>> impact the configuration process, resulting in tests that verify the process 
>> to fail.
>> 
>> Updated on 5/23/2024
>> 
>> Provide a template `jaxp-strict.template` instead of a properties file. This 
>> template can be used to create custom configuration files.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a template instead of a property file; remove implNote; update test and 
> make script accordingly.

Marked as reviewed by alanb (Reviewer).

Magnus's suggestion for the suffix to be .properties.template makes sense, 
consistent with the one .template that the JDK currently includes in the 
run-time image.

Overall this looks okay, I'm happy that the implNote update is removed from the 
proposal as it read too much like the introducing a new supported interface and 
would have been confusing to have two configurations in the conf directory.

-

PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2076667358
PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2129320078

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

[Pavel Rappo]

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

Pavel Rappo has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 26 commits:

 - Fix test failure
   
   Caused by 4e6d851f3f061b4a9c2b5d2e3fba6a0277ac1f34:
   
   8325324: Implement JEP 477: Implicitly Declared Classes
and Instance Main Methods (Third Preview)
 - Merge branch 'master' into 8305457-Implement-java.io.IO
 - Merge remote-tracking branch 'jdk/master' into 8305457-Implement-java.io.IO
 - Merge branch 'master' into 8305457-Implement-java.io.IO
 - Force reasonable terminal size
   
   JLine outputs unexpected stuff if the terminal isn't dumb and small,
   such as that of our CI machines:
   
   if (newLines.size() > displaySize && !isTerminalDumb()) {
   StringBuilder sb = new StringBuilder(">");
   
   This causes the IO test to fail with timeout, because the expected
   prompt is never matched. To avoid that, we reasonably size the
   terminal.
 - Restructure the test
 - Add diagnostic output
 - Use "expect" that was found
 - Merge branch 'master' into 8305457-Implement-java.io.IO
   
   # Conflicts:
   #src/java.base/share/classes/java/io/ProxyingConsole.java
   #src/java.base/share/classes/jdk/internal/io/JdkConsole.java
   #src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
   #
src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
   #src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
 - Escape prompt
 - ... and 16 more: https://git.openjdk.org/jdk/compare/239c1b33...5edf686d

-

Changes: https://git.openjdk.org/jdk/pull/19112/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19112=13
  Stats: 696 lines in 15 files changed: 691 ins; 1 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/19112.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112

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

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

[Jan Lahoda]

On Thu, 23 May 2024 17:14:19 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 23 commits:
> 
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Force reasonable terminal size
>
>JLine outputs unexpected stuff if the terminal isn't dumb and small,
>such as that of our CI machines:
>
>if (newLines.size() > displaySize && !isTerminalDumb()) {
>StringBuilder sb = new StringBuilder(">");
>
>This causes the IO test to fail with timeout, because the expected
>prompt is never matched. To avoid that, we reasonably size the
>terminal.
>  - Restructure the test
>  - Add diagnostic output
>  - Use "expect" that was found
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>
># Conflicts:
>#  src/java.base/share/classes/java/io/ProxyingConsole.java
>#  src/java.base/share/classes/jdk/internal/io/JdkConsole.java
>#  src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>#  
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
>#  src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
>  - Escape prompt
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Clarify input charset
>  - Make IO final
>  - ... and 13 more: https://git.openjdk.org/jdk/compare/e19a421c...258ce133

JShell-related changes look good to me.

-

Marked as reviewed by jlahoda (Reviewer).

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

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

[Claes Redestad]

On Fri, 24 May 2024 08:24:15 GMT, Adam Sotona  wrote:

>> Hi,
>> During performance optimization work on Class-File API as JDK lambda 
>> generator we found some static initialization killers.
>> One of them is `java.lang.classfile.Attributes` with tens of static fields 
>> initialized with individual attribute mappers, and common set of all 
>> mappers, and static map from attribute names to the mappers.
>> 
>> I propose to turn all the static fields into lazy-initialized static methods 
>> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` 
>> static mapping method from the `Attributes` API class.
>>  
>> Please let me know your comments or objections and please review the 
>> [PR](https://github.com/openjdk/jdk/pull/19006) and 
>> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 
>> 23.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 16 commits:
> 
>  - fixed jdeps.Dependencies
>  - Merge branch 'master' into JDK-8331291-attributes
>  - addressed CSR review comments
>  - fixed CompilationIDMapper does not allow multiple instances
>  - fixed tests
>  - fixed tests
>  - fixed tests
>  - updated LimitsTest
>  - Merge branch 'master' into JDK-8331291-attributes
>
># Conflicts:
>#  test/jdk/jdk/classfile/SignaturesTest.java
>  - Merge branch 'master' into JDK-8331291-attributes
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/239c1b33...37f7f63f

Looks good after revisions.

-

Marked as reviewed by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19006#pullrequestreview-2076508421

Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v13]

[Magnus Ihse Bursie]

On Fri, 24 May 2024 05:26:40 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated on 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>> 
>> Updated on 5/18/2024
>> 
>> Withdraw changes to jaxp.properties. The original idea was to match 
>> jaxp-strict.properties with regard to the properties. However, that change 
>> impact the configuration process, resulting in tests that verify the process 
>> to fail.
>> 
>> Updated on 5/23/2024
>> 
>> Provide a template `jaxp-strict.template` instead of a properties file. This 
>> template can be used to create custom configuration files.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a template instead of a property file; remove implNote; update test and 
> make script accordingly.

I would recommend naming the new file `jaxp-strict.properties.template` 
instead. This would follow the pattern we have used in the JDK, and I think it 
is much better at providing clarify as to what this file actually is  -- a 
template for a `.properties` file.

-

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

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option

[Michael Hall]

> On May 24, 2024, at 3:08 AM, Michael Hall  wrote:
> 
>> On May 23, 2024, at 8:13 PM, Alexander Matveev > > wrote:
>> 
>> otherwise add additional content as post-processing step.
> 
> Doesn’t this still leave you with an application that isn’t validly signed? 
> And probably won’t run because of that.
> 
>> 2) jpackage --type app-image -n Test --app-content ReadMe ...
> 
> For your example. This almost seems like an Apple bug if you can add a 
> directory to the Contents directory but not a file? 

Sorry I made my prior off-list.

Would it also generally be a good idea to include a final codesign verify to 
fail the build if something is wrong with the signature?

Something like…

echo '***'
echo 'verifying signature'
echo '***'
codesign -v --verbose=4 outputdir/HalfPipe.app

Expected output…

***
verifying signature
***
outputdir/HalfPipe.app: valid on disk
outputdir/HalfPipe.app: satisfies its Designated Requirement

I think I have suggested this before but don’t remember if I did an enhancement 
request. Maybe you do that and I’m just not aware of it if it doesn’t appear in 
the jpackage output.

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

[Magnus Ihse Bursie]

On Fri, 24 May 2024 07:24:13 GMT, Matthias Baesken  wrote:

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

This looks much safer. Thank you! 

I think the code can be simplified a bit, as commented. It does not matter 
much, so you can keep the current code as well if you think it looks better.

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

> 561: offset = copystrings(buf, offset, >envv[0]);
> 562: if (c->pdir != NULL) {
> 563: if (sp.dirlen > 0) {

As long as c->pdir is non-null, I think the code below is safe to execute. 
`memcpy(a, b, len)` should be okay if `len` is 0, as long as `a` and `b` are 
non-null, right?

So this check here is not needed, I think.

-

PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2128960961
PR Review Comment: https://git.openjdk.org/jdk/pull/19329#discussion_r1613102923

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

[Adam Sotona]

> Hi,
> During performance optimization work on Class-File API as JDK lambda 
> generator we found some static initialization killers.
> One of them is `java.lang.classfile.Attributes` with tens of static fields 
> initialized with individual attribute mappers, and common set of all mappers, 
> and static map from attribute names to the mappers.
> 
> I propose to turn all the static fields into lazy-initialized static methods 
> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` 
> static mapping method from the `Attributes` API class.
>  
> Please let me know your comments or objections and please review the 
> [PR](https://github.com/openjdk/jdk/pull/19006) and 
> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23.
> 
> Thank you,
> Adam

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

 - fixed jdeps.Dependencies
 - Merge branch 'master' into JDK-8331291-attributes
 - addressed CSR review comments
 - fixed CompilationIDMapper does not allow multiple instances
 - fixed tests
 - fixed tests
 - fixed tests
 - updated LimitsTest
 - Merge branch 'master' into JDK-8331291-attributes
   
   # Conflicts:
   #test/jdk/jdk/classfile/SignaturesTest.java
 - Merge branch 'master' into JDK-8331291-attributes
 - ... and 6 more: https://git.openjdk.org/jdk/compare/239c1b33...37f7f63f

-

Changes: https://git.openjdk.org/jdk/pull/19006/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19006=09
  Stats: 2277 lines in 145 files changed: 960 ins; 613 del; 704 mod
  Patch: https://git.openjdk.org/jdk/pull/19006.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006

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

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

[Alan Bateman]

On Thu, 23 May 2024 23:24:16 GMT, Chen Liang  wrote:

> Hmm, actually, looking at the specs of the method again, does it imply that 
> Proxy classes are never unloaded once defined in a ClassLoader, as seen in 
> `Proxy::getProxyClass`:

It's not specified, Proxy pre-dates hidden classes although its Proxy did 
require some changes to specify that it can't be a proxy to a hidden class. 
Given the getProxyClass is deprecated then it may be better to have it work the 
same way as it has always done.

If Proxy::newInstanceClass is changed to return an instance of a hidden class 
then spec changes are needed. Maybe too early to think about that now as there 
is a lot of analysis work required to do before going near code.

-

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

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

[Adam Sotona]

> Hi,
> During performance optimization work on Class-File API as JDK lambda 
> generator we found some static initialization killers.
> One of them is `java.lang.classfile.Attributes` with tens of static fields 
> initialized with individual attribute mappers, and common set of all mappers, 
> and static map from attribute names to the mappers.
> 
> I propose to turn all the static fields into lazy-initialized static methods 
> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` 
> static mapping method from the `Attributes` API class.
>  
> Please let me know your comments or objections and please review the 
> [PR](https://github.com/openjdk/jdk/pull/19006) and 
> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23.
> 
> Thank you,
> Adam

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

 - addressed CSR review comments
 - fixed CompilationIDMapper does not allow multiple instances

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19006/files
  - new: https://git.openjdk.org/jdk/pull/19006/files/b4203cfd..21515ec2

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

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

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

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

[Matthias Baesken]

On Thu, 23 May 2024 07:26:14 GMT, Matthias Baesken  wrote:

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

Hi Magnus, I added handling of the special case that  memcpy src is a null 
pointer and non-0 length was given .

-

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

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

[Matthias Baesken]

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

Matthias Baesken has updated the pull request incrementally with one additional 
commit since the last revision:

  handle special case that memcpy src is NULL but a len larger than 0 was given

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19329/files
  - new: https://git.openjdk.org/jdk/pull/19329/files/b03ff0c5..aad00366

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

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

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

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

[Fei Yang]

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

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

Hi, RISC-V part of change seems fine. "java/math/BigInteger" test result is 
clean on linux-riscv64 platform. Thanks for the ping.

-

Marked as reviewed by fyang (Reviewer).

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

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

[Daniel Jeliński]

On Thu, 23 May 2024 19:26:10 GMT, Scott Gibbons  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 268:
>> 
>>> 266:   __ cmpq(needle_len_p, 0);
>>> 267:   __ jg_b(L_nextCheck);
>>> 268:   __ xorq(rax, rax);
>> 
>> out of curiosity, is there any advantage to using `xorq` instead of `xorl` 
>> here?
>> 
>> https://stackoverflow.com/a/33668295/7707617 suggests that `xorl` might be 
>> better, but it's a bit dated now.
>
> Thanks for finding this.  It was ignorance on my part as I thought the xorq 
> would have logic to not emit the REX prefix if not necessary, but it doesn't. 
>  Fixed.

Right, it seems to surprise people. There's a lot of preexisting uses of xorq / 
xorptr to zero a register. I think it would make sense to implement this logic 
in xorq. I can do this if others agree.

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 449:
>> 
>>> 447:   __ cmpq(r13, NUMBER_OF_CASES - 1);
>>> 448:   __ ja(L_smallCaseDefault);
>>> 449:   __ mov64(r15, (int64_t)small_jump_table);
>> 
>> would it make sense to use `lea` here?
>
> It may, but I believe the movq is shorter (although maybe not to r15).  I'll 
> do some experimentation.

the RIP-relative lea should have a shorter encoding. I think something like 
`lea(r15, ExternalAddress(small_jump_table))` should produce it (untested)

>> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 803:
>> 
>>> 801: __ movq(index, needle_len);
>>> 802: __ andq(index, 0xf);  // nLen % 16
>>> 803: __ movq(offset, 0x10);
>> 
>> `movl` or `movptr` would produce a shorter encoding
>
> I tried to be consistent with the whole {q,l} syntax throughout when 
> referring to each symbolic register.  I feel that changing this would ripple 
> through the code.  @sviswa7 what do you think?

Right, that makes sense. I wonder if there's any reason why the logic to select 
the best mov variant is in movptr, and not in movq. Usually the `ptr` functions 
just select the `l` or `q` overload depending on the target system, `movptr` is 
an exception here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612907959
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612908115
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612908219