Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

On Thu, 10 Dec 2020 10:47:48 GMT, Andrew Haley  wrote:

>> Should we have a separate RegSet type for FloatRegisters to avoid mixing 
>> them up?
>
> Absolutely. I'd make an AbstractRegSet and use it as a base type
> for both RegSet and FloatRegSet, then we can get rid of the casts
> altogether.

OK I've done that in the latest commit.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

On Thu, 10 Dec 2020 13:45:21 GMT, Jorn Vernee  wrote:

>> Nick Gasson has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments
>
> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3104:
> 
>> 3102:const 
>> GrowableArray& output_registers) {
>> 3103:   BufferBlob* _invoke_native_blob =
>> 3104: BufferBlob::create("nep_invoker_blob", 
>> MethodHandles::adapter_code_size);
> 
> That reminds me; this should _not_ use MethodHandles::adapter_code_size, but 
> a separate constant, since the former is tailored specifically to method 
> handle stubs (and is too large for this case). I still need to update the one 
> for x86 as well (looks like I forgot to do that one before when I changed 
> them for invoker/upcall handler). I think 1024 bytes should be more than 
> enough, but would need to test it.

I've changed it to 1024 for AArch64, definitely large enough.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v3]

[Nick Gasson]

> This is more-or-less a straight port of the x86 code to AArch64.
> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
> to generate a blob that jumps to the native function entry point. This
> simply switches the thread state from Java to native and handles the
> safepoint poll on return from native code.
> 
> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
> pointer) may hold a live oop over the MachCallNative node. Normally this
> would be saved by RegisterSaver::save_live_registers() but the native
> invoker blob is not a "proper" stub routine so doesn't have an oop map.
> I copied the x86 solution to this where the frame pointer register is
> saved to the stack and a pointer to that is stored in the frame anchor.
> This is then read back and added to the register map when walking the
> stack. I saw in the PR comments [1] that this might be a temporary fix,
> but I'm not sure if there's any plan to change that now?
> 
> Another potential fix might be to change the C2 register definition of
> R29 and R29_H to be save-on-call as below. This works for the
> TestStackWalk.java test case but I don't know whether it has other
> unintended side-effects.
> 
>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
> 
> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
> working:
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
> ns/op
> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
> ns/op
> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
> ns/op
> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
> ns/op
> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
> ns/op
> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
> ns/op
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
> ns/op
> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
> ns/op
> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
> ns/op
> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
> ns/op
> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
> ns/op
> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
> ns/op
> 
> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326

Nick Gasson 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:

 - Separate RegSet and FloatRegSet
 - Merge branch 'master' into 8257882
 - Review comments
 - 8257882: Implement linkToNative intrinsic on AArch64
   
   This is more-or-less a straight port of the x86 code to AArch64.
   GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
   to generate a blob that jumps to the native function entry point. This
   simply switches the thread state from Java to native and handles the
   safepoint poll on return from native code.
   
   AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
   pointer) may hold a live oop over the MachCallNative node. Normally this
   would be saved by RegisterSaver::save_live_registers() but the native
   invoker blob is not a "proper" stub routine so doesn't have an oop map.
   I copied the x86 solution to this where the frame pointer register is
   saved to the stack and a pointer to that is stored in the frame anchor.
   This is then read back and added to the register map when walking the
   stack. I saw in the PR comments [1] that this might be a temporary fix,
   but I'm not sure if there's any plan to change that now?
 

Withdrawn: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region"

[Yumin Qi]

On Mon, 7 Dec 2020 05:01:27 GMT, Yumin Qi  wrote:

> Hi, Please review
>   Windows mapping for file into memory could not happen to reserved memory. 
> In mapping CDS archive we first reserve enough memory then before mapping, 
> release them. For cds archive and using class space, need split the whole 
> space into two, that is, release the whole reserved space and do reservation 
> to the two split spaces again, which is problematic that there is possibility 
> other thread or system can kick in to take the released space.
>   The fix is the first step of two steps:
>1) Do not split reserved memory;
>2) Remove splitting memory.
>This fix is first step, for Windows and use requested mapping address, 
> reserved for cds archive and ccs on a contiguous space separately, so there 
> is no need to call split. If any reservation failed, release them, go to 
> other way, but do not do the 'real' split either. For Windows (and using 
> class space), the reserved space will be released anyway. 
> 
> Tests:tier1-5,tier7

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/1657

Re: RFR: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region" [v5]

[Yumin Qi]

On Tue, 8 Dec 2020 06:12:36 GMT, Yumin Qi  wrote:

>> Changes requested by iklam (Reviewer).
>
> Please check 03. 02 is generated when merge with most current and remote head 
> not updated correctly. After set remote head correct, 03 is regenerated and 
> is correct one for review. Thanks

This branch has many conflicts, something wrong since push-02, closed this PR 
and will send a single patch in new PR.

-

PR: https://git.openjdk.java.net/jdk/pull/1657

Re: [jdk16] RFR: 8257598: Clarify what component values are used in Record::equals

[Joe Darcy]

On Fri, 11 Dec 2020 05:02:25 GMT, Vicente Romero  wrote:

> Please review this patch which modifies the spec for method 
> java.lang.Record::equals. It states that the implementation of this method 
> should use the record fields for the comparison instead of the accessors.
> 
> TIA,
> Vicente

Looks fine.

-

Marked as reviewed by darcy (Reviewer).

PR: https://git.openjdk.java.net/jdk16/pull/5

Re: RFR: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region" [v5]

[Yumin Qi]

> Hi, Please review
>   Windows mapping for file into memory could not happen to reserved memory. 
> In mapping CDS archive we first reserve enough memory then before mapping, 
> release them. For cds archive and using class space, need split the whole 
> space into two, that is, release the whole reserved space and do reservation 
> to the two split spaces again, which is problematic that there is possibility 
> other thread or system can kick in to take the released space.
>   The fix is the first step of two steps:
>1) Do not split reserved memory;
>2) Remove splitting memory.
>This fix is first step, for Windows and use requested mapping address, 
> reserved for cds archive and ccs on a contiguous space separately, so there 
> is no need to call split. If any reservation failed, release them, go to 
> other way, but do not do the 'real' split either. For Windows (and using 
> class space), the reserved space will be released anyway. 
> 
> Tests:tier1-5,tier7

Yumin Qi has updated the pull request with a new target base due to a merge or 
a rebase. The pull request now contains 35 commits:

 - Added test case; Fixed an nmt issue caused by bitmap region not released 
after archive loading failed; Unmap bitmap after archive failure. Fixed 
reserved region name for adding reserved region.
 - Add total_space_rs, total reserved space to release_reserved_spaces and 
reserve_address_space_for_archives, made changes to check failed output on test.
 - 8253762: JFR: getField(String) should be able to access subfields
   
   Reviewed-by: mgronlun
 - 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
   
   Reviewed-by: jnimeh
 - 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on 
x86_32
   
   Reviewed-by: kvn
 - 8257211: C2: Enable call devirtualization during post-parse phase
   
   Reviewed-by: kvn, neliasso, thartmann
 - 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
   
   Reviewed-by: ihse, alanb, dcubed, erikj
 - 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
   
   Reviewed-by: redestad, kvn
 - 8257799: Update JLS cross-references in java.compiler
   
   Reviewed-by: jjg
 - 8254939: macOS: unused function 'replicate4_imm'
   
   Reviewed-by: redestad, thartmann
 - ... and 25 more: https://git.openjdk.java.net/jdk/compare/29a09c89...0421943d

-

Changes: https://git.openjdk.java.net/jdk/pull/1657/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1657=04
  Stats: 8183 lines in 159 files changed: 4666 ins; 2763 del; 754 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1657.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1657/head:pull/1657

PR: https://git.openjdk.java.net/jdk/pull/1657

Re: RFR: 8257598: Clarify what component values are used in Record::equals

[Vicente Romero]

On Fri, 11 Dec 2020 02:07:50 GMT, Vicente Romero  wrote:

> Please review this patch which modifies the spec for method 
> java.lang.Record::equals. It states that the implementation of this method 
> should use the record fields for the comparison instead of the accessors.
> 
> TIA,
> Vicente

> _Mailing list message from [Joe Darcy](mailto:joe.da...@oracle.com) on 
> [compiler-dev](mailto:compiler-...@openjdk.java.net):_
> 
> If this is meant for JDK 16, it is easier overall if PR is done against
> the JDK 16 repo.
> 
> -Joe
> 
> On 12/10/2020 6:13 PM, Vicente Romero wrote:

true, thanks, I have created another PR based on jdk16: 
https://github.com/openjdk/jdk16/pull/5

-

PR: https://git.openjdk.java.net/jdk/pull/1742

[jdk16] RFR: 8257598: Clarify what component values are used in Record::equals

[Vicente Romero]

Please review this patch which modifies the spec for method 
java.lang.Record::equals. It states that the implementation of this method 
should use the record fields for the comparison instead of the accessors.

TIA,
Vicente

-

Commit messages:
 - 8257598: Clarify what component values are used in Record::equals

Changes: https://git.openjdk.java.net/jdk16/pull/5/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk16=5=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257598
  Stats: 135 lines in 2 files changed: 132 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk16/pull/5.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/5/head:pull/5

PR: https://git.openjdk.java.net/jdk16/pull/5

Withdrawn: 8257598: Clarify what component values are used in Record::equals

[Vicente Romero]

On Fri, 11 Dec 2020 02:07:50 GMT, Vicente Romero  wrote:

> Please review this patch which modifies the spec for method 
> java.lang.Record::equals. It states that the implementation of this method 
> should use the record fields for the comparison instead of the accessors.
> 
> TIA,
> Vicente

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/1742

Re: RFR: 8257598: Clarify what component values are used in Record::equals

[Joe Darcy]

If this is meant for JDK 16, it is easier overall if PR is done against 
the JDK 16 repo.


-Joe

On 12/10/2020 6:13 PM, Vicente Romero wrote:

Please review this patch which modifies the spec for method 
java.lang.Record::equals. It states that the implementation of this method 
should use the record fields for the comparison instead of the accessors.

TIA,
Vicente

-

Commit messages:
  - Merge branch 'master' into JDK-8257598
  - Merge branch 'master' into JDK-8257598
  - 8257598: Clarify what component values are used in Record::equals

Changes: https://git.openjdk.java.net/jdk/pull/1742/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1742=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8257598
   Stats: 135 lines in 2 files changed: 132 ins; 1 del; 2 mod
   Patch: https://git.openjdk.java.net/jdk/pull/1742.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/1742/head:pull/1742

PR: https://git.openjdk.java.net/jdk/pull/1742

RFR: 8257598: Clarify what component values are used in Record::equals

[Vicente Romero]

Please review this patch which modifies the spec for method 
java.lang.Record::equals. It states that the implementation of this method 
should use the record fields for the comparison instead of the accessors.

TIA,
Vicente

-

Commit messages:
 - Merge branch 'master' into JDK-8257598
 - Merge branch 'master' into JDK-8257598
 - 8257598: Clarify what component values are used in Record::equals

Changes: https://git.openjdk.java.net/jdk/pull/1742/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1742=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257598
  Stats: 135 lines in 2 files changed: 132 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1742.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1742/head:pull/1742

PR: https://git.openjdk.java.net/jdk/pull/1742

Re: RFR: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST [v2]

[Joe Wang]

On Fri, 11 Dec 2020 01:25:48 GMT, Naoto Sato  wrote:

>> src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
>>  line 78:
>> 
>>> 76: // CalendarData value types
>>> 77: private static final int CD_FIRSTDAYOFWEEK = 0;
>>> 78: private static final int CD_MINIMALDAYSINFIRSTWEEK = 1;
>> 
>> Do we want to keep the naming consistent, doing the same change to, e.g. the 
>> corresponding macosx impl?
>
> The constants are for native methods, which differ between macOS and Windows. 
> Thus I thought it would be clearer to align the name with Windows' constants.

Make sense.

-

PR: https://git.openjdk.java.net/jdk/pull/1741

Re: RFR: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST [v2]

[Naoto Sato]

On Fri, 11 Dec 2020 00:15:49 GMT, Joe Wang  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added comment for LOCALE_IFIRSTWEEKOFYEAR Windows return values
>
> src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
>  line 78:
> 
>> 76: // CalendarData value types
>> 77: private static final int CD_FIRSTDAYOFWEEK = 0;
>> 78: private static final int CD_MINIMALDAYSINFIRSTWEEK = 1;
> 
> Do we want to keep the naming consistent, doing the same change to, e.g. the 
> corresponding macosx impl?

The constants are for native methods, which differ between macOS and Windows. 
Thus I thought it would be clearer to align the name with Windows' constants.

> src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
>  line 373:
> 
>> 371: CD_FIRSTWEEKOFYEAR);
>> 372: return switch (firstWeek) {
>> 373: case 1 -> 7;
> 
> Would it be good to use constants or enum instead of literal, or maybe at 
> least a note for the case numbers.

Could be better by making them as enums, but there are other locations 
similarly using ints. I added extra comments to explain those ints for better 
readability.

> test/jdk/java/util/Locale/LocaleProvidersRun.java line 177:
> 
>> 175: 
>> 176: //testing 8257964 fix. (macOS/Windows only)
>> 177: testRun("HOST", "bug8257964Test", "", "", "");
> 
> This test runs only if the platform locale is en-GB. Do we know if the test 
> system run tests on multiple locales? From the console output unfortunately, 
> it's impossible to tell which specific tests were run

Could be, but it is enough to test one locale to demonstrate platform settings 
are used for minimal days in first week. Each test case name can be found in 
the command line to the java launcher which is logged in the .jtr file.

-

PR: https://git.openjdk.java.net/jdk/pull/1741

Re: RFR: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST [v2]

[Naoto Sato]

> Hello,
> 
> Please review the changes to the subject issue. getMinimalDaysInFirstWeek() 
> for Windows has been implemented to suffice the bug claim.

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

  Added comment for LOCALE_IFIRSTWEEKOFYEAR Windows return values

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1741/files
  - new: https://git.openjdk.java.net/jdk/pull/1741/files/a9ea9f16..3f40c3db

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1741=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1741=00-01

  Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1741.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1741/head:pull/1741

PR: https://git.openjdk.java.net/jdk/pull/1741

Integrated: 8247402: Documentation for Map::compute contains confusing implementation requirements

[John Lin]

On Sat, 17 Oct 2020 02:50:28 GMT, John Lin 
 wrote:

> This is from the mailing list: 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html
> 
> -
> ### Progress
> - [x] Change must not contain extraneous whitespace
> - [x] Commit message must refer to an issue
> - [ ] Change must be properly reviewed
> 
> ### Testing
> 
> | | Linux x64 | Windows x64 | macOS x64 |
> | --- | - | - | - |
> | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) |
> | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) |
> 
> 
> 
> ### Download
> `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714`
> `$ git checkout pull/714`

This pull request has now been integrated.

Changeset: 37dc675c
Author:John Lin 
Committer: Pavel Rappo 
URL:   https://git.openjdk.java.net/jdk/commit/37dc675c
Stats: 13 lines in 1 file changed: 1 ins; 7 del; 5 mod

8247402: Documentation for Map::compute contains confusing implementation 
requirements

Reviewed-by: prappo, martin

-

PR: https://git.openjdk.java.net/jdk/pull/714

Re: RFR: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST

[Joe Wang]

On Thu, 10 Dec 2020 21:12:29 GMT, Naoto Sato  wrote:

> Hello,
> 
> Please review the changes to the subject issue. getMinimalDaysInFirstWeek() 
> for Windows has been implemented to suffice the bug claim.

Looks good to me. Some minor comments below.

src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
 line 78:

> 76: // CalendarData value types
> 77: private static final int CD_FIRSTDAYOFWEEK = 0;
> 78: private static final int CD_MINIMALDAYSINFIRSTWEEK = 1;

Do we want to keep the naming consistent, doing the same change to, e.g. the 
corresponding macosx impl?

src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
 line 373:

> 371: CD_FIRSTWEEKOFYEAR);
> 372: return switch (firstWeek) {
> 373: case 1 -> 7;

Would it be good to use constants or enum instead of literal, or maybe at least 
a note for the case numbers.

test/jdk/java/util/Locale/LocaleProvidersRun.java line 177:

> 175: 
> 176: //testing 8257964 fix. (macOS/Windows only)
> 177: testRun("HOST", "bug8257964Test", "", "", "");

This test runs only if the platform locale is en-GB. Do we know if the test 
system run tests on multiple locales? From the console output unfortunately, 
it's impossible to tell which specific tests were run

-

Marked as reviewed by joehw (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1741

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements [v2]

[John Lin]

On Thu, 10 Dec 2020 17:59:03 GMT, Martin Buchholz  wrote:

>> John Lin has updated the pull request with a new target base due to a merge 
>> or a rebase. The pull request now contains one commit:
>> 
>>   8247402: Documentation for Map::compute contains confusing implementation 
>> requirements
>
> Marked as reviewed by martin (Reviewer).

Thank you all for the review and discussion. This is my first time contributing 
code in jdk, and it's a really nice experience.

-

PR: https://git.openjdk.java.net/jdk/pull/714

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array [v3]

[Brian Burkhalter]

> Please review this small verbiage change to specify clearly the behavior of 
> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8248383: Re-indent subclasses in @see tages

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1737/files
  - new: https://git.openjdk.java.net/jdk/pull/1737/files/d8497f0f..7fbc77f7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1737=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1737=01-02

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

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array [v2]

[Brian Burkhalter]

On Thu, 10 Dec 2020 23:12:57 GMT, Naoto Sato  wrote:

>> Brian Burkhalter 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 five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'JDK-8248383-Reader-read' of https://github.com/bplb/jdk 
>> into JDK-8248383-Reader-read
>>  - 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array
>>  - 8248383: Add test ReadIntoZeroLengthArray
>>  - 8248383: Fix alignment of @see tags
>>  - 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array
>
> Looks good to me, Brian.

For the sake of sanity I added a minimal test to verify that the behavior is in 
fact in accord with the added verbiage.

-

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array [v2]

[Brian Burkhalter]

> Please review this small verbiage change to specify clearly the behavior of 
> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.

Brian Burkhalter 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 five additional 
commits since the last revision:

 - Merge branch 'JDK-8248383-Reader-read' of https://github.com/bplb/jdk into 
JDK-8248383-Reader-read
 - 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array
 - 8248383: Add test ReadIntoZeroLengthArray
 - 8248383: Fix alignment of @see tags
 - 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1737/files
  - new: https://git.openjdk.java.net/jdk/pull/1737/files/0dc2e243..d8497f0f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1737=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1737=00-01

  Stats: 35454 lines in 901 files changed: 26596 ins; 5818 del; 3040 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1737/head:pull/1737

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

[Naoto Sato]

On Thu, 10 Dec 2020 17:22:06 GMT, Brian Burkhalter  wrote:

> Please review this small verbiage change to specify clearly the behavior of 
> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.

Looks good to me, Brian.

-

Marked as reviewed by naoto (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Naoto Sato]

On Mon, 7 Dec 2020 14:27:45 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [ ] java.base
>> - [ ] java.desktop
>> - [x] jdk.compiler
>> - [ ] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Move to share/data, and move jdwp.spec to java.se

Reviewed changes to `characterdata`, `charsetmapping`, `cldr`, `currency`, 
`lsrdata`, `tzdata`, and `unicodedata` with minor comment. Looks good overall.

test/jdk/java/util/Locale/LSRDataTest.java line 57:

> 55: // path to the lsr file from the make folder, this test relies on the
> 56: // relative path to the file in the make folder, considering
> 57: // test and make will always exist in the same jdk layout

The comment refers to the "make" folder. (line 55 as well).

-

Marked as reviewed by naoto (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Mandy Chung]

On Thu, 10 Dec 2020 22:56:34 GMT, Mandy Chung  wrote:

>> Marked as reviewed by chegar (Reviewer).
>
> Hi Remi,
> 
>> For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i 
>> prefer VM to be oblivious about java.lang.Record.
>> And in the future, the real fix is to change the spec of Field.set() to say 
>> that it's only allowed for plain java classes and have the implementation to 
>> directly asks the VM is a non static field is trusted or not.
> 
> This reply was before you realized that records are are permanent Java SE 16 
> feature :-)  If the future ended up requiring/desiring to allow final fields 
> of a record class be modifiable reflectively (I double that!), `Field::set` 
> spec can be updated to remove that restriction with no compatibility risk
> 
>> And there are also a related issue with the validation of the 
>> InnerClass/Enclosing attribute were the VM does a handshake between the 
>> inner class attribute and the enclosing class attribute when calling 
>> Class.getSimpleName(), again using the JLS definition of what an inner class 
>> is instead of using the VM definition, the content of the InnerClass 
>> attribute with no validation.
> 
> It's the implementation details of the core reflection how to determine if a 
> class is a member of another class.   The `getSimpleName` spec and other 
> related APIs (see JDK-8250226) need to define the behavior when there is an 
> issue in determining the declaring class.

I have created a new branch off jdk16: 
https://github.com/mlchung/jdk16/tree/8257596.  It fixed the attribute name as 
Harold pointed out.   

@hseigel tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed.

-

PR: https://git.openjdk.java.net/jdk/pull/1706

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Mandy Chung]

On Thu, 10 Dec 2020 14:13:27 GMT, Chris Hegarty  wrote:

>> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on 
>> classes with `RecordComponents` attributes.  That introduces a regression in 
>> `InstanceKlass::is_record` that returns true on a non-record class which has 
>> `RecordComponents` attribute present.   This causes unexpected semantics in 
>> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust 
>> final fields for non-record classes.
>> 
>> I propose to change `InstanceKlass::is_record` to match the JLS semantic of 
>> a record class, i.e. final direct subclass of `java.lang.Record` with the 
>> presence of `RecordComponents` attribute.  There is no change to JVM class 
>> file validation.   Also I propose clearly define:
>> - `JVM_IsRecord` returns true if the given class is a record i.e. final 
>> and direct subclass of `java.lang.Record` with `RecordComponents` attribute
>> - `JVM_GetRecordComponents` returns an `RecordComponents` array  if 
>> `RecordComponents` attribute is present; otherwise, returns NULL.  This does 
>> not check if it's a record class or not.  So it may return non-null on a 
>> non-record class if it has `RecordComponents` attribute.  So 
>> `JVM_GetRecordComponents` returns the content of the classfile.
>
> Marked as reviewed by chegar (Reviewer).

Hi Remi,

> For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i 
> prefer VM to be oblivious about java.lang.Record.
> And in the future, the real fix is to change the spec of Field.set() to say 
> that it's only allowed for plain java classes and have the implementation to 
> directly asks the VM is a non static field is trusted or not.

This reply was before you realized that records are are permanent Java SE 16 
feature :-)  If the future ended up requiring/desiring to allow final fields of 
a record class be modifiable reflectively (I double that!), `Field::set` spec 
can be updated to remove that restriction with no compatibility risk

> And there are also a related issue with the validation of the 
> InnerClass/Enclosing attribute were the VM does a handshake between the inner 
> class attribute and the enclosing class attribute when calling 
> Class.getSimpleName(), again using the JLS definition of what an inner class 
> is instead of using the VM definition, the content of the InnerClass 
> attribute with no validation.

It's the implementation details of the core reflection how to determine if a 
class is a member of another class.   The `getSimpleName` spec and other 
related APIs (see JDK-8250226) need to define the behavior when there is an 
issue in determining the declaring class.

-

PR: https://git.openjdk.java.net/jdk/pull/1706

RFR: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST

[Naoto Sato]

Hello,

Please review the changes to the subject issue. getMinimalDaysInFirstWeek() for 
Windows has been implemented to suffice the bug claim.

-

Commit messages:
 - JDK-8257964

Changes: https://git.openjdk.java.net/jdk/pull/1741/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1741=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257964
  Stats: 53 lines in 4 files changed: 50 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1741.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1741/head:pull/1741

PR: https://git.openjdk.java.net/jdk/pull/1741

Re: Optimization potential in Reader#read(CharBuffer)

[Brian Burkhalter]

Awesome, Pavel, thanks!

Brian

> On Dec 10, 2020, at 1:02 PM, Pavel Rappo  wrote:
> 
> I found this relevant issue created 17 years ago: 
> https://bugs.openjdk.java.net/browse/JDK-4926314
> 
>> […]
>> 
>> Do you have the ability to file an issue? If not, I can do so.

Re: Optimization potential in Reader#read(CharBuffer)

[Pavel Rappo]

I found this relevant issue created 17 years ago: 
https://bugs.openjdk.java.net/browse/JDK-4926314

> On 10 Dec 2020, at 20:23, Brian Burkhalter  
> wrote:
> 
> Hi Philippe,
> 
>> On Dec 10, 2020, at 12:03 PM, Philippe Marschall 
>>  wrote:
>> 
>> I recently came across Reader#read(CharBuffer) and noticed it was
>> missing an optimization for heap buffers. […]
> 
> These seem like good suggestions.
> 
>> Sorry if this is the wrong mailing list and should go to core-libs-dev
>> or a different list instead.
> 
> I think that core-libs-dev (CCed) would be more appropriate as java.io 
> package changes are usually discussed there.
> 
> Do you have the ability to file an issue? If not, I can do so.
> 
> Thanks,
> 
> Brian

Re: Optimization potential in Reader#read(CharBuffer)

[Brian Burkhalter]

Hi Philippe,

> On Dec 10, 2020, at 12:03 PM, Philippe Marschall 
>  wrote:
> 
> I recently came across Reader#read(CharBuffer) and noticed it was
> missing an optimization for heap buffers. […]

These seem like good suggestions.

> Sorry if this is the wrong mailing list and should go to core-libs-dev
> or a different list instead.

I think that core-libs-dev (CCed) would be more appropriate as java.io package 
changes are usually discussed there.

Do you have the ability to file an issue? If not, I can do so.

Thanks,

Brian

Re: RFR: 8257620: Do not use objc_msgSend_stret to get macOS version

[Anton Kozlov]

On Wed, 2 Dec 2020 19:27:25 GMT, Phil Race  wrote:

>> Please review a small change that replaces use of objc_msgSend_stret in 
>> macOS platform code with pure ObjC code. It's also a prerequisite for 
>> macOS/AArch64 support, where objc_msgSend_stret is not available.
>
> Surely these days you can just call [NSProcessInfo operatingSystemVersion] 
> directly ?
> If I read the doc below it is in the 10.10 SDK and later.
> https://developer.apple.com/documentation/foundation/nsprocessinfo/1410906-operatingsystemversion?language=occ

@prrace could you look at this?

-

PR: https://git.openjdk.java.net/jdk/pull/1569

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

[Brian Burkhalter]

On Thu, 10 Dec 2020 18:28:54 GMT, Lance Andersen  wrote:

>> Please review this small verbiage change to specify clearly the behavior of 
>> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
>> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.
>
> Hi Brian,
> 
> Are there existing tests that cover the behavior described in the proposed 
> change?

Hi @LanceAndersen. I don't see any tests specific to this issue, but then there 
are ten Reader classes in `java.io`, seven of which are not abstract.

-

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

[Lance Andersen]

On Thu, 10 Dec 2020 17:22:06 GMT, Brian Burkhalter  wrote:

> Please review this small verbiage change to specify clearly the behavior of 
> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.

Hi Brian,

Are there existing tests that cover the behavior described in the proposed 
change?

-

PR: https://git.openjdk.java.net/jdk/pull/1737

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Maurizio Cimadamore]

On Thu, 10 Dec 2020 09:15:47 GMT, Nick Gasson  wrote:

>> This is more-or-less a straight port of the x86 code to AArch64.
>> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
>> to generate a blob that jumps to the native function entry point. This
>> simply switches the thread state from Java to native and handles the
>> safepoint poll on return from native code.
>> 
>> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
>> pointer) may hold a live oop over the MachCallNative node. Normally this
>> would be saved by RegisterSaver::save_live_registers() but the native
>> invoker blob is not a "proper" stub routine so doesn't have an oop map.
>> I copied the x86 solution to this where the frame pointer register is
>> saved to the stack and a pointer to that is stored in the frame anchor.
>> This is then read back and added to the register map when walking the
>> stack. I saw in the PR comments [1] that this might be a temporary fix,
>> but I'm not sure if there's any plan to change that now?
>> 
>> Another potential fix might be to change the C2 register definition of
>> R29 and R29_H to be save-on-call as below. This works for the
>> TestStackWalk.java test case but I don't know whether it has other
>> unintended side-effects.
>> 
>>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
>> 
>> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
>> working:
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
>> ns/op
>> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
>> ns/op
>> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
>> ns/op
>> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
>> ns/op
>> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
>> ns/op
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
>> ns/op
>> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
>> ns/op
>> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
>> ns/op
>> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
>> ns/op
>> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
>> ns/op
>> 
>> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326
>
> Nick Gasson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comments

Looks great - thanks for putting in the effort to do this!

-

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements [v2]

[Martin Buchholz]

On Sat, 28 Nov 2020 08:35:20 GMT, John Lin 
 wrote:

>> This is from the mailing list: 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html
>> 
>> -
>> ### Progress
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> - [ ] Change must be properly reviewed
>> 
>> ### Testing
>> 
>> | | Linux x64 | Windows x64 | macOS x64 |
>> | --- | - | - | - |
>> | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) |
>> | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) |
>> 
>> 
>> 
>> ### Download
>> `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714`
>> `$ git checkout pull/714`
>
> John Lin has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains one commit:
> 
>   8247402: Documentation for Map::compute contains confusing implementation 
> requirements

Marked as reviewed by martin (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/714

RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

[Brian Burkhalter]

Please review this small verbiage change to specify clearly the behavior of 
`Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
`Reader::read(char[] cbuf, int off, int len)` when `len` is zero.

-

Commit messages:
 - 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

Changes: https://git.openjdk.java.net/jdk/pull/1737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1737=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8248383
  Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1737/head:pull/1737

PR: https://git.openjdk.java.net/jdk/pull/1737

Integrated: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems

[Severin Gehwolf]

On Mon, 7 Dec 2020 17:48:01 GMT, Severin Gehwolf  wrote:

> This has been implemented for cgroups v1 with 
> [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was 
> lacking some tooling support for cgroups v2. With podman 2.2.0 release this 
> could now be implemented (and tested). The idea is the same as for the 
> cgroups v1 fix. If we've got no swap limit capabilities, return the memory 
> limit only.
> 
> Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, 
> thus, doesn't have `getMemoryAndSwapFailCount()` and 
> `getMemoryAndSwapMaxUsage()`.
> 
> Testing:
> - [x] submit testing
> - [x] container tests on cgroups v2 with swapaccount=0.
> - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2.
> 
> Thoughts?

This pull request has now been integrated.

Changeset: 66936111
Author:Severin Gehwolf 
URL:   https://git.openjdk.java.net/jdk/commit/66936111
Stats: 45 lines in 2 files changed: 24 ins; 1 del; 20 mod

8253797: [cgroups v2] Account for the fact that swap accounting is disabled on 
some systems

Reviewed-by: hseigel

-

PR: https://git.openjdk.java.net/jdk/pull/1672

Integrated: 8257450: Start of release updates for JDK 17

[Joe Darcy]

On Tue, 1 Dec 2020 06:22:51 GMT, Joe Darcy  wrote:

> Start of JDK 17 updates.

This pull request has now been integrated.

Changeset: 6be1f567
Author:Joe Darcy 
URL:   https://git.openjdk.java.net/jdk/commit/6be1f567
Stats: 7607 lines in 77 files changed: 7548 ins; 0 del; 59 mod

8257450: Start of release updates for JDK 17
8257451: Add SourceVersion.RELEASE_17
8257453: Add source 17 and target 17 to javac

Reviewed-by: dholmes, erikj, iris, mikael, jjg, jlahoda, jwilhelm, mchung, ihse

-

PR: https://git.openjdk.java.net/jdk/pull/1531

Re: RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems

[Severin Gehwolf]

On Thu, 10 Dec 2020 16:28:59 GMT, Harold Seigel  wrote:

> The changes look good. Thanks for doing this.

Thanks for the review!

-

PR: https://git.openjdk.java.net/jdk/pull/1672

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements [v2]

[Pavel Rappo]

On Sat, 28 Nov 2020 08:35:20 GMT, John Lin 
 wrote:

>> This is from the mailing list: 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html
>> 
>> -
>> ### Progress
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> - [ ] Change must be properly reviewed
>> 
>> ### Testing
>> 
>> | | Linux x64 | Windows x64 | macOS x64 |
>> | --- | - | - | - |
>> | Build | ✔️ (5/5 passed) | ✔️ (2/2 passed) | ✔️ (2/2 passed) |
>> | Test (tier1) | ✔️ (9/9 passed) | ✔️ (9/9 passed) | ✔️ (9/9 passed) |
>> 
>> 
>> 
>> ### Download
>> `$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714`
>> `$ git checkout pull/714`
>
> John Lin has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains one commit:
> 
>   8247402: Documentation for Map::compute contains confusing implementation 
> requirements

This looks good to me.

-

Marked as reviewed by prappo (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/714

Re: RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems

[Harold Seigel]

On Mon, 7 Dec 2020 17:48:01 GMT, Severin Gehwolf  wrote:

> This has been implemented for cgroups v1 with 
> [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was 
> lacking some tooling support for cgroups v2. With podman 2.2.0 release this 
> could now be implemented (and tested). The idea is the same as for the 
> cgroups v1 fix. If we've got no swap limit capabilities, return the memory 
> limit only.
> 
> Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, 
> thus, doesn't have `getMemoryAndSwapFailCount()` and 
> `getMemoryAndSwapMaxUsage()`.
> 
> Testing:
> - [x] submit testing
> - [x] container tests on cgroups v2 with swapaccount=0.
> - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2.
> 
> Thoughts?

The changes look good.  Thanks for doing this.
Harold

-

Marked as reviewed by hseigel (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1672

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Andrew Haley]

On Thu, 10 Dec 2020 13:55:25 GMT, Jorn Vernee  wrote:

> Thanks for the amazing work!
> 
> FWIW, on x86 RBP was being passed as debug info (see last line in 
> MachCallNode::in_RegMask), so the solution Vlad I proposed would be to 
> override MachCallNativeNode::in_RegMask to not include it IIRC. I haven't had 
> time to look into it yet though.
> 
> The situation on AArch64 seems to be different though? The RFP is not passed 
> as debug info but as part of the normal calling convention maybe?

On AArch64 JIT-compiled code, RFP is a callee-saved register that's free for 
any use. It's very useful for a spilled value across calls.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257837: Performance regression in heap byte buffer views

[Claes Redestad]

On Thu, 10 Dec 2020 13:15:41 GMT, Maurizio Cimadamore  
wrote:

> As a result of the recent integration of the foreign memory access API, some 
> of the buffer implementations now use `ScopedMemoryAccess` instead of 
> `Unsafe`. While this works generally well, there are situations where profile 
> pollution arises, which result in a considerable slowdown. The profile 
> pollution occurs because the same ScopedMemoryAccess method (e.g. 
> `getIntUnaligned`) is called with two different buffer kinds (e.g. an off 
> heap buffer where base == null, and an on-heap buffer where base == byte[]). 
> Because of that, unsafe access cannot be optimized, since C2 can't guess what 
> the unsafe base access is.
> 
> In reality, this problem was already known (and solved) elsewhere: the 
> sun.misc.Unsafe wrapper does basically the same thing that ScopedMemoryAccess 
> does. To make sure that profile pollution does not occur in those cases, 
> argument profiling is enabled for sun.misc.Unsafe as well. This patch adds 
> yet another case for ScopedMemoryAccess.
> 
> Here are the benchmark results:
> 
> Before:
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.612 ? 0.005 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  2.740 ? 0.039 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.504 ? 0.020 
>  ms/op
> 
> After
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.613 ? 0.007 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  0.304 ? 0.002 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.491 ? 0.004 
>  ms/op

src/hotspot/share/oops/methodData.cpp line 1593:

> 1591:   ResourceMark rm;
> 1592:   char* name = inv.name()->as_C_string();
> 1593:   if (!strncmp(name, "get", 3) || !strncmp(name, "put", 3)) {

Pre-existing, but `!strncmp(name, "get", 3)` seem a very circumspect way of 
writing `inv->name()->starts_with("get")` - which shouldn't need a 
`ResourceMark` either. Another observation is that `inv.klass()` isn't inlined 
(defined in bytecode.cpp), so introducing a local for `inv.klass()` avoids 
multiple calls. How about this:

  if (inv.is_invokevirtual()) {
Symbol* klass = inv.klass();
if (klass == vmSymbols::jdk_internal_misc_Unsafe() ||
klass == vmSymbols::sun_misc_Unsafe() ||
klass == vmSymbols::jdk_internal_misc_ScopedMemoryAccess()) {
  Symbol* name = inv.name();
  if (name->starts_with("get") || name->starts_with("put")) {
return true;
  }
}
  }

-

PR: https://git.openjdk.java.net/jdk/pull/1733

Integrated: 8257837: Performance regression in heap byte buffer views

[Maurizio Cimadamore]

On Thu, 10 Dec 2020 13:15:41 GMT, Maurizio Cimadamore  
wrote:

> As a result of the recent integration of the foreign memory access API, some 
> of the buffer implementations now use `ScopedMemoryAccess` instead of 
> `Unsafe`. While this works generally well, there are situations where profile 
> pollution arises, which result in a considerable slowdown. The profile 
> pollution occurs because the same ScopedMemoryAccess method (e.g. 
> `getIntUnaligned`) is called with two different buffer kinds (e.g. an off 
> heap buffer where base == null, and an on-heap buffer where base == byte[]). 
> Because of that, unsafe access cannot be optimized, since C2 can't guess what 
> the unsafe base access is.
> 
> In reality, this problem was already known (and solved) elsewhere: the 
> sun.misc.Unsafe wrapper does basically the same thing that ScopedMemoryAccess 
> does. To make sure that profile pollution does not occur in those cases, 
> argument profiling is enabled for sun.misc.Unsafe as well. This patch adds 
> yet another case for ScopedMemoryAccess.
> 
> Here are the benchmark results:
> 
> Before:
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.612 ? 0.005 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  2.740 ? 0.039 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.504 ? 0.020 
>  ms/op
> 
> After
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.613 ? 0.007 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  0.304 ? 0.002 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.491 ? 0.004 
>  ms/op

This pull request has now been integrated.

Changeset: 37043b05
Author:Maurizio Cimadamore 
URL:   https://git.openjdk.java.net/jdk/commit/37043b05
Stats: 121 lines in 3 files changed: 120 ins; 0 del; 1 mod

8257837: Performance regression in heap byte buffer views

Reviewed-by: chegar, roland

-

PR: https://git.openjdk.java.net/jdk/pull/1733

Re: RFR: 8257837: Performance regression in heap byte buffer views

[Roland Westrelin]

On Thu, 10 Dec 2020 13:15:41 GMT, Maurizio Cimadamore  
wrote:

> As a result of the recent integration of the foreign memory access API, some 
> of the buffer implementations now use `ScopedMemoryAccess` instead of 
> `Unsafe`. While this works generally well, there are situations where profile 
> pollution arises, which result in a considerable slowdown. The profile 
> pollution occurs because the same ScopedMemoryAccess method (e.g. 
> `getIntUnaligned`) is called with two different buffer kinds (e.g. an off 
> heap buffer where base == null, and an on-heap buffer where base == byte[]). 
> Because of that, unsafe access cannot be optimized, since C2 can't guess what 
> the unsafe base access is.
> 
> In reality, this problem was already known (and solved) elsewhere: the 
> sun.misc.Unsafe wrapper does basically the same thing that ScopedMemoryAccess 
> does. To make sure that profile pollution does not occur in those cases, 
> argument profiling is enabled for sun.misc.Unsafe as well. This patch adds 
> yet another case for ScopedMemoryAccess.
> 
> Here are the benchmark results:
> 
> Before:
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.612 ? 0.005 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  2.740 ? 0.039 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.504 ? 0.020 
>  ms/op
> 
> After
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.613 ? 0.007 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  0.304 ? 0.002 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.491 ? 0.004 
>  ms/op

Marked as reviewed by roland (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1733

Re: RFR: 8257837: Performance regression in heap byte buffer views

[Chris Hegarty]

On Thu, 10 Dec 2020 13:15:41 GMT, Maurizio Cimadamore  
wrote:

> As a result of the recent integration of the foreign memory access API, some 
> of the buffer implementations now use `ScopedMemoryAccess` instead of 
> `Unsafe`. While this works generally well, there are situations where profile 
> pollution arises, which result in a considerable slowdown. The profile 
> pollution occurs because the same ScopedMemoryAccess method (e.g. 
> `getIntUnaligned`) is called with two different buffer kinds (e.g. an off 
> heap buffer where base == null, and an on-heap buffer where base == byte[]). 
> Because of that, unsafe access cannot be optimized, since C2 can't guess what 
> the unsafe base access is.
> 
> In reality, this problem was already known (and solved) elsewhere: the 
> sun.misc.Unsafe wrapper does basically the same thing that ScopedMemoryAccess 
> does. To make sure that profile pollution does not occur in those cases, 
> argument profiling is enabled for sun.misc.Unsafe as well. This patch adds 
> yet another case for ScopedMemoryAccess.
> 
> Here are the benchmark results:
> 
> Before:
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.612 ? 0.005 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  2.740 ? 0.039 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.504 ? 0.020 
>  ms/op
> 
> After
> 
> BenchmarkMode  Cnt  Score   Error 
>  Units
> LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.613 ? 0.007 
>  ms/op
> LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  0.304 ? 0.002 
>  ms/op
> LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.491 ? 0.004 
>  ms/op

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1733

Re: JNLP

[Daniel Peintner]

Hello,

I am not sure if this is the right list for such kind of discussions but
there is also
https://openwebstart.com/

Hope this helps,

-- Daniel


On Thu, Dec 10, 2020 at 3:24 PM Bernd Eckenfels 
wrote:

> Hello,
>
> We ported a big JWS gui app to stand-alone swing with a home made
> installer/update mechanism. This was very easy to do (it had a main method
> for debugging anyway). The installer is not the most comfortable, but we
> can live with it since the whole application will be replaced by an web app
> anyway.
>
> Gruss
> Bernd
>
>
> --
> http://bernd.eckenfels.net
> 
> Von: core-libs-dev  im Auftrag von
> Thomas Vatter 
> Gesendet: Thursday, December 10, 2020 1:12:45 PM
> An: core-libs-dev@openjdk.java.net 
> Betreff: JNLP
>
> I'm using JNLP, how should I go on?
>
> --
> Network Inventory Software
> IBM-Partner, RedHat- und SUSE-Partner, Oracle Technet Member
>
> www.network-inventory.de
> Tel. 030-79782510
> Fax 030-79782512
> E-Mail: thomas.vat...@network-inventory.de
>
>

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Remi Forax]

- Mail original -
> De: "Mandy Chung" 
> À: "core-libs-dev" , "hotspot-runtime-dev" 
> 
> Envoyé: Mercredi 9 Décembre 2020 01:43:34
> Objet: Re: RFR: 8257596: Clarify trusted final fields for record classes

> On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung  wrote:
> 
>> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on
>> classes with Record attributes.  That introduces a regression in
>> `InstanceKlass::is_record` that returns true on a non-record class which has
>> `RecordComponents` attribute present.   This causes unexpected semantics in
>> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust
>> final fields for non-record classes.
>> 
>> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a
>> record class, i.e. final direct subclass of `java.lang.Record` with the
>> presence of `RecordComponents` attribute.  There is no change to JVM class 
>> file
>> validation.   Also I propose clearly define:
>> - `JVM_IsRecord` returns true if the given class is a record i.e. final 
>> and
>> direct subclass of `java.lang.Record` with `RecordComponents` attribute
>> - `JVM_GetRecordComponents` returns an `RecordComponents` array  if
>> `RecordComponents` attribute is present; otherwise, returns NULL.  This 
>> does
>> not check if it's a record class or not.  So it may return non-null on a
>> non-record class if it has `RecordComponents` attribute.  So
>> `JVM_GetRecordComponents` returns the content of the classfile.
> 
> Hi Remi,
> 
>> It's not an issue, the JVM view of what a record is and the JLS view of what 
>> a
>> record is doesn't have to be identical,
>> only aligned. It's fine for the VM to consider any class that have a record
>> Attribute is a record.
>> 
>> We already had this discussion on amber-spec-expert list,
>> see
>> https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html
> 
> What is the conclusion (sorry it was unclear to me)?  Drop TNSFF for records?
> 
> This issue is to fix the regression introduced by JDK-8255342.   I expect
> someone else will file a new JBS issue and implement what amber-spec-experts
> decided.
> 
>> We already know that the JLS definition of a record will have to be changed 
>> for
>> inline record (an inline record is not direct subclass of j.l.Record because
>> you have the reference projection in the middle).
> 
> Yes I saw that.   I updated
> [JDK-8251041](https://bugs.openjdk.java.net/browse/JDK-8251041) to follow up.
> 
>> The real issue is that the JIT optimisation and Field.set() should be 
>> aligned,
>> VarHandle deosn't let you change a final field and Unsafe is unsafe, so the
>> current problem is that Field.set() relies on the reflection api by calling
>> Class.isRecord() which is not good because Classs.isRecord() returns the JLS
>> view of the world not the JVM view of the world.
>>
>> As said in the mail chain above, for the JIT optimization, instead of listing
>> all the new constructs, record, inline, etc, i propose to check the other 
>> way,
>> only allow to set a final field is only allowed for plain old java class, so
>> the VM should not have a method InstanceKlass::is_record but a method that
>> return if a class is a plain class or not and this method should be called by
>> the JIT and by Field.set (through a JVM entry point)
>> so the fact the optimization will be done or not is not related to what the 
>> JLS
>> think a record is, those are separated concern.
> 
> I agree the current situation is not ideal which requires to declare all the 
> new
> constructs explicitly for TNSFF.   However, it's a reasonable tradeoff to get
> the JIT optimization for records in time while waiting for true TNSFF
> investigation like JMM and other relevant specs.   I see this just a stop-gap
> solution.  When the long-term TNSFF is in place, the spec can be updated to
> drop the explicit list of record, inline, etc.
> 
> A related issue is
> [JDK-8233873](https://bugs.openjdk.java.net/browse/JDK-8233873).   I'm happy 
> if
> we can do TNSFF in a different solution.
> 
> Again this PR intends to fix the regression.  Two options:
> 1. Keep [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) and
> implement as this proposed patch
> 2. Backout [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444)
> 
> I expect Chris and/or others will follow up the decision made by the
> amber-spec-experts w.r.t. trusting finals in records.   I'm okay with either
> option.

For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i prefer 
VM to be oblivious about java.lang.Record.
And in the future, the real fix is to change the spec of Field.set() to say 
that it's only allowed for plain java classes and have the implementation to 
directly asks the VM is a non static field is trusted or not.

And there are also a related issue with the validation of the 
InnerClass/Enclosing attribute were the VM does a 

Re: JNLP

[Bernd Eckenfels]

Hello,

We ported a big JWS gui app to stand-alone swing with a home made 
installer/update mechanism. This was very easy to do (it had a main method for 
debugging anyway). The installer is not the most comfortable, but we can live 
with it since the whole application will be replaced by an web app anyway.

Gruss
Bernd


--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von Thomas 
Vatter 
Gesendet: Thursday, December 10, 2020 1:12:45 PM
An: core-libs-dev@openjdk.java.net 
Betreff: JNLP

I'm using JNLP, how should I go on?

--
Network Inventory Software
IBM-Partner, RedHat- und SUSE-Partner, Oracle Technet Member

www.network-inventory.de
Tel. 030-79782510
Fax 030-79782512
E-Mail: thomas.vat...@network-inventory.de

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Chris Hegarty]

On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung  wrote:

> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on 
> classes with `RecordComponents` attributes.  That introduces a regression in 
> `InstanceKlass::is_record` that returns true on a non-record class which has 
> `RecordComponents` attribute present.   This causes unexpected semantics in 
> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust 
> final fields for non-record classes.
> 
> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a 
> record class, i.e. final direct subclass of `java.lang.Record` with the 
> presence of `RecordComponents` attribute.  There is no change to JVM class 
> file validation.   Also I propose clearly define:
> - `JVM_IsRecord` returns true if the given class is a record i.e. final 
> and direct subclass of `java.lang.Record` with `RecordComponents` attribute
> - `JVM_GetRecordComponents` returns an `RecordComponents` array  if 
> `RecordComponents` attribute is present; otherwise, returns NULL.  This does 
> not check if it's a record class or not.  So it may return non-null on a 
> non-record class if it has `RecordComponents` attribute.  So 
> `JVM_GetRecordComponents` returns the content of the classfile.

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1706

Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v4]

[Jorn Vernee]

On Thu, 10 Dec 2020 09:46:07 GMT, Chris Hegarty  wrote:

>> Marked as reviewed by bpb (Reviewer).
>
> While the updated set of scenarios covered by this benchmark is
> reasonably (and vastly improves coverage), I find myself reluctant to
> add the last remaining buffer property - read-only views. It's time to
> template the generation of this code!

If the cases get to be too many, you might also want to consider splitting the 
benchmark class into several classes that cover different cases (we did this 
for the memory access benchmarks as well). That would also make it easier to 
run a subset of cases in isolation.

-

PR: https://git.openjdk.java.net/jdk/pull/1430

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Harold Seigel]

On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung  wrote:

> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on 
> classes with `RecordComponents` attributes.  That introduces a regression in 
> `InstanceKlass::is_record` that returns true on a non-record class which has 
> `RecordComponents` attribute present.   This causes unexpected semantics in 
> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust 
> final fields for non-record classes.
> 
> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a 
> record class, i.e. final direct subclass of `java.lang.Record` with the 
> presence of `RecordComponents` attribute.  There is no change to JVM class 
> file validation.   Also I propose clearly define:
> - `JVM_IsRecord` returns true if the given class is a record i.e. final 
> and direct subclass of `java.lang.Record` with `RecordComponents` attribute
> - `JVM_GetRecordComponents` returns an `RecordComponents` array  if 
> `RecordComponents` attribute is present; otherwise, returns NULL.  This does 
> not check if it's a record class or not.  So it may return non-null on a 
> non-record class if it has `RecordComponents` attribute.  So 
> `JVM_GetRecordComponents` returns the content of the classfile.

Hi Mandy,
Could you replace the comment starting at line 1854 in jvm.cpp with:
// A class is a record if and only if it is final and a direct subclass
// of java.lang.Record and the presence of `Record` attribute;
// otherwise, it is not a record.

Also, replace the comment starting at line 1872 in jvm.cpp with:
// Returns an array containing the components of the Record attribute,
// or NULL if the attribute is not present.
//
// Note that this function returns the components of the Record
// attribute even if the class is not a Record.



Also, please change the name of the attribute in the comments added
to Class.java from RecordComponent to Record.

Thanks, Harold

-

PR: https://git.openjdk.java.net/jdk/pull/1706

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Jorn Vernee]

On Thu, 10 Dec 2020 09:15:47 GMT, Nick Gasson  wrote:

>> This is more-or-less a straight port of the x86 code to AArch64.
>> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
>> to generate a blob that jumps to the native function entry point. This
>> simply switches the thread state from Java to native and handles the
>> safepoint poll on return from native code.
>> 
>> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
>> pointer) may hold a live oop over the MachCallNative node. Normally this
>> would be saved by RegisterSaver::save_live_registers() but the native
>> invoker blob is not a "proper" stub routine so doesn't have an oop map.
>> I copied the x86 solution to this where the frame pointer register is
>> saved to the stack and a pointer to that is stored in the frame anchor.
>> This is then read back and added to the register map when walking the
>> stack. I saw in the PR comments [1] that this might be a temporary fix,
>> but I'm not sure if there's any plan to change that now?
>> 
>> Another potential fix might be to change the C2 register definition of
>> R29 and R29_H to be save-on-call as below. This works for the
>> TestStackWalk.java test case but I don't know whether it has other
>> unintended side-effects.
>> 
>>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
>> 
>> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
>> working:
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
>> ns/op
>> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
>> ns/op
>> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
>> ns/op
>> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
>> ns/op
>> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
>> ns/op
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
>> ns/op
>> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
>> ns/op
>> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
>> ns/op
>> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
>> ns/op
>> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
>> ns/op
>> 
>> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326
>
> Nick Gasson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comments

Thanks for the amazing work!

FWIW, on x86 RBP was being passed as debug info, so the solution Vlad I 
proposed would be to override MachCallNativeNode::in_RegMask to not include it 
IIRC. I haven't had time to look into it yet though.

The situation on AArch64 seems to be different though? The RFP is not passed as 
debug info but as part of the normal calling convention maybe?

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3104:

> 3102:const 
> GrowableArray& output_registers) {
> 3103:   BufferBlob* _invoke_native_blob =
> 3104: BufferBlob::create("nep_invoker_blob", 
> MethodHandles::adapter_code_size);

That reminds me; this should _not_ use MethodHandles::adapter_code_size, but a 
separate constant, since the former is tailored specifically to method handle 
stubs (and is too large for this case). I still need to update the one for x86 
as well (looks like I forgot to do that one before when I changed them for 
invoker/upcall handler). I think 1024 bytes should be more than enough, but 
would need to test it.

-

Marked as reviewed by 

Re: RFR: 8257596: Clarify trusted final fields for record classes

[Harold Seigel]

On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung  wrote:

> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on 
> classes with `RecordComponents` attributes.  That introduces a regression in 
> `InstanceKlass::is_record` that returns true on a non-record class which has 
> `RecordComponents` attribute present.   This causes unexpected semantics in 
> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust 
> final fields for non-record classes.
> 
> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a 
> record class, i.e. final direct subclass of `java.lang.Record` with the 
> presence of `RecordComponents` attribute.  There is no change to JVM class 
> file validation.   Also I propose clearly define:
> - `JVM_IsRecord` returns true if the given class is a record i.e. final 
> and direct subclass of `java.lang.Record` with `RecordComponents` attribute
> - `JVM_GetRecordComponents` returns an `RecordComponents` array  if 
> `RecordComponents` attribute is present; otherwise, returns NULL.  This does 
> not check if it's a record class or not.  So it may return non-null on a 
> non-record class if it has `RecordComponents` attribute.  So 
> `JVM_GetRecordComponents` returns the content of the classfile.

Hi Mandy,
Could you regression test this change against the JCK lang and vm test suites 
and Mach5 tiers 1 and 2?
Thanks, Harold

-

PR: https://git.openjdk.java.net/jdk/pull/1706

RFR: 8257837: Performance regression in heap byte buffer views

[Maurizio Cimadamore]

As a result of the recent integration of the foreign memory access API, some of 
the buffer implementations now use `ScopedMemoryAccess` instead of `Unsafe`. 
While this works generally well, there are situations where profile pollution 
arises, which result in a considerable slowdown. The profile pollution occurs 
because the same ScopedMemoryAccess method (e.g. `getIntUnaligned`) is called 
with two different buffer kinds (e.g. an off heap buffer where base == null, 
and an on-heap buffer where base == byte[]). Because of that, unsafe access 
cannot be optimized, since C2 can't guess what the unsafe base access is.

In reality, this problem was already known (and solved) elsewhere: the 
sun.misc.Unsafe wrapper does basically the same thing that ScopedMemoryAccess 
does. To make sure that profile pollution does not occur in those cases, 
argument profiling is enabled for sun.misc.Unsafe as well. This patch adds yet 
another case for ScopedMemoryAccess.

Here are the benchmark results:

Before:

BenchmarkMode  Cnt  Score   Error  
Units
LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.612 ? 0.005  
ms/op
LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  2.740 ? 0.039  
ms/op
LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.504 ? 0.020  
ms/op

After

BenchmarkMode  Cnt  Score   Error  
Units
LoopOverPollutedBuffer.direct_byte_buffer_get_float  avgt   30  0.613 ? 0.007  
ms/op
LoopOverPollutedBuffer.heap_byte_buffer_get_int  avgt   30  0.304 ? 0.002  
ms/op
LoopOverPollutedBuffer.unsafe_get_float  avgt   30  0.491 ? 0.004  
ms/op

-

Commit messages:
 - Add argument profiling for ScopedMemoryAccess

Changes: https://git.openjdk.java.net/jdk/pull/1733/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1733=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257837
  Stats: 121 lines in 3 files changed: 120 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1733.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1733/head:pull/1733

PR: https://git.openjdk.java.net/jdk/pull/1733

JNLP

[Thomas Vatter]

I'm using JNLP, how should I go on?

-- 
Network Inventory Software
IBM-Partner, RedHat- und SUSE-Partner, Oracle Technet Member

www.network-inventory.de
Tel. 030-79782510
Fax 030-79782512
E-Mail: thomas.vat...@network-inventory.de

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]

[Kim Barrett]

On Tue, 8 Dec 2020 17:30:11 GMT, Mandy Chung  wrote:

>> Kim Barrett 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 five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into no_isenqueued
>>  - move REMOVE and RETAIN decls and init
>>  - update WeakReferenceGC test
>>  - update ReferenceQueue test
>>  - update ReferencesGC test
>
> Marked as reviewed by mchung (Reviewer).

Thanks for reviews @mlchung and @tschatzl

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Andrew Haley]

On Thu, 10 Dec 2020 10:25:33 GMT, Nick Gasson  wrote:

>> push_fp() doesn't make much sense if the RegSet is a set of Registers, which 
>> are by definition not FloatRegisters. That casting of Register to 
>> FloatRegister in gc/z is evil.
>
> Should we have a separate RegSet type for FloatRegisters to avoid mixing them 
> up?

Absolutely. I'd make an AbstractRegSet and use it as a base type
for both RegSet and FloatRegSet, then we can get rid of the casts
altogether.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Integrated: 8257876: Avoid Reference.isEnqueued in tests

[Kim Barrett]

On Tue, 8 Dec 2020 09:52:51 GMT, Kim Barrett  wrote:

> Please review this change that eliminates the use of Reference.isEnqueued by
> tests.  There were three tests using it:
> 
> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
> jdk/java/lang/ref/ReferenceEnqueue.java
> 
> In each of them, some combination of using Reference.refersTo and
> ReferenceQueue.remove with a timeout were used to eliminate the use of
> Reference.isEnqueued.
> 
> I also cleaned up ReferencesGC.java in various respects.  It contained
> several bits of dead code, and the failure checks were made stronger.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).

This pull request has now been integrated.

Changeset: db5da961
Author:Kim Barrett 
URL:   https://git.openjdk.java.net/jdk/commit/db5da961
Stats: 104 lines in 3 files changed: 23 ins; 39 del; 42 mod

8257876: Avoid Reference.isEnqueued in tests

Reviewed-by: mchung, tschatzl

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]

[Kim Barrett]

> Please review this change that eliminates the use of Reference.isEnqueued by
> tests.  There were three tests using it:
> 
> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
> jdk/java/lang/ref/ReferenceEnqueue.java
> 
> In each of them, some combination of using Reference.refersTo and
> ReferenceQueue.remove with a timeout were used to eliminate the use of
> Reference.isEnqueued.
> 
> I also cleaned up ReferencesGC.java in various respects.  It contained
> several bits of dead code, and the failure checks were made stronger.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).

Kim Barrett 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 five additional commits since 
the last revision:

 - Merge branch 'master' into no_isenqueued
 - move REMOVE and RETAIN decls and init
 - update WeakReferenceGC test
 - update ReferenceQueue test
 - update ReferencesGC test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1691/files
  - new: https://git.openjdk.java.net/jdk/pull/1691/files/01710567..d5355342

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1691=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1691=01-02

  Stats: 7952 lines in 328 files changed: 5091 ins; 1646 del; 1215 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1691.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

On Thu, 10 Dec 2020 10:21:09 GMT, Andrew Haley  wrote:

>> Er... no. But not because of the cast. The `push(fp_spills)` below should be 
>> `push_fp(fp_spills)`. I'll add a FloatRegister constructor to RegSet so it 
>> doesn't need that any more. There's one other place that does it in 
>> cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp
>
> push_fp() doesn't make much sense if the RegSet is a set of Registers, which 
> are by definition not FloatRegisters. That casting of Register to 
> FloatRegister in gc/z is evil.

Should we have a separate RegSet type for FloatRegisters to avoid mixing them 
up?

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Andrew Haley]

On Thu, 10 Dec 2020 10:01:49 GMT, Nick Gasson  wrote:

>> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3192:
>> 
>>> 3190:   spills += RegSet::of(output->as_Register());
>>> 3191: } else if (output->is_FloatRegister()) {
>>> 3192:   fp_spills += RegSet::of((Register)output->as_FloatRegister());
>> 
>> This looks very strange. Does it generate the correct code for 
>> FloatRegisters?
>
> Er... no. But not because of the cast. The `push(fp_spills)` below should be 
> `push_fp(fp_spills)`. I'll add a FloatRegister constructor to RegSet so it 
> doesn't need that any more. There's one other place that does it in 
> cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp

push_fp() doesn't make much sense if the RegSet is a set of Registers, which 
are by definition not FloatRegisters. That casting of Register to FloatRegister 
in gc/z is evil.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

On Thu, 10 Dec 2020 09:36:44 GMT, Andrew Haley  wrote:

>> Nick Gasson has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments
>
> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3192:
> 
>> 3190:   spills += RegSet::of(output->as_Register());
>> 3191: } else if (output->is_FloatRegister()) {
>> 3192:   fp_spills += RegSet::of((Register)output->as_FloatRegister());
> 
> This looks very strange. Does it generate the correct code for FloatRegisters?

Er... no. But not because of the cast. The `push(fp_spills)` below should be 
`push_fp(fp_spills)`. I'll add a FloatRegister constructor to RegSet so it 
doesn't need that any more. There's one other place that does it in 
cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v4]

[Chris Hegarty]

On Mon, 30 Nov 2020 20:54:09 GMT, Brian Burkhalter  wrote:

>> Chris Hegarty has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Add explicitly allocated heap carrier buffer tests
>>  - Replace Single with Loop
>
> Marked as reviewed by bpb (Reviewer).

While the updated set of scenarios covered by this benchmark is
reasonably (and vastly improves coverage), I find myself reluctant to
add the last remaining buffer property - read-only views. It's time to
template the generation of this code!

-

PR: https://git.openjdk.java.net/jdk/pull/1430

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Andrew Haley]

On Thu, 10 Dec 2020 09:15:47 GMT, Nick Gasson  wrote:

>> This is more-or-less a straight port of the x86 code to AArch64.
>> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
>> to generate a blob that jumps to the native function entry point. This
>> simply switches the thread state from Java to native and handles the
>> safepoint poll on return from native code.
>> 
>> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
>> pointer) may hold a live oop over the MachCallNative node. Normally this
>> would be saved by RegisterSaver::save_live_registers() but the native
>> invoker blob is not a "proper" stub routine so doesn't have an oop map.
>> I copied the x86 solution to this where the frame pointer register is
>> saved to the stack and a pointer to that is stored in the frame anchor.
>> This is then read back and added to the register map when walking the
>> stack. I saw in the PR comments [1] that this might be a temporary fix,
>> but I'm not sure if there's any plan to change that now?
>> 
>> Another potential fix might be to change the C2 register definition of
>> R29 and R29_H to be save-on-call as below. This works for the
>> TestStackWalk.java test case but I don't know whether it has other
>> unintended side-effects.
>> 
>>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
>> 
>> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
>> working:
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
>> ns/op
>> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
>> ns/op
>> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
>> ns/op
>> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
>> ns/op
>> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
>> ns/op
>> 
>> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
>> 
>> BenchmarkMode  Cnt Score Error  
>> Units
>> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
>> ns/op
>> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
>> ns/op
>> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
>> ns/op
>> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
>> ns/op
>> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
>> ns/op
>> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
>> ns/op
>> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
>> ns/op
>> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
>> ns/op
>> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
>> ns/op
>> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
>> ns/op
>> 
>> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326
>
> Nick Gasson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comments

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3192:

> 3190:   spills += RegSet::of(output->as_Register());
> 3191: } else if (output->is_FloatRegister()) {
> 3192:   fp_spills += RegSet::of((Register)output->as_FloatRegister());

This looks very strange. Does it generate the correct code for FloatRegisters?

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]

[Thomas Schatzl]

On Thu, 10 Dec 2020 08:56:25 GMT, Kim Barrett  wrote:

>> I understand that the test code previously just forwarded the 
>> `InterruptedException` if it happened in the `Thread.sleep()` call too. So 
>> this may only be an exiting issue and please ignore this comment.
>> Not catching `InterruptedException` here only seems to be a cause for 
>> unnecessary failure. Then again, it probably does not happen a lot.
>
> Nothing in the test calls Thread.interrupt(), so there isn't a risk of
> failure due to not handling that exception in some "interesting" way. But
> InterruptedException must be "handled" somehow, because it's a checked
> exception. That's already dealt with by the run() method declaring that it
> throws that type, and main declaring that it throws Exception.  The other
> tests modified in this change don't take that approach (just let it
> propagate out through main), instead wrapping the interruptable calls in
> try/catch, though again just to satisfy the requirement that a checked
> exception must be statically verified to be handled, even though there
> aren't going to be any thrown.

Okay.

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]

[Thomas Schatzl]

On Thu, 10 Dec 2020 09:01:54 GMT, Kim Barrett  wrote:

>> Please review this change that eliminates the use of Reference.isEnqueued by
>> tests.  There were three tests using it:
>> 
>> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
>> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
>> jdk/java/lang/ref/ReferenceEnqueue.java
>> 
>> In each of them, some combination of using Reference.refersTo and
>> ReferenceQueue.remove with a timeout were used to eliminate the use of
>> Reference.isEnqueued.
>> 
>> I also cleaned up ReferencesGC.java in various respects.  It contained
>> several bits of dead code, and the failure checks were made stronger.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).
>
> Kim Barrett has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   move REMOVE and RETAIN decls and init

Also good with deferring the changes to the comments and the move of the 
statics.

-

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

On Wed, 9 Dec 2020 09:58:26 GMT, Andrew Haley  wrote:

>> Nick Gasson has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments
>
> src/hotspot/cpu/aarch64/aarch64.ad line 16057:
> 
>> 16055: 
>> 16056:   format %{ "CALL, native $meth" %}
>> 16057: 
> 
> It might be better to expand this a little to indicate a near or a far call, 
> if that's possible.

That would be nice but I can't see how to do it: adlc doesn't allow arbitrary 
code inside `format %{ %}`.

> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3186:
> 
>> 3184: // Make sure that native code does not change SVE vector length.
>> 3185: __ verify_sve_vector_length();
>> 3186:   }
> 
> Do we have to surround every call to verify_sve_vector_length() with if 
> (UseSVE > 0) ?
> Why is that check not inside verify_sve_vector_length() ? What is the meaning 
> of the
>> 0 comparison? Can it be negative? So many questions.  :-)

The valid values are {0,1,2} so > 0 is supposed to read as "SVE1 or SVE2". But 
I also think it's neater if the check is inside `verify_sve_vector_length` so 
I've moved it there.

> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3098:
> 
>> 3096: MacroAssembler* masm = _masm;
>> 3097: if (reg->is_Register()) {
>> 3098:   __ push(RegSet::of(reg->as_Register()), sp);
> 
> Is this right? SP is 16-aligned, so this will use 16 bytes of stack for every 
> 8-byte register. Does it get used a lot?

It's used to preserve the native result registers around the runtime calls in 
the safepoint and reguard slow paths. With the way the intrinsic works 
currently, there'll actually only ever be a single output register. I did it 
this way originally to keep the code similar to x86 but actually it's just as 
easy to build a `RegSet` directly and push that, so I've removed these 
functions.

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64

[Nick Gasson]

On Wed, 9 Dec 2020 08:20:38 GMT, Nick Gasson  wrote:

> This is more-or-less a straight port of the x86 code to AArch64.
> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
> to generate a blob that jumps to the native function entry point. This
> simply switches the thread state from Java to native and handles the
> safepoint poll on return from native code.
> 
> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
> pointer) may hold a live oop over the MachCallNative node. Normally this
> would be saved by RegisterSaver::save_live_registers() but the native
> invoker blob is not a "proper" stub routine so doesn't have an oop map.
> I copied the x86 solution to this where the frame pointer register is
> saved to the stack and a pointer to that is stored in the frame anchor.
> This is then read back and added to the register map when walking the
> stack. I saw in the PR comments [1] that this might be a temporary fix,
> but I'm not sure if there's any plan to change that now?
> 
> Another potential fix might be to change the C2 register definition of
> R29 and R29_H to be save-on-call as below. This works for the
> TestStackWalk.java test case but I don't know whether it has other
> unintended side-effects.
> 
>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
> 
> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
> working:
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
> ns/op
> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
> ns/op
> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
> ns/op
> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
> ns/op
> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
> ns/op
> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
> ns/op
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
> ns/op
> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
> ns/op
> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
> ns/op
> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
> ns/op
> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
> ns/op
> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
> ns/op
> 
> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326

@JornVernee and @mcimadamore could you take a look too?

-

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257882: Implement linkToNative intrinsic on AArch64 [v2]

[Nick Gasson]

> This is more-or-less a straight port of the x86 code to AArch64.
> GraphKit::make_native_call() calls SharedRuntime::make_native_invoker()
> to generate a blob that jumps to the native function entry point. This
> simply switches the thread state from Java to native and handles the
> safepoint poll on return from native code.
> 
> AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame
> pointer) may hold a live oop over the MachCallNative node. Normally this
> would be saved by RegisterSaver::save_live_registers() but the native
> invoker blob is not a "proper" stub routine so doesn't have an oop map.
> I copied the x86 solution to this where the frame pointer register is
> saved to the stack and a pointer to that is stored in the frame anchor.
> This is then read back and added to the register map when walking the
> stack. I saw in the PR comments [1] that this might be a temporary fix,
> but I'm not sure if there's any plan to change that now?
> 
> Another potential fix might be to change the C2 register definition of
> R29 and R29_H to be save-on-call as below. This works for the
> TestStackWalk.java test case but I don't know whether it has other
> unintended side-effects.
> 
>   reg_def R29 ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()); // fp
>   reg_def R29_H   ( SOC,  NS, Op_RegI, 29, r29->as_VMReg()->next());
> 
> JMH results from jdk/incubator/foreign/CallOverhead.java to show it's
> working:
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.450 ?   0.363  
> ns/op
> CallOverhead.jni_identityavgt   3054.145 ?   0.627  
> ns/op
> CallOverhead.panama_args10   avgt   30  1914.431 ?  73.771  
> ns/op
> CallOverhead.panama_args5avgt   30  1394.274 ?  49.369  
> ns/op
> CallOverhead.panama_blankavgt   30   872.878 ?  20.716  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30   873.852 ?  21.350  
> ns/op
> CallOverhead.panama_identity avgt   30  1058.729 ?  20.229  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   30  1041.648 ?  22.930  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3212.483 ? 151.627  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30  1056.398 ?  18.779  
> ns/op
> 
> -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true:
> 
> BenchmarkMode  Cnt Score Error  
> Units
> CallOverhead.jni_blank   avgt   3051.519 ?   0.345  
> ns/op
> CallOverhead.jni_identityavgt   3054.689 ?   0.687  
> ns/op
> CallOverhead.panama_args10   avgt   3042.856 ?   0.760  
> ns/op
> CallOverhead.panama_args5avgt   3042.192 ?   0.712  
> ns/op
> CallOverhead.panama_blankavgt   3041.934 ?   0.349  
> ns/op
> CallOverhead.panama_blank_trivialavgt   30 2.806 ?   0.545  
> ns/op
> CallOverhead.panama_identity avgt   3044.043 ?   0.398  
> ns/op
> CallOverhead.panama_identity_memory_address  avgt   3045.021 ?   0.409  
> ns/op
> CallOverhead.panama_identity_struct  avgt   30  3206.829 ? 175.750  
> ns/op
> CallOverhead.panama_identity_trivial avgt   30 7.849 ?   1.651  
> ns/op
> 
> [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326

Nick Gasson has updated the pull request incrementally with one additional 
commit since the last revision:

  Review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1711/files
  - new: https://git.openjdk.java.net/jdk/pull/1711/files/a5f1b33b..99390b92

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1711=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1711=00-01

  Stats: 70 lines in 3 files changed: 12 ins; 45 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1711.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1711/head:pull/1711

PR: https://git.openjdk.java.net/jdk/pull/1711

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]

[Kim Barrett]

> Please review this change that eliminates the use of Reference.isEnqueued by
> tests.  There were three tests using it:
> 
> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
> jdk/java/lang/ref/ReferenceEnqueue.java
> 
> In each of them, some combination of using Reference.refersTo and
> ReferenceQueue.remove with a timeout were used to eliminate the use of
> Reference.isEnqueued.
> 
> I also cleaned up ReferencesGC.java in various respects.  It contained
> several bits of dead code, and the failure checks were made stronger.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).

Kim Barrett has updated the pull request incrementally with one additional 
commit since the last revision:

  move REMOVE and RETAIN decls and init

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1691/files
  - new: https://git.openjdk.java.net/jdk/pull/1691/files/e87206a8..01710567

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1691=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1691=00-01

  Stats: 6 lines in 1 file changed: 4 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1691.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]

[Kim Barrett]

On Wed, 9 Dec 2020 13:59:09 GMT, Thomas Schatzl  wrote:

>> test/jdk/java/lang/ref/ReferenceEnqueue.java line 58:
>> 
>>> 56: for (int i = 0; i < iterations; i++) {
>>> 57: System.gc();
>>> 58: enqueued = (queue.remove(100) == ref);
>> 
>> The code does not catch `InterruptedException` like it does in the other 
>> files.
>
> I understand that the test code previously just forwarded the 
> `InterruptedException` if it happened in the `Thread.sleep()` call too. So 
> this may only be an exiting issue and please ignore this comment.
> Not catching `InterruptedException` here only seems to be a cause for 
> unnecessary failure. Then again, it probably does not happen a lot.

Nothing in the test calls Thread.interrupt(), so there isn't a risk of
failure due to not handling that exception in some "interesting" way. But
InterruptedException must be "handled" somehow, because it's a checked
exception. That's already dealt with by the run() method declaring that it
throws that type, and main declaring that it throws Exception.  The other
tests modified in this change don't take that approach (just let it
propagate out through main), instead wrapping the interruptable calls in
try/catch, though again just to satisfy the requirement that a checked
exception must be statically verified to be handled, even though there
aren't going to be any thrown.

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests

[Kim Barrett]

On Wed, 9 Dec 2020 13:26:04 GMT, Thomas Schatzl  wrote:

>> Please review this change that eliminates the use of Reference.isEnqueued by
>> tests.  There were three tests using it:
>> 
>> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
>> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
>> jdk/java/lang/ref/ReferenceEnqueue.java
>> 
>> In each of them, some combination of using Reference.refersTo and
>> ReferenceQueue.remove with a timeout were used to eliminate the use of
>> Reference.isEnqueued.
>> 
>> I also cleaned up ReferencesGC.java in various respects.  It contained
>> several bits of dead code, and the failure checks were made stronger.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).
>
> test/hotspot/jtreg/vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java line 
> 129:
> 
>> 127: }
>> 128: 
>> 129: int REMOVE = (int) (RANGE * RATIO);
> 
> These two constants could be factored out as static finals to match the 
> casing.

I'm making REMOVE and RETAIN statics, near RANGE and RATIO.  (Meant to do that 
before, but forgot.)  They can't be final though, because RANGE and RATIO 
aren't final, and can be set from command line arguments.  So they'll get 
initialized in parseArgs.

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257876: Avoid Reference.isEnqueued in tests

[Kim Barrett]

On Wed, 9 Dec 2020 13:28:44 GMT, Thomas Schatzl  wrote:

>> Please review this change that eliminates the use of Reference.isEnqueued by
>> tests.  There were three tests using it:
>> 
>> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
>> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java
>> jdk/java/lang/ref/ReferenceEnqueue.java
>> 
>> In each of them, some combination of using Reference.refersTo and
>> ReferenceQueue.remove with a timeout were used to eliminate the use of
>> Reference.isEnqueued.
>> 
>> I also cleaned up ReferencesGC.java in various respects.  It contained
>> several bits of dead code, and the failure checks were made stronger.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) ran all three tests with each GC (including Shenandoah).
>
> Changes requested by tschatzl (Reviewer).

> [pre-existing] The topWeakReferenceGC.java description at the top describes 
> that the test calls System.gc() explicitly to trigger garbage collections at 
> the end. It does not. Maybe this could be weasel-worded around like in the 
> other cases in that text.

There are a lot of things much more wrong with that comment.  Doing more GCs
doesn't cause more enqueues to happen.  The "non-deterministic" enqueuing is
just a race.  The GC adds references to the pending list.  The reference
processing thread transfers references from the pending list to their
associated queue (if any).  The test code is racing with that.  The change
to use Reference.remove with a timeout eliminates all that, and one GC
should be.  Addressing all that would be a substantial rewrite of this
test though.  Mind if I defer that to a new RFE?

-

PR: https://git.openjdk.java.net/jdk/pull/1691

Re: RFR: 8257450: Start of release updates for JDK 17 [v8]

[Joe Darcy]

> Start of JDK 17 updates.

Joe Darcy 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 18 additional commits since the 
last revision:

 - Merge branch 'master' into JDK-8257450
 - Merge branch 'master' into JDK-8257450
 - Merge branch 'master' into JDK-8257450
 - Merge branch 'master' into JDK-8257450
 - Get in JEP 390 changes.
 - Merge branch 'master' into JDK-8257450
 - Merge branch 'master' into JDK-8257450
 - Update symbol files for JDK 16b27.
 - Merge branch 'master' into JDK-8257450
 - Merge branch 'master' into JDK-8257450
 - ... and 8 more: https://git.openjdk.java.net/jdk/compare/61f30b72...553d134f

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1531/files
  - new: https://git.openjdk.java.net/jdk/pull/1531/files/766c2c01..553d134f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=06-07

  Stats: 554 lines in 7 files changed: 324 ins; 220 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1531.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1531/head:pull/1531

PR: https://git.openjdk.java.net/jdk/pull/1531