RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

[liach]

Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
hash map with a simple lookup, similar to what's done in 
[JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

-

Commit messages:
 - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

Changes: https://git.openjdk.java.net/jdk/pull/8801/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8801=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287064
  Stats: 79 lines in 1 file changed: 14 ins; 41 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8801.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v2]

[liach]

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

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

  remove unused field

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8800/files
  - new: https://git.openjdk.java.net/jdk/pull/8800/files/c1b522d5..64a70479

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

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

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

RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization

[liach]

Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
`Class.forName(String)`. `make test 
TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
`LazyInitializationTest` failing the eager initialization check on the baseline 
and passing with this patch.

On a side note, this might reduce the number of methods that can be encoded in 
a proxy due to code attribute size restrictions; we probably would address that 
in another issue, as we never mandated a count of methods that the proxy must 
be able to implement.

Mandy, would you mind review this?

-

Commit messages:
 - whitespace
 - Copyright year
 - typo
 - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid 
unnecessary class initialization
 - Test for eager initialization

Changes: https://git.openjdk.java.net/jdk/pull/8800/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8800=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285401
  Stats: 78 lines in 2 files changed: 74 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8800.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8800/head:pull/8800

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

Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe

[Michael Hall]

> On May 19, 2022, at 7:14 PM, Michael Hall  wrote:
> 
> 
> 
>> 
>> Alexander
>> 
>>> 
>>> I think it will be a problem to implement this for native launchers.
>> 
> 
> Basically you are telling the developer in 
> https://bugs.openjdk.java.net/browse/JDK-8286122 
>  that it isn’t currently 
> possible for their application to get into the Mac App Store as is.
> I was thinking you could possibly in the comments mention the possibility the 
> developer could provide these in an archive themselves that they manually 
> manage as a workaround. 
> Leave them with a possible path forward to achieve this.
> I have successfully done this for something similar if not identical.
> 

I think it can work for java commands as well. You have to set them executable 
after extraction and point to the embedded jdk lib directory

DYLD_LIBRARY_PATH=JavaCommand.app/Contents/runtime/Contents/Home/lib 
JavaCommand.app/Contents/MacOS/JavaCommand 
java version "18" 2022-03-22
Java(TM) SE Runtime Environment (build 18+36-2087)
Java HotSpot(TM) 64-Bit Server VM (build 18+36-2087, mixed mode)

I can provide the full JavaCommand source code and jpackage invocation if of 
interest. But yes, it works.

From…

String v = rtexec(new String[] { 
Paths.get(dataLoc,"java").toString(),"-version" });
System.out.println(v);

You’d have to figure out how to provide the environment variable on the runtime 
invocation. That I didn’t verify, but it doesn’t seem impossible.
I added that after getting…

JavaCommand.app/Contents/MacOS/JavaCommand
dyld[5725]: Library not loaded: @rpath/libjli.dylib
  Referenced from: /Users/mjh/testLoc/java
  Reason: tried: '/libjli.dylib' (no such file), 
'/Users/mjh/JavaCommand.app/Contents/app/libjli.dylib' (no such file), 
'/Users/mjh/testLoc/./libjli.dylib' (no such file), 
'/Users/mjh/testLoc/../lib/libjli.dylib' (no such file), 
'/Users/mjh/testLoc/./libjli.dylib' (no such file), 
'/Users/mjh/testLoc/../lib/libjli.dylib' (no such file), 
'/usr/lib/libjli.dylib' (no such file)

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v4]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

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

  Update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/502dad82..26bde80b

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

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

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[liach]

On Fri, 20 May 2022 00:08:36 GMT, Mandy Chung  wrote:

> a named module can't have unnamed package

Since this is mandated by the Java Language Specification 7.4.2, I am tempted 
to change the thrown exception to `InternalError`, but I cannot find any 
restriction in the JVM Specification that prevents declaring a package-private 
superinterface in the unnamed package of a named module. If we can find a 
reference in the JVM Specification, I'm more than glad to refer to that in a 
comment and throw `InternalError` instead.

-

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

Withdrawn: 8282178: Replace simple iterations of Map.entrySet with Map.forEach calls

[duke]

On Wed, 23 Feb 2022 22:33:42 GMT, liach  wrote:

> Replaces simple `for (Map.Entry entry : map.entrySet())` with 
> `map.forEach((k, v) ->)` calls. This change is better for thread-safety and 
> reduces allocation for some map implementations.
> 
> A more in-depth description of benefits is available at 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086201.html
>  and at the JBS issue itself.
> 
> A [jmh 
> comparison](https://jmh.morethan.io/?sources=https://gist.githubusercontent.com/liach/0c0f79f0c0b9b78f474d65cda2c5f7b5/raw/4f2a160c51164aefdfac6ab5a19bdbc8c65f5fcf/base-results.json,https://gist.githubusercontent.com/liach/0c0f79f0c0b9b78f474d65cda2c5f7b5/raw/4f2a160c51164aefdfac6ab5a19bdbc8c65f5fcf/head-results.json)
>  on the performance of the existing `HashMapBench` shows that the performance 
> of `putAll` for `HashMap` has not significantly changed.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe

[Michael Hall]

> 
> Alexander
> 
>> 
>> I think it will be a problem to implement this for native launchers.
> 

Basically you are telling the developer in 
https://bugs.openjdk.java.net/browse/JDK-8286122 
 that it isn’t currently 
possible for their application to get into the Mac App Store as is.
I was thinking you could possibly in the comments mention the possibility the 
developer could provide these in an archive themselves that they manually 
manage as a workaround. 
Leave them with a possible path forward to achieve this.
I have successfully done this for something similar if not identical.

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[Mandy Chung]

On Thu, 19 May 2022 23:44:12 GMT, liach  wrote:

>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
>> the interfaces are iterated twice. The two passes can be merged into one, 
>> yielding the whole proxy definition context (module, package, whether 
>> there's package-private interface) when determining the module.
>> 
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a 
>> good cleanup on its own.
>
> liach 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:
> 
>  - Updates suggested by mandy
>  - Merge branch 'master' into fix/proxy-single-pass
>  - Don't need to complexify module cache
>  - 8284942: Proxy building can just iterate superinterfaces once

Looks good with a couple comments.

src/java.base/share/classes/java/lang/reflect/Proxy.java line 509:

> 507: if (!m.isNamed())
> 508: throw new InternalError("unnamed module: " + m);
> 509: } else if (proxyPkg.isEmpty() && m.isNamed()) {

With the refactoring, it may be easier to understand to make this check for 
both public and non-public access (i.e. drop the `else`) for clarity - a named 
module can't have unnamed package.

src/java.base/share/classes/java/lang/reflect/Proxy.java line 534:

> 532:  * Generate the specified proxy class.
> 533:  */
> 534: accessFlags |= Modifier.FINAL;

It can be inlined in the call to `generateProxyClass`:


byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, 
proxyName, interfaces, accessFlags | Modifier.FINAL);

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v7]

[Paul Sandoz]

On Thu, 19 May 2022 21:11:41 GMT, Jatin Bhateja  wrote:

>> Hi All,
>> 
>> Patch adds the planned support for new vector operations and APIs targeted 
>> for [JEP 426: Vector API (Fourth 
>> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
>> 
>> Following is the brief summary of changes:-
>> 
>> 1)  Extends the scope of existing lanewise API for following new vector 
>> operations.
>>-  VectorOperations.BIT_COUNT: counts the number of one-bits
>>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero 
>> bits
>>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing 
>> zero bits
>>- VectorOperations.REVERSE: reversing the order of bits
>>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>>- compress and expand bits: Semantics are based on Hacker's Delight 
>> section 7-4 Compress, or Generalized Extract.
>> 
>> 2)  Adds following new APIs to perform cross lane vector compress and 
>> expansion operations under the influence of a mask.
>>- Vector.compress
>>- Vector.expand 
>>- VectorMask.compress
>> 
>> 3) Adds predicated and non-predicated versions of following new APIs to load 
>> and store the contents of vector from foreign MemorySegments. 
>>   - Vector.fromMemorySegment
>>   - Vector.intoMemorySegment
>> 
>> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support 
>> for each newly added operation.
>> 
>> 
>>  Patch has been regressed over AARCH64 and X86 targets different AVX levels. 
>> 
>>  Kindly review and share your feedback.
>> 
>>  Best Regards,
>>  Jatin
>
> Jatin Bhateja has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Changes to enable jdk.incubator.vector to be treated as preview 
> participant. Code re-organization related to Reverse/ReverseByte IR 
> transforms.
>  - 8284960: Adding --enable-preview in vectorAPI benchmarks.
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Review comments resolution.
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Correcting a typo.
>  - 8284960: Integrating changes from panama-vector (Add @since 19 tags).
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/9f562ef7...311f3233

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 132:

> 130:  * @return true if {@code s} is participating in the preview of 
> {@code previewSymbol}
> 131:  */
> 132: public boolean isPreviewParticipating(Symbol s, Symbol 
> previewSymbol) {

Some feedback from a colleague:
Suggestion:

/**
 * Returns true if {@code s} is deemed to participate in the preview of 
{@code previewSymbol}, and
 * therefore no warnings or errors will be produced.
 *
 * @param s the symbol depending on the preview symbol
 * @param previewSymbol the preview symbol marked with @Preview
 * @return true if {@code s} is participating in the preview of {@code 
previewSymbol}
 */
public boolean participatesInPreview(Symbol s, Symbol previewSymbol) {

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

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

 - Updates suggested by mandy
 - Merge branch 'master' into fix/proxy-single-pass
 - Don't need to complexify module cache
 - 8284942: Proxy building can just iterate superinterfaces once

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/d58bb7e0..502dad82

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

  Stats: 259862 lines in 4149 files changed: 190963 ins; 46360 del; 22539 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Integrated: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character

[Ichiroh Takiguchi]

On Sun, 24 Apr 2022 09:18:54 GMT, Ichiroh Takiguchi  
wrote:

> On JDK19 with Linux ja_JP.eucjp locale,
> System.getenv() returns unexpected value if environment variable has Japanese 
> EUC characters.
> It seems this issue happens because of JEP 400.
> Arguments for ProcessBuilder have same kind of issue.

This pull request has now been integrated.

Changeset: 890771e7
Author:Ichiroh Takiguchi 
URL:   
https://git.openjdk.java.net/jdk/commit/890771e708277c5f7ea9460ff7bcc7e4cae87eab
Stats: 264 lines in 5 files changed: 217 ins; 24 del; 23 mod

8285517: System.getenv() returns unexpected value if environment variable has 
non ASCII character

Reviewed-by: naoto, rriggs

-

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

Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe

[Michael Hall]

> On May 19, 2022, at 5:38 PM, Alexander Matveev  
> wrote:
> 
> Hi Michael,

Alexander

> 
> I think it will be a problem to implement this for native launchers.

Each of the native commands is it’s own app launcher? I didn’t know this. I 
don’t think it was ever really indicated in the discussion why the commands 
needed the embedded plist. I think Apple DTS guessed it had something to do 
with a TCC issue.

> - If we extract native launchers inside installed app bundle it will 
> invalidate signature and most likely will require privileged permissions to 
> write inside app bundle.

I thought maybe jpackage unsigned and resigned the jdk itself. Maybe not. I was 
talking about simply including a jar in input that ends up in the ‘app’ 
directory and the app then has the jar contents extracted from there. No 
additional write permission’s are required. 

> - If we extract to some known and accessible location as you suggesting, then 
> how our launchers will figure out which runtime to use? All other runtime 
> files will be inside app bundle.

Again this possibly my not understanding what additional requirements there are 
for the commands to be ‘launchers’. I was thinking if the embedded plist was 
still a problem the developer could possibly use commands from a compatible 
linux distribution. OS/X and linux are both Unix right? It did occur to me that 
extracting native commands out of a jar might lose the executable posix 
permission. I’m not sure if jar has any builtin meta information to retain 
these permissions or if nio allows you to set executable. I haven’t had to 
address this. 

> - If user deletes application, then how we will cleanup extracted files? Most 
> likely it will require uninstall script in known location which user will 
> need to run in order to cleanup extracted files.

I haven’t tried to address this yet myself for my application. I’m not sure a 
lot of OS/X application’s do. But it would probably be up to the app to provide 
an uninstall of some kind. Some app’s are obviously going to need external data.

> 
> Thanks,
> Alexander
> 

Again I’m suggesting it as a roughed out workaround suggestion for the 
developer. There might be issues they would need to work out and might even be 
problems where it won’t work.
I’m not suggesting it as a fix you would try to implement.

Thanks
Mike

Re: RFR: 8281682: Redundant .ico files in Windows app-image cause unnecessary bloat

[Alexander Matveev]

On Thu, 19 May 2022 19:13:12 GMT, Alexey Semenyuk  wrote:

> 8281682: Redundant .ico files in Windows app-image cause unnecessary bloat

Marked as reviewed by almatvee (Reviewer).

-

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

Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe

[Alexander Matveev]

Hi Michael,

I think it will be a problem to implement this for native launchers.
- If we extract native launchers inside installed app bundle it will invalidate 
signature and most likely will require privileged permissions to write inside 
app bundle.
- If we extract to some known and accessible location as you suggesting, then 
how our launchers will figure out which runtime to use? All other runtime files 
will be inside app bundle.
- If user deletes application, then how we will cleanup extracted files? Most 
likely it will require uninstall script in known location which user will need 
to run in order to cleanup extracted files.

Thanks,
Alexander

On May 18, 2022, at 6:44 PM, Michael Hall 
mailto:mik3h...@gmail.com>> wrote:



On May 11, 2022, at 4:39 PM, Alexander Matveev 
mailto:almat...@openjdk.java.net>> wrote:

- It is not possible to support native JDK commands such as "java" inside Mac 
App Store bundles due to embedded info.plist. Workarounds suggested in 
JDK-8286122 does not seems to be visible.

I was just thinking about this. If you wanted a workaround to suggest to the 
user on the original issue. You could jar the native executables, extract them 
to a known accessible location, and then runtime them.

Having the commands jar’d would get them past the App Store check. Runtime on 
the client machine I doubt would object to the duplicate bundle id’s on 
absolute path execution but I haven’t double checked that. Also avoids issues 
with quarantine xattr’s.
I’ve done something like this a couple times for interfacing an app to other 
languages.

For example…

if (Files.exists(rscriptCmd)) {
isR = true;
System.out.println("InitialFinance: RScript available");
// Where is the finance data directory?
String data = prefs.get("data","N/A");

if (data.equals("N/A")) {
Application app = Application.getApplication();
Path documents = app.getFolder(DataTypes.DOCUMENTS);
dataLoc = Paths.get(documents.toString(),"finance");
}
else {
dataLoc = Paths.get(data);
}
System.out.println("InitialFinance: data location is " + dataLoc);
Path resourceJar = Paths.get(System.getProperty("app.path"),"resource.jar");
System.out.println("InitialFinance: Checking resources for updates");
extractArchive(resourceJar,dataLoc);
}
else {
System.out.println("InitialFinance: " + rscriptCmd + " for " + initialRscript + 
" does not exist");
isR = false;
}

// 
https://stackoverflow.com/questions/1529611/how-to-write-a-java-program-which-can-extract-a-jar-file-and-store-its-data-in-s
public static void extractArchive(Path archiveFile, Path destPath) throws 
IOException {
Files.createDirectories(destPath); // create dest path folder(s)
try (ZipFile archive = new ZipFile(archiveFile.toFile())) {

 @SuppressWarnings("unchecked")
 Enumeration entries = (Enumeration) 
archive.entries();

// copy or create new or updated
 while (entries.hasMoreElements()) {
 ZipEntry entry = entries.nextElement();

 if (!entry.getName().startsWith("finance/") || 
!(entry.getName().length() > 8)) {
 continue;
 }
 String fileName = entry.getName().substring(8);
 FileTime archiveTime = entry.getLastModifiedTime();

Path entryDest = destPath.resolve(fileName);
//if (Files.isDirectory(entryDest)) continue;
//Files.createDirectories(entryDest);
 if (!Files.exists(entryDest)) {
 Files.copy(archive.getInputStream(entry), entryDest, 
StandardCopyOption.REPLACE_EXISTING);
 continue;
 }
BasicFileAttributes destAttr =
Files.readAttributes(entryDest, 
BasicFileAttributes.class);

if (archiveTime.compareTo(destAttr.creationTime()) > 0) {
Files.copy(archive.getInputStream(entry), entryDest, 
StandardCopyOption.REPLACE_EXISTING);
}
}
}
catch (IOException ioex) {
 throw ioex;
}
}

boolean debug = Boolean.getBoolean("R.debug");
rtexec(new String[] { RSCRIPT, script.toString() },debug);

RFR: 8281682: Redundant .ico files in Windows app-image cause unnecessary bloat

[Alexey Semenyuk]

8281682: Redundant .ico files in Windows app-image cause unnecessary bloat

-

Commit messages:
 - Trailing whitespace removed
 - Drop unused WinIconVerifierException
 - remove accidentally added space char
 - jtreg descriptions fixed
 - 8281682: Redundant .ico files in Windows app-image cause unnecessary bloat

Changes: https://git.openjdk.java.net/jdk/pull/8794/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8794=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8281682
  Stats: 169 lines in 8 files changed: 150 ins; 6 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8794.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8794/head:pull/8794

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

Integrated: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable

[Andrey Turbanov]

On Thu, 13 Jan 2022 08:25:22 GMT, Andrey Turbanov  wrote:

> Method `Class.isAssignableFrom` is often used in form of:
> 
> if (clazz.isAssignableFrom(obj.getClass())) {
> Such condition could be simplified to more shorter and performarnt code
> 
> if (clazz.isInstance(obj)) {
> 
> Replacement is equivalent if it's known that `obj != null`.
> In JDK codebase there are many such places.
> 
> I tried to measure performance via JMH
> 
> Class integerClass = Integer.class;
> Class numberClass = Number.class;
> 
> Object integerObject = 45666;
> Object doubleObject = 8777d;
> 
> @Benchmark
> public boolean integerInteger_isInstance() {
> return integerClass.isInstance(integerObject);
> }
> 
> @Benchmark
> public boolean integerInteger_isAssignableFrom() {
> return integerClass.isAssignableFrom(integerObject.getClass());
> }
> 
> @Benchmark
> public boolean integerDouble_isInstance() {
> return integerClass.isInstance(doubleObject);
> }
> 
> @Benchmark
> public boolean integerDouble_isAssignableFrom() {
> return integerClass.isAssignableFrom(doubleObject.getClass());
> }
> 
> @Benchmark
> public boolean numberDouble_isInstance() {
> return numberClass.isInstance(doubleObject);
> }
> 
> @Benchmark
> public boolean numberDouble_isAssignableFrom() {
> return numberClass.isAssignableFrom(doubleObject.getClass());
> }
> 
> @Benchmark
> public boolean numberInteger_isInstance() {
> return numberClass.isInstance(integerObject);
> }
> 
> @Benchmark
> public boolean numberInteger_isAssignableFrom() {
> return numberClass.isAssignableFrom(integerObject.getClass());
> }
> 
> @Benchmark
> public void numberIntegerDouble_isInstance(Blackhole bh) {
> bh.consume(numberClass.isInstance(integerObject));
> bh.consume(numberClass.isInstance(doubleObject));
> }
> 
> @Benchmark
> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
> }
> 
> `isInstance` is a bit faster than `isAssignableFrom`
> 
> Benchmark  Mode  Cnt  Score   Error  Units
> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op

This pull request has now been integrated.

Changeset: de74e0e2
Author:Andrey Turbanov 
URL:   
https://git.openjdk.java.net/jdk/commit/de74e0e25a195084745891419f0c4a8ad286560c
Stats: 25 lines in 10 files changed: 0 ins; 3 del; 22 mod

8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable

Reviewed-by: prr, rriggs

-

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

RFR: 8287053: Avoid redundant HashMap.containsKey calls in ZoneInfoFile.getZoneInfo0

[Andrey Turbanov]

Instead of pair `HashMap.containsKey`/`HashMap.get` method calls, we can use 
single call `HashMap.getOrDefault`.
It's faster and clearer.

-

Commit messages:
 - [PATCH] Avoid redundant HashMap.containsKey calls in ZoneInfoFile

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

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

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Andrey Turbanov]

On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8280035: Use Class.isInstance instead of Class.isAssignableFrom where 
> applicable
>   apply suggestion to avoid second Method.invoke call

Thank you for review!

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v7]

[Paul Sandoz]

On Thu, 19 May 2022 21:11:41 GMT, Jatin Bhateja  wrote:

>> Hi All,
>> 
>> Patch adds the planned support for new vector operations and APIs targeted 
>> for [JEP 426: Vector API (Fourth 
>> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
>> 
>> Following is the brief summary of changes:-
>> 
>> 1)  Extends the scope of existing lanewise API for following new vector 
>> operations.
>>-  VectorOperations.BIT_COUNT: counts the number of one-bits
>>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero 
>> bits
>>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing 
>> zero bits
>>- VectorOperations.REVERSE: reversing the order of bits
>>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>>- compress and expand bits: Semantics are based on Hacker's Delight 
>> section 7-4 Compress, or Generalized Extract.
>> 
>> 2)  Adds following new APIs to perform cross lane vector compress and 
>> expansion operations under the influence of a mask.
>>- Vector.compress
>>- Vector.expand 
>>- VectorMask.compress
>> 
>> 3) Adds predicated and non-predicated versions of following new APIs to load 
>> and store the contents of vector from foreign MemorySegments. 
>>   - Vector.fromMemorySegment
>>   - Vector.intoMemorySegment
>> 
>> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support 
>> for each newly added operation.
>> 
>> 
>>  Patch has been regressed over AARCH64 and X86 targets different AVX levels. 
>> 
>>  Kindly review and share your feedback.
>> 
>>  Best Regards,
>>  Jatin
>
> Jatin Bhateja has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Changes to enable jdk.incubator.vector to be treated as preview 
> participant. Code re-organization related to Reverse/ReverseByte IR 
> transforms.
>  - 8284960: Adding --enable-preview in vectorAPI benchmarks.
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Review comments resolution.
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - 8284960: Correcting a typo.
>  - 8284960: Integrating changes from panama-vector (Add @since 19 tags).
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/9f562ef7...311f3233

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 50:

> 48: import java.util.Set;
> 49: 
> 50: import static com.sun.tools.javac.code.Flags.PREVIEW_API;

Suggestion:


Redundant import (sorry i should have checked before i sent you updates to this 
area)

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]

[Jatin Bhateja]

On Thu, 19 May 2022 15:33:49 GMT, Jatin Bhateja  wrote:

>> Do you mean it's important to apply the transformation at the right node 
>> (pick the right node as the root) and it is hard to make a decision during 
>> GVN?
>
> Yes, that what I meant, but with recently added 
> Node::Flag_is_predicated_using_blend it could be possible to move this 
> transformation ahead into idealization routines of reverse/reverse bytes IR 
> nodes.

Addressed this after internally discussing with Sandhya. Moved the transforms 
from final graph re-shaping back to vector intrinsic routines.

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v7]

[Jatin Bhateja]

> Hi All,
> 
> Patch adds the planned support for new vector operations and APIs targeted 
> for [JEP 426: Vector API (Fourth 
> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
> 
> Following is the brief summary of changes:-
> 
> 1)  Extends the scope of existing lanewise API for following new vector 
> operations.
>-  VectorOperations.BIT_COUNT: counts the number of one-bits
>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero 
> bits
>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing 
> zero bits
>- VectorOperations.REVERSE: reversing the order of bits
>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>- compress and expand bits: Semantics are based on Hacker's Delight 
> section 7-4 Compress, or Generalized Extract.
> 
> 2)  Adds following new APIs to perform cross lane vector compress and 
> expansion operations under the influence of a mask.
>- Vector.compress
>- Vector.expand 
>- VectorMask.compress
> 
> 3) Adds predicated and non-predicated versions of following new APIs to load 
> and store the contents of vector from foreign MemorySegments. 
>   - Vector.fromMemorySegment
>   - Vector.intoMemorySegment
> 
> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support 
> for each newly added operation.
> 
> 
>  Patch has been regressed over AARCH64 and X86 targets different AVX levels. 
> 
>  Kindly review and share your feedback.
> 
>  Best Regards,
>  Jatin

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

 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Changes to enable jdk.incubator.vector to be treated as preview 
participant. Code re-organization related to Reverse/ReverseByte IR transforms.
 - 8284960: Adding --enable-preview in vectorAPI benchmarks.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Review comments resolution.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Correcting a typo.
 - 8284960: Integrating changes from panama-vector (Add @since 19 tags).
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - ... and 6 more: https://git.openjdk.java.net/jdk/compare/9f562ef7...311f3233

-

Changes: https://git.openjdk.java.net/jdk/pull/8425/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8425=06
  Stats: 38049 lines in 228 files changed: 16683 ins; 16923 del; 4443 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8425.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8425/head:pull/8425

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

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Roger Riggs]

On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8280035: Use Class.isInstance instead of Class.isAssignableFrom where 
> applicable
>   apply suggestion to avoid second Method.invoke call

Marked as reviewed by rriggs (Reviewer).

-

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

Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v2]

[Srinivas Vamsi Parasa]

On Wed, 18 May 2022 14:59:49 GMT, Quan Anh Mai  wrote:

>> Hi,
>> 
>> This patch optimises the matching rules for floating-point comparison with 
>> respects to eq/ne on x86-64
>> 
>> 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF` 
>> is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which 
>> improves the sequence of `If (CmpF x x) (Bool ne)` from
>> 
>> ucomiss xmm0, xmm0
>> jp  label
>> jne label
>> 
>> into
>> 
>> ucomiss xmm0, xmm0
>> jp  label
>> 
>> 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as 
>> `x == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of 
>> fixing the flags, such as
>> 
>> xorlecx, ecx
>> ucomiss xmm0, xmm1
>> jnp done
>> pushf
>> andq[rsp], 0xff2b
>> popf
>> done:
>> movleax, 1
>> cmovel  eax, ecx
>> 
>> The patch changes this sequence into
>> 
>> xorlecx, ecx
>> ucomiss xmm0, xmm1
>> movleax, 1
>> cmovpl  eax, ecx
>> cmovnel eax, ecx
>> 
>> 3, The patch also changes the pattern of `isInfinite` to be more optimised 
>> by using `Math.abs` to reduce 1 comparison and compares the result with 
>> `MAX_VALUE` since `>` is more optimised than `==` for floating-point types.
>> 
>> The benchmark results are as follow:
>> 
>> Before:
>> Benchmark  Mode  Cnt Score Error  Units
>> FPComparison.equalDouble   avgt5  2876.242 ±  58.875  ns/op
>> FPComparison.equalFloatavgt5  3062.430 ±  31.371  ns/op
>> FPComparison.isFiniteDoubleavgt5   475.749 ±  19.027  ns/op
>> FPComparison.isFiniteFloat avgt5   506.525 ±  14.417  ns/op
>> FPComparison.isInfiniteDouble  avgt5  1232.800 ±  31.677  ns/op
>> FPComparison.isInfiniteFloat   avgt5  1234.708 ±  70.239  ns/op
>> FPComparison.isNanDouble   avgt5  2255.847 ±   7.238  ns/op
>> FPComparison.isNanFloatavgt5  2567.044 ±  36.078  ns/op
>> 
>> After:
>> Benchmark  Mode  Cnt Score Error  Units
>> FPComparison.equalDouble   avgt5   594.636 ±   8.922  ns/op
>> FPComparison.equalFloatavgt5   663.849 ±   3.656  ns/op
>> FPComparison.isFiniteDoubleavgt5   518.309 ± 107.352  ns/op
>> FPComparison.isFiniteFloat avgt5   515.576 ±  14.669  ns/op
>> FPComparison.isInfiniteDouble  avgt5   621.185 ±  11.935  ns/op
>> FPComparison.isInfiniteFloat   avgt5   623.566 ±  15.206  ns/op
>> FPComparison.isNanDouble   avgt5   400.124 ±   0.762  ns/op
>> FPComparison.isNanFloatavgt5   546.486 ±   1.509  ns/op
>> 
>> Thank you very much.
>
> Quan Anh Mai has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - incidental ws
>  - add tests
>  - Merge branch 'master' into fpcompare
>  - fix tests
>  - test
>  - improve infinity
>  - remove expensive rules
>  - improve fp comparison

Could you pls show the performance delta with the baseline and after the patch? 
Otherwise, people reviewing this PR have to manually compute how much 
improvement is obtained. For example, `FPComparison.isNanFloat` is showing 
`4.7x` improvement. Kindly fill the delta column for rest of the data points. 

Instead of two separate tables, the suggested table format for each row would 
be:
` ,  , , `

-

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

Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v9]

[Roger Riggs]

On Tue, 17 May 2022 01:19:30 GMT, Ichiroh Takiguchi  
wrote:

>> On JDK19 with Linux ja_JP.eucjp locale,
>> System.getenv() returns unexpected value if environment variable has 
>> Japanese EUC characters.
>> It seems this issue happens because of JEP 400.
>> Arguments for ProcessBuilder have same kind of issue.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8285517: System.getenv() returns unexpected value if environment variable 
> has non ASCII character

Looks fine.
(I don't personally like how {@return looks in the source; but its done now. 
Usually it is preferred to stick to the style in the file and not mix stylistic 
changes with functional changes).
Thanks for the updates.

-

Marked as reviewed by rriggs (Reviewer).

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Ioi Lam]

On Thu, 19 May 2022 19:59:18 GMT, Ioi Lam  wrote:

>>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
>> 
>> With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange, 
>> but then again it's a bit of a contrived example as those paths come from 
>> `/proc` parsing. Anyway, this is code that got added with 
>> [JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not 
>> something I've written and to be honest, I'm not sure this branch is needed, 
>> but I didn't want to change the existing behaviour with this patch. I have 
>> no more insight than you in terms of why that approach has been taken.
>> 
>>> Maybe this block should be combined with the new `else` block you're adding?
>> 
>> Maybe, but I'm not sure if it would break something.
>> 
>>> However, the behavior seems opposite between these two blocks of code:
>>> 
>>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of 
>>> cgroup_path
>>> 
>>> ```
>>>   TestCase substring_match = {
>>> "/sys/fs/cgroup/memory",   // mount_path
>>> "/user.slice/user-1000.slice", // root_path
>>> "/user.slice/user-1000.slice/user@1001.service",   // cgroup_path
>>> "/sys/fs/cgroup/memory/user@1001.service"  // expected_path
>>>   };
>>> ```
>> 
>> Yes. Though, I cannot comment on why that has been chosen. It's been there 
>> since day one :-/
>> 
>>> The lower block: The beginning of _root is a prefix of cgroup_path, we take 
>>> the **head** of cgroup_path
>>> 
>>> ```
>>>  TestCase prefix_matched_cg = {
>>> "/sys/fs/cgroup/memory",   // mount_path
>>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>>>   };
>>> ```
>>> 
>>> I find the behavior hard to understand. I think the rules (and reasons) 
>>> should be added to the comment block above the function.
>> 
>> The reason why I've gone down the path of adding the head of cgroup_path is 
>> because of this document (in conjunction to what the user was observing on 
>> an affected system):
>> https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/
>> 
>> The user was observing paths as listed in the test:
>> 
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>> 
>> This very much looks like systemd managed. Given that and knowing that 
>> systemd adds processes into `scopes` or `services` and groups them via 
>> `slices` and also knowing that cgroups are hierarchical (i.e. limits of 
>> `/foo/bar` also apply to `/foo/bar/baz`), it seems likely that if there are 
>> any limits, then it'll be on `/user.slice/user-1000.slice` within the 
>> mounted controller. Unfortunately, I'm not able to reproduce this myself.
>
> I am wondering if the problem is this:
> 
> We have systemd running on the host, and a different copy of systemd that 
> runs inside the container.
> 
> - They both set up `/user.slice/user-1000.slice/session-??.scope` within 
> their own file systems
> - For some reason, when you're looking inside the container, 
> `/proc/self/cgroup` might use a path in the containerized file system whereas 
> `/proc/self/mountinfo` uses a path in the host file system. These two paths 
> may look alike but they have absolutely no relation to each other.
> 
> I have asked the reporter for more information:
> 
> https://gist.github.com/gaol/4d96eace8290e6549635fdc0ea41d0b4?permalink_comment_id=4172593#gistcomment-4172593
> 
> Meanwhile, I think the current method of finding "which directory under 
> /sys/fs/cgroup/memory controls my memory usage" is broken. As mentioned 
> about, the path you get from  `/proc/self/cgroup`  and `/proc/self/mountinfo` 
> have no relation to each other, but we use them anyway to get our answer, 
> with many ad-hoc methods that are not documented in the code.
> 
> Maybe we should do this instead?
> 
> - Read /proc/self/cgroup
> - Find the `10:memory:` line
> - If `/sys/fs/cgroup/memory//tasks` contains my PID, this is the path
> - Otherwise, scan all `tasks` files under  `/sys/fs/cgroup/memory/`. Exactly 
> one of them contains my PID.
> 
> For example, here's a test with docker:
> 
> 
> INSIDE CONTAINER
> # cat /proc/self/cgroup | grep memory
> 10:memory:/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050
> # cat /proc/self/mountinfo | grep memory
> 801 791 0:42 
> /docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050 
> /sys/fs/cgroup/memory ro,nosuid,nodev,noexec,relatime master:23 - cgroup 
> cgroup rw,memory
> # cat 
> /sys/fs/cgroup/memory/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050/tasks
> cat: 
> 

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v2]

[Mandy Chung]

On Thu, 21 Apr 2022 03:44:18 GMT, liach  wrote:

>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
>> the interfaces are iterated twice. The two passes can be merged into one, 
>> yielding the whole proxy definition context (module, package, whether 
>> there's package-private interface) when determining the module.
>> 
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a 
>> good cleanup on its own.
>
> liach has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Don't need to complexify module cache

src/java.base/share/classes/java/lang/reflect/Proxy.java line 498:

> 496: new ClassLoaderValue<>();
> 497: 
> 498: private record ProxyContext(Module module, String pkg, boolean 
> packagePrivate) {}

This represents the context for a proxy class.   It seems `ProxyClassContext` 
would be clearer.   I suggest the context to have the accessFlags for a proxy 
class rather than the boolean whether it's package private.   The constructor 
should check that it must be 0 or `Modifier.PUBLIC`.   `FINAL` will be added to 
the access flags when it generates the proxy class.

src/java.base/share/classes/java/lang/reflect/Proxy.java line 766:

> 764:  * Reads edge and qualified exports are added for dynamic module 
> to access.
> 765:  */
> 766: private static ProxyContext setupContext(ClassLoader loader,

Perhaps name this method `proxyClassContext` that returns `ProxyClassContext`.

src/java.base/share/classes/java/lang/reflect/Proxy.java line 831:

> 829: // All proxy interfaces are public.  So maps to a dynamic 
> proxy module
> 830: // and add reads edge and qualified exports, if necessary
> 831: var context = getDynamicModuleContext(loader, nonExported);

I suggest to keep the `getDynamicModule` method as creating a dynamic module of 
a given class loader is a clear function.   The context can be created in this 
method body.


var targetModule = getDynamicModule(loader);
var pkgName = nonExported ? PROXY_PACKAGE_PREFIX + '.' + 
targetModule.getName()
  : targetModule.getName();
var context = new ProxyClassContext(targetModule, pkgName, 
Modifier.PUBLIC);

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Ioi Lam]

On Thu, 19 May 2022 09:06:18 GMT, Severin Gehwolf  wrote:

>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 63:
>> 
>>> 61:   } else {
>>> 62: char *p = strstr(cgroup_path, _root);
>>> 63: if (p != NULL && p == cgroup_path) {
>> 
>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
>> 
>> Maybe this block should be combined with the new `else` block you're adding? 
>> However, the behavior seems opposite between these two blocks of code:
>> 
>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of 
>> cgroup_path
>> 
>> 
>>   TestCase substring_match = {
>> "/sys/fs/cgroup/memory",   // mount_path
>> "/user.slice/user-1000.slice", // root_path
>> "/user.slice/user-1000.slice/user@1001.service",   // cgroup_path
>> "/sys/fs/cgroup/memory/user@1001.service"  // expected_path
>>   };
>> 
>> 
>> The lower block: The beginning of _root is a prefix of cgroup_path, we take 
>> the **head** of cgroup_path
>> 
>> 
>>  TestCase prefix_matched_cg = {
>> "/sys/fs/cgroup/memory",   // mount_path
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>>   };
>> 
>> 
>> I find the behavior hard to understand. I think the rules (and reasons) 
>> should be added to the comment block above the function.
>
>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
> 
> With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange, 
> but then again it's a bit of a contrived example as those paths come from 
> `/proc` parsing. Anyway, this is code that got added with 
> [JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not 
> something I've written and to be honest, I'm not sure this branch is needed, 
> but I didn't want to change the existing behaviour with this patch. I have no 
> more insight than you in terms of why that approach has been taken.
> 
>> Maybe this block should be combined with the new `else` block you're adding?
> 
> Maybe, but I'm not sure if it would break something.
> 
>> However, the behavior seems opposite between these two blocks of code:
>> 
>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of 
>> cgroup_path
>> 
>> ```
>>   TestCase substring_match = {
>> "/sys/fs/cgroup/memory",   // mount_path
>> "/user.slice/user-1000.slice", // root_path
>> "/user.slice/user-1000.slice/user@1001.service",   // cgroup_path
>> "/sys/fs/cgroup/memory/user@1001.service"  // expected_path
>>   };
>> ```
> 
> Yes. Though, I cannot comment on why that has been chosen. It's been there 
> since day one :-/
> 
>> The lower block: The beginning of _root is a prefix of cgroup_path, we take 
>> the **head** of cgroup_path
>> 
>> ```
>>  TestCase prefix_matched_cg = {
>> "/sys/fs/cgroup/memory",   // mount_path
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>>   };
>> ```
>> 
>> I find the behavior hard to understand. I think the rules (and reasons) 
>> should be added to the comment block above the function.
> 
> The reason why I've gone down the path of adding the head of cgroup_path is 
> because of this document (in conjunction to what the user was observing on an 
> affected system):
> https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/
> 
> The user was observing paths as listed in the test:
> 
> "/user.slice/user-1000.slice/session-50.scope",// root_path
> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
> 
> This very much looks like systemd managed. Given that and knowing that 
> systemd adds processes into `scopes` or `services` and groups them via 
> `slices` and also knowing that cgroups are hierarchical (i.e. limits of 
> `/foo/bar` also apply to `/foo/bar/baz`), it seems likely that if there are 
> any limits, then it'll be on `/user.slice/user-1000.slice` within the mounted 
> controller. Unfortunately, I'm not able to reproduce this myself.

I am wondering if the problem is this:

We have systemd running on the host, and a different copy of systemd that runs 
inside the container.

- They both set up `/user.slice/user-1000.slice/session-??.scope` within their 
own file systems
- For some reason, when you're looking inside the container, 
`/proc/self/cgroup` might use a path in the containerized file system whereas 
`/proc/self/mountinfo` uses a path in the host file system. These two paths may 
look alike but they have absolutely no relation to each other.

I have asked the reporter for more information:

Integrated: 8286182: [BACKOUT] x86: Handle integral division overflow during parsing

[Quan Anh Mai]

On Wed, 18 May 2022 15:44:10 GMT, Quan Anh Mai  wrote:

> This patch backs out the changes made by 
> [JDK-8285390](https://bugs.openjdk.java.net/browse/JDK-8285390) and 
> [JDK-8284742](https://bugs.openjdk.java.net/browse/JDK-8284742) since there 
> are failures due to div nodes floating above their validity checks.
> 
> Thanks.

This pull request has now been integrated.

Changeset: 079312c8
Author:Quan Anh Mai 
Committer: Martin Doerr 
URL:   
https://git.openjdk.java.net/jdk/commit/079312c835a75e2ed5329d061583add5ac9fa2e0
Stats: 1065 lines in 25 files changed: 364 ins; 575 del; 126 mod

8286182: [BACKOUT] x86: Handle integral division overflow during parsing
8287035: [BACKOUT] PPC64: Handle integral division overflow during parsing

Reviewed-by: mdoerr, thartmann

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]

[Alexey Ivanov]

On Thu, 19 May 2022 09:38:40 GMT, Kevin Walls  wrote:

>> Alexey Ivanov has updated the pull request incrementally with seven 
>> additional commits since the last revision:
>> 
>>  - ...set to the values...
>>  - ...will result in a Zip64 Extra (EXT) header
>>  - ...in addition to the main attributes...
>>  - ...and the address of the current archive
>>  - Merges the stack at the given bci...
>>  - Returns a single ...
>>  - ...when a peer shuts down an association.
>
> OK.  I started with serviceability but then went through everything as it's 
> hard to record what you have/haven't seen in these long reviews.

Thank you @kevinjwalls for your suggestions. I've incorporated them.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]

[Alexey Ivanov]

On Thu, 19 May 2022 08:47:47 GMT, Kevin Walls  wrote:

>> Alexey Ivanov has updated the pull request incrementally with seven 
>> additional commits since the last revision:
>> 
>>  - ...set to the values...
>>  - ...will result in a Zip64 Extra (EXT) header
>>  - ...in addition to the main attributes...
>>  - ...and the address of the current archive
>>  - Merges the stack at the given bci...
>>  - Returns a single ...
>>  - ...when a peer shuts down an association.
>
> src/jdk.jdi/share/classes/com/sun/jdi/ClassType.java line 348:
> 
>> 346: 
>> 347: /**
>> 348:  * Returns the single non-abstract {@link Method} visible from
> 
> I would think "Returns a single ..." because the implementation returns the 
> first match it finds, from possibly many.

I've accepted it, yet I'm still unsure whether ‘a‘ or ‘the’…

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]

[Alexey Ivanov]

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

Alexey Ivanov has updated the pull request incrementally with seven additional 
commits since the last revision:

 - ...set to the values...
 - ...will result in a Zip64 Extra (EXT) header
 - ...in addition to the main attributes...
 - ...and the address of the current archive
 - Merges the stack at the given bci...
 - Returns a single ...
 - ...when a peer shuts down an association.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8768/files
  - new: https://git.openjdk.java.net/jdk/pull/8768/files/c7e73658..0669cdc1

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

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

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

Integrated: 8267038: Update IANA Language Subtag Registry to Version 2022-03-02

[Naoto Sato]

On Wed, 18 May 2022 16:28:02 GMT, Naoto Sato  wrote:

> This is to incorporate the latest language subtag registry definition 
> (version 2022-03-02) into JDK19.

This pull request has now been integrated.

Changeset: 7b19226b
Author:Naoto Sato 
URL:   
https://git.openjdk.java.net/jdk/commit/7b19226be24356572df493446e3b0a9380b3d217
Stats: 281 lines in 2 files changed: 271 ins; 2 del; 8 mod

8267038: Update IANA Language Subtag Registry to Version 2022-03-02

Reviewed-by: rriggs

-

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

Integrated: 8286654: Add an optional description accessor on ToolProvider interface

[Christian Stein]

On Wed, 18 May 2022 14:56:47 GMT, Christian Stein  wrote:

> This PR adds an optional description accessor on `ToolProvider` interface.
> 
> This PR also adds short description for each of the listed tool:
> - `jar`
> - `javac`
> - `javadoc`
> - `javap` and `jdeps`
> - `jlink` and `jmod`
> - `jpackage`

This pull request has now been integrated.

Changeset: 655500a4
Author:Christian Stein 
Committer: Joe Darcy 
URL:   
https://git.openjdk.java.net/jdk/commit/655500a4f5e3abcff176599604deceefb6ca6640
Stats: 115 lines in 19 files changed: 97 ins; 2 del; 16 mod

8286654: Add an optional description accessor on ToolProvider interface

Reviewed-by: jjg, darcy, lancea

-

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Lance Andersen]

On Wed, 18 May 2022 14:56:47 GMT, Christian Stein  wrote:

> This PR adds an optional description accessor on `ToolProvider` interface.
> 
> This PR also adds short description for each of the listed tool:
> - `jar`
> - `javac`
> - `javadoc`
> - `javap` and `jdeps`
> - `jlink` and `jmod`
> - `jpackage`

Marked as reviewed by lancea (Reviewer).

src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 28:

> 26: ## tool
> 27: 
> 28: jar.description=create an archive for classes and resources, and 
> manipulate or restore individual classes or resources from an archive

Not sure I love the use of  _manipulate_ but I understand that this is the 
wording from the tools guide for jar.

-

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Jonathan Gibbons]

On Wed, 18 May 2022 14:56:47 GMT, Christian Stein  wrote:

> This PR adds an optional description accessor on `ToolProvider` interface.
> 
> This PR also adds short description for each of the listed tool:
> - `jar`
> - `javac`
> - `javadoc`
> - `javap` and `jdeps`
> - `jlink` and `jmod`
> - `jpackage`

Marked as reviewed by jjg (Reviewer).

-

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

Re: RFR: 8202449: overflow handling in Random.doubles [v2]

[Raffaello Giulietti]

> Extend the range of Random.doubles(double, double) and similar methods.

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

  8202449: overflow handling in Random.doubles

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8791/files
  - new: https://git.openjdk.java.net/jdk/pull/8791/files/936a5bc1..62322ac1

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

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

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

Re: RFR: 8286182: [BACKOUT] x86: Handle integral division overflow during parsing [v2]

[Quan Anh Mai]

On Thu, 19 May 2022 15:29:29 GMT, Quan Anh Mai  wrote:

>> This patch backs out the changes made by 
>> [JDK-8285390](https://bugs.openjdk.java.net/browse/JDK-8285390) and 
>> [JDK-8284742](https://bugs.openjdk.java.net/browse/JDK-8284742) since there 
>> are failures due to div nodes floating above their validity checks.
>> 
>> Thanks.
>
> Quan Anh Mai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove added test

I have backed out also the division test and created JBS issues as instructed 
in the developer guide. Hope I have done it correctly. Thanks a lot for your 
reviews.

-

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

RFR: 8202449: overflow handling in Random.doubles

[Raffaello Giulietti]

Extend the range of Random.doubles(double, double) and similar methods.

-

Commit messages:
 - 8202449: overflow handling in Random.doubles

Changes: https://git.openjdk.java.net/jdk/pull/8791/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8791=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8202449
  Stats: 108 lines in 4 files changed: 47 ins; 25 del; 36 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8791.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8791/head:pull/8791

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Joe Darcy]

On Wed, 18 May 2022 14:56:47 GMT, Christian Stein  wrote:

> This PR adds an optional description accessor on `ToolProvider` interface.
> 
> This PR also adds short description for each of the listed tool:
> - `jar`
> - `javac`
> - `javadoc`
> - `javap` and `jdeps`
> - `jlink` and `jmod`
> - `jpackage`

Marked as reviewed by darcy (Reviewer).

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]

[Jatin Bhateja]

On Wed, 18 May 2022 23:35:54 GMT, Vladimir Ivanov  wrote:

>> It was an attempt to facilitate in-lining of these APIs over targets which 
>> do not intrinsify them. I agree its not a generic fix since three APIs are 
>> piggybacking on same entry point and without the knowledge of opcode it will 
>> be inappropriate to take any call at this place, lazy intrinsification gives 
>> opportunity for some of the predications to concertize as compilation 
>> happens under closed world assumptions.
>
> Still not clear why the code is shaped the way it is.
> 
> `Matcher::match_rule_supported_vector()` already checks that there are 
> relevant matching rules.
> 
> The checks require both `CompressM` and `CompressV` to be present to enable 
> the intrinsic. Is it important?
> 
> Also, it doesn't take `EnableVectorSupport` into account while all other 
> vector intrinsics respect it.

Yes, the code was modified to accommodate your comments. 
https://github.com/openjdk/jdk/pull/8425/files#diff-a9dd7e411772c1ee37b54c5ab868a01fe82af905758350f0ba1c370f422c3fe6R718

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]

[Jatin Bhateja]

On Wed, 18 May 2022 23:28:22 GMT, Vladimir Ivanov  wrote:

>> Its more of a chicken-egg problem here, for masked reverse operation, 
>> Reverse IR node is followed by a Blend Node, thus in such a case doing an 
>> eager Identity transform in Reverse::Identity will not work, also deferring 
>> this to blend may also not work since it could be a non-masked reverse 
>> operation, we could have handled it as a special case in 
>> inline_vector_nary_operation, but handling such special case in final graph 
>> reshaping looked more appropriate.
>> 
>> https://github.com/openjdk/panama-vector/pull/182#discussion_r845678080
>
> Do you mean it's important to apply the transformation at the right node 
> (pick the right node as the root) and it is hard to make a decision during 
> GVN?

Yes, that what I meant.

-

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

Re: RFR: 8286182: C2: crash with SIGFPE when executing compiled code [v2]

[Quan Anh Mai]

> This patch backs out the changes made by 
> [JDK-8285390](https://bugs.openjdk.java.net/browse/JDK-8285390) and 
> [JDK-8284742](https://bugs.openjdk.java.net/browse/JDK-8284742) since there 
> are failures due to div nodes floating above their validity checks.
> 
> Thanks.

Quan Anh Mai has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove added test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8774/files
  - new: https://git.openjdk.java.net/jdk/pull/8774/files/bf963e6b..1e2f12d7

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

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

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Xue-Lei Andrew Fan]

On Thu, 19 May 2022 09:31:07 GMT, Kevin Walls  wrote:

>> Replaces usages of articles that follow each other in all combinations: 
>> a/the, an?/an?, the/the…
>> 
>> Also, I fixed a couple of spelling mistakes.
>
> test/jdk/sun/security/tools/jarsigner/OldSig.java line 32:
> 
>> 30: /*
>> 31:  * See also PreserveRawManifestEntryAndDigest.java for tests with 
>> arbitrarily
>> 32:  * formatted individual sections in addition the main attributes tested
> 
> "in addition to the..."

+1.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Xue-Lei Andrew Fan]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

The security/crypto parts look good to me.

-

Marked as reviewed by xuelei (Reviewer).

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

Re: RFR: 8267038: Update IANA Language Subtag Registry to Version 2022-03-02

[Roger Riggs]

On Wed, 18 May 2022 16:28:02 GMT, Naoto Sato  wrote:

> This is to incorporate the latest language subtag registry definition 
> (version 2022-03-02) into JDK19.

Looks fine.

-

Marked as reviewed by rriggs (Reviewer).

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

Re: Question: Is it possible to introduce ASCII coder for compact strings to optimize performance for ASCII compatible encoding?

[Claes Redestad]

Yes, I think 20% is close to an upper bound, with less gain on smaller
strings due improved cache-locality. And as you say such a small
speed-up might not be very noticeable in practice when I/O is involved.

/Claes

On 2022-05-19 14:17, Glavo wrote:
Thank you very much for your work, it seems that the decoding speed of 
the new version of JDK has been greatly improved.


However, `hasNegatives` still has a cost on my device. I tried removing 
the call to hasNegatives in `String.encodeUTF8` based on JDK 17.0.3,
then I tested its performance through JMH. I ran the JMH benchmark on 
two of my x86 machines (The CPU of one of the machines is Ryzen 7 3700X 
and the other is Xeon W-2175)

and got about a 20% performance boost on both machines.

Of course, The decoding performance in the new JDK is better than I 
thought. For an ASCII string of length 1, hasNegatives takes about 
150 nanoseconds, and the duration increases linearly with the length.
Considering that these operations usually accompany IO, the overhead is 
really not as noticeable as I thought.


Thank you again for your contribution to OpenJDK and your patience in 
replying.


On Wed, May 18, 2022 at 10:47 PM Claes Redestad 
mailto:claes.redes...@oracle.com>> wrote:


Hi,

so your suggestion is that String::coder would be ASCII if all
codepoints are 0-127, then LATIN1 if it contains at least one char in
the 128-255 range, otherwise UTF16? As you say this would mean we could
skip scanning StringCoding::hasNegatives scans and similar overheads
when converting known-to-be-ASCII Strings to UTF-8 and other ASCII
compatible encoding. But it might also complicate logic in various
places, so we need a clear win to motivate such work.

So the first question to answer might be how much does the hasNegatives
calls cost us. Depending on your hardware (and JDK version) the answer
might be "almost nothing"!

I did some work last year to speed up encoding and decoding ASCII-only
data to/from various encodings. When I was done there was no longer much
of a difference when encoding from String to an ASCII-compatible charset
[1]. Maybe a scaling ~10% advantage for latin-1 in some microbenchmark
when decoding on really large strings[2].

But with the suggested change the decoding advantage would likely
disappear: we maintain the invariant that Strings are equals if and only
if their coders are equal, so no we'd now have to scan latin-1 encoded
streams for non-ASCII bytes to disambiguate between ASCII and LATIN1.

Maybe there are some other case that would be helped, but with some
likely negatives and only a modest potential win then my gut feeling is
that this wouldn't pay off.

Best regards
Claes

[1] https://cl4es.github.io/2021/10/17/Faster-Charset-Encoding.html


[2] https://cl4es.github.io/2021/02/23/Faster-Charset-Decoding.html



(All the improvements discussed in the blog entires should be available
since 17.0.2.)

On 2022-05-18 14:07, Glavo wrote:
 > After the introduction of compact strings in Java 9, the current
String may
 > store byte arrays encoded as Latin1 or UTF-16.
 > Here's a troublesome thing: Latin1 is not compatible with UTF-8.
Not all
 > Latin1 byte strings are legal UTF-8 byte strings:
 > They are only compatible within the ASCII range, when there are
code points
 > greater than 127, Latin1 uses a one-byte
 > representation, while UTF-8 requires two bytes.
 >
 > As an example, every time `JavaLangAccess::getBytesNoRepl` is
called to
 > convert a string to a UTF-8 array,
 > the internal implementation needs to call
`StringCoding.hasNegatives` to
 > scan the content byte array to determine that
 > the string can be encoded in ASCII, and thus eliminate array copies.
 > Similar steps are performed when calling `str.getBytes(UTF_8)`.
 >
 > So, is it possible to introduce a third possible value for
`String::coder`:
 > ASCII?
 > This looks like an attractive option, and if we do this, we can
introduce
 > fast paths for many methods.
 >
 > Of course, I know that this change is not completely free, and
some methods
 > may bring slight performance
 > degradation due to the need to judge the coder, in particular,
there may be
 > an impact on the performance of the
 > StringBuilder/StringBuffer. However, given that UTF-8 is by far
the most
 > commonly used file encoding, the performance
 

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v2]

[liach]

On Thu, 21 Apr 2022 03:44:18 GMT, liach  wrote:

>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
>> the interfaces are iterated twice. The two passes can be merged into one, 
>> yielding the whole proxy definition context (module, package, whether 
>> there's package-private interface) when determining the module.
>> 
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a 
>> good cleanup on its own.
>
> liach has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Don't need to complexify module cache

Can anyone, such as Mandy, review this cleanup?

-

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

Re: Question: Is it possible to introduce ASCII coder for compact strings to optimize performance for ASCII compatible encoding?

[Glavo]

Thank you very much for your work, it seems that the decoding speed of the
new version of JDK has been greatly improved.

However, `hasNegatives` still has a cost on my device. I tried removing the
call to hasNegatives in `String.encodeUTF8` based on JDK 17.0.3,
then I tested its performance through JMH. I ran the JMH benchmark on two
of my x86 machines (The CPU of one of the machines is Ryzen 7 3700X and the
other is Xeon W-2175)
and got about a 20% performance boost on both machines.

Of course, The decoding performance in the new JDK is better than I
thought. For an ASCII string of length 1, hasNegatives takes about 150
nanoseconds, and the duration increases linearly with the length.
Considering that these operations usually accompany IO, the overhead is
really not as noticeable as I thought.

Thank you again for your contribution to OpenJDK and your patience in
replying.

On Wed, May 18, 2022 at 10:47 PM Claes Redestad 
wrote:

> Hi,
>
> so your suggestion is that String::coder would be ASCII if all
> codepoints are 0-127, then LATIN1 if it contains at least one char in
> the 128-255 range, otherwise UTF16? As you say this would mean we could
> skip scanning StringCoding::hasNegatives scans and similar overheads
> when converting known-to-be-ASCII Strings to UTF-8 and other ASCII
> compatible encoding. But it might also complicate logic in various
> places, so we need a clear win to motivate such work.
>
> So the first question to answer might be how much does the hasNegatives
> calls cost us. Depending on your hardware (and JDK version) the answer
> might be "almost nothing"!
>
> I did some work last year to speed up encoding and decoding ASCII-only
> data to/from various encodings. When I was done there was no longer much
> of a difference when encoding from String to an ASCII-compatible charset
> [1]. Maybe a scaling ~10% advantage for latin-1 in some microbenchmark
> when decoding on really large strings[2].
>
> But with the suggested change the decoding advantage would likely
> disappear: we maintain the invariant that Strings are equals if and only
> if their coders are equal, so no we'd now have to scan latin-1 encoded
> streams for non-ASCII bytes to disambiguate between ASCII and LATIN1.
>
> Maybe there are some other case that would be helped, but with some
> likely negatives and only a modest potential win then my gut feeling is
> that this wouldn't pay off.
>
> Best regards
> Claes
>
> [1] https://cl4es.github.io/2021/10/17/Faster-Charset-Encoding.html
> [2] https://cl4es.github.io/2021/02/23/Faster-Charset-Decoding.html
>
> (All the improvements discussed in the blog entires should be available
> since 17.0.2.)
>
> On 2022-05-18 14:07, Glavo wrote:
> > After the introduction of compact strings in Java 9, the current String
> may
> > store byte arrays encoded as Latin1 or UTF-16.
> > Here's a troublesome thing: Latin1 is not compatible with UTF-8. Not all
> > Latin1 byte strings are legal UTF-8 byte strings:
> > They are only compatible within the ASCII range, when there are code
> points
> > greater than 127, Latin1 uses a one-byte
> > representation, while UTF-8 requires two bytes.
> >
> > As an example, every time `JavaLangAccess::getBytesNoRepl` is called to
> > convert a string to a UTF-8 array,
> > the internal implementation needs to call `StringCoding.hasNegatives` to
> > scan the content byte array to determine that
> > the string can be encoded in ASCII, and thus eliminate array copies.
> > Similar steps are performed when calling `str.getBytes(UTF_8)`.
> >
> > So, is it possible to introduce a third possible value for
> `String::coder`:
> > ASCII?
> > This looks like an attractive option, and if we do this, we can introduce
> > fast paths for many methods.
> >
> > Of course, I know that this change is not completely free, and some
> methods
> > may bring slight performance
> > degradation due to the need to judge the coder, in particular, there may
> be
> > an impact on the performance of the
> > StringBuilder/StringBuffer. However, given that UTF-8 is by far the most
> > commonly used file encoding, the performance
> > benefits of fast paths are likely to cover more scenarios. In addition to
> > this, other ASCII compatible encodings may
> > also benefit, such as GB18030(GBK), or ISO 8859 variants. And if frozen
> > arrays were introduced into the JDK,
> > there would be more scenarios to enjoy performance improvements.
> >
> > So I would like to ask, is it possible for JDK to improve String storage
> in
> > a similar way in the future?
> > Has anyone explored this issue before?
> >
> > Sorry to bother you all, but I'm very much looking forward to the answer
> to
> > this question.
>

Re: RFR: 8287013: StringConcatFactory: remove short and byte mixers/prependers

[Jim Laskey]

On Thu, 19 May 2022 10:47:23 GMT, Claes Redestad  wrote:

> The current short and byte mixers and prependers simply delegate to the int 
> variants. At the LambdaForm level the values has already been implicitly cast 
> to int, so removing this indirection and directly adapting to call the 
> int-based variants is possible.
> 
> This is a cleanup with minimal effect on bootstrap overhead and peak 
> performance, since all the LF logic only deals with basic types (where byte 
> and short and other subword primitives has been widened to ints anyhow).

Marked as reviewed by jlaskey (Reviewer).

-

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

RFR: 8287013: StringConcatFactory: remove short and byte mixers/prependers

[Claes Redestad]

The current short and byte mixers and prependers simply delegate to the int 
variants. At the LambdaForm level the values has already been implicitly cast 
to int, so removing this indirection and directly adapting to call the 
int-based variants is possible.

This is a cleanup with minimal effect on bootstrap overhead and peak 
performance, since all the LF logic only deals with basic types (where byte and 
short and other subword primitives has been widened to ints anyhow).

-

Commit messages:
 - 8287013: StringConcatFactory: remove short and byte mixers/prependers

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

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Christian Stein]

On Thu, 19 May 2022 06:21:40 GMT, Christian Stein  wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
>>  line 387:
>> 
>>> 385: 
>>> 386: javac.description=Read Java declarations and compile them into class 
>>> files
>>> 387: 
>> 
>> for your general consideration, what grammatical style do you recommend 
>> here? Should it be 3rd person (Reads) instead of 2nd person (Read)
>
> I took the phrases in 2nd person style from 
> https://docs.oracle.com/en/java/javase/18/docs/specs/man/ as templates, 
> shortening some of them to around 80 characters for better fitting in 
> standard terminal widths.
> 
> Perhaps, those texts should be used "as-is" as descriptions?
> - 2nd person phrase-style
> - lowercase start
> - no terminating period
> - no shortening

Took the phrases "as-is" from the overview site for the man pages: [Java® 
Development Kit Version 18 Tool 
Specifications](https://docs.oracle.com/en/java/javase/18/docs/specs/man/)

-

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Christian Stein]

On Wed, 18 May 2022 21:47:25 GMT, Jonathan Gibbons  wrote:

>> This PR adds an optional description accessor on `ToolProvider` interface.
>> 
>> This PR also adds short description for each of the listed tool:
>> - `jar`
>> - `javac`
>> - `javadoc`
>> - `javap` and `jdeps`
>> - `jlink` and `jmod`
>> - `jpackage`
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavacToolProvider.java
>  line 30:
> 
>> 28: import com.sun.tools.javac.util.JavacMessages;
>> 29: import java.io.PrintWriter;
>> 30: import java.util.Optional;
> 
> at least in javac, we normally sort `java.*` and `javax.*` imports before 
> other imports.

Sorted imports accordingly.

> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties 
> line 387:
> 
>> 385: 
>> 386: javac.description=Read Java declarations and compile them into class 
>> files
>> 387: 
> 
> for your general consideration, what grammatical style do you recommend here? 
> Should it be 3rd person (Reads) instead of 2nd person (Read)

I took the phrases in 2nd person style from 
https://docs.oracle.com/en/java/javase/18/docs/specs/man/ as templates, 
shortening some of them to around 80 characters for better fitting in standard 
terminal widths.

Perhaps, those texts should be used "as-is" as descriptions?
- 2nd person phrase-style
- lowercase start
- no terminating period
- no shortening

-

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

Re: RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Jonathan Gibbons]

On Wed, 18 May 2022 14:56:47 GMT, Christian Stein  wrote:

> This PR adds an optional description accessor on `ToolProvider` interface.
> 
> This PR also adds short description for each of the listed tool:
> - `jar`
> - `javac`
> - `javadoc`
> - `javap` and `jdeps`
> - `jlink` and `jmod`
> - `jpackage`

Main issue is the one I raised at the beginning, about what grammatical form 
you should be using for the short description.

Also, is the description a phrase (probably not capitalized, and no terminating 
period) or a sentence (capitalized with a terminating period.)

src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavacToolProvider.java 
line 30:

> 28: import com.sun.tools.javac.util.JavacMessages;
> 29: import java.io.PrintWriter;
> 30: import java.util.Optional;

at least in javac, we normally sort `java.*` and `javax.*` imports before other 
imports.

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties 
line 387:

> 385: 
> 386: javac.description=Read Java declarations and compile them into class 
> files
> 387: 

for your general consideration, what grammatical style do you recommend here? 
Should it be 3rd person (Reads) instead of 2nd person (Read)

-

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

RFR: 8286654: Add an optional description accessor on ToolProvider interface

[Christian Stein]

This PR adds an optional description accessor on `ToolProvider` interface.

This PR also adds short description for each of the listed tool:
- `jar`
- `javac`
- `javadoc`
- `javap` and `jdeps`
- `jlink` and `jmod`
- `jpackage`

-

Commit messages:
 - Add localized description for `jpackage`
 - Add localized description for `jlink` and `jmod`
 - Add localized description for `javap` and `jdeps`
 - Add localized description for `javadoc`
 - Add localized description for `jar`
 - Add localized description for `javac`
 - 8286654: Add an optional description accessor on ToolProvider interface

Changes: https://git.openjdk.java.net/jdk/pull/8772/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8772=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286654
  Stats: 115 lines in 19 files changed: 97 ins; 2 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8772.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8772/head:pull/8772

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

Re: RFR: 8286956: Loom: Define test groups for development/porting use

[Aleksey Shipilev]

On Wed, 18 May 2022 11:10:58 GMT, Aleksey Shipilev  wrote:

> It would be beneficial to bring over the Loom-specific test groups from the 
> loom repo to aid development/porting work.
> 
> https://github.com/openjdk/loom/blob/fibers/test/jdk/TEST.groups#L97-L108
> https://github.com/openjdk/loom/blob/6f42923b3342e41d95b262733205283068802b40/test/hotspot/jtreg/TEST.groups#L111-L125
> 
> I had to drop `jdk/incubator/concurrent` from JDK definition, because there 
> are no tests in that folder and tests break.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug `hotspot_loom jdk_loom`
>  - [x] Linux AArch64 fastdebug `hotspot_loom jdk_loom`

Thanks! Any other comments? I'd like Loom porters to have it sooner.

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Severin Gehwolf]

On Thu, 19 May 2022 05:58:31 GMT, Ioi Lam  wrote:

>> Severin Gehwolf has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Refactor hotspot gtest
>>  - Separate into function. Fix comment.
>
> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 102:
> 
>> 100:   for (int i = 0; *s1 == *s2 && *s1 != 0; i++) {
>> 101: if (*s1 == '/') {
>> 102:   last_matching_slash_pos = i;
> 
> I found the behavior a little hard to understand. Is it intentional?
> 
> 
> "/cat/cow", "/cat/dog"-> "/cat/"
> "/cat/","/cat/dog"-> "/cat/"
> "/cat", "/cat/dog"-> "/"
> 
> 
> The name `find_last_slash_pos` doesn't properly describe the behavior. I 
> thought about `find_common_path_prefix`, but that doesn't match the current 
> behavior (for the 3rd case, the common path prefix is `"/cat"`).

It's supposed to find the common path prefix as you say. I'll fix it. Open to 
suggestions to make it easier to understand.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

OK.  I started with serviceability but then went through everything as it's 
hard to record what you have/haven't seen in these long reviews.

test/jdk/java/net/InterfaceAddress/Equals.java line 81:

> 79: /**
> 80:  * Returns an InterfaceAddress instance with its fields set the values
> 81:  * specificed.

probably a typo for "set to the values specified."

-

Marked as reviewed by kevinw (Committer).

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 84:

> 82: 
> 83: /**
> 84:  * Create a Zip file that will result in an Zip64 Extra (EXT) header

"result in a Zip64..."

test/jdk/sun/security/tools/jarsigner/OldSig.java line 32:

> 30: /*
> 31:  * See also PreserveRawManifestEntryAndDigest.java for tests with 
> arbitrarily
> 32:  * formatted individual sections in addition the main attributes tested

"in addition to the..."

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Severin Gehwolf]

On Thu, 19 May 2022 06:00:06 GMT, Ioi Lam  wrote:

>> Severin Gehwolf has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Refactor hotspot gtest
>>  - Separate into function. Fix comment.
>
> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 86:
> 
>> 84:   const char* cgroup_p = cgroup_path;
>> 85:   int last_slash = find_last_slash_pos(root_p, cgroup_p);
>> 86:   assert(last_slash >= 0, "not an absolute path?");
> 
> Are root_p and cgroup_p directly read from the /proc/xxx files. If so, do we 
> validate the input to make sure they are absolute paths?
> 
> It seems like our code cannot handle trailing '/' in the input. If so, we 
> should clear all trailing '/' from the input string. Then, in functions that 
> process them, we should assert that they don't end with slash. See my comment 
> in find_last_slash_pos().

Yes, those values come from `/proc/self/mountinfo` and `/proc/self/cgroup`. 
There is no validation being done. Then again, we only end up in this branch if 
the root path is not a substring of the cgroup path. In that case trailing 
slashes don't matter, since there would not be a character by character match 
earlier.

I'll add handling of trailing slashes and appropriate asserts where it makes 
sense.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

src/hotspot/share/cds/filemap.cpp line 1914:

> 1912: 
> 1913: // the current value of the pointers to be patched must be within 
> this
> 1914: // range (i.e., must be between the requested base address, and the 
> of the current archive).

"the of the" ? 
Maybe "..and the address of the current archive",  or maybe it was a typo for 
"and that of the current archive".

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Severin Gehwolf]

On Thu, 19 May 2022 05:53:19 GMT, Ioi Lam  wrote:

>> Severin Gehwolf has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Refactor hotspot gtest
>>  - Separate into function. Fix comment.
>
> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 60:
> 
>> 58: strncpy(buf, _mount_point, MAXPATHLEN);
>> 59: buf[MAXPATHLEN-1] = '\0';
>> 60: _path = os::strdup(buf);
> 
> Comment on the old code: why this cannot be simply
> 
> 
> _path = os::strdup(_mount_point);
> 
> 
> Also, all the strncat calls in this function seem problematic, and should be 
> converted to stringStream for consistency.

Agreed. I've filed https://bugs.openjdk.java.net/browse/JDK-8287007 to track 
this. I'd like to limit the changes of this patch to a minimum. It seems 
orthogonal.

> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?

With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange, 
but then again it's a bit of a contrived example as those paths come from 
`/proc` parsing. Anyway, this is code that got added with 
[JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not 
something I've written and to be honest, I'm not sure this branch is needed, 
but I didn't want to change the existing behaviour with this patch. I have no 
more insight than you in terms of why that approach has been taken.

> Maybe this block should be combined with the new `else` block you're adding?

Maybe, but I'm not sure if it would break something.

> However, the behavior seems opposite between these two blocks of code:
> 
> The upper block: _root is a prefix of cgroup_path, we take the **tail** of 
> cgroup_path
> 
> ```
>   TestCase substring_match = {
> "/sys/fs/cgroup/memory",   // mount_path
> "/user.slice/user-1000.slice", // root_path
> "/user.slice/user-1000.slice/user@1001.service",   // cgroup_path
> "/sys/fs/cgroup/memory/user@1001.service"  // expected_path
>   };
> ```

Yes. Though, I cannot comment on why that has been chosen. It's been there 
since day one :-/

> The lower block: The beginning of _root is a prefix of cgroup_path, we take 
> the **head** of cgroup_path
> 
> ```
>  TestCase prefix_matched_cg = {
> "/sys/fs/cgroup/memory",   // mount_path
> "/user.slice/user-1000.slice/session-50.scope",// root_path
> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>   };
> ```
> 
> I find the behavior hard to understand. I think the rules (and reasons) 
> should be added to the comment block above the function.

The reason why I've gone down the path of adding the head of cgroup_path is 
because of this document (in conjunction to what the user was observing on an 
affected system):
https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/

The user was observing paths as listed in the test:

"/user.slice/user-1000.slice/session-50.scope",// root_path
"/user.slice/user-1000.slice/session-3.scope", // cgroup_path

This very much looks like systemd managed. Given that and knowing that systemd 
adds processes into `scopes` or `services` and groups them via `slices` and 
also knowing that cgroups are hierarchical (i.e. limits of `/foo/bar` also 
apply to `/foo/bar/baz`), it seems likely that if there are any limits, then 
it'll be on `/user.slice/user-1000.slice` within the mounted controller. 
Unfortunately, I'm not able to reproduce this myself.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

src/hotspot/share/interpreter/bytecodeUtils.cpp line 186:

> 184:   static const int _max_cause_detail = 5;
> 185: 
> 186:   // Merges the stack the given bci with the given stack. If there

"...the stack at the given bci..."

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v6]

[Xiaohong Gong]

On Thu, 19 May 2022 08:53:31 GMT, Ningsheng Jian  wrote:

>>> LUT should be generated only if UsePopCountInsturction is false 
>> 
>> Should there be `!UsePopCountInsturction` check then?
>> 
>>> restrict the scope of flag to only scalar popcount operation
>> 
>> Interesting. But AArch64 code does cover vector cases which just adds 
>> confusion.
>
>> Interesting. But AArch64 code does cover vector cases which just adds 
>> confusion.
> 
> `UsePopCountInsturction` is always true in AArch64. @XiaohongGong removed the 
> `predicate` in aarch64 rules, and I think we can even remove the option check 
> in match_rule_supported().

Ok , I will remove the check for it. Thanks!

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

src/hotspot/share/opto/graphKit.cpp line 3626:

> 3624: // The optional arguments are for specialized use by intrinsics:
> 3625: //  - If 'extra_slow_test' if not null is an extra condition for the 
> slow-path.
> 3626: //  - If 'return_size_val', report the total object size to the caller.

"reports the total"

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v6]

[Ningsheng Jian]

On Wed, 18 May 2022 23:22:42 GMT, Vladimir Ivanov  wrote:

> Interesting. But AArch64 code does cover vector cases which just adds 
> confusion.

`UsePopCountInsturction` is always true in AArch64. @XiaohongGong removed the 
`predicate` in aarch64 rules, and I think we can even remove the option check 
in match_rule_supported().

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

src/jdk.jdi/share/classes/com/sun/jdi/ClassType.java line 348:

> 346: 
> 347: /**
> 348:  * Returns the single non-abstract {@link Method} visible from

I would think "Returns a single ..." because the implementation returns the 
first match it finds, from possibly many.

-

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

Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base

[Kevin Walls]

On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> Also, I fixed a couple of spelling mistakes.

src/jdk.sctp/share/classes/com/sun/nio/sctp/ShutdownNotification.java line 28:

> 26: 
> 27: /**
> 28:  * Notification emitted when a peers shutdowns the association.

Maybe: "...when a peer shuts down an association."
(could be "shuts down the association" if there is only ever one, but using 
"an" looks safe)

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]

[Severin Gehwolf]

On Thu, 19 May 2022 07:02:13 GMT, Thomas Stuefe  wrote:

>> I'm not convinced the extra function makes the code more readable, but here 
>> it is. I can revert back if this is too much.
>
> @jerboaa Feel free to go back to your original variant. This was only a 
> proposal.

understood.

-

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

Integrated: 8286462: Incorrect copyright year for src/java.base/share/classes/jdk/internal/vm/FillerObject.java

[zhw970623]

On Tue, 10 May 2022 14:58:28 GMT, zhw970623  wrote:

> The copyright year of 
> src/java.base/share/classes/jdk/internal/vm/FillerObject.java should be 2022.

This pull request has now been integrated.

Changeset: 022e7170
Author:yyrrzhang 
Committer: Jie Fu 
URL:   
https://git.openjdk.java.net/jdk/commit/022e71704ce81d9b47624fb9fb93a4017dae62a0
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8286462: Incorrect copyright year for 
src/java.base/share/classes/jdk/internal/vm/FillerObject.java

Reviewed-by: jiefu

-

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]

[Thomas Stuefe]

On Wed, 18 May 2022 18:10:45 GMT, Severin Gehwolf  wrote:

>> @iklam yes I meant `Find the longest common prefix`. Fixed the comment.
>
> I'm not convinced the extra function makes the code more readable, but here 
> it is. I can revert back if this is too much.

@jerboaa Feel free to go back to your original variant. This was only a 
proposal.

-

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

Re: RFR: 8286182: C2: crash with SIGFPE when executing compiled code

[Christian Hagedorn]

On Wed, 18 May 2022 15:44:10 GMT, Quan Anh Mai  wrote:

> This patch backs out the changes made by 
> [JDK-8285390](https://bugs.openjdk.java.net/browse/JDK-8285390) and 
> [JDK-8284742](https://bugs.openjdk.java.net/browse/JDK-8284742) since there 
> are failures due to div nodes floating above their validity checks.
> 
> Thanks.

Backout looks good except that you did not remove the test 
`test/hotspot/jtreg/compiler/integerArithmetic/TestDivision.java` which you've 
added here:
https://github.com/openjdk/jdk/commit/b4a85cdae14eee895a0de2f26a2ffdd62b72bebc#diff-4daba70b215edefe9fd70b24f1e1052a51effdd48a1809e2ce5a06bc21a755c5

It's just a correctness test, so it would be okay to leave it in. But I guess 
it's better to backout everything.

-

Changes requested by chagedorn (Reviewer).

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

Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Ioi Lam]

On Wed, 18 May 2022 18:14:52 GMT, Severin Gehwolf  wrote:

>> Please review this change to the cgroup v1 subsystem which makes it more 
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to 
>> re-create a similar system as the reporter. The idea of using the longest 
>> substring match for the cgroupv1 file paths was based on the fact that on 
>> systemd systems processes run in separate scopes and the maven forked test 
>> runner might exhibit this property. For that it makes sense to use the 
>> common ancestor path. Nothing changes in the common cases where the 
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the 
>> former, host system the latter).
>> 
>> In addition, the code paths which are susceptible to throw NPE have been 
>> hardened to catch those situations. Should it happen in the future it makes 
>> more sense (to me) to not have accurate container detection in favor of 
>> continuing to keep running.
>> 
>> Finally, with the added unit-tests a bug was uncovered on the "substring" 
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never 
>> point to `_root` as it's used as the "needle" to find in "haystack" 
>> `cgroup_path` (not the other way round).
>> 
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> Severin Gehwolf has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Refactor hotspot gtest
>  - Separate into function. Fix comment.

Changes requested by iklam (Reviewer).

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 60:

> 58: strncpy(buf, _mount_point, MAXPATHLEN);
> 59: buf[MAXPATHLEN-1] = '\0';
> 60: _path = os::strdup(buf);

Comment on the old code: why this cannot be simply


_path = os::strdup(_mount_point);


Also, all the strncat calls in this function seem problematic, and should be 
converted to stringStream for consistency.

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 63:

> 61:   } else {
> 62: char *p = strstr(cgroup_path, _root);
> 63: if (p != NULL && p == cgroup_path) {

What happens if cgroup_path is "/foo/bar" and _root is "/fo"?

Maybe this block should be combined with the new `else` block you're adding? 
However, the behavior seems opposite between these two blocks of code:

The upper block: _root is a prefix of cgroup_path, we take the **tail** of 
cgroup_path


  TestCase substring_match = {
"/sys/fs/cgroup/memory",   // mount_path
"/user.slice/user-1000.slice", // root_path
"/user.slice/user-1000.slice/user@1001.service",   // cgroup_path
"/sys/fs/cgroup/memory/user@1001.service"  // expected_path
  };


The lower block: The beginning of _root is a prefix of cgroup_path, we take the 
**head** of cgroup_path


 TestCase prefix_matched_cg = {
"/sys/fs/cgroup/memory",   // mount_path
"/user.slice/user-1000.slice/session-50.scope",// root_path
"/user.slice/user-1000.slice/session-3.scope", // cgroup_path
"/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
  };


I find the behavior hard to understand. I think the rules (and reasons) should 
be added to the comment block above the function.

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 86:

> 84:   const char* cgroup_p = cgroup_path;
> 85:   int last_slash = find_last_slash_pos(root_p, cgroup_p);
> 86:   assert(last_slash >= 0, "not an absolute path?");

Are root_p and cgroup_p directly read from the /proc/xxx files. If so, do we 
validate the input to make sure they are absolute paths?

It seems like our code cannot handle trailing '/' in the input. If so, we 
should clear all trailing '/' from the input string. Then, in functions that 
process them, we should assert that they don't end with slash. See my comment 
in find_last_slash_pos().

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 102:

> 100:   for (int i = 0; *s1 == *s2 && *s1 != 0; i++) {
> 101: if (*s1 == '/') {
> 102:   last_matching_slash_pos = i;

I found the behavior a little hard to understand. Is it intentional?


"/cat/cow", "/cat/dog"-> "/cat/"
"/cat/","/cat/dog"-> "/cat/"
"/cat", "/cat/dog"-> "/"


The name `find_last_slash_pos` doesn't properly describe the behavior. I 
thought about `find_common_path_prefix`, but that doesn't match the current 
behavior (for the 3rd case, the common path prefix is `"/cat"`).

-

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

Re: RFR: 8286182: C2: crash with SIGFPE when executing compiled code

[Tobias Hartmann]

On Wed, 18 May 2022 15:44:10 GMT, Quan Anh Mai  wrote:

> This patch backs out the changes made by 
> [JDK-8285390](https://bugs.openjdk.java.net/browse/JDK-8285390) and 
> [JDK-8284742](https://bugs.openjdk.java.net/browse/JDK-8284742) since there 
> are failures due to div nodes floating above their validity checks.
> 
> Thanks.

Looks good to me too but the backout should adhere to the process defined here: 
https://openjdk.java.net/guide/#backing-out-a-change

I would suggest "Alternative 2" of 3.

-

Marked as reviewed by thartmann (Reviewer).

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