Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]

[Tagir F . Valeev]

On Mon, 28 Jun 2021 19:45:34 GMT, Roger Riggs  wrote:

>> Add java.util.Objects.newIdentity to supply a unique object with identity.
>> This is a replacement code can be used today for the traditional new 
>> Object() idiom, which will be deprecated under Project Valhalla.
>> Refer to [JEP 401: Primitive Objects 
>> (Preview)](https://openjdk.java.net/jeps/401) for background.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add test synchronizing on return value of Objecst.newIdentity()

Probably it would be better to have an inner class named like `Identity` 
instead of anonymous class? When debugging or analyzing memory dumps, it would 
be more user-friendly to see `Objects$Identity` than `Objects$1`.

Probably, not the part of this feature request, but it would be nice to add 
another method with string parameter, like `Objects.newIdentity("MY 
SENTINEL")`. The string should be stored in the field and returned from 
toString(). Again, this would make it easier to find where the object comes 
from during debugging or memory dump analysis. For the record, here's what we 
have in IntelliJ IDEA sources (Apache 2.0 licensed):


public final class ObjectUtils {
  private ObjectUtils() { }

  ...

  /**
   * Creates a new object which could be used as sentinel value (special value 
to distinguish from any other object). It does not equal
   * to any other object. Usually should be assigned to the static final field.
   *
   * @param name an object name, returned from {@link #toString()} to simplify 
the debugging or heap dump analysis
   * (guaranteed to be stored as sentinel object field). If 
sentinel is assigned to the static final field,
   * it's recommended to supply that field name (possibly qualified 
with the class name).
   * @return a new sentinel object
   */
  public static @NotNull Object sentinel(@NotNull @NonNls String name) {
return new Sentinel(name);
  }

  private static final class Sentinel {
private final String myName;

Sentinel(@NotNull String name) {
  myName = name;
}

@Override
public String toString() {
  return myName;
}
  }

-

PR: https://git.openjdk.java.net/jdk17/pull/112

Re: [jdk17] RFR: JDK-8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}

[Iris Clark]

On Tue, 29 Jun 2021 04:39:28 GMT, Jonathan Gibbons  wrote:

> Please review a trivial `noreg-doc` fix for some javadoc tags for 
> `java.lang.Runtime` for JDK17

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/167

Re: [jdk17] RFR: JDK-8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}

[Athijegannathan Sundararajan]

On Tue, 29 Jun 2021 04:39:28 GMT, Jonathan Gibbons  wrote:

> Please review a trivial `noreg-doc` fix for some javadoc tags for 
> `java.lang.Runtime` for JDK17

LGTM

-

Marked as reviewed by sundar (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/167

[jdk17] RFR: JDK-8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}

[Jonathan Gibbons]

Please review a trivial `noreg-doc` fix for some javadoc tags for 
`java.lang.Runtime` for JDK17

-

Commit messages:
 - JDK-8249646: Runtime.exec(String, String[], File) documentation contains 
literal {@link ...}

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

PR: https://git.openjdk.java.net/jdk17/pull/167

Withdrawn: 8269384: Determine native byte order for StringUTF16 via ByteOrder

[Yi Yang]

On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang  wrote:

> Prefer using ByteOrder to compute byte order for StringUTF16 to determining 
> byte order by native method StringUTF16.isBigEndian.

This pull request has been closed without being integrated.

-

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

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant

[David Holmes]

On 29/06/2021 12:52 am, Christoph Göttschkes wrote:

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph Göttschkes  wrote:


Hi,

please review this small fix. The test case uses a custom launcher and before launching the JVM, it adds the 
"lib" and "lib/server" directories to the environment variable which controls the native library 
search path. For non server variants, the second directory is not called "lib/server", but 
"lib/client", for instance.

I changed the test case to use the utility methods in `Platform` to get the 
correct paths, dependent on the VM variant.


Thanks for the reviews.


assuming the Platform utility methods do as would be expected


Yes, the utility method does the switch between "bin" and "lib" for windows and 
non-windows

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L346-L350

and it does the switch for the variant

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L361-L369

I only have a linux setup, so I tested this with a client and a server VM only 
on linux.
Sorry, I didn't enable github workflows for my jdk17 fork yet. I did so now and 
hopefully the next commit will trigger it.


You can also trigger it manually. Go to your fork, go to the Actions 
tab, select the workflow, select the branch to run it on.


Cheers,
David


-

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder

[Yi Yang]

On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang  wrote:

> Prefer using ByteOrder to compute byte order for StringUTF16 to determining 
> byte order by native method StringUTF16.isBigEndian.

Thanks for the detailed clarification!

The purpose of this PR is to skip the native call and use ByteOrder. Now I'm 
convinced that we should not change the initialization order casually, this 
will break some "future code" and make UnsafeConstants unable to use String.

-

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

Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

[Jaikiran Pai]

Hello Lance,

Please take your time.

-Jaikiran

On 29/06/21 4:17 am, Lance Andersen wrote:

Hi Jaikiran,

This is on my list to look at but did not get to today.

Best
Lance
On Jun 27, 2021, at 11:52 PM, Jaikiran Pai 
mailto:j...@openjdk.java.net>> wrote:

Can I please get a review for this proposed fix for the issue reported in 
https://bugs.openjdk.java.net/browse/JDK-8190753?

The commit here checks for the size of the zip entry before trying to create a 
`ByteArrayOutputStream` for that entry's content. A new jtreg test has been 
included in this commit to reproduce the issue and verify the fix.

P.S: It's still a bit arguable whether it's a good idea to create the 
`ByteArrayOutputStream` with an initial size potentially as large as the 
`MAX_ARRAY_SIZE` or whether it's better to just use some smaller default value. 
However, I think that can be addressed separately while dealing with 
https://bugs.openjdk.java.net/browse/JDK-8011146

-

Commit messages:
- 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative 
initial size for ByteArrayOutputStream

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

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

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

[Seán Coffey]

Hi Valerie,

many thanks for the thorough review. I've taken all your feedback on 
board with the latest push. Some of the test anomalies were a result of 
previous iterations of test edits I had been making.


Regarding the extra edits in 
"src/java.base/share/lib/security/default.policy", I had assumed it 
would be ok to tidy up the module under edit but I've reverted the 
unrelated changes now.


I was doubtful about removing the AccessController.doPrivileged blocks 
around the InnocuousThread.newSystemThread calls given that I wasn't 
sure which path the calling code would come from but on re-examination, 
I think it's ok to remove. The module provides the necessary permissions 
already and use of InnocuousThread solves the original permissions 
issue. Thanks for spotting!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


Just curious, why split the loop into 2 and set the SecurityManager in 
between the two loops? Can't we just set the policy/security manager 
before the loop?
The test infra requires various permissions including the problem 
setContextClassLoader permission. I figured it was better to set up the 
test infra first and then trigger the security manager.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:


Sufficient permissions missing if this code was ever to run with 
SecurityManager.

Cleanest approach appears to be use of InnocuousThread to create the 
cleaner/poller threads.
Test case coverage extended to cover the SecurityManager scenario.

Reviewer request: @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:


129: permission java.lang.RuntimePermission 
"accessClassInPackage.com.sun.crypto.provider";
130: permission java.lang.RuntimePermission 
"accessClassInPackage.jdk.internal.misc";
131: permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:


140: permission java.security.SecurityPermission 
"clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission 
"getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:


950: AccessController.doPrivileged((PrivilegedAction) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:


954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:


1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:


1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:


1210:
1211: this.token = token;
1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:


54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }

Move this if-check block of code up before the for-loop?

-

PR: 

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v4]

[Joe Wang]

On Mon, 28 Jun 2021 20:33:29 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Missed one.

Marked as reviewed by joehw (Reviewer).

Thanks for the update. As for "of", apparently the native speakers were fine 
with it ;-)

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v3]

[Sean Coffey]

> Sufficient permissions missing if this code was ever to run with 
> SecurityManager. 
> 
> Cleanest approach appears to be use of InnocuousThread to create the 
> cleaner/poller threads.
> Test case coverage extended to cover the SecurityManager scenario.
> 
> Reviewer request: @valeriepeng

Sean Coffey 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:

 - Edits from review
 - Merge remote-tracking branch 'origin/master' into pkcs11-perms
 - Move TokenPoller to Runnable
 - 8269034: AccessControlException for SunPKCS11 daemon threads

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/117/files
  - new: https://git.openjdk.java.net/jdk17/pull/117/files/03af6494..e961ff09

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

  Stats: 3102 lines in 121 files changed: 2073 ins; 670 del; 359 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/117.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/117/head:pull/117

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: RFR: Merge jdk17 [v2]

[Jesper Wilhelmsson]

> Forwardport JDK 17 -> JDK 18

Jesper Wilhelmsson has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 103 commits:

 - Merge
 - 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
   
   Reviewed-by: lancea, naoto
 - 8269433: Remove effectively unused ReferenceProcessor::_enqueuing_is_done
   
   Reviewed-by: kbarrett, tschatzl
 - 8268902: Testing for threadObj != NULL is unnecessary in suspend handshake
   
   Reviewed-by: pchilanomate, dcubed
 - 8269222: Incorrect number of workers reported for reference processing
   
   Reviewed-by: tschatzl, sangheki
 - 8269122: The use of "extern const" for Register definitions generates poor 
code
   
   Reviewed-by: adinn, kbarrett, kvn
 - 8269003: Update the java manpage for JDK 18
   
   Reviewed-by: minqi
 - Merge
 - 8269261: The PlaceHolder code uses Thread everywhere but is always dealing 
with JavaThreads
   
   Reviewed-by: ccheung, coleenp
 - 8269129: Multiple tier1 tests in hotspot/jtreg/compiler are failing for 
client VMs
   
   Reviewed-by: kvn, iveresov
 - ... and 93 more: https://git.openjdk.java.net/jdk/compare/56240690...8863e7a7

-

Changes: https://git.openjdk.java.net/jdk/pull/4619/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4619=01
  Stats: 27175 lines in 592 files changed: 16042 ins; 9481 del; 1652 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4619.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4619/head:pull/4619

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

Integrated: Merge jdk17

[Jesper Wilhelmsson]

On Mon, 28 Jun 2021 21:58:36 GMT, Jesper Wilhelmsson  
wrote:

> Forwardport JDK 17 -> JDK 18

This pull request has now been integrated.

Changeset: 03d54e6e
Author:Jesper Wilhelmsson 
URL:   
https://git.openjdk.java.net/jdk/commit/03d54e6ef1a40ee78b0cc65ca0aea276fbdbc7b7
Stats: 224 lines in 20 files changed: 163 ins; 15 del; 46 mod

Merge

-

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

Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

[Lance Andersen]

Hi Jaikiran,

This is on my list to look at but did not get to today.

Best
Lance
On Jun 27, 2021, at 11:52 PM, Jaikiran Pai 
mailto:j...@openjdk.java.net>> wrote:

Can I please get a review for this proposed fix for the issue reported in 
https://bugs.openjdk.java.net/browse/JDK-8190753?

The commit here checks for the size of the zip entry before trying to create a 
`ByteArrayOutputStream` for that entry's content. A new jtreg test has been 
included in this commit to reproduce the issue and verify the fix.

P.S: It's still a bit arguable whether it's a good idea to create the 
`ByteArrayOutputStream` with an initial size potentially as large as the 
`MAX_ARRAY_SIZE` or whether it's better to just use some smaller default value. 
However, I think that can be addressed separately while dealing with 
https://bugs.openjdk.java.net/browse/JDK-8011146

-

Commit messages:
- 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative 
initial size for ByteArrayOutputStream

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

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

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]

[Roger Riggs]

> Add java.util.Objects.newIdentity to supply a unique object with identity.
> This is a replacement code can be used today for the traditional new Object() 
> idiom, which will be deprecated under Project Valhalla.
> Refer to [JEP 401: Primitive Objects 
> (Preview)](https://openjdk.java.net/jeps/401) for background.

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

  Add test synchronizing on return value of Objecst.newIdentity()

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/112/files
  - new: https://git.openjdk.java.net/jdk17/pull/112/files/eef1029c..f99d0846

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=112=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=112=03-04

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

PR: https://git.openjdk.java.net/jdk17/pull/112

RFR: Merge jdk17

[Jesper Wilhelmsson]

Forwardport JDK 17 -> JDK 18

-

Commit messages:
 - Merge
 - 8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass
 - 8267952: async logging supports to dynamically change tags and decorators
 - 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from 
ProblemList.txt
 - 8269403: Fix jpackage tests to gracefully handle jpackage app launcher 
crashes
 - 8269304: Regression ~5% in 2005 in b27
 - 8268236: The documentation of the String.regionMatches method contains error

The webrevs contain the adjustments done while merging with regards to each 
parent branch:
 - master: https://webrevs.openjdk.java.net/?repo=jdk=4619=00.0
 - jdk17: https://webrevs.openjdk.java.net/?repo=jdk=4619=00.1

Changes: https://git.openjdk.java.net/jdk/pull/4619/files
  Stats: 224 lines in 20 files changed: 163 ins; 15 del; 46 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4619.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4619/head:pull/4619

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

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v4]

[Lance Andersen]

On Mon, 28 Jun 2021 20:33:29 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Missed one.

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/163

RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes

[Ian Graves]

8199594: Add doc describing how (?x) ignores spaces in character classes

-

Commit messages:
 - Updating documentation on comments mode and character class whitespace

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

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

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v4]

[Iris Clark]

On Mon, 28 Jun 2021 20:33:29 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Missed one.

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v4]

[Naoto Sato]

> Please review this small doc change to the system property. Accompanying CSR 
> has also been created.

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

  Missed one.

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/163/files
  - new: https://git.openjdk.java.net/jdk17/pull/163/files/da119219..3b6ff65d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=163=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=163=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/163.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/163/head:pull/163

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Naoto Sato]

On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording.

Thanks. I updated the PR and CSR accordingly. As to removing "of", I think the 
current form is fine, but I am not a native English speaker, so happy to 
correct it if it is wrong.

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v3]

[Naoto Sato]

> Please review this small doc change to the system property. Accompanying CSR 
> has also been created.

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

  Refined wording #2.

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/163/files
  - new: https://git.openjdk.java.net/jdk17/pull/163/files/3d25418c..da119219

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

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

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Joe Wang]

On Mon, 28 Jun 2021 18:45:31 GMT, Lance Andersen  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Refined wording.
>
> src/java.base/share/classes/java/util/Locale.java line 460:
> 
>> 458:  * back to that of before Java SE 17. If the system property is set to
>> 459:  * {@code true}, those three current language codes are mapped to their
>> 460:  * backward compatible forms. It is only read at Java runtime startup, 
>> so a
> 
> I had thought about some of some minor word smithing in your prior commit, 
> but chose not to request a change.
> 
> In the above sentence,  It could be clearer what "It" is.  Perhaps something 
> along the lines of:
> 
> This property is only read at Java runtime startup  and subsequents calls to 
> "

Also, "backward compatible behavior" maybe not needed to be repeated, e.g. 
"subsequent calls to ... will have no effect.

"back to that of before Java SE 17" -- "of" may be removed.

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]

[Jorn Vernee]

On Mon, 28 Jun 2021 19:45:34 GMT, Roger Riggs  wrote:

>> Add java.util.Objects.newIdentity to supply a unique object with identity.
>> This is a replacement code can be used today for the traditional new 
>> Object() idiom, which will be deprecated under Project Valhalla.
>> Refer to [JEP 401: Primitive Objects 
>> (Preview)](https://openjdk.java.net/jeps/401) for background.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add test synchronizing on return value of Objecst.newIdentity()

Marked as reviewed by jvernee (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/112

[jdk17] Integrated: JDK-8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass

[Mandy Chung]

On Mon, 28 Jun 2021 16:09:36 GMT, Mandy Chung  wrote:

> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
> There are several tests under `` directory.  As the tests under 
> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
> the parent directory.

This pull request has now been integrated.

Changeset: 56240690
Author:Mandy Chung 
URL:   
https://git.openjdk.java.net/jdk17/commit/56240690f62f9048a45a53525efccffdec235a8d
Stats: 9 lines in 6 files changed: 0 ins; 0 del; 9 mod

8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass

Reviewed-by: jvernee

-

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Joe Wang]

On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording.

Marked as reviewed by joehw (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Iris Clark]

On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording.

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: JDK-8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass

[Mandy Chung]

On Mon, 28 Jun 2021 16:09:36 GMT, Mandy Chung  wrote:

> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
> There are several tests under `` directory.  As the tests under 
> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
> the parent directory.

Thanks Jorn.

-

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: JDK-8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass [v2]

[Mandy Chung]

> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
> There are several tests under `` directory.  As the tests under 
> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
> the parent directory.

Mandy Chung has updated the pull request incrementally with one additional 
commit since the last revision:

  Keep the tests under a directory

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/162/files
  - new: https://git.openjdk.java.net/jdk17/pull/162/files/43cf6670..a7f8b892

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

  Stats: 0 lines in 6 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/162.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/162/head:pull/162

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: JDK-8269426: Rename test/jdk/java/lang/invoke/t8150782 to accessClassAndFindClass [v2]

[Jorn Vernee]

On Mon, 28 Jun 2021 19:13:45 GMT, Mandy Chung  wrote:

>> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
>> There are several tests under `` directory.  As the tests under 
>> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
>> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
>> the parent directory.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Keep the tests under a directory

Marked as reviewed by jvernee (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v2]

[Roger Riggs]

On Mon, 28 Jun 2021 18:12:34 GMT, Jim Laskey  wrote:

>> The wording of the @implSpec referred to internal methods in the 
>> description. The patch rewords the @implSpec to be more descriptive of the 
>> algorithm than the methods used.
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Not averaging

src/java.base/share/classes/java/util/random/RandomGenerator.java line 644:

> 642:  *
> 643:  * @implSpec The default implementation checks that {@code bound} is 
> a
> 644:  * positive int. Then invokes {@code nextInt()}, limiting the result 
> to

I think if you are refering to the Java type it should be in {@ code xxx }.
"int" - > {@ code int}.  Or use "integer".
Ditto "ints", "long", and "longs" in the overridden methods below.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 674:

> 672:  * If {@code bound} is a power of two then limiting is a simple 
> masking
> 673:  * operation. Otherwise, the result is re-calculated  by invoking
> 674:  * {@code nextInt()} until the result is greater equal {@code origin}

greater equal -> greater than or equal
and in the overloads below (and above)

-

PR: https://git.openjdk.java.net/jdk17/pull/151

Integrated: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.
> 
> Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

This pull request has now been integrated.

Changeset: e9b2c058
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/e9b2c058a4ed5de29b991360f78fc1c5263c9268
Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod

8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

Reviewed-by: lancea, naoto

-

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

Re: [jdk17] RFR: JDK-8269426: Move tests from test/jdk/java/lang/invoke/t8150782 to its parent directory

[Jorn Vernee]

On Mon, 28 Jun 2021 18:56:24 GMT, Mandy Chung  wrote:

> That's a fair point. I personally prefer using a descriptive directory name 
> rather than bug ID since that can give the reader what the tests are intended 
> for. What about renaming t8150782 to accessClassAndFindClass?

Me too! That sounds like an excellent idea :)

-

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Lance Andersen]

On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording.

src/java.base/share/classes/java/util/Locale.java line 460:

> 458:  * back to that of before Java SE 17. If the system property is set to
> 459:  * {@code true}, those three current language codes are mapped to their
> 460:  * backward compatible forms. It is only read at Java runtime startup, 
> so a

I had thought about some of some minor word smithing in your prior commit, but 
chose not to request a change.

In the above sentence,  It could be clearer what "It" is.  Perhaps something 
along the lines of:

This property is only read at Java runtime startup  and subsequents calls to 
"

-

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: JDK-8269426: Move tests from test/jdk/java/lang/invoke/t8150782 to its parent directory

[Mandy Chung]

On Mon, 28 Jun 2021 16:09:36 GMT, Mandy Chung  wrote:

> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
> There are several tests under `` directory.  As the tests under 
> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
> the parent directory.

That's a fair point.  I personally prefer using a descriptive directory name 
rather than bug ID since that can give the reader what the tests are intended 
for.   What about renaming `t8150782` to `accessClassAndFindClass`?

-

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant [v2]

[Jorn Vernee]

On Mon, 28 Jun 2021 14:52:41 GMT, Christoph Göttschkes  wrote:

>> Hi,
>> 
>> please review this small fix. The test case uses a custom launcher and 
>> before launching the JVM, it adds the "lib" and "lib/server" directories to 
>> the environment variable which controls the native library search path. For 
>> non server variants, the second directory is not called "lib/server", but 
>> "lib/client", for instance.
>> 
>> I changed the test case to use the utility methods in `Platform` to get the 
>> correct paths, dependent on the VM variant.
>
> Christoph Göttschkes has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Renames serverDir to vmDir.

> I only have a linux setup, so I tested this with a client and a server VM 
> only on linux.

The test also passes on my Windows machine with a server VM.

-

Marked as reviewed by jvernee (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Brian Burkhalter]

On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato  wrote:

>> Please review this small doc change to the system property. Accompanying CSR 
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording.

Looks fine.

-

Marked as reviewed by bpb (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Lance Andersen]

On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.
> 
> Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

The revisions you made as part of the push to JDK 18 look fine Max.

-

Marked as reviewed by lancea (Reviewer).

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

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]

[Naoto Sato]

> Please review this small doc change to the system property. Accompanying CSR 
> has also been created.

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

  Refined wording.

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/163/files
  - new: https://git.openjdk.java.net/jdk17/pull/163/files/92a8bfc6..3d25418c

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

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

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v4]

[Jorn Vernee]

On Wed, 23 Jun 2021 19:21:02 GMT, Roger Riggs  wrote:

>> Add java.util.Objects.newIdentity to supply a unique object with identity.
>> This is a replacement code can be used today for the traditional new 
>> Object() idiom, which will be deprecated under Project Valhalla.
>> Refer to [JEP 401: Primitive Objects 
>> (Preview)](https://openjdk.java.net/jeps/401) for background.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated spec of Objects.newIdentity with:
>   "The class does not override any of the methods of {@code 
> java.lang.Object}."

test/jdk/java/util/Objects/BasicObjectsTest.java line 48:

> 46: errors += testNonNull();
> 47: errors += testNonNullOf();
> 48: errors += testNewIdentity();

The javadoc of `Objects::newIdentity` claims `[the returned object] can be used 
for synchronization`. Would it be useful to add a test that tries to 
synchronize on the result of `Objects.newIdentity` for that as well?

-

PR: https://git.openjdk.java.net/jdk17/pull/112

Re: [jdk17] RFR: 8268566: java/foreign/TestResourceScope.java timed out

[Jorn Vernee]

On Mon, 14 Jun 2021 15:42:03 GMT, Maurizio Cimadamore  
wrote:

> This patch contains another minor tweak to TestResourceScope as we have seen 
> this test timing out at least once (on arm64).
> 
> I realized that some of the logic recently introduced in the test could lead 
> to the test waiting forever: for instance, if all threads are executed right 
> away before the main thread gets a chance to do anything, we'd be in a state 
> where the lock count is zero, and no other thread will update it. I've 
> removed the while loop - as that's not essential to make sure that the test 
> works (what we want is to trigger a close operation that is concurrent with 
> respect some acquire/release).
> 
> I've also lowered the number of threads - which was using 10K - that was good 
> for stress testing, but it's not a good number in more realistic conditions.

LGTM

-

Marked as reviewed by jvernee (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/45

Re: [jdk17] RFR: JDK-8269426: Move tests from test/jdk/java/lang/invoke/t8150782 to its parent directory

[Jorn Vernee]

On Mon, 28 Jun 2021 16:09:36 GMT, Mandy Chung  wrote:

> `test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
> There are several tests under `` directory.  As the tests under 
> `test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
> `Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to 
> the parent directory.

Hi Mandy,

It looks like the test in question has some test support files associated with 
it (p/Foo.java & q/Bar.java). I personally think it's nice if tests like these 
are in a separate directory, so that it's easier to see which test support 
files belong to which test (and to avoid potential name clashes when 2 tests 
want to use a support file with the same name).

How would you feel about dropping just the `t` prefix from the test directory & 
package names? Then it would match the existing tests in `` directories.

-

PR: https://git.openjdk.java.net/jdk17/pull/162

RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

More refactoring to limit the scope of `@SuppressWarnings` annotations.

Sometimes I introduce new methods. Please feel free to suggest method names you 
like to use.

Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

-

Commit messages:
 - copy all code change from jdk17

Changes: https://git.openjdk.java.net/jdk/pull/4615/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4615=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269409
  Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4615.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4615/head:pull/4615

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

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant [v2]

[Mandy Chung]

On Mon, 28 Jun 2021 14:52:41 GMT, Christoph Göttschkes  wrote:

>> Hi,
>> 
>> please review this small fix. The test case uses a custom launcher and 
>> before launching the JVM, it adds the "lib" and "lib/server" directories to 
>> the environment variable which controls the native library search path. For 
>> non server variants, the second directory is not called "lib/server", but 
>> "lib/client", for instance.
>> 
>> I changed the test case to use the utility methods in `Platform` to get the 
>> correct paths, dependent on the VM variant.
>
> Christoph Göttschkes has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Renames serverDir to vmDir.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v2]

[Jim Laskey]

On Mon, 28 Jun 2021 15:57:21 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/util/random/RandomGenerator.java line 648:
>> 
>>> 646:  * power of two then limiting is a simple masking operation. 
>>> Otherwise, a
>>> 647:  * new result is re-calculated by averaging the previous result and
>>> 648:  * {@code nextInt()} until the final result is greater equal zero 
>>> and less
>> 
>> I don't see how 'averaging' comes from the invocation of boundedNextInt 
>> which appears to choose the first candidate that meets the criteria.
>> The comment also applies to nextLong overloads.
>
> If the first value calculated doesn't meet the criteria, then you either mask 
> (power of 2) or go into this loop which does the averaging. The intent is not 
> to bias the outcome.
> 
> 
> for (long u = r >>> 1;// ensure nonnegative
>  u + m - (r = u % n) < 0L;// rejection check
>  u = rng.nextLong() >>> 1) // retry
> ;

Yea you're right. I was focused on the divide by two, which appears to be to 
just remove the sign. Clearer interpretation:


r = rng.nextLong();
n = range;
m = n - 1;   // mask
for (long u = r / 2; // ensure nonnegative
 u + m < r;  // rejection check
 u = rng.nextLong() / 2) // retry
r = u % n;

-

PR: https://git.openjdk.java.net/jdk17/pull/151

Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v2]

[Jim Laskey]

> The wording of the @implSpec referred to internal methods in the description. 
> The patch rewords the @implSpec to be more descriptive of the algorithm than 
> the methods used.

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Not averaging

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/151/files
  - new: https://git.openjdk.java.net/jdk17/pull/151/files/3b8e19be..bfb61c8d

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

  Stats: 14 lines in 1 file changed: 0 ins; 2 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/151.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/151/head:pull/151

PR: https://git.openjdk.java.net/jdk17/pull/151

[jdk17] Withdrawn: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property

[Lance Andersen]

On Mon, 28 Jun 2021 16:57:15 GMT, Naoto Sato  wrote:

> Please review this small doc change to the system property. Accompanying CSR 
> has also been created.

Looks good Naoto

-

Marked as reviewed by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/163

Re: RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v5]

[Brian Burkhalter]

> Modify `java.io.ByteArrayInputStream` methods `read(byte[])` and 
> `read(byte[],int,int)` to return zero per the `InputStream` specification 
> when the byte array actual or specified length is zero.

Brian Burkhalter has updated the pull request incrementally with two additional 
commits since the last revision:

 - 6766844: Move API note of read(byte[],int,int) to normative text
 - 6766844: Change "equivalent" to "overridden"

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4591/files
  - new: https://git.openjdk.java.net/jdk/pull/4591/files/a53849a2..5435fe81

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4591=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4591=03-04

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

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

Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC

[Jorn Vernee]

On Fri, 25 Jun 2021 17:38:32 GMT, Jorn Vernee  wrote:

> This patch rewrites the prologue and epilogue of panama upcalls, in order to 
> fix the test failure from the title.
> 
> Previously, we did a call to potentially attach the current thread to the VM, 
> and then afterwards did the same suspend and stack reguard checks that we do 
> on the back-edge of a native downcall. Then, on the back edge of the upcall 
> we did another conditional call to detach the thread.
> 
> I've changed these 2 calls to mimic what is done by JavaCallWrapper instead 
> (with attach and detach included), and removed the old suspend and stack 
> reguard checks (now handled by the call).
> 
> FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code. This 
> is now written in C++. Also, MacroAssembler code was added to save/restore 
> the result of the upcall around the call on the back-edge, which was 
> previously missing. Since the new code allocates a handle block as well, I've 
> added handling for those oops to frame & OptimizedUpcallBlob.
> 
> Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3

src/hotspot/cpu/arm/frame_arm.cpp line 320:

> 318:   return nullptr;
> 319: }
> 320: 

FWIW, stubs were missing on some platforms, and this seems to have been fine 
before, but now started causing compilation errors, so I've added them here.

src/hotspot/share/runtime/safepoint.cpp line 931:

> 929: // See if return type is an oop.
> 930: bool return_oop = nm->method()->is_returning_oop();
> 931: HandleMark hm(self);

I was seeing an `assert(_handle_mark_nesting > 1) failed: memory leak: 
allocating handle outside HandleMark` when the existing code allocates the 
Handle for `return_value` in the code down below. It seems to be a simple case 
of a missing handle mark, so I've added it here. (looking at the stack trace in 
the error log, the caller frame is native code, so I don't think we can expect 
the caller to have a HandleMark).

src/hotspot/share/runtime/thread.cpp line 1974:

> 1972:   assert(deferred_card_mark().is_empty(), "Should be empty during GC");
> 1973: 
> 1974:   // Traverse the GCHandles

I reduced some duplication here while debugging, but this change is not 
strictly needed (though a nice cleanup, IMO). Let me know, and I could remove 
this part if preferred.

test/jdk/java/foreign/stackwalk/TestAsyncStackWalk.java line 78:

> 76:  *   TestAsyncStackWalk
> 77:  */
> 78:  

Suggestion:

test/jdk/java/foreign/stackwalk/TestStackWalk.java line 78:

> 76:  *   TestStackWalk
> 77:  */
> 78:  

Suggestion:

-

PR: https://git.openjdk.java.net/jdk17/pull/149

[jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC

[Jorn Vernee]

This patch rewrites the prologue and epilogue of panama upcalls, in order to 
fix the test failure from the title.

Previously, we did a call to potentially attach the current thread to the VM, 
and then afterwards did the same suspend and stack reguard checks that we do on 
the back-edge of a native downcall. Then, on the back edge of the upcall we did 
another conditional call to detach the thread.

I've changed these 2 calls to mimic what is done by JavaCallWrapper instead 
(with attach and detach included), and removed the old suspend and stack 
reguard checks (now handled by the call).

FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code. This 
is now written in C++. Also, MacroAssembler code was added to save/restore the 
result of the upcall around the call on the back-edge, which was previously 
missing. Since the new code allocates a handle block as well, I've added 
handling for those oops to frame & OptimizedUpcallBlob.

Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3

-

Commit messages:
 - Remove whitespace
 - Add Shenandoah test case as well
 - Add zgc requires tags
 - Typo in MACOS_AARCH64_ONLY part
 - Polish
 - Some more build fixes
 - build fixes
 - re-write upcall code to mimic JavaCallWrapper

Changes: https://git.openjdk.java.net/jdk17/pull/149/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=149=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269240
  Stats: 516 lines in 18 files changed: 338 ins; 147 del; 31 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/149.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/149/head:pull/149

PR: https://git.openjdk.java.net/jdk17/pull/149

[jdk17] Integrated: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt

[Daniel D . Daugherty]

On Mon, 28 Jun 2021 17:05:49 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java 
> from ProblemList.txt

This pull request has now been integrated.

Changeset: 20640a57
Author:Daniel D. Daugherty 
URL:   
https://git.openjdk.java.net/jdk17/commit/20640a57f3a352a046006d4795afa4a64f4dc92d
Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod

8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from 
ProblemList.txt

Reviewed-by: iignatyev, tschatzl

-

PR: https://git.openjdk.java.net/jdk17/pull/164

Re: [jdk17] RFR: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt

[Daniel D . Daugherty]

On Mon, 28 Jun 2021 17:15:23 GMT, Igor Ignatyev  wrote:

>> A trivial fix to remove 
>> java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt
>
> Marked as reviewed by iignatyev (Reviewer).

@iignatev and @tschatzl - Thanks for the fast reviews.

-

PR: https://git.openjdk.java.net/jdk17/pull/164

Re: [jdk17] RFR: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt

[Igor Ignatyev]

On Mon, 28 Jun 2021 17:05:49 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java 
> from ProblemList.txt

Marked as reviewed by iignatyev (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/164

Re: [jdk17] RFR: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt

[Thomas Schatzl]

On Mon, 28 Jun 2021 17:05:49 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java 
> from ProblemList.txt

Lgtm and trivial.

-

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/164

[jdk17] RFR: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt

[Daniel D . Daugherty]

A trivial fix to remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java 
from ProblemList.txt

-

Commit messages:
 - 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from 
ProblemList.txt

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

PR: https://git.openjdk.java.net/jdk17/pull/164

[jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property

[Naoto Sato]

Please review this small doc change to the system property. Accompanying CSR 
has also been created.

-

Commit messages:
 - 8269513: Clarify the spec wrt `useOldISOCodes` system property

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

PR: https://git.openjdk.java.net/jdk17/pull/163

[jdk17] Integrated: 8269403: Fix jpackage tests to gracefully handle jpackage app launcher crashes

[Alexey Semenyuk]

On Fri, 25 Jun 2021 22:41:10 GMT, Alexey Semenyuk  wrote:

> jpackage app launcher randomly crashes JVM at termination in EMS enabled JDK 
> builds.
> Until the root cause of the issue is understood and fixed let's add a 
> workaround to jpackage tests to run test app few more times if the first 
> attempt resulted in app crash.

This pull request has now been integrated.

Changeset: efe8423d
Author:Alexey Semenyuk 
URL:   
https://git.openjdk.java.net/jdk17/commit/efe8423d8c584f793e05128e7e69feede382b3e7
Stats: 9 lines in 2 files changed: 5 ins; 0 del; 4 mod

8269403: Fix jpackage tests to gracefully handle jpackage app launcher crashes

Reviewed-by: almatvee, herrick

-

PR: https://git.openjdk.java.net/jdk17/pull/153

[jdk17] RFR: JDK-8269426: Move tests from test/jdk/java/lang/invoke/t8150782 to its parent directory

[Mandy Chung]

`test/jdk/java/lang/invoke/t8150782` is the only one using that convention. 
There are several tests under `` directory.  As the tests under 
`test/jdk/java/lang/invoke/t8150782` are mostly unit tests for 
`Lookup::accessClass` and `Lookup::findClass`, they can simply be moved to the 
parent directory.

-

Commit messages:
 - JDK-8269426:  Move tests from test/jdk/java/lang/invoke/t8150782 to its 
parent directory

Changes: https://git.openjdk.java.net/jdk17/pull/162/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=162=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269426
  Stats: 9 lines in 6 files changed: 0 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/162.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/162/head:pull/162

PR: https://git.openjdk.java.net/jdk17/pull/162

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang  wrote:

>> More refactoring to limit the scope of `@SuppressWarnings` annotations.
>> 
>> Sometimes I introduce new methods. Please feel free to suggest method names 
>> you like to use.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   one more

I'm going to move this to jdk18.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant

[Christoph Göttschkes]

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph Göttschkes  wrote:

> Hi,
> 
> please review this small fix. The test case uses a custom launcher and before 
> launching the JVM, it adds the "lib" and "lib/server" directories to the 
> environment variable which controls the native library search path. For non 
> server variants, the second directory is not called "lib/server", but 
> "lib/client", for instance.
> 
> I changed the test case to use the utility methods in `Platform` to get the 
> correct paths, dependent on the VM variant.

Thanks for the reviews.

> assuming the Platform utility methods do as would be expected

Yes, the utility method does the switch between "bin" and "lib" for windows and 
non-windows

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L346-L350

and it does the switch for the variant

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L361-L369

I only have a linux setup, so I tested this with a client and a server VM only 
on linux.
Sorry, I didn't enable github workflows for my jdk17 fork yet. I did so now and 
hopefully the next commit will trigger it.

-

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: JDK-8266313 (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes

[Jim Laskey]

On Mon, 28 Jun 2021 15:26:45 GMT, Roger Riggs  wrote:

>> The wording of the @implSpec referred to internal methods in the 
>> description. The patch rewords the @implSpec to be more descriptive of the 
>> algorithm than the methods used.
>
> src/java.base/share/classes/java/util/random/RandomGenerator.java line 648:
> 
>> 646:  * power of two then limiting is a simple masking operation. 
>> Otherwise, a
>> 647:  * new result is re-calculated by averaging the previous result and
>> 648:  * {@code nextInt()} until the final result is greater equal zero 
>> and less
> 
> I don't see how 'averaging' comes from the invocation of boundedNextInt which 
> appears to choose the first candidate that meets the criteria.
> The comment also applies to nextLong overloads.

If the first value calculated doesn't meet the criteria, then you either mask 
(power of 2) or go into this loop with does the averaging. The intent is not to 
bias the outcome.


for (long u = r >>> 1;// ensure nonnegative
 u + m - (r = u % n) < 0L;// rejection check
 u = rng.nextLong() >>> 1) // retry
;

-

PR: https://git.openjdk.java.net/jdk17/pull/151

Re: [jdk17] RFR: JDK-8266313 (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes

[Roger Riggs]

On Fri, 25 Jun 2021 18:53:59 GMT, Jim Laskey  wrote:

> The wording of the @implSpec referred to internal methods in the description. 
> The patch rewords the @implSpec to be more descriptive of the algorithm than 
> the methods used.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 648:

> 646:  * power of two then limiting is a simple masking operation. 
> Otherwise, a
> 647:  * new result is re-calculated by averaging the previous result and
> 648:  * {@code nextInt()} until the final result is greater equal zero 
> and less

I don't see how 'averaging' comes from the invocation of boundedNextInt which 
appears to choose the first candidate that meets the criteria.
The comment also applies to nextLong overloads.

-

PR: https://git.openjdk.java.net/jdk17/pull/151

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant [v2]

[Christoph Göttschkes]

On Mon, 28 Jun 2021 13:44:41 GMT, David Holmes  wrote:

>> Christoph Göttschkes has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Renames serverDir to vmDir.
>
> test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java line 53:
> 
>> 51: 
>> 52: String libDir = Platform.libDir().toString();
>> 53: String serverDir = Platform.jvmLibDir().toString();
> 
> Perhaps `vmDir` would be a more suitable name?

Makes sense, thank you.

-

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant [v2]

[Christoph Göttschkes]

> Hi,
> 
> please review this small fix. The test case uses a custom launcher and before 
> launching the JVM, it adds the "lib" and "lib/server" directories to the 
> environment variable which controls the native library search path. For non 
> server variants, the second directory is not called "lib/server", but 
> "lib/client", for instance.
> 
> I changed the test case to use the utility methods in `Platform` to get the 
> correct paths, dependent on the VM variant.

Christoph Göttschkes has updated the pull request incrementally with one 
additional commit since the last revision:

  Renames serverDir to vmDir.

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/159/files
  - new: https://git.openjdk.java.net/jdk17/pull/159/files/84b23542..71e0e5b6

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

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

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant

[Thomas Stuefe]

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph Göttschkes  wrote:

> Hi,
> 
> please review this small fix. The test case uses a custom launcher and before 
> launching the JVM, it adds the "lib" and "lib/server" directories to the 
> environment variable which controls the native library search path. For non 
> server variants, the second directory is not called "lib/server", but 
> "lib/client", for instance.
> 
> I changed the test case to use the utility methods in `Platform` to get the 
> correct paths, dependent on the VM variant.

Looks reasonable (Davids suggestion sounds good).

..Thomas

-

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Mon, 28 Jun 2021 12:20:38 GMT, Daniel Fuchs  wrote:

>> This cast is only to tell the compiler which overloaded method to call, and 
>> I don't think there will be a real cast at runtime. It might look a little 
>> ugly but extracting it into a variable declaration/definition plus a new 
>> `initStatic` method seems not worth doing, IMHO.
>
> Why not simply declare a local variable in the static initializer below?
> 
> 
> private static final long CURRENT_PID;
> private static final boolean ALLOW_ATTACH_SELF;
> static {
> PrivilegedAction pa = ProcessHandle::current;
> @SuppressWarnings("removal")
> long pid = AccessController.doPrivileged(pa).pid();
> CURRENT_PID = pid;
> String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
> ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
> }

I've just pushed a commit with a different fix:

private static final long CURRENT_PID = pid();

@SuppressWarnings("removal")
private static long pid() {
PrivilegedAction pa = () -> ProcessHandle.current();
return AccessController.doPrivileged(pa).pid();
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v3]

[Weijun Wang]

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

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

  one more refinement

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/152/files
  - new: https://git.openjdk.java.net/jdk17/pull/152/files/774eb9da..2e4a8ba7

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

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

PR: https://git.openjdk.java.net/jdk17/pull/152

[jdk17] Integrated: JDK-8268236 The documentation of the String.regionMatches method contains error

[Jim Laskey]

On Mon, 28 Jun 2021 12:28:06 GMT, Jim Laskey  wrote:

> The documentation of the String.regionMatches method contains this fragment: 
> this.substring(toffset, len).codePoints(), but String.substring method takes 
> index, not the length, as the second argument.

This pull request has now been integrated.

Changeset: e4c54463
Author:Jim Laskey 
URL:   
https://git.openjdk.java.net/jdk17/commit/e4c5446340605b112e0918fa9dcb48aaeaa730c8
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod

8268236: The documentation of the String.regionMatches method contains error

Reviewed-by: rriggs, dfuchs

-

PR: https://git.openjdk.java.net/jdk17/pull/157

Re: [jdk17] RFR: 8269486: CallerAccessTest fails for non server variant

[David Holmes]

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph Göttschkes  wrote:

> Hi,
> 
> please review this small fix. The test case uses a custom launcher and before 
> launching the JVM, it adds the "lib" and "lib/server" directories to the 
> environment variable which controls the native library search path. For non 
> server variants, the second directory is not called "lib/server", but 
> "lib/client", for instance.
> 
> I changed the test case to use the utility methods in `Platform` to get the 
> correct paths, dependent on the VM variant.

This generally seems fine to me (assuming the Platform utility methods do as 
would be expected).

One suggestion below.

Thanks,
David

test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java line 53:

> 51: 
> 52: String libDir = Platform.libDir().toString();
> 53: String serverDir = Platform.jvmLibDir().toString();

Perhaps `vmDir` would be a more suitable name?

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder

[David Holmes]

On Mon, 28 Jun 2021 03:25:09 GMT, Yi Yang  wrote:

>> Prefer using ByteOrder to compute byte order for StringUTF16 to determining 
>> byte order by native method StringUTF16.isBigEndian.
>
> Hi Aleksey, do you have a concrete issue/discussion about bootstrapping 
> problems? I don't see it because I can build JDK and passes tier1 tests w/o 
> problems. But I admit this may cause potential problems. In order to reduce 
> potential risks, how about changing the class initialization orders, i.e. 
> initialize UUnsafeConstants_klas earlier, which seems reasonable:
> 
> 
> void Threads::initialize_java_lang_classes(JavaThread* main_thread, TRAPS) {
>   TraceTime timer("Initialize java.lang classes", TRACETIME_LOG(Info, 
> startuptime));
> 
>   if (EagerXrunInit && Arguments::init_libraries_at_startup()) {
> create_vm_init_libraries();
>   }
> 
> +#ifdef ASSERT
> +  InstanceKlass *k = vmClasses::UnsafeConstants_klass();
> +  assert(k->is_not_initialized(), "UnsafeConstants should not already be 
> initialized");
> +#endif
> +
> +  // initialize the hardware-specific constants needed by Unsafe
> +  initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), CHECK);
> +  jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
> 
>   initialize_class(vmSymbols::java_lang_String(), CHECK);
> 
>   // Inject CompactStrings value after the static initializers for String ran.
>   java_lang_String::set_compact_strings(CompactStrings);
>   ...

@kelthuzadx , if you want to discuss initialization order changes please take 
this to hotspot-runtime-dev. Simply building okay and running a few tests can 
never validate a change in VM initialization order - there are a lot of 
subtleties involved, platform differences, affect of different flags, etc etc. 
Thanks @adinn for the details on this specific proposal - I agree that we 
neither want to make UnsafeConstants unable to use String, nor expose it for 
use by String.

Backing up a step what is the motivation for change here? Is there something 
wrong with the way String currently does this? Or is it just that it seemed on 
the surface that you could skip the native call and "just use Byteorder"?

-

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

Re: [jdk17] RFR: JDK-8268236 The documentation of the String.regionMatches method contains error

[Daniel Fuchs]

On Mon, 28 Jun 2021 12:28:06 GMT, Jim Laskey  wrote:

> The documentation of the String.regionMatches method contains this fragment: 
> this.substring(toffset, len).codePoints(), but String.substring method takes 
> index, not the length, as the second argument.

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/157

Re: [jdk17] RFR: JDK-8268236 The documentation of the String.regionMatches method contains error

[Roger Riggs]

On Mon, 28 Jun 2021 12:28:06 GMT, Jim Laskey  wrote:

> The documentation of the String.regionMatches method contains this fragment: 
> this.substring(toffset, len).codePoints(), but String.substring method takes 
> index, not the length, as the second argument.

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/157

[jdk17] RFR: 8269486: CallerAccessTest fails for non server variant

[Christoph Göttschkes]

Hi,

please review this small fix. The test case uses a custom launcher and before 
launching the JVM, it adds the "lib" and "lib/server" directories to the 
environment variable which controls the native library search path. For non 
server variants, the second directory is not called "lib/server", but 
"lib/client", for instance.

I changed the test case to use the utility methods in `Platform` to get the 
correct paths, dependent on the VM variant.

-

Commit messages:
 - 8269486: CallerAccessTest fails for non server variant

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

PR: https://git.openjdk.java.net/jdk17/pull/159

Re: [jdk17] RFR: 8269403: Fix jpackage tests to gracefully handle jpackage app launcher crashes

[Andy Herrick]

On Fri, 25 Jun 2021 22:41:10 GMT, Alexey Semenyuk  wrote:

> jpackage app launcher randomly crashes JVM at termination in EMS enabled JDK 
> builds.
> Until the root cause of the issue is understood and fixed let's add a 
> workaround to jpackage tests to run test app few more times if the first 
> attempt resulted in app crash.

Marked as reviewed by herrick (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/153

[jdk17] RFR: JDK-8268236 The documentation of the String.regionMatches method contains error

[Jim Laskey]

The documentation of the String.regionMatches method contains this fragment: 
this.substring(toffset, len).codePoints(), but String.substring method takes 
index, not the length, as the second argument.

-

Commit messages:
 - Update javadoc

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

PR: https://git.openjdk.java.net/jdk17/pull/157

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Daniel Fuchs]

On Sat, 26 Jun 2021 23:55:46 GMT, Weijun Wang  wrote:

>> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java 
>> line 53:
>> 
>>> 51: private static final long CURRENT_PID = 
>>> AccessController.doPrivileged(
>>> 52: (PrivilegedAction) 
>>> ProcessHandle::current).pid();
>>> 53: 
>> 
>> The original code separated out the declaration of the PrivilegedAction to 
>> avoid this cast. If you move the code from the original static initializer 
>> into a static method that it called from initializer then it might provide 
>> you with a cleaner way to refactor this. There are several other places in 
>> this patch that could do with similar cleanup.
>
> This cast is only to tell the compiler which overloaded method to call, and I 
> don't think there will be a real cast at runtime. It might look a little ugly 
> but extracting it into a variable declaration/definition plus a new 
> `initStatic` method seems not worth doing, IMHO.

Why not simply declare a local variable in the static initializer below?


private static final long CURRENT_PID;
private static final boolean ALLOW_ATTACH_SELF;
static {
PrivilegedAction pa = ProcessHandle::current;
@SuppressWarnings("removal")
long pid = AccessController.doPrivileged(pa).pid();
CURRENT_PID = pid;
String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder

[Andrew Dinn]

On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang  wrote:

> Prefer using ByteOrder to compute byte order for StringUTF16 to determining 
> byte order by native method StringUTF16.isBigEndian.

Hi Yi Yang,

This is more complex than it seems. The general policy is not to change boot 
order if at all possible and for good reason. there are many ways it can cause 
unexpected consequences.

Regarding this specific case, UnsafeConstants exists in order to allow class 
Unsafe to define a few final fields derived from runtime environment/hardware 
parameters provided as final static *primitive* fields of UnsafeConstants. The 
relevant fields in UnsafeConstants are injected by the JVM after they have been 
initialized to dummy values by the class's  method. So, the purpose of 
the class is simply to pre-cache these runtime/hardware values as Java state, 
avoiding the need for Unsafe to call out to the JVM using native code. Before 
UnsafeConstants was defined these constants used to be retrieved using native 
callouts.

What you are asking for is to make String use the same pre-cache as Unsafe i.e. 
to expand the set of classes which depend on UnsafeConstants to include String. 
While that might work as currently defined it is not clear that it will always 
continue to work in the future. That's because String is a rather special case, 
much like a primitive class. Instances of String are created to represent 
literal terms in the language. Note that these String terms are themselves 
*constants*.

UnsafeConstants is not meant to have any other uses except to cache the 
runtime-specific/hardware constants used by Unsafe. So, it ought not to depend 
on other classes. However, it is also important that no other classes depend on 
it and that includes String. Now imagine someone were to update UnsafeConstants 
to include an injected String constant -- say, the current setting for LC_LANG, 
or the processor CPU name or some other legitimate text value derived form the 
runtime or hardware. Your change would cause a problem because initialization 
of UnsafeConstants would require String already to be initialized.

Of course, neither of those examples is a likely candidate for inclusion in 
UnsafeConstants but it is hard to say whether other more realistic cases might 
arise in future.

-

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

Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v2]

[Jaikiran Pai]

> Can I please get a review for this proposed fix for the issue reported in 
> https://bugs.openjdk.java.net/browse/JDK-8190753?
> 
> The commit here checks for the size of the zip entry before trying to create 
> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been 
> included in this commit to reproduce the issue and verify the fix.
> 
> P.S: It's still a bit arguable whether it's a good idea to create the 
> `ByteArrayOutputStream` with an initial size potentially as large as the 
> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default 
> value. However, I think that can be addressed separately while dealing with 
> https://bugs.openjdk.java.net/browse/JDK-8011146

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

  add @requires to the new test to selectively decide where the test runs

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4607/files
  - new: https://git.openjdk.java.net/jdk/pull/4607/files/8d6f3407..127acbcc

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

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

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

Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

[Jaikiran Pai]

Hello Alan,

On 28/06/21 1:00 pm, Alan Bateman wrote:


I didn't study the test too closely but just to mention that tests with zip 
entries > 2GB can be problematic to test. The test will probably need the 
@requires tag to limit it to 64-bit systems and maybe some minimum memory size. It 
may also need testing on a wide range of systems to get some idea of run time. 
Test machines with spinning rust (HDDs) come to mind.

That's a good point and I had completely overlooked it. There's an 
existing test test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java 
(unrelated to this issue) which uses a 5GB sized entry in the zips. In 
fact, the idea of creating the zip entry in this manner was borrowed 
from there. That one doesn't have any @requires for it. However, taking 
a closer look at that existing test, that one just creates these large 
entries but never loads (nor tries to load) those entries into memory 
and probably explains why it doesn't need special care when it comes to 
running the test.


I'll take a look at some other existing tests to see what kind of 
@requires I can add here to make it a bit more selective on where it 
gets run. Thank you for that input.


-Jaikiran

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

> I am a bit lost currently. So what actually do I have to do next? You want 
> _me_ to write these tests, or shall I just refactor my implementation and 
> _you_ will provide these tests (I would prefer that, actually)?

The existing test for InputStream.transferTo mostly exercises the default 
implementation. There are overrides in other input stream implementation but 
the test coverage appears to be spotty. This PR adds an overwrite with four 
implementations and it doesn't appear that these are exercised by any tests. So 
yes, tests are needed. This PR, or another PR that is integrated in advance, 
either is fine.

-

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

Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream

[Alan Bateman]

On Mon, 28 Jun 2021 03:41:20 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this proposed fix for the issue reported in 
> https://bugs.openjdk.java.net/browse/JDK-8190753?
> 
> The commit here checks for the size of the zip entry before trying to create 
> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been 
> included in this commit to reproduce the issue and verify the fix.
> 
> P.S: It's still a bit arguable whether it's a good idea to create the 
> `ByteArrayOutputStream` with an initial size potentially as large as the 
> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default 
> value. However, I think that can be addressed separately while dealing with 
> https://bugs.openjdk.java.net/browse/JDK-8011146

This may be just moving the problem because writing to the BAOS will fail when 
the deflated size is too large to fit in a byte array. The zip provider can use 
a temporary file so maybe it should use that when appending to existing zip 
entries that are larger than some threshold. At some point we may need deeper 
changes here, e.g. start out with a BAOS and spill over to a temporary file 
when the deflated size reaches some threshold.

I didn't study the test too closely but just to mention that tests with zip 
entries > 2GB can be problematic to test. The test will probably need the 
@requires tag to limit it to 64-bit systems and maybe some minimum memory size. 
It may also need testing on a wide range of systems to get some idea of run 
time. Test machines with spinning rust (HDDs) come to mind.

-

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

Re: RFR: 8260621: Avoid memory leak in ImageBufferCache [v2]

[Alan Bateman]

On Wed, 3 Feb 2021 01:29:02 GMT, Bo Zhang 
 wrote:

>> Previously, `ImageBufferCache` contains a ThreadLocal field which holds
>> strong reference to `ImageBufferCache$BufferReference.class`. When loaded
>> from `jrt-fs.jar`, this will keep `JrtFileSystemProvider$JrtFsLoader`
>> in memory forever and never being GCed.
>> 
>> The fix replace the old `ImageBufferCache$BufferReference` class with
>> `WeakReference`, which is verified by provided test.
>
> Bo Zhang has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

This issue was fixed with PR 3849 so I assume PR 2307 can be closed.

-

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

Withdrawn: 8260621: Avoid memory leak in ImageBufferCache

[Bo Zhang]

On Fri, 29 Jan 2021 05:53:57 GMT, Bo Zhang 
 wrote:

> Previously, `ImageBufferCache` contains a ThreadLocal field which holds
> strong reference to `ImageBufferCache$BufferReference.class`. When loaded
> from `jrt-fs.jar`, this will keep `JrtFileSystemProvider$JrtFsLoader`
> in memory forever and never being GCed.
> 
> The fix replace the old `ImageBufferCache$BufferReference` class with
> `WeakReference`, which is verified by provided test.

This pull request has been closed without being integrated.

-

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