Integrated: 8286694: Incorrect argument processing in java launcher
On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga wrote: > GCC 12 reports as following: This pull request has now been integrated. Changeset: 26c7c92b Author:Yasumasa Suenaga URL: https://git.openjdk.java.net/jdk/commit/26c7c92bc93f3eecf7ce69c69f1999ba879d1d60 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8286694: Incorrect argument processing in java launcher Reviewed-by: dholmes - PR: https://git.openjdk.java.net/jdk/pull/8694
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
> On May 11, 2022, at 4:39 PM, Alexander Matveev
> 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);
Re: RFR: 8286462: Incorrect copyright year for src/java.base/share/classes/jdk/internal/vm/FillerObject.java
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. Looks good and trivial. Thanks for spotting and fixing it. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8633
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]
On Fri, 13 May 2022 08:24:24 GMT, Jatin Bhateja wrote:
>> src/hotspot/share/opto/c2compiler.cpp line 521:
>>
>>> 519: if (!Matcher::match_rule_supported(Op_SignumF)) return false;
>>> 520: break;
>>> 521: case vmIntrinsics::_VectorComExp:
>>
>> Why `_VectorComExp` intrinsic is special? Other vector intrinsics are
>> handled later and in a different manner.
>>
>> What about `ExpandV` case?
>
> 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.
>> src/hotspot/share/opto/compile.cpp line 3416:
>>
>>> 3414:
>>> 3415: case Op_ReverseBytesV:
>>> 3416: case Op_ReverseV: {
>>
>> Can you elaborate, please, why it is performed so late in the optimization
>> phase (at the very end during graph reshaping) and not during GVN?
>
> 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?
-
PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v6]
On Fri, 13 May 2022 08:24:21 GMT, Jatin Bhateja 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. - PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]
On Wed, 20 Apr 2022 16:14:42 GMT, XenoAmess wrote: >> Need to add apiNote documentation section to capacity-based constructors >> like for maps. > >> Need to add apiNote documentation section to capacity-based constructors >> like for maps. > > @liach done. @XenoAmess oops, sorry for the delay. I think it would be good to get these into 19 as companions to `HashMap.newHashMap` et. al. As before, I'd suggest reducing the number of changes to use sites in order to make review easier. I would suggest keeping the changes under src in java.base, java.net.http, java.rmi, and jdk.zipfs, and omitting all the other changes. Also keep the changes under test/jdk. There needs to be a test for these new methods. I haven't thought much how to do that. My first attempt would be to modify our favorite WhiteBoxResizeTest and add a bit of machinery that asserts the table length of the HashMap contained within the created HashSet/LinkedHashSet. I haven't looked at it though, so it might not work out, in which case you should pursue an alternative approach. I'll look at the specs and suggest updates as necessary and then handle filing of a CSR. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v3]
On Thu, 12 May 2022 07:59:36 GMT, John Hendrikx wrote:
>> Brian Burkhalter has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - 6478546: Clean up io_util.c
>> - Merge
>> - 6478546: Decrease malloc'ed buffer maximum size to 64 kB
>> - 6478546: FileInputStream.read() throws OutOfMemoryError when there is
>> plenty available
>
> src/java.base/share/native/libjava/io_util.c line 79:
>
>> 77: jint off, jint len, jfieldID fid)
>> 78: {
>> 79: char stackBuf[STACK_BUF_SIZE];
>
> This was in the original code already, but doesn't this always allocate 8k on
> the stack, regardless of whether this buffer will be used (if len > 8k or len
> == 0)? Wouldn't it be better to allocate this only in the `else` case?
>
> Would apply to the write code as well.
This is intentional. We want to avoid having a malloc + free in every call and
so avoid it for small buffers.
> src/java.base/share/native/libjava/io_util.c line 81:
>
>> 79: char stackBuf[STACK_BUF_SIZE];
>> 80: char *buf = NULL;
>> 81: jint buf_size, read_size;;
>
> minor: double semi colon
FIxed in 10f14bb3faef2b55ab59a85016874d12815f3c87.
> src/java.base/share/native/libjava/io_util.c line 129:
>
>> 127: off += n;
>> 128: } else if (n == -1) {
>> 129: JNU_ThrowIOExceptionWithLastError(env, "Read error");
>
> The original code would have `nread` set to `-1` here, and thus exit with
> `-1`. The new code would exit with the last value for `nread` which could be
> anything.
The returned value of `nread` would in this case indicate the number of bytes
actually read so far, which is intentional.
> src/java.base/share/native/libjava/io_util.c line 201:
>
>> 199: write_size = len < buf_size ? len : buf_size;
>> 200: (*env)->GetByteArrayRegion(env, bytes, off, write_size,
>> (jbyte*)buf);
>> 201: if (!(*env)->ExceptionOccurred(env)) {
>
> Wouldn't this result in an infinite loop if `GetByteArrayRegion` triggered an
> exception and `len > 0` ? It would never enter the `if` and `len` is never
> changed then...
Good catch, you are correct. Fixed in 10f14bb3faef2b55ab59a85016874d12815f3c87.
-
PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v4]
> Modify native multi-byte read-write code used by the `java.io` classes to > limit the size of the allocated native buffer thereby decreasing off-heap > memory footprint and increasing throughput. Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - 6478546: Add break in write loop on ExceptionOccurred - Merge - 6478546: Clean up io_util.c - Merge - 6478546: Decrease malloc'ed buffer maximum size to 64 kB - 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available - Changes: - all: https://git.openjdk.java.net/jdk/pull/8235/files - new: https://git.openjdk.java.net/jdk/pull/8235/files/5c3a3446..10f14bb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8235=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8235=02-03 Stats: 232900 lines in 3152 files changed: 173230 ins; 42904 del; 16766 mod Patch: https://git.openjdk.java.net/jdk/pull/8235.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8235/head:pull/8235 PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. javac and javadoc changes look OK test/langtools/tools/javac/modules/T8168854/module-info.java line 4: > 2: * @test > 3: * @bug 8168854 > 4: * @summary javac erroneously reject a service interface inner class in a > provides clause FYI, this duplication was in the JBS issue summary; now fixed there. - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8771
RFR: 8286462: Incorrect copyright year for src/java.base/share/classes/jdk/internal/vm/FillerObject.java
The copyright year of src/java.base/share/classes/jdk/internal/vm/FillerObject.java should be 2022. - Commit messages: - [PATCH] 8286462: The copyright year of src/java.base/share/classes/jdk/internal/vm/FillerObject.java should be 2022 Changes: https://git.openjdk.java.net/jdk/pull/8633/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8633=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286462 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8633.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8633/head:pull/8633 PR: https://git.openjdk.java.net/jdk/pull/8633
Integrated: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On Wed, 11 May 2022 21:31:44 GMT, Alexander Matveev 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. > - With proposed fix we will enforce "--strip-native-commands" option for > jlink, so native JDK commands are not included when generating Mac App Store > bundles. > - Custom runtime provided via --runtime-image should not contain native > commands as well, otherwise jpackage will throw error. > - Added two tests to validate fix. This pull request has now been integrated. Changeset: b523c884 Author:Alexander Matveev URL: https://git.openjdk.java.net/jdk/commit/b523c88480ba5c8f9d78537c9de0abcbf1f867c0 Stats: 221 lines in 7 files changed: 216 ins; 1 del; 4 mod 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe Reviewed-by: asemenyuk, kcr - PR: https://git.openjdk.java.net/jdk/pull/8666
Integrated: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java
On Wed, 20 Apr 2022 15:37:13 GMT, Shruthi wrote: > Removing the Duplicate keys present in XSLTErrorResources.java and > XPATHErrorResources.java > > The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 This pull request has now been integrated. Changeset: b5a3d284 Author:Shruthi Committer: Joe Wang URL: https://git.openjdk.java.net/jdk/commit/b5a3d2843be3c093cd3a534caece87a32e5c47cc Stats: 50 lines in 13 files changed: 8 ins; 13 del; 29 mod 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java Reviewed-by: joehw, tsteele - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Tue, 17 May 2022 06:18:25 GMT, Ioi Lam wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use stringStream to simplify controller path assembly > > src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92: > >> 90: } >> 91: ss.print_raw(_root, last_matching_slash_pos); >> 92: _path = os::strdup(ss.base()); > > Do you mean `Find the longest common prefix`? Maybe give an example in the > comments? Text parsing in C code is really difficult to understand. @iklam yes I meant `Find the longest common prefix`. Fixed the comment. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Wed, 18 May 2022 18:09:54 GMT, Severin Gehwolf wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92:
>>
>>> 90: }
>>> 91: ss.print_raw(_root, last_matching_slash_pos);
>>> 92: _path = os::strdup(ss.base());
>>
>> Do you mean `Find the longest common prefix`? Maybe give an example in the
>> comments? Text parsing in C code is really difficult to understand.
>
> @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.
>> test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp line 63:
>>
>>> 61: ASSERT_STREQ(expected_cg_paths[i], ctrl->subsystem_path());
>>> 62: }
>>> 63: }
>>
>> I found it hard to relate the different paths. Could you create a new struct
>> like this?
>>
>>
>> struct TestCase {
>> char* mount_path;
>> char* root_paths;
>> char* cgroup_path;
>> char* expected_cg_paths;
>> } = {
>> { "/sys/fs/cgroup/memory", // mount
>>"/", // root,
>>
>
> Yes, makes sense. Will do.
Done now.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
> 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: - all: https://git.openjdk.java.net/jdk/pull/8629/files - new: https://git.openjdk.java.net/jdk/pull/8629/files/66241aa5..4b8e92fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8629=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8629=01-02 Stats: 93 lines in 3 files changed: 56 ins; 25 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8629.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8629/head:pull/8629 PR: https://git.openjdk.java.net/jdk/pull/8629
RFR: 8286825: Linker naming cleanup
This patch is a batch naming cleanup for the foreign linker implementation. The naming changes are as follows: - ProgrammableInvoker -> DowncallLinker - ProgrammableUpcallHandler -> UpcallLinker - 'native invoker' -> 'downcall stub' - 'optimzed entry blob' -> 'upcall stub' - OptimizedEntryBlob -> UpcallStub - optimized_entry_frame -> upcall_stub_frame Then renaming of some hotspot files: - universalNativeInvoker* -> downcallLinker* - universalUpcallHandler* -> upcallLinker* - foreign_globals* -> foreignGlobals* (to match existing convention) Method, field, and other variable names are also adjusted accordingly. - Commit messages: - 8275648: Linker naming bikeshed Changes: https://git.openjdk.java.net/jdk/pull/8777/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8777=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286825 Stats: 2397 lines in 84 files changed: 1086 ins; 1090 del; 221 mod Patch: https://git.openjdk.java.net/jdk/pull/8777.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8777/head:pull/8777 PR: https://git.openjdk.java.net/jdk/pull/8777
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
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. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284213: Replace usages of 'a the' in xml
On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > I tried to avoid changing external libraries, there are quite a few such > typos. > Let me know if I should revert any files. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8769
Re: RFR: 8284213: Replace usages of 'a the' in xml
On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > I tried to avoid changing external libraries, there are quite a few such > typos. > Let me know if I should revert any files. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8769
Re: RFR: 8284213: Replace usages of 'a the' in xml
On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > I tried to avoid changing external libraries, there are quite a few such > typos. > Let me know if I should revert any files. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8769
RFR: 8267038: Update IANA Language Subtag Registry to Version 2022-03-02
This is to incorporate the latest language subtag registry definition (version 2022-03-02) into JDK19. - Commit messages: - LSR 2022-03-02 - LSR 2021-12-29 - LSR 2021-08-06 - LSR 2021-07-21 Changes: https://git.openjdk.java.net/jdk/pull/8776/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8776=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267038 Stats: 281 lines in 2 files changed: 271 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8776.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8776/head:pull/8776 PR: https://git.openjdk.java.net/jdk/pull/8776
RFR: 8286969: Add a new test library API to execute kinit in SecurityTools.java
A new API to execute kinit. - Commit messages: - 8286969: Add a new test library API to execute kinit in SecurityTools.java Changes: https://git.openjdk.java.net/jdk/pull/8775/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8775=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286969 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8775.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8775/head:pull/8775 PR: https://git.openjdk.java.net/jdk/pull/8775
Re: RFR: 8286182: C2: crash with SIGFPE when executing compiled code
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 its validity checks. > > Thanks. LGTM. - Marked as reviewed by mdoerr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8774
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
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. LGTM for i18n changes. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Logging/JNDI OK - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8771
Integrated: 8286669: Replace MethodHandle specialization with ASM in mainline
On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee wrote: > Hi, > > This PR brings over commits from the panama-foreign repo. These commits > mostly pertain to the switch to ASM and away from MethodHandle combinators > for the binding recipe specialization. But, there is one more commit which > adds freeing of downcall stubs, since those changes were mostly Java as well. > > Thanks, > Jorn > > Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on > Windows and Linux. This pull request has now been integrated. Changeset: ee45a0ac Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/ee45a0ac63613312b4f17dcd55e8defa94c34669 Stats: 2142 lines in 24 files changed: 1357 ins; 660 del; 125 mod 8286669: Replace MethodHandle specialization with ASM in mainline Co-authored-by: Jorn Vernee Co-authored-by: Maurizio Cimadamore Reviewed-by: mcimadamore - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v2]
On Wed, 4 May 2022 23:16:41 GMT, Vladimir Kozlov wrote:
>> src/hotspot/cpu/x86/x86_64.ad line 6998:
>>
>>> 6996: ins_encode %{
>>> 6997: __ cmovl(Assembler::parity, $dst$$Register, $src$$Register);
>>> 6998: __ cmovl(Assembler::notEqual, $dst$$Register, $src$$Register);
>>
>> Should this be `equal`?
>
> I see that you swapped `src, dst` in `match()` but `format` is sill incorrect
> and the code is confusing.
This is a flipping the sense of the test by flipping the input of the `CMove`,
so this is essentially the same as in the above rule. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8525
RFR: 8286182: C2: crash with SIGFPE when executing compiled code
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 its validity checks. Thanks. - Commit messages: - Revert "8284742: x86: Handle integral division overflow during parsing" - Revert "8285390: PPC64: Handle integral division overflow during parsing" Changes: https://git.openjdk.java.net/jdk/pull/8774/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8774=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286182 Stats: 967 lines in 25 files changed: 364 ins; 476 del; 127 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: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 15:05:26 GMT, Jorn Vernee wrote:
>> It's not quite that simple since a binding recipe for a single parameter can
>> have multiple VMStores for instance if a struct is decomposed into multiple
>> values.
>>
>> It can be done by pulling the binding loops up to the `specialize` method,
>> and have if statements for VMStore and VMLoad, like this:
>>
>> for (Binding binding : callingSequence.argumentBindings(i)) {
>> if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forDowncall()) {
>> emitSetOutput(vms.type());
>> } else if (binding instanceof Binding.VMLoad &&
>> callingSequence.forUpcall()) {
>> emitGetInput();
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> And for returns:
>>
>> for (Binding binding : callingSequence.returnBindings()) {
>> if (binding instanceof Binding.VMLoad vml &&
>> callingSequence.forDowncall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitRestoreReturnValue(vml.type());
>> } else {
>> emitReturnBufferLoad(vml);
>> }
>> } else if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forUpcall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitSaveReturnValue(vms.type());
>> } else {
>> emitReturnBufferStore(vms);
>> }
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> But, maybe that's better?
>
> I think someone might look at this and think "why aren't these bindings
> handled by `doBinding`"?
Let's leave it as is for the time being. I guess what I'm trying to get at is
to try and make the code more explicit - but that is hard, given the
constraints.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 118 commits: > > - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into > JEP-19-ASM > - Merge branch 'pr/7959' into JEP-19-ASM > - Merge branch 'master' into JEP-19-VM-IMPL2 > - ifdef NOT_PRODUCT -> ifndef PRODUCT > - Missing ASSERT -> NOT_PRODUCT > - Cleanup UL usage > - Fix failure with SPEC disabled (accidentally dropped change) > - indentation > - fix space > - Merge branch 'master' into JEP-19-ASM > - ... and 108 more: > https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a Looks good! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Looked at -java.security.jgss. LGTM - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
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. Looked at - java.base/.../sun/security - jdk.crypto.* - test/*/com/sun/crypto/provider - test/*/java/lang/SecurityManager - test/*/java/security - test/*/krb5 - test/*/jarsigner and those look fine. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 14:51:02 GMT, Jorn Vernee wrote:
>> I wasn't suggesting to add more bindings. I was more suggesting to filter
>> out the load/store from the set of bindings (since these are virtualized
>> anyways) that are passed to `doBindings`. Then, before executing a set of
>> bindings, (if we are in downcall mode) we load the corresponding input local
>> var. After executing bindings (if we are in upcall mode) we store result in
>> corresponding var.
>>
>> E.g. make the logic that load locals and store locals explicit in the
>> `specialize` method, rather than have parts of it execute "in disguise" as
>> "binding interpretation".
>
> It's not quite that simple since a binding recipe for a single parameter can
> have multiple VMStores for instance if a struct is decomposed into multiple
> values.
>
> It can be done by pulling the binding loops up to the `specialize` method,
> and have if statements for VMStore and VMLoad, like this:
>
> for (Binding binding : callingSequence.argumentBindings(i)) {
> if (binding instanceof Binding.VMStore vms &&
> callingSequence.forDowncall()) {
> emitSetOutput(vms.type());
> } else if (binding instanceof Binding.VMLoad &&
> callingSequence.forUpcall()) {
> emitGetInput();
> } else {
> doBinding(binding);
> }
> }
>
> And for returns:
>
> for (Binding binding : callingSequence.returnBindings()) {
> if (binding instanceof Binding.VMLoad vml &&
> callingSequence.forDowncall()) {
> if (!callingSequence.needsReturnBuffer()) {
> emitRestoreReturnValue(vml.type());
> } else {
> emitReturnBufferLoad(vml);
> }
> } else if (binding instanceof Binding.VMStore vms &&
> callingSequence.forUpcall()) {
> if (!callingSequence.needsReturnBuffer()) {
> emitSaveReturnValue(vms.type());
> } else {
> emitReturnBufferStore(vms);
> }
> } else {
> doBinding(binding);
> }
> }
>
> But, maybe that's better?
I think someone might look at this and think "why aren't these bindings handled
by `doBinding`"?
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8284213: Replace usages of 'a the' in xml
On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > I tried to avoid changing external libraries, there are quite a few such > typos. > Let me know if I should revert any files. Marked as reviewed by dmarkov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8769
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
On Wed, 4 May 2022 01:59:17 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. I have reverted the changes to `java.lang.Float` and `java.lang.Double` to not interfere with the intrinsic PR. More tests are added to cover all cases regarding floating-point comparison of compiled code. The rules for fp comparison that output the result to `rFlagRegsU` are expensive and should be avoided. As a result, I removed the shortcut rules with memory or constant operands to reduce the number of match rules. Only the basic rules are kept. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8525/files - new: https://git.openjdk.java.net/jdk/pull/8525/files/b64e04b5..ba93dcf2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8525=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8525=00-01 Stats: 210103 lines in 2627 files changed: 159508 ins; 36691 del; 13904 mod Patch: https://git.openjdk.java.net/jdk/pull/8525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8525/head:pull/8525 PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 14:21:55 GMT, Maurizio Cimadamore
wrote:
>> I'm not sure if there is anything actionable here?
>>
>> I've thought in the past that it might be nice to have
>> `GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding
>> operators as well, to make the inputs/outputs more explicit in the recipe.
>> But, it doesn't seem like that would make things _much_ better...
>
> I wasn't suggesting to add more bindings. I was more suggesting to filter out
> the load/store from the set of bindings (since these are virtualized anyways)
> that are passed to `doBindings`. Then, before executing a set of bindings,
> (if we are in downcall mode) we load the corresponding input local var. After
> executing bindings (if we are in upcall mode) we store result in
> corresponding var.
>
> E.g. make the logic that load locals and store locals explicit in the
> `specialize` method, rather than have parts of it execute "in disguise" as
> "binding interpretation".
It's not quite that simple since a binding recipe for a single parameter can
have multiple VMStores for instance if a struct is decomposed into multiple
values.
It can be done by pulling the binding loops up to the `specialize` method, and
have if statements for VMStore and VMLoad, like this:
for (Binding binding : callingSequence.argumentBindings(i)) {
if (binding instanceof Binding.VMStore vms &&
callingSequence.forDowncall()) {
emitSetOutput(vms.type());
} else if (binding instanceof Binding.VMLoad &&
callingSequence.forUpcall()) {
emitGetInput();
} else {
doBinding(binding);
}
}
And for returns:
for (Binding binding : callingSequence.returnBindings()) {
if (binding instanceof Binding.VMLoad vml &&
callingSequence.forDowncall()) {
if (!callingSequence.needsReturnBuffer()) {
emitRestoreReturnValue(vml.type());
} else {
emitReturnBufferLoad(vml);
}
} else if (binding instanceof Binding.VMStore vms &&
callingSequence.forUpcall()) {
if (!callingSequence.needsReturnBuffer()) {
emitSaveReturnValue(vms.type());
} else {
emitReturnBufferStore(vms);
}
} else {
doBinding(binding);
}
}
But, maybe that's better?
-
PR: https://git.openjdk.java.net/jdk/pull/8685
RFR: 8284209: Replace remaining usages of 'a the' in source code
Replaces usages of articles that follow each other in all combinations: a/the, an?/an?, the/the… It's the last issue in the series, and it still touches different areas of the code. - Commit messages: - 8284209: Replace remaining usages of 'the a' in source code - 8284209: Replace remaining usages of 'the a' in source code - 8284209: Replace remaining usages of 'the a' in source code - 8284209: Replace remaining usages of 'the the' in source code - 8284209: Replace remaining usages of 'the the' in source code - 8284209: Replace remaining usages of 'the the' in source code - 8284209: Replace remaining usages of 'the the' in source code - 8284209: Replace remaining usages of 'an? an?' in source code - 8284209: Replace remaining usages of 'a the' in source code - 8284209: Replace remaining usages of 'an the' in source code - ... and 1 more: https://git.openjdk.java.net/jdk/compare/69ff86a3...8290c07e Changes: https://git.openjdk.java.net/jdk/pull/8771/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8771=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284209 Stats: 51 lines in 41 files changed: 0 ins; 2 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/8771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8771/head:pull/8771 PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v5]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - 8282662: Revert wrong copyright year change - 8282662: Revert ProxyGenerator - 8282662: Revert ProxyGenerator - 8282662: Revert dubious changes in MethodType - 8282662: Revert dubious changes - 8282662: Use List/Set.of() factory methods to save memory - Changes: https://git.openjdk.java.net/jdk/pull/7729/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7729=04 Stats: 12 lines in 4 files changed: 1 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8284213: Replace usages of 'a the' in xml
On Wed, 18 May 2022 13:58:22 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > I tried to avoid changing external libraries, there are quite a few such > typos. > Let me know if I should revert any files. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8769
Re: Question: Is it possible to introduce ASCII coder for compact strings to optimize performance for ASCII compatible encoding?
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: 8284191: Replace usages of 'a the' in hotspot and java.base
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. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 11:23:07 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
>> line 336:
>>
>>> 334:
>>> 335: if (callingSequence.forUpcall()) {
>>> 336: // for upcalls, recipes have a result, which we handle
>>> here
>>
>> I find this part a bit confusing. The comment speaks about recipes input and
>> outputs, hinting at the fact that downcall bindings have inputs, while
>> upcall bindings have outputs.
>> In reality, if we look at the bindings themselves, they look pretty
>> symmetric:
>>
>> * UNBOX_ADDRESS = pops an Addressable and push a long on the stack
>> * BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
>>
>> That is, in both cases it appears we have an input and an output (and the
>> binding is just the function which maps one into another).
>>
>> I think the input/output asymmetry comes in when we consider the full
>> recipes. In downcalls, for a given argument we will have the following
>> sequence:
>>
>> Binding0, Binding1, ... BindingN-1, VMStore
>>
>> While for upcalls we will have:
>>
>> VMLoad, Binding1, Binding2, ... BindingN
>>
>> So (ignoring return buffer for simplicity), for downcalls, it is obvious
>> that before we can execute Binding0, we need some kind of "input value"
>> (e.g. the value of some local). For upcalls, this is not needed, as the
>> VMLoad binding will basically do that for free.
>> When we finished executing the bindings, again, for downcalls there's
>> nothing to do, as the chain always ends with a VMStore (which stores binding
>> result into some local), while for upcalls we do need to set the output
>> value explicitly.
>>
>> In other words, it seems to me that having VMLoad and VMStore in the chains
>> of bindings to be interpreted/specialized does more harm than good here.
>> These low level bindings make sense for the VM code, as the VM needs to know
>> how to load a value from a register, or how to store a value into a
>> register. But in terms of the specializing Java code, these bindings are
>> immaterial. At the same time, the fact that we have these bindings, and that
>> they are virtualized, makes the code hard to follow, as some preparation
>> happens in `BindingSpecializer::specialize`, while some other preparation
>> happens inside `BindingSpecializer::doBindings`, as part of the virtualized
>> impl of VMLoad/VMStore. And, this virtualized implementation ends up doing
>> similar steps as what the preparation code before/after the binding
>> execution does (e.g. for downcalls/VMStore we call setOutput, while for
>> upcalls/VMLoad we call getInput).
>>
>> All I'm saying here is that, for bindings to work, we _always_ need to call
>> both getInput and setOutput - but it is easy to overlook this when looking
>> at the code, because the getInput/setOutput calls are spread across two
>> different places in the specializing code, perhaps making things more
>> asymmetric than they need to be. (I realize I'm simplifying here, and that
>> some details of the return buffer are not 100% quite as symmetric).
>
> I'm not sure if there is anything actionable here?
>
> I've thought in the past that it might be nice to have
> `GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding
> operators as well, to make the inputs/outputs more explicit in the recipe.
> But, it doesn't seem like that would make things _much_ better...
I wasn't suggesting to add more bindings. I was more suggesting to filter out
the load/store from the set of bindings (since these are virtualized anyways)
that are passed to `doBindings`. Then, before executing a set of bindings, (if
we are in downcall mode) we load the corresponding input local var. After
executing bindings (if we are in upcall mode) we store result in corresponding
var.
E.g. make the logic that load locals and store locals explicit in the
`specialize` method, rather than have parts of it execute "in disguise" as
"binding interpretation".
-
PR: https://git.openjdk.java.net/jdk/pull/8685
RFR: 8284213: Replace usages of 'a the' in xml
Replaces usages of articles that follow each other in all combinations: a/the, an?/an?, the/the… I tried to avoid changing external libraries, there are quite a few such typos. Let me know if I should revert any files. - Commit messages: - 8284213: Replace usages of 'the a' in xml - 8284213: Replace usages of 'the the' in xml - 8284213: Replace usages of 'the the' in xml - 8284213: Replace usages of 'the the' in xml - 8284213: Replace usages of 'an? an?' in xml Changes: https://git.openjdk.java.net/jdk/pull/8769/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8769=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284213 Stats: 17 lines in 9 files changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/8769.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8769/head:pull/8769 PR: https://git.openjdk.java.net/jdk/pull/8769
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 118 commits: > > - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into > JEP-19-ASM > - Merge branch 'pr/7959' into JEP-19-ASM > - Merge branch 'master' into JEP-19-VM-IMPL2 > - ifdef NOT_PRODUCT -> ifndef PRODUCT > - Missing ASSERT -> NOT_PRODUCT > - Cleanup UL usage > - Fix failure with SPEC disabled (accidentally dropped change) > - indentation > - fix space > - Merge branch 'master' into JEP-19-ASM > - ... and 108 more: > https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a LGTM ! - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 118 commits: > > - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into > JEP-19-ASM > - Merge branch 'pr/7959' into JEP-19-ASM > - Merge branch 'master' into JEP-19-VM-IMPL2 > - ifdef NOT_PRODUCT -> ifndef PRODUCT > - Missing ASSERT -> NOT_PRODUCT > - Cleanup UL usage > - Fix failure with SPEC disabled (accidentally dropped change) > - indentation > - fix space > - Merge branch 'master' into JEP-19-ASM > - ... and 108 more: > https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a I think I've addressed most review comments now. Please take another look. - PR: https://git.openjdk.java.net/jdk/pull/8685
RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
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. - Commit messages: - 8284191: Replace usages of 'the an' in hotspot and java.base - 8284191: Replace usages of 'the an' in hotspot and java.base - 8284191: Replace usages of 'the an' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - 8284191: Replace usages of 'the the' in hotspot and java.base - ... and 13 more: https://git.openjdk.java.net/jdk/compare/69ff86a3...c7e73658 Changes: https://git.openjdk.java.net/jdk/pull/8768/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8768=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284191 Stats: 202 lines in 162 files changed: 0 ins; 11 del; 191 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
Re: RFR: 8286956: Loom: Define test groups for development/porting use
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` Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8765
Question: Is it possible to introduce ASCII coder for compact strings to optimize performance for ASCII compatible encoding?
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: 8286669: Replace MethodHandle specialization with ASM in mainline [v2]
On Tue, 17 May 2022 10:19:04 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java
>> line 109:
>>
>>> 107: * @return the caller method type.
>>> 108: */
>>> 109: public MethodType callerMethodType() {
>>
>> Would it make sense to either rename these, or to point to the fact that
>> these are equivalent to Linker::downcallType/upcallType ?
>
> I don't think renaming them to down/upcallType makes sense, since both
> down/up calls use both the caller and callee method type. There can also be
> things that make these not equivalent to downcallType/upcallType, such as the
> SysV need to pass the number of floating point arguments for variadic calls
> as well:
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java#L109-L111
I've left this as is for now.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 08:41:59 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> BootstrapMethodError -> ExceptionInInitializerError
>
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 291:
>
>> 289: emitGetStatic(Binding.Context.class, "DUMMY",
>> BINDING_CONTEXT_DESC);
>> 290: } else {
>> 291: emitInvokeStatic(Binding.Context.class, "ofSession",
>> OF_SESSION_DESC);
>
> Maybe this is micro-optimization, but I realized that in reality we probably
> don't need any scope/session if there are no "ToSegment" bindings.
Done
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 336:
>
>> 334:
>> 335: if (callingSequence.forUpcall()) {
>> 336: // for upcalls, recipes have a result, which we handle
>> here
>
> I find this part a bit confusing. The comment speaks about recipes input and
> outputs, hinting at the fact that downcall bindings have inputs, while upcall
> bindings have outputs.
> In reality, if we look at the bindings themselves, they look pretty symmetric:
>
> * UNBOX_ADDRESS = pops an Addressable and push a long on the stack
> * BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
>
> That is, in both cases it appears we have an input and an output (and the
> binding is just the function which maps one into another).
>
> I think the input/output asymmetry comes in when we consider the full
> recipes. In downcalls, for a given argument we will have the following
> sequence:
>
> Binding0, Binding1, ... BindingN-1, VMStore
>
> While for upcalls we will have:
>
> VMLoad, Binding1, Binding2, ... BindingN
>
> So (ignoring return buffer for simplicity), for downcalls, it is obvious that
> before we can execute Binding0, we need some kind of "input value" (e.g. the
> value of some local). For upcalls, this is not needed, as the VMLoad binding
> will basically do that for free.
> When we finished executing the bindings, again, for downcalls there's nothing
> to do, as the chain always ends with a VMStore (which stores binding result
> into some local), while for upcalls we do need to set the output value
> explicitly.
>
> In other words, it seems to me that having VMLoad and VMStore in the chains
> of bindings to be interpreted/specialized does more harm than good here.
> These low level bindings make sense for the VM code, as the VM needs to know
> how to load a value from a register, or how to store a value into a register.
> But in terms of the specializing Java code, these bindings are immaterial. At
> the same time, the fact that we have these bindings, and that they are
> virtualized, makes the code hard to follow, as some preparation happens in
> `BindingSpecializer::specialize`, while some other preparation happens inside
> `BindingSpecializer::doBindings`, as part of the virtualized impl of
> VMLoad/VMStore. And, this virtualized implementation ends up doing similar
> steps as what the preparation code before/after the binding execution does
> (e.g. for downcalls/VMStore we call setOutput, while for upcalls/VMLoad we
> call getInput).
>
> All I'm saying here is that, for bindings to work, we _always_ need to call
> both getInput and setOutput - but it is easy to overlook this when looking at
> the code, because the getInput/setOutput calls are spread across two
> different places in the specializing code, perhaps making things more
> asymmetric than they need to be. (I realize I'm simplifying here, and that
> some details of the return buffer are not 100% quite as symmetric).
I'm not sure if there is anything actionable here?
I've thought in the past that it might be nice to have
`GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding
operators as well, to make the inputs/outputs more explicit in the recipe. But,
it doesn't seem like that would make things _much_ better...
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 11:18:20 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
>> line 943:
>>
>>> 941: Z, B, S, C, I, J, F, D, L;
>>> 942:
>>> 943: static BasicType of(Class cls) {
>>
>> This seems a duplication of ASM Type class for no good reason,
>> Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper
>> variant opcode
>
> Didn't know about that. Neat!
Removed the `BasicType` enum, switched to using `Type` and `Type.getSort()`.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 06:00:37 GMT, Rémi Forax wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> BootstrapMethodError -> ExceptionInInitializerError
>
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 157:
>
>> 155: }
>> 156:
>> 157: static MethodHandle doSpecialize(MethodHandle leafHandle,
>> CallingSequence callingSequence, ABIDescriptor abi) {
>
> I think thise method should be split in to, one version for the upcall, one
> for the downcall, i do not see the point of merging the two codes.
I've split this method into a specializeDowncall and specializeUpcall method.
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 816:
>
>> 814: return;
>> 815: }
>> 816: if (con instanceof Integer) {
>
> those can use the instanceof + type pattern
Done
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 967:
>
>> 965:
>> 966: // unaligned constants
>> 967: public final static ValueLayout.OfBoolean JAVA_BOOLEAN_UNALIGNED =
>> JAVA_BOOLEAN;
>
> as far as i understand, those constants are accessed from the bytecode, they
> should be in their own class to cleanly separate the specializer part and the
> runtime part.
I've put these in a nest `Runtime` class, with a comment explaining that they
are referenced from the generated code.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 10:08:00 GMT, Jorn Vernee wrote: >> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java >> line 131: >> >>> 129: private int[] scopeSlots; >>> 130: private int curScopeLocalIdx = -1; >>> 131: private int RETURN_ALLOCATOR_IDX = -1; >> >> These are not constants, so it is odd to see the name capitalized > > Right, habit of writing lambda forms, where name variables are capitalized. Changed to lowercase. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
> Hi, > > This PR brings over commits from the panama-foreign repo. These commits > mostly pertain to the switch to ASM and away from MethodHandle combinators > for the binding recipe specialization. But, there is one more commit which > adds freeing of downcall stubs, since those changes were mostly Java as well. > > Thanks, > Jorn > > Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on > Windows and Linux. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 118 commits: - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into JEP-19-ASM - Merge branch 'pr/7959' into JEP-19-ASM - Merge branch 'master' into JEP-19-VM-IMPL2 - ifdef NOT_PRODUCT -> ifndef PRODUCT - Missing ASSERT -> NOT_PRODUCT - Cleanup UL usage - Fix failure with SPEC disabled (accidentally dropped change) - indentation - fix space - Merge branch 'master' into JEP-19-ASM - ... and 108 more: https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a - Changes: https://git.openjdk.java.net/jdk/pull/8685/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8685=04 Stats: 2142 lines in 24 files changed: 1357 ins; 660 del; 125 mod Patch: https://git.openjdk.java.net/jdk/pull/8685.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8685/head:pull/8685 PR: https://git.openjdk.java.net/jdk/pull/8685
RFR: 8286956: Loom: Define test groups for development/porting use
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` - [ ] Linux AArch64 fastdebug `hotspot_loom jdk_loom` - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/8765/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8765=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286956 Stats: 28 lines in 2 files changed: 28 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8765.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8765/head:pull/8765 PR: https://git.openjdk.java.net/jdk/pull/8765
Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]
On Wed, 18 May 2022 07:40:43 GMT, Thomas Stuefe wrote:
>> src/java.base/share/native/libjli/java.c line 1631:
>>
>>> 1629: const char *arg = jargv[i];
>>> 1630: if (arg[0] == '-' && arg[1] == 'J') {
>>> 1631: assert(arg[2] != '\0' && "Invalid JAVA_ARGS or
>>> EXTRA_JAVA_ARGS defined by build");
>>
>> Interesting trick.
>
> Since we pass NDEBUG, this assert is disabled in release builds. This is the
> supposed behavior, right? That we don't do anything on release builds?
Right - that is what I would expect from an assert.
-
PR: https://git.openjdk.java.net/jdk/pull/8694
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v4]
On Tue, 17 May 2022 16:22:15 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'pr/7959' into JEP-19-ASM > - BootstrapMethodError -> ExceptionInInitializerError > - Use unaligned layout constants when filling in reconstituted structs (was > accidentally dropped change) > - Fix LinkUpcall benchmark > - 8286306: Upcall wrapper class sharing > >Reviewed-by: mcimadamore > - Polish > - 8281595: ASM-ify scope acquire/release for down call parameters >8281387: Some downcall shapes show unexpected allocations > >Co-authored-by: Maurizio Cimadamore >Reviewed-by: mcimadamore > - 8281228: Preview branch's CLinker.downcallHandle crashes inside asm > >Reviewed-by: sundar, jvernee > - 8278414: Replace binding recipe customization using MH combinators with > bytecode spinning > >Reviewed-by: mcimadamore > - 8276648: Downcall stubs are never freed > >Reviewed-by: mcimadamore The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout JEP-19-ASM git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push - PR: https://git.openjdk.java.net/jdk/pull/8685
Integrated: 8283689: Update the foreign linker VM implementation
On Fri, 25 Mar 2022 13:48:20 GMT, Jorn Vernee wrote: > Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 This pull request has now been integrated. Changeset: 81e4bdbe Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/81e4bdbe1358b7feced08ba758ddb66415968036 Stats: 6914 lines in 155 files changed: 2577 ins; 3219 del; 1118 mod 8283689: Update the foreign linker VM implementation Co-authored-by: Jorn Vernee Co-authored-by: Nick Gasson Reviewed-by: mcimadamore, vlivanov, rehn - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Mon, 16 May 2022 15:29:38 GMT, Сергей Цыпанов wrote: >> Even in the no exceptions case, the `exceptionTypes` array still has to be >> allocated/copied by `Method.getExceptionTypes()`[^1] when the `ProxyMethod` >> constructor[^2] is invoked. >> >> So if anything, the `List.of(…)` call should be moved into the `ProxyMethod` >> constructor. >> And maybe the call to `Method.getExceptionTypes()` should be changed to >> `Method.getSharedExceptionTypes()`[^3]. >> >> [^1]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L340-L343 >> [^2]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L710-L714 >> [^3]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L305-L308 > >> So if anything, the List.of(…) call should be moved into the ProxyMethod >> constructor. And maybe the call to Method.getExceptionTypes() should be >> changed to Method.getSharedExceptionTypes() > > Makes sense. Do you want me to do this within this PR (this would be a > deviation from ticket's target), or it's better to create another one? Ok, I've reverted `ProxyGenerator` and will include the changes into another PR - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v4]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - 8282662: Revert ProxyGenerator - 8282662: Revert ProxyGenerator - 8282662: Revert dubious changes in MethodType - 8282662: Revert dubious changes - 8282662: Use List/Set.of() factory methods to save memory - Changes: - all: https://git.openjdk.java.net/jdk/pull/7729/files - new: https://git.openjdk.java.net/jdk/pull/7729/files/e765c42a..87c6623b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7729=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7729=02-03 Stats: 575367 lines in 7617 files changed: 421069 ins; 87224 del; 67074 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]
On Wed, 18 May 2022 06:12:41 GMT, David Holmes wrote:
>> Yasumasa Suenaga 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 three additional
>> commits since the last revision:
>>
>> - Use assert() to check jargv
>> - Merge remote-tracking branch 'upstream/master' into JDK-8286694
>> - 8286694: Incorrect argument processing in java launcher
>
> src/java.base/share/native/libjli/java.c line 1631:
>
>> 1629: const char *arg = jargv[i];
>> 1630: if (arg[0] == '-' && arg[1] == 'J') {
>> 1631: assert(arg[2] != '\0' && "Invalid JAVA_ARGS or
>> EXTRA_JAVA_ARGS defined by build");
>
> Interesting trick.
Since we pass NDEBUG, this assert is disabled in release builds. This is the
supposed behavior, right? That we don't do anything on release builds?
-
PR: https://git.openjdk.java.net/jdk/pull/8694
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v8]
On Mon, 16 May 2022 18:27:23 GMT, Joe Wang wrote: >> Do not enclose the skara command within backquotes. > >> Do not enclose the skara command within backquotes. > > Thanks Naoto! I didn't realize that's what he did :-) @JoeWang-Java I have issued the **integrate** command. Now it is ready to be sponsored. Please do the needful - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option [v3]
> ### Problem description > Minimal jdk image created by jlink with the only jdk.compiler module and its > dependencies > fails to run java source launcher correctly (for example when --source N is > specified). > Failing source launcher is only one the expressions of internal jdk.compiler > malfunction, another example is failure to run javac with --release option. > > ### Root cause > Module jdk.compiler requires zip filesystem (jdk.zipfs module) to parse > ct.sym file and so to provide full functionality. > Module jdk.zipfs is not included in the minimal JDK image, because module > jdk.compiler does not declare it as "requires" in its module info. > > ### Alternative patch > The problem can be alternatively resolved by complex code checks in > jdk.compiler to provide user with valid error message, however that solution > would be just a workaround for jdk.compiler dual functionality (with or > without presence of jdk.zipfs module). Compiler functionality without access > to ct.sym through jdk.zipfs is very limited. > > ### Proposed fix > This patch fixes the problem by explicit declaration of jdk.compiler > dependency on jdk.zipfs. > Plus added specific test case. > > Please review the patch or raise objections against declaration of > jdk.compiler dependent on jdk.zipfs. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: corrected LimitedImage test description - Changes: - all: https://git.openjdk.java.net/jdk/pull/8747/files - new: https://git.openjdk.java.net/jdk/pull/8747/files/6bdf4a9a..0e55575b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8747=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8747=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8747.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8747/head:pull/8747 PR: https://git.openjdk.java.net/jdk/pull/8747
Re: RFR: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option [v2]
On Wed, 18 May 2022 02:50:49 GMT, Jaikiran Pai wrote: > Hello Adam, I don't have necessary knowledge of this area, so I don't know > what approach would be preferred. > > But a couple of questions: > > * If we do decide to add the `jdk.zipfs` dependency to `jdk.compiler` module, > do you think the `LimitedImage` test is still relevant? Or should it be > removed? The comment on that test states: > > > @summary Verify javac behaves properly in absence of zip/jar > > FileSystemProvider > > and it does so by using `--limit-modules`. But now with the `jdk.zipfs` being > a dependency, the zip/jar FileSystemProvider will not be absent and the test > then would seem unnecessary. You are right, the test description should change. The test name is LimitedImage and it verifies that javac behaves properly in limited JDK image. I think the purpose of the test is still the same, just results are different (now it pass compilation instead of failing with specific error). Thanks for pointing it out. > > * In the JBS issue corresponding to this PR, you noted that: > > > There are actually two different issues: > > First in JDKPlatformProvider, which silently swallows > > ProviderNotFoundException. > > Should something be done there? I see that the catch block happens to reside > in the `static` block of that class (which also happens to be a > implementation of a Java `service`), so I'm unsure what if anything can be > done there. Silent swallow of an exception in static initializer of a service is not very good practice and it has significant effect on this issue right now. However it will express itself only in corner cases (like for example broken ct.sym file) after this patch is integrated. - PR: https://git.openjdk.java.net/jdk/pull/8747
Re: RFR: 8286694: Incorrect argument processing in java launcher
On Tue, 17 May 2022 13:54:59 GMT, Yasumasa Suenaga wrote:
>> You forced me to look at this in depth. The values are set via the build
>> system. In the build system it is impossible to get just -J because the -J
>> is only added as a prefix for an existing string. So basically this is
>> impossible code to reach unless you manually override the build system
>> definition. So as far as I can see this is a classic case where we want an
>> assert.
>>
>>
>> if (arg[0] == '-' && arg[1] == 'J') {
>> assert(arg[2] != '\0', "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by
>> build");
>> *nargv++ = JLI_StringDup(arg + 2);
>> }
>
> @dholmes-ora Thanks for your comment!
>
> We cannot use `assert(cond, message)` at here because it is not HotSpot code.
> In imageFile.cpp for libjimage. It uses `assert(cond && message)`, so I use
> this style in new commit, and the warning has gone.
>
> I can change to "normal" `assert` usage at here if it is strange.
@YaSuenag I know this is not hotspot code. Please see existing use of assert in
related code i.e. src/java.base/share/native/libjli/args.c
-
PR: https://git.openjdk.java.net/jdk/pull/8694
Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]
On Tue, 17 May 2022 13:55:43 GMT, Yasumasa Suenaga wrote:
>> GCC 12 reports as following:
>
> Yasumasa Suenaga 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 three additional
> commits since the last revision:
>
> - Use assert() to check jargv
> - Merge remote-tracking branch 'upstream/master' into JDK-8286694
> - 8286694: Incorrect argument processing in java launcher
Looks fine to me.
Thanks.
Sorry I see what you mean, it is just an `assert(cond)` not `assert(cond,
message)`, but that is fine.
src/java.base/share/native/libjli/java.c line 1631:
> 1629: const char *arg = jargv[i];
> 1630: if (arg[0] == '-' && arg[1] == 'J') {
> 1631: assert(arg[2] != '\0' && "Invalid JAVA_ARGS or
> EXTRA_JAVA_ARGS defined by build");
Interesting trick.
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8694
