Re: RFR: 8255785: X11 libraries should not be required by configure for headless only
On 2020-11-03 05:27, Sergey Bylokhov wrote: On Tue, 3 Nov 2020 01:07:51 GMT, Phil Race wrote: If we build a headless only JDK, configure will require X11 libraries and headers to be present. This used to be necessary, but thanks to massive cleanups in the AWT headless code, this is no longer the case. We should fix configure so that headless can be built without X11 libraries and headers. Marked as reviewed by prr (Reviewer). Do we have a tier 1-build task or something that will check that we did not break building headless w/o x11? There is no regular testing of headless-only. However, the only test needed for this fix was a single confirmation that the headless libraries indeed compile even without X_CFLAGS and X_LIBS. (This was apparent from the code since the make rules did not use X_CFLAGS and X_LIBS, but just to be 100% sure, I also tried building it.) /Magnus
Re: RFR: 8255785: X11 libraries should not be required by configure for headless only
On Tue, 3 Nov 2020 04:25:37 GMT, Sergey Bylokhov wrote: >> Marked as reviewed by prr (Reviewer). > > Do we have a tier 1-build task or something that will check that we did not > break building headless w/o x11? I think you are confusing the modest goal of making it possible, with ensure .. Would you want a headless build to actually fail if the build environment had X11 libraries ? If your real question is "will anyone bite my head off if I break this by accident" ? No. - PR: https://git.openjdk.java.net/jdk/pull/1025
Re: RFR: 8255785: X11 libraries should not be required by configure for headless only
On Tue, 3 Nov 2020 01:07:51 GMT, Phil Race wrote: >> If we build a headless only JDK, configure will require X11 libraries and >> headers to be present. This used to be necessary, but thanks to massive >> cleanups in the AWT headless code, this is no longer the case. >> >> We should fix configure so that headless can be built without X11 libraries >> and headers. > > Marked as reviewed by prr (Reviewer). Do we have a tier 1-build task or something that will check that we did not break building headless w/o x11? - PR: https://git.openjdk.java.net/jdk/pull/1025
Re: BUG：j2re-image lost two dll files, j2sdk-image/jre have them
Hi, On 29/10/2020 1:21 pm, 柳鲲鹏 wrote: Hello! I have built OpenJDK8-272 on windows 10. I found there is a bug: Compre files between j2re-image and j2sdk-image, j2re-image lost two files: sawindbg.dll attach.dll A JRE and a JDK have different contents - this is one of those differences. A JRE is just for running Java applications and doesn't have the development tools that are part of the JDK. Those two libraries are development tools. Cheers, David I build severial times, it always ocurred. Thanks. Quantum6, Tai Shan IT.
Integrated: 8255785: X11 libraries should not be required by configure for headless only
On Tue, 3 Nov 2020 00:33:09 GMT, Magnus Ihse Bursie wrote: > If we build a headless only JDK, configure will require X11 libraries and > headers to be present. This used to be necessary, but thanks to massive > cleanups in the AWT headless code, this is no longer the case. > > We should fix configure so that headless can be built without X11 libraries > and headers. This pull request has now been integrated. Changeset: f97ec359 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/f97ec359 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod 8255785: X11 libraries should not be required by configure for headless only Reviewed-by: mikael, prr - PR: https://git.openjdk.java.net/jdk/pull/1025
Re: RFR: 8255785: X11 libraries should not be required by configure for headless only
On Tue, 3 Nov 2020 00:33:09 GMT, Magnus Ihse Bursie wrote: > If we build a headless only JDK, configure will require X11 libraries and > headers to be present. This used to be necessary, but thanks to massive > cleanups in the AWT headless code, this is no longer the case. > > We should fix configure so that headless can be built without X11 libraries > and headers. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1025
Re: RFR: 8255785: X11 libraries should not be required by configure for headless only
On Tue, 3 Nov 2020 00:33:09 GMT, Magnus Ihse Bursie wrote: > If we build a headless only JDK, configure will require X11 libraries and > headers to be present. This used to be necessary, but thanks to massive > cleanups in the AWT headless code, this is no longer the case. > > We should fix configure so that headless can be built without X11 libraries > and headers. Marked as reviewed by mikael (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1025
RFR: 8255785: X11 libraries should not be required by configure for headless only
If we build a headless only JDK, configure will require X11 libraries and headers to be present. This used to be necessary, but thanks to massive cleanups in the AWT headless code, this is no longer the case. We should fix configure so that headless can be built without X11 libraries and headers. - Commit messages: - 8255785: X11 libraries should not be required by configure for headless only Changes: https://git.openjdk.java.net/jdk/pull/1025/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1025&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255785 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1025.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1025/head:pull/1025 PR: https://git.openjdk.java.net/jdk/pull/1025
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Tue, 3 Nov 2020 00:00:26 GMT, Kiran Sidhartha Ravikumar wrote: >> My question is why it is failing. Have you figured it? The existing >> exceptions are either negative DST or last rule beyond 2037, which javazic >> cannot handle. The changes introduced with 2020d does not meet either of >> them. Unless we know why it is failing, we cannot be sure we can exclude it. > > Thanks for getting back Naoto, I believe this is a long-standing issue - > https://bugs.openjdk.java.net/browse/JDK-8227293. > > Looking back at the integration of tzdata2019a/tzdata2019b, the same issue > was addressed by updating the source code - > https://hg.openjdk.java.net/jdk/jdk/rev/79036e5e744b#l13.1. > > Here is some history to the issue - > https://mail.openjdk.java.net/pipermail/i18n-dev/2019-July/002887.html > > Please let me know your thoughts Should we then remove the hack in the ZoneInfoFile.java that excludes Gaza/Hebron instead? - PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 18:14:47 GMT, Naoto Sato wrote: >> It's probably these last rule what is causing the issue >> >> Rule Palestine 2020max - Mar Sat>=24 0:001:00 >> S >> Rule Palestine 2020max - Oct Sat>=24 1:000 >> - >> >> The failure seen is >> >> ** >> Asia/Gaza : Asia/Gaza >> - >> SimpleTimeZone (NG) >> >> stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0] >> >> stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0] > > My question is why it is failing. Have you figured it? The existing > exceptions are either negative DST or last rule beyond 2037, which javazic > cannot handle. The changes introduced with 2020d does not meet either of > them. Unless we know why it is failing, we cannot be sure we can exclude it. Thanks for getting back Naoto, I believe this is a long-standing issue - https://bugs.openjdk.java.net/browse/JDK-8227293. Looking back at the integration of tzdata2019a/tzdata2019b, the same issue was addressed by updating the source code - https://hg.openjdk.java.net/jdk/jdk/rev/79036e5e744b#l13.1. Here is some history to the issue - https://mail.openjdk.java.net/pipermail/i18n-dev/2019-July/002887.html Please let me know your thoughts - PR: https://git.openjdk.java.net/jdk/pull/1012
Integrated: JDK-8255732: OpenJDK fails to build if $A is set to a value with spaces
On Mon, 2 Nov 2020 14:40:35 GMT, Erik Joelsson wrote: > We have at least one java file with a '$' in the name. As we have learned > over the years, having $ in unexpected places quickly leads to unexpected > behavior in a shell/make based build. In this case it's our override > mechanism of java files that needs protection against expanding such > occurrences of $. This pull request has now been integrated. Changeset: 184db64d Author:Erik Joelsson URL: https://git.openjdk.java.net/jdk/commit/184db64d Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8255732: OpenJDK fails to build if $A is set to a value with spaces Reviewed-by: ihse - PR: https://git.openjdk.java.net/jdk/pull/1007
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Mon, 2 Nov 2020 20:22:21 GMT, Vladimir Kozlov wrote: >> Bernhard Urban-Forster 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: >> >> - add missing precompiled.hpp include >> - Merge remote-tracking branch 'upstream/master' into >> 8254827-enable-jvmci-win-aarch64 >> - rename argument to canUsePlatformRegister >> - comment for platformRegister >> - 8254827: JVMCI: Enable it for Windows+AArch64 >> >>Use r18 as allocatable register on Linux only. >> >>A bootstrap works now (it has been crashing before due to r18 being >> allocated): >>```console >>$ ./windows-aarch64-server-fastdebug/bin/java.exe >> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI >> -version >>Bootstrapping JVMCI. in 17990 ms >>(compiled 3330 methods) >>openjdk version "16-internal" 2021-03-16 >>OpenJDK Runtime Environment (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >>OpenJDK 64-Bit Server VM (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >>``` >> >>Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. > > Marked as reviewed by kvn (Reviewer). Thanks for the review Tom and Magnus, and for the comments to Vladimir and Doug 🙂 - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 18:15:09 GMT, Jan Lahoda wrote:
>> This is an update to javac and javadoc, to introduce support for Preview
>> APIs, and generally improve javac and javadoc behavior to more closely
>> adhere to JEP 12.
>>
>> The notable changes are:
>>
>> * adding support for Preview APIs (javac until now supported primarily only
>> preview language features, and APIs associated with preview language
>> features). This includes:
>> * the @PreviewFeature annotation has boolean attribute "reflective",
>> which should be true for reflective Preview APIs, false otherwise. This
>> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>> * the preview warnings for preview APIs are auto-suppressed as
>> described in the JEP 12. E.g. the module that declares the preview API is
>> free to use it without warnings
>> * improving error/warning messages. Please see [1] for a list of
>> cases/examples.
>> * class files are only marked as preview if they are using a preview
>> feature. [1] also shows if a class file is marked as preview or not.
>> * the PreviewFeature annotation has been moved to jdk.internal.javac
>> package (originally was in the jdk.internal package).
>> * Preview API in JDK's javadoc no longer needs to specify @preview tag in
>> the source files. javadoc will auto-generate a note for @PreviewFeature
>> elements, see e.g. [2] and [3] (non-reflective and reflective API,
>> respectively). A summary of preview elements is also provided [4]. Existing
>> uses of @preview have been updated/removed.
>> * non-JDK javadoc is also enhanced to auto-generate preview notes for uses
>> of Preview elements, and for declaring elements using preview language
>> features [5].
>>
>> Please also see the CSR [6] for more information.
>>
>> [1]
>> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>> [2]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>> [3]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>> [4]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>> [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>> [6] https://bugs.openjdk.java.net/browse/JDK-8250769
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 46 commits:
>
> - Removing trailing whitespace.
> - Merging master into JDK-8250768.
> - Updating tests after records are a final feature.
> - Fixing tests.
> - Finalizing removal of record preview hooks.
> - Merging master into JDK-8250768
> - Reflecting review comments.
> - Merge branch 'master' into JDK-8250768
> - Removing unnecessary cast.
> - Using a more correct way to get URLs.
> - ... and 36 more:
> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900
I have read all the files.
I have added a n umber of various minor non-blocking comments (no need for
re-review( to fix these. But I have a couple of comments/questions before
finally giving approval.
There's a comment in `PreviewListWriter` about annotation members that needs
too be addressed, and I wonder is RECORD and RECORD_COMPONENT need to be added
into PreviewElementKind.
src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 75:
> 73: * A key for testing.
> 74: */
> 75: TEST,
Slightly weird
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTaskPool.java line
257:
> 255: //when patching modules (esp. java.base), it may be
> impossible to
> 256: //clear the symbols read from the patch path:
> 257: polluted |=
> get(JavaFileManager.class).hasLocation(StandardLocation.PATCH_MODULE_PATH);
OK, but looks unrelated to primary work
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 218:
> 216: return Errors.PreviewFeatureDisabledClassfile(classfile,
> majorVersionToSource.get(majorVersion).name);
> 217: }
> 218:
Up above in isPreview, lines 185-188, I'm supervised it's not a `switch`
statement. (Can't annotate those lines directly)
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
line 89:
> 87: @Override
> 88: protected Content getDeprecatedOrPreviewLink(Element member) {
> 89: Content content = new ContentBuilder();
Yeah the shorter name is good here and more in keeping with the code style
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
line 93:
> 91: if (!utils.isConstructor(member)) {
> 92: content.add(".");
> 93: content.add(member.getSimpleName());
this is OK, but generally FYI, `Content` is now set up to allow chained method
calls.
src/jdk.javadoc/share
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Tue, 27 Oct 2020 16:09:45 GMT, Jan Lahoda wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
>> line 88:
>>
>>> 86:
>>> 87: @Override
>>> 88: protected Content getDeprecatedOrPreviewLink(Element member) {
>>
>> This name is a side-effect of the `ElementFlag` question. We should either
>> use explicit field and method names, or we should use `ElementFlag` more
>> consistently. This method name works OK for now, but if if ever gets to
>> have another `orFoo` component in the name, the signature should be
>> parameterized with something like `ElementFlag` or its equivalent.
>
> Note this method returns the same link for deprecate or preview - it just was
> named "getDeprecatedLink", and when using it work preview, I renamed it
> "getDeprecatedOrPreviewLink". We may need to think of a better name.
This name is OK for now. Maybe we'll have m ore insight into a better name
if/when we add a third variant ;-)
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Fri, 16 Oct 2020 16:07:41 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 46 commits:
>>
>> - Removing trailing whitespace.
>> - Merging master into JDK-8250768.
>> - Updating tests after records are a final feature.
>> - Fixing tests.
>> - Finalizing removal of record preview hooks.
>> - Merging master into JDK-8250768
>> - Reflecting review comments.
>> - Merge branch 'master' into JDK-8250768
>> - Removing unnecessary cast.
>> - Using a more correct way to get URLs.
>> - ... and 36 more:
>> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/util/RawDiagnosticFormatter.java
> line 166:
>
>> 164: s = "compiler.misc.tree.tag." +
>> StringUtils.toLowerCase(((Tag) arg).name());
>> 165: } else if (arg instanceof Source && arg == Source.DEFAULT &&
>> 166:
>> CODES_NEEDING_SOURCE_NORMALIZATION.contains(diag.getCode())) {
>
> Nice trick to keep raw output constant across versions :-)
👍
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Tue, 20 Oct 2020 15:46:36 GMT, Bernhard Urban-Forster wrote: >> Use r18 as allocatable register on Linux only. >> >> A bootstrap works now (it has been crashing before due to r18 being >> allocated): >> $ ./windows-aarch64-server-fastdebug/bin/java.exe >> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI >> -version >> Bootstrapping JVMCI. in 17990 ms >> (compiled 3330 methods) >> openjdk version "16-internal" 2021-03-16 >> OpenJDK Runtime Environment (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >> >> Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. > > Bernhard Urban-Forster 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: > > - add missing precompiled.hpp include > - Merge remote-tracking branch 'upstream/master' into > 8254827-enable-jvmci-win-aarch64 > - rename argument to canUsePlatformRegister > - comment for platformRegister > - 8254827: JVMCI: Enable it for Windows+AArch64 > >Use r18 as allocatable register on Linux only. > >A bootstrap works now (it has been crashing before due to r18 being > allocated): >```console >$ ./windows-aarch64-server-fastdebug/bin/java.exe > -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI > -version >Bootstrapping JVMCI. in 17990 ms >(compiled 3330 methods) >openjdk version "16-internal" 2021-03-16 >OpenJDK Runtime Environment (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >OpenJDK 64-Bit Server VM (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >``` > >Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. Marked as reviewed by kvn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Mon, 2 Nov 2020 20:05:10 GMT, Bernhard Urban-Forster wrote: >> make/autoconf/jvm-features.m4 line 309: >> >>> 307: if test "x$OPENJDK_TARGET_CPU" = "xx86_64"; then >>> 308: AC_MSG_RESULT([yes]) >>> 309: elif test "x$OPENJDK_TARGET_CPU" = "xaarch64"; then >> >> You are missing the same change for JVM_FEATURES_CHECK_JVMCI. >> Unless it is done intentionally. > > It's done a couple lines below: > https://github.com/openjdk/jdk/pull/685/files#diff-a09b08bcd422d0a8fb32a95ccf85051ac1e69bef2bd420d579f74d8efa286d2fL343 > > Or do you mean something else? Uhh. Sorry, I thought JVMCI definition will be first, before Graal and did not look below. - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Mon, 2 Nov 2020 19:33:39 GMT, Vladimir Kozlov wrote: >> Bernhard Urban-Forster 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: >> >> - add missing precompiled.hpp include >> - Merge remote-tracking branch 'upstream/master' into >> 8254827-enable-jvmci-win-aarch64 >> - rename argument to canUsePlatformRegister >> - comment for platformRegister >> - 8254827: JVMCI: Enable it for Windows+AArch64 >> >>Use r18 as allocatable register on Linux only. >> >>A bootstrap works now (it has been crashing before due to r18 being >> allocated): >>```console >>$ ./windows-aarch64-server-fastdebug/bin/java.exe >> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI >> -version >>Bootstrapping JVMCI. in 17990 ms >>(compiled 3330 methods) >>openjdk version "16-internal" 2021-03-16 >>OpenJDK Runtime Environment (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >>OpenJDK 64-Bit Server VM (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >>``` >> >>Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. > > make/autoconf/jvm-features.m4 line 309: > >> 307: if test "x$OPENJDK_TARGET_CPU" = "xx86_64"; then >> 308: AC_MSG_RESULT([yes]) >> 309: elif test "x$OPENJDK_TARGET_CPU" = "xaarch64"; then > > You are missing the same change for JVM_FEATURES_CHECK_JVMCI. > Unless it is done intentionally. It's done a couple lines below: https://github.com/openjdk/jdk/pull/685/files#diff-a09b08bcd422d0a8fb32a95ccf85051ac1e69bef2bd420d579f74d8efa286d2fL343 Or do you mean something else? - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Tue, 20 Oct 2020 15:46:36 GMT, Bernhard Urban-Forster wrote: >> Use r18 as allocatable register on Linux only. >> >> A bootstrap works now (it has been crashing before due to r18 being >> allocated): >> $ ./windows-aarch64-server-fastdebug/bin/java.exe >> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI >> -version >> Bootstrapping JVMCI. in 17990 ms >> (compiled 3330 methods) >> openjdk version "16-internal" 2021-03-16 >> OpenJDK Runtime Environment (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >> >> Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. > > Bernhard Urban-Forster 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: > > - add missing precompiled.hpp include > - Merge remote-tracking branch 'upstream/master' into > 8254827-enable-jvmci-win-aarch64 > - rename argument to canUsePlatformRegister > - comment for platformRegister > - 8254827: JVMCI: Enable it for Windows+AArch64 > >Use r18 as allocatable register on Linux only. > >A bootstrap works now (it has been crashing before due to r18 being > allocated): >```console >$ ./windows-aarch64-server-fastdebug/bin/java.exe > -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI > -version >Bootstrapping JVMCI. in 17990 ms >(compiled 3330 methods) >openjdk version "16-internal" 2021-03-16 >OpenJDK Runtime Environment (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >OpenJDK 64-Bit Server VM (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >``` > >Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. Changes requested by kvn (Reviewer). make/autoconf/jvm-features.m4 line 309: > 307: if test "x$OPENJDK_TARGET_CPU" = "xx86_64"; then > 308: AC_MSG_RESULT([yes]) > 309: elif test "x$OPENJDK_TARGET_CPU" = "xaarch64"; then You are missing the same change for JVM_FEATURES_CHECK_JVMCI. Unless it is done intentionally. - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64 [v3]
On Tue, 20 Oct 2020 15:46:36 GMT, Bernhard Urban-Forster wrote: >> Use r18 as allocatable register on Linux only. >> >> A bootstrap works now (it has been crashing before due to r18 being >> allocated): >> $ ./windows-aarch64-server-fastdebug/bin/java.exe >> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI >> -version >> Bootstrapping JVMCI. in 17990 ms >> (compiled 3330 methods) >> openjdk version "16-internal" 2021-03-16 >> OpenJDK Runtime Environment (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >> >> Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. > > Bernhard Urban-Forster 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: > > - add missing precompiled.hpp include > - Merge remote-tracking branch 'upstream/master' into > 8254827-enable-jvmci-win-aarch64 > - rename argument to canUsePlatformRegister > - comment for platformRegister > - 8254827: JVMCI: Enable it for Windows+AArch64 > >Use r18 as allocatable register on Linux only. > >A bootstrap works now (it has been crashing before due to r18 being > allocated): >```console >$ ./windows-aarch64-server-fastdebug/bin/java.exe > -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI > -version >Bootstrapping JVMCI. in 17990 ms >(compiled 3330 methods) >openjdk version "16-internal" 2021-03-16 >OpenJDK Runtime Environment (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) >OpenJDK 64-Bit Server VM (fastdebug build > 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) >``` > >Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. Marked as reviewed by never (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 17:43:31 GMT, Andrew Haley wrote: >> https://github.com/openjdk/jdk/pull/1013 > >> @lewurm >> This patch seems to break on linux-aarch64 with gcc: > > Builds cleanly on Linux/GCC or me. @theRealAph what gcc version? I can reproduce with $ gcc --version gcc (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008 which ships in Ubuntu 19.10 as default - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 18:06:28 GMT, Kiran Sidhartha Ravikumar
wrote:
>> test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
>>
>>> 199: zid.equals("Iran") || // last rule mismatch
>>> 200: zid.equals("Asia/Gaza") || // last rule mismatch
>>> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
>>
>> Can you explain why these zones are failing? The criteria for excluding
>> failing tests here is that the zone has negative dst and last rules beyond
>> 2037, and I don't think those newly excluded zones suffice those.
>
> It's probably these last rule what is causing the issue
>
> Rule Palestine2020max - Mar Sat>=24 0:001:00
> S
> Rule Palestine2020max - Oct Sat>=24 1:000
> -
>
> The failure seen is
>
> **
> Asia/Gaza : Asia/Gaza
> -
> SimpleTimeZone (NG)
>
> stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0]
>
> stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0]
My question is why it is failing. Have you figured it? The existing exceptions
are either negative DST or last rule beyond 2037, which javazic cannot handle.
The changes introduced with 2020d does not meet either of them. Unless we know
why it is failing, we cannot be sure we can exclude it.
-
PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
> This is an update to javac and javadoc, to introduce support for Preview > APIs, and generally improve javac and javadoc behavior to more closely adhere > to JEP 12. > > The notable changes are: > > * adding support for Preview APIs (javac until now supported primarily only > preview language features, and APIs associated with preview language > features). This includes: > * the @PreviewFeature annotation has boolean attribute "reflective", > which should be true for reflective Preview APIs, false otherwise. This > replaces the existing "essentialAPI" attribute with roughly inverted meaning. > * the preview warnings for preview APIs are auto-suppressed as described > in the JEP 12. E.g. the module that declares the preview API is free to use > it without warnings > * improving error/warning messages. Please see [1] for a list of > cases/examples. > * class files are only marked as preview if they are using a preview > feature. [1] also shows if a class file is marked as preview or not. > * the PreviewFeature annotation has been moved to jdk.internal.javac package > (originally was in the jdk.internal package). > * Preview API in JDK's javadoc no longer needs to specify @preview tag in > the source files. javadoc will auto-generate a note for @PreviewFeature > elements, see e.g. [2] and [3] (non-reflective and reflective API, > respectively). A summary of preview elements is also provided [4]. Existing > uses of @preview have been updated/removed. > * non-JDK javadoc is also enhanced to auto-generate preview notes for uses > of Preview elements, and for declaring elements using preview language > features [5]. > > Please also see the CSR [6] for more information. > > [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html > [2] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html > [3] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html > [4] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html > [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ > [6] https://bugs.openjdk.java.net/browse/JDK-8250769 Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Removing trailing whitespace. - Merging master into JDK-8250768. - Updating tests after records are a final feature. - Fixing tests. - Finalizing removal of record preview hooks. - Merging master into JDK-8250768 - Reflecting review comments. - Merge branch 'master' into JDK-8250768 - Removing unnecessary cast. - Using a more correct way to get URLs. - ... and 36 more: https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 - Changes: https://git.openjdk.java.net/jdk/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=05 Stats: 3012 lines in 142 files changed: 2521 ins; 260 del; 231 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 17:10:34 GMT, Naoto Sato wrote:
>> Hi Guys,
>>
>> Please review the integration of tzdata2020d to JDK.
>>
>> Details regarding the change can be viewed at -
>> https://mm.icann.org/pipermail/tz-announce/2020-October/62.html
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8255226
>>
>> TestZoneInfo310.java test failure is addressed along with it. The last rule
>> affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test.
>>
>> Regression Tests pass along with JCK.
>>
>> Please let me know if the changes are good to push.
>>
>> Thanks,
>> Kiran
>
> test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
>
>> 199: zid.equals("Iran") || // last rule mismatch
>> 200: zid.equals("Asia/Gaza") || // last rule mismatch
>> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
>
> Can you explain why these zones are failing? The criteria for excluding
> failing tests here is that the zone has negative dst and last rules beyond
> 2037, and I don't think those newly excluded zones suffice those.
It's probably these last rule what is causing the issue
Rule Palestine 2020max - Mar Sat>=24 0:001:00S
Rule Palestine 2020max - Oct Sat>=24 1:000 -
The failure seen is
**
Asia/Gaza : Asia/Gaza
-
SimpleTimeZone (NG)
stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0]
stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0]
-
PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:26:05 GMT, Jan Lahoda wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java >> line 1288: >> >>> 1286: case FIELD: case INSTANCE_INIT: case LOCAL_VARIABLE: >>> case PARAMETER: >>> 1287: case RESOURCE_VARIABLE: case STATIC_INIT: case >>> TYPE_PARAMETER: >>> 1288: case RECORD_COMPONENT: >> >> I'm not saying this is wrong, but I'd like to understand why it is necessary. > > HtmlDocletWriter.getPreviewNotes analyzes classes and their members, to see > if some are using aspects that are under preview. When looking at the > members, it uses utils.isIncluded on the member, and that eventually gets > here. And if the member is a RECORD_COMPONENT, it would fail here. But we can > avoid asking for RECORD_COMPONENTS, if needed. ok - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:43:56 GMT, Jan Lahoda wrote: >> I don't think there should be much interaction with -source . >> We don't support preview features from previous version or preview class >> files from previous versions, so I think it should be enough to handle the >> currently present preview features. > > We don't support preview features from previous releases, AFAIK. So javadoc > for JDK 16 should not accept e.g. record class when running with -source 15. Yeah, my recollection is that I was wondering whether preview-related code needs to be "guarded" to only work in the current release. But, I guess we may get the right effect (of forbidding preview features in older code) from the javac front end, so that in javadoc we can be assured that there are no instances of what may still be preview features in older code (i.e with older --source/--rlease options) - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 17:05:19 GMT, Bernhard Urban-Forster wrote: >> @lewurm Open a new JBS issue with the bug. If you can find a fix in a short >> amount of time (which I would believe should be possible; probably just need >> a proper cast) it's acceptable to fix it directly. What amounts to a "short >> amount of time" is left to reasonable judgement; minutes to hours are okay, >> days are not. >> >> Otherwise, create an anti-delta (revert changeset) to back out your changes, >> and open yet another JBS issue for re-implementing them correctly. >> >> In this case, an alternative short-term fix could also be to remove the >> assert instead of backing out the entire patch. > > https://github.com/openjdk/jdk/pull/1013 > @lewurm > This patch seems to break on linux-aarch64 with gcc: Builds cleanly on Linux/GCC or me. - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 16:29:07 GMT, Kiran Sidhartha Ravikumar
wrote:
> Hi Guys,
>
> Please review the integration of tzdata2020d to JDK.
>
> Details regarding the change can be viewed at -
> https://mm.icann.org/pipermail/tz-announce/2020-October/62.html
> Bug: https://bugs.openjdk.java.net/browse/JDK-8255226
>
> TestZoneInfo310.java test failure is addressed along with it. The last rule
> affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test.
>
> Regression Tests pass along with JCK.
>
> Please let me know if the changes are good to push.
>
> Thanks,
> Kiran
The test case needs copyright year change to 2020.
test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
> 199: zid.equals("Iran") || // last rule mismatch
> 200: zid.equals("Asia/Gaza") || // last rule mismatch
> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
Can you explain why these zones are failing? The criteria for excluding failing
tests here is that the zone has negative dst and last rules beyond 2037, and I
don't think those newly excluded zones suffice those.
-
Changes requested by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 16:16:25 GMT, Magnus Ihse Bursie wrote: >> @magicus I did test the initial version of this PR on linux+arm64, but not >> the latest iteration. sorry about that 🙁 >> >> What is the policy here? Submit a revert right away or investigate a fix? > > @lewurm Open a new JBS issue with the bug. If you can find a fix in a short > amount of time (which I would believe should be possible; probably just need > a proper cast) it's acceptable to fix it directly. What amounts to a "short > amount of time" is left to reasonable judgement; minutes to hours are okay, > days are not. > > Otherwise, create an anti-delta (revert changeset) to back out your changes, > and open yet another JBS issue for re-implementing them correctly. > > In this case, an alternative short-term fix could also be to remove the > assert instead of backing out the entire patch. https://github.com/openjdk/jdk/pull/1013 - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 16:52:02 GMT, Coleen Phillimore wrote: >> I thought that we didn't load the archived heap from CDS, if JVMTI heap >> walker capabilities are in place, as we didn't want this kind of >> interactions. But maybe I'm missing something, since you said having this if >> statement here made a difference. > > Now I remember. I added an assert in JvmtiTagMapTable::find() for oop != > NULL which didn't exist in the current hashmap code. The current hashmap > code just didn't find a null oop. I tracked it down to the fact that we're > finding dormant objects whose class hasn't been loaded yet. So I think we do load the archived heap from CDS. The heap walker capabilities can be added dynamically. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:45:18 GMT, Erik Österlund wrote: >> Because it crashed with my changes and didn't without. I cannot recollect >> why. > > I thought that we didn't load the archived heap from CDS, if JVMTI heap > walker capabilities are in place, as we didn't want this kind of > interactions. But maybe I'm missing something, since you said having this if > statement here made a difference. Now I remember. I added an assert in JvmtiTagMapTable::find() for oop != NULL which didn't exist in the current hashmap code. The current hashmap code just didn't find a null oop. I tracked it down to the fact that we're finding dormant objects whose class hasn't been loaded yet. - PR: https://git.openjdk.java.net/jdk/pull/967
RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
Hi Guys, Please review the integration of tzdata2020d to JDK. Details regarding the change can be viewed at - https://mm.icann.org/pipermail/tz-announce/2020-October/62.html Bug: https://bugs.openjdk.java.net/browse/JDK-8255226 TestZoneInfo310.java test failure is addressed along with it. The last rule affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test. Regression Tests pass along with JCK. Please let me know if the changes are good to push. Thanks, Kiran - Commit messages: - 8255226: (tz) Upgrade time-zone data to tzdata2020d Changes: https://git.openjdk.java.net/jdk/pull/1012/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1012&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255226 Stats: 54 lines in 4 files changed: 34 ins; 2 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1012.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1012/head:pull/1012 PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: 8247872: Upgrade HarfBuzz to the latest 2.7.2 [v2]
On Mon, 2 Nov 2020 12:33:17 GMT, Magnus Ihse Bursie wrote: > I'm just a bit curious about the added, empty, > `src/java.desktop/share/native/libharfbuzz/abc.txt`... > > If it really is in upstream source, I'm not saying you should remove it. It > just looks very odd. It's not a merge artifact? > > (I could not even add review comments to an empty file in github! 😮) oops. that should not have been in that folder. It was me practicing a different white space removal script. I thought I checked for any extraneous files. I'll get rid of it now. - PR: https://git.openjdk.java.net/jdk/pull/993
Re: RFR: 8247872: Upgrade HarfBuzz to the latest 2.7.2 [v2]
> This upgrades JDK to import the current 2.7.2 version of harfbuzz - an > OpenType text shaping library > > https://bugs.openjdk.java.net/browse/JDK-8247872 > > This has passed building and headless and headful automated tests on all > platforms. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8247872: Upgrade HarfBuzz to the latest 2.7.2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/993/files - new: https://git.openjdk.java.net/jdk/pull/993/files/8d1ea467..1f47957b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=993&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=993&range=00-01 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/993.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/993/head:pull/993 PR: https://git.openjdk.java.net/jdk/pull/993
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:18:43 GMT, Erik Österlund wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code review comments from StefanK.
>
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp
> line 656:
>
>> 654: result = NSK_FALSE;
>> 655:
>> 656: printf("Object free events %d\n", ObjectFreeEventsCount);
>
> Is this old debug info you forgot to remove? Other code seems to use
> NSK_DISPLAY macros instead.
yes, removed it.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
>
>> 343:
>> 344: // Check if we have to process for concurrent GCs.
>> 345: check_hashmap(false);
>
> Maybe add a comment stating the parameter name, as was done in other
> callsites for check_hashmap.
Ok, will I run afoul of the ZGC people putting the parameter name after the
parameter and the rest of the code, it is before?
> src/hotspot/share/prims/jvmtiTagMap.cpp line 3009:
>
>> 3007: // Lock each hashmap from concurrent posting and cleaning
>> 3008: MutexLocker ml(tag_map->lock(),
>> Mutex::_no_safepoint_check_flag);
>> 3009: tag_map->hashmap()->unlink_and_post(tag_map->env());
>
> This could call unlink_and_post_locked instead of manually locking.
Ok, 2 requests. I can call that then and move the logging.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 16:06:15 GMT, Bernhard Urban-Forster
wrote:
>> @lewurm
>> This patch seems to break on linux-aarch64 with gcc:
>> open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error:
>> comparison of integer expressions of different signedness: 'size_t' {aka
>> 'long unsigned int'} and 'int' [-Werror=sign-compare]
>> 1501 | assert(StackOverflow::stack_shadow_zone_size() ==
>> (int)StackOverflow::stack_shadow_zone_size(), "must be same");
>> |
>> ^~~
>>
>> Did you test building this on any aarch64 platforms besides Windows?
>
> @magicus I did test the initial version of this PR on linux+arm64, but not
> the latest iteration. sorry about that 🙁
>
> What is the policy here? Submit a revert right away or investigate a fix?
@lewurm Open a new JBS issue with the bug. If you can find a fix in a short
amount of time (which I would believe should be possible; probably just need a
proper cast) it's acceptable to fix it directly. What amounts to a "short
amount of time" is left to reasonable judgement; minutes to hours are okay,
days are not.
Otherwise, create an anti-delta (revert changeset) to back out your changes,
and open yet another JBS issue for re-implementing them correctly.
In this case, an alternative short-term fix could also be to remove the assert
instead of backing out the entire patch.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 12:50:23 GMT, Coleen Phillimore wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 954: >> >>> 952: o->klass()->external_name()); >>> 953: return; >>> 954: } >> >> Why is this done as a part of this RFE? Is this a bug fix that should be >> done as a separate patch? > > Because it crashed with my changes and didn't without. I cannot recollect > why. I thought that we didn't load the archived heap from CDS, if JVMTI heap walker capabilities are in place, as we didn't want this kind of interactions. But maybe I'm missing something, since you said having this if statement here made a difference. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore wrote:
>> This change turns the HashTable that JVMTI uses for object tagging into a
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
>> rehashing. Instead of pointing directly to oops so that GC has to walk the
>> table to follow oops and then to rehash the table, this table points to
>> WeakHandle. GC walks the backing OopStorages concurrently.
>>
>> The hash function for the table is a hash of the lower 32 bits of the
>> address. A flag is set during GC (gc_notification if in a safepoint, and
>> through a call to JvmtiTagMap::needs_processing()) so that the table is
>> rehashed at the next use.
>>
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti
>> to post ObjectFree events. In concurrent GCs there can be a window of time
>> between weak oop marking where the oop is unmarked, so dead (the phantom
>> load in peek returns NULL) but the gc_notification hasn't been done yet. In
>> this window, a heap walk or GetObjectsWithTags call would not find an object
>> before the ObjectFree event is posted. This is dealt with in two ways:
>>
>> 1. In the Heap walk, there's an unconditional table walk to post events if
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
>> required, we use the VM thread to post the event.
>>
>> Event posting cannot be done in a JavaThread because the posting needs to be
>> done while holding the table lock, so that the JvmtiEnv state doesn't change
>> before posting is done. ObjectFree callbacks are limited in what they can
>> do as per the JVMTI Specification. The allowed callbacks to the VM already
>> have code to allow NonJava threads.
>>
>> To avoid rehashing, I also tried to use object->identity_hash() but this
>> breaks because entries can be added to the table during heapwalk, where the
>> objects use marking. The starting markWord is saved and restored. Adding a
>> hashcode during this operation makes restoring the former markWord (locked,
>> inflated, etc) too complicated. Plus we don't want all these objects to
>> have hashcodes because locking operations after tagging would have to always
>> use inflated locks.
>>
>> Much of this change is to remove serial weak oop processing for the
>> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
>> jvmti code.
>>
>> It has also been tested with tier1-6.
>>
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Code review comments from StefanK.
Looks great in general. Great work Coleen, and thanks again for fixing this. I
like all the red lines in the GC code. I added a few nits/questions.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp
line 656:
> 654: result = NSK_FALSE;
> 655:
> 656: printf("Object free events %d\n", ObjectFreeEventsCount);
Is this old debug info you forgot to remove? Other code seems to use
NSK_DISPLAY macros instead.
src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
> 343:
> 344: // Check if we have to process for concurrent GCs.
> 345: check_hashmap(false);
Maybe add a comment stating the parameter name, as was done in other callsites
for check_hashmap.
src/hotspot/share/prims/jvmtiTagMap.cpp line 3009:
> 3007: // Lock each hashmap from concurrent posting and cleaning
> 3008: MutexLocker ml(tag_map->lock(),
> Mutex::_no_safepoint_check_flag);
> 3009: tag_map->hashmap()->unlink_and_post(tag_map->env());
This could call unlink_and_post_locked instead of manually locking.
-
Changes requested by eosterlund (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/967
Integrated: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov wrote: > We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an > experimental feature. We shipped Graal as an experimental JIT compiler in JDK > 10. We haven't seen much use of these features, and the effort required to > support and enhance them is significant. We therefore intend to disable these > features in Oracle builds as of JDK 16. > > We'll leave the sources for these features in the repository, in case any one > else is interested in building them. But we will not update or test them. > > We'll continue to build and ship JVMCI as an experimental feature in Oracle > builds. > > Tested changes in all tiers. > > I verified that with these changes I still able to build Graal in open repo > and run graalunit testing: > > `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh > /mydir/graalunit_lib/` > `open$ bash configure --with-debug-level=fastdebug > --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` > `open$ make jdk-image` > `open$ make test-image` > `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` This pull request has now been integrated. Changeset: 2f7d34f2 Author:Vladimir Kozlov URL: https://git.openjdk.java.net/jdk/commit/2f7d34f2 Stats: 36 lines in 4 files changed: 21 ins; 11 del; 4 mod 8255616: Disable AOT and Graal in Oracle OpenJDK Reviewed-by: iignatyev, vlivanov, iveresov, ihse - PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 15:41:06 GMT, Magnus Ihse Bursie wrote:
>> Thank you Andrew.
>
> @lewurm
> This patch seems to break on linux-aarch64 with gcc:
> open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error:
> comparison of integer expressions of different signedness: 'size_t' {aka
> 'long unsigned int'} and 'int' [-Werror=sign-compare]
> 1501 | assert(StackOverflow::stack_shadow_zone_size() ==
> (int)StackOverflow::stack_shadow_zone_size(), "must be same");
> |
> ^~~
>
> Did you test building this on any aarch64 platforms besides Windows?
@magicus I did test the initial version of this PR on linux+arm64, but not the
latest iteration. sorry about that 🙁
What is the policy here? Submit a revert right away or investigate a fix?
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Code review comments from StefanK. Shenandoah part looks good. - Marked as reviewed by zgu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
> This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Code review comments from StefanK. - Changes: - all: https://git.openjdk.java.net/jdk/pull/967/files - new: https://git.openjdk.java.net/jdk/pull/967/files/534326da..cb4c83e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=00-01 Stats: 75 lines in 9 files changed: 12 ins; 48 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/967.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967 PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 14:00:33 GMT, Bernhard Urban-Forster
wrote:
>>> Would you mind to sponsor it @theRealAph or @magicus?
>>
>> Hmm, I think you have to integrate it first.
>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor
>
> Thank you Andrew.
@lewurm
This patch seems to break on linux-aarch64 with gcc:
open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error:
comparison of integer expressions of different signedness: 'size_t' {aka 'long
unsigned int'} and 'int' [-Werror=sign-compare]
1501 | assert(StackOverflow::stack_shadow_zone_size() ==
(int)StackOverflow::stack_shadow_zone_size(), "must be same");
|
^~~
Did you test building this on any aarch64 platforms besides Windows?
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: JDK-8255732: OpenJDK fails to build if $A is set to a value with spaces
On Mon, 2 Nov 2020 14:40:35 GMT, Erik Joelsson wrote: > We have at least one java file with a '$' in the name. As we have learned > over the years, having $ in unexpected places quickly leads to unexpected > behavior in a shell/make based build. In this case it's our override > mechanism of java files that needs protection against expanding such > occurrences of $. Looks good to me. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1007
Integrated: JDK-8255673: Wrong version in docs bundles
On Fri, 30 Oct 2020 17:19:56 GMT, Erik Joelsson wrote: > In JDK-8206311, when I introduced a new docs build profile, I forgot to add > the version configure arguments to it. This causes the docs bundles for > CI/promoted builds to be generated with the default adhoc version information > instead of the specific version for that build. This pull request has now been integrated. Changeset: b0280743 Author:Erik Joelsson URL: https://git.openjdk.java.net/jdk/commit/b0280743 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod 8255673: Wrong version in docs bundles Reviewed-by: tbell, ihse - PR: https://git.openjdk.java.net/jdk/pull/959
RFR: JDK-8255732: OpenJDK fails to build if $A is set to a value with spaces
We have at least one java file with a '$' in the name. As we have learned over the years, having $ in unexpected places quickly leads to unexpected behavior in a shell/make based build. In this case it's our override mechanism of java files that needs protection against expanding such occurrences of $. - Commit messages: - Add DoubleDollar call to protect java class file names with dollar in them Changes: https://git.openjdk.java.net/jdk/pull/1007/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1007&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255732 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1007.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1007/head:pull/1007 PR: https://git.openjdk.java.net/jdk/pull/1007
Re: RFR: JDK-8255673: Wrong version in docs bundles
On Fri, 30 Oct 2020 17:19:56 GMT, Erik Joelsson wrote: > In JDK-8206311, when I introduced a new docs build profile, I forgot to add > the version configure arguments to it. This causes the docs bundles for > CI/promoted builds to be generated with the default adhoc version information > instead of the specific version for that build. LGTM - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/959
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v21]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when de
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Mon, 2 Nov 2020 13:41:53 GMT, Andrew Haley wrote: >> Marked as reviewed by aph (Reviewer). > >> Would you mind to sponsor it @theRealAph or @magicus? > > Hmm, I think you have to integrate it first. > https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor Thank you Andrew. - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Tue, 27 Oct 2020 14:04:04 GMT, Andrew Haley wrote: >> Bernhard Urban-Forster has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - uppercase suffix >> - add assert > > Marked as reviewed by aph (Reviewer). > Would you mind to sponsor it @theRealAph or @magicus? Hmm, I think you have to integrate it first. https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor - PR: https://git.openjdk.java.net/jdk/pull/530
Integrated: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build
On Tue, 6 Oct 2020 18:09:05 GMT, Bernhard Urban-Forster
wrote:
> I organized this PR so that each commit contains the warning emitted by MSVC
> as commit message and its relevant fix.
>
> Verified on
> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
> still works. Just mentioning this here, because it's yet another toolchain
> (Xcode / clang) that needs to be kept happy [going
> forward](https://openjdk.java.net/jeps/391).
This pull request has now been integrated.
Changeset: d2812f78
Author:Bernhard Urban-Forster
Committer: Andrew Haley
URL: https://git.openjdk.java.net/jdk/commit/d2812f78
Stats: 23 lines in 8 files changed: 2 ins; 0 del; 21 mod
8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build
Reviewed-by: ihse, aph
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:46:31 GMT, Erik Joelsson wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > Build changes look ok. There should be a thank you emoji that doesn't send email except maybe to the person reviewing the code. Thank you @erikj79 and @magicus for reviewing the build changes. There should also be a 'fixed' emoji. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:08:53 GMT, Stefan Karlsson wrote:
>> This change turns the HashTable that JVMTI uses for object tagging into a
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
>> rehashing. Instead of pointing directly to oops so that GC has to walk the
>> table to follow oops and then to rehash the table, this table points to
>> WeakHandle. GC walks the backing OopStorages concurrently.
>>
>> The hash function for the table is a hash of the lower 32 bits of the
>> address. A flag is set during GC (gc_notification if in a safepoint, and
>> through a call to JvmtiTagMap::needs_processing()) so that the table is
>> rehashed at the next use.
>>
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti
>> to post ObjectFree events. In concurrent GCs there can be a window of time
>> between weak oop marking where the oop is unmarked, so dead (the phantom
>> load in peek returns NULL) but the gc_notification hasn't been done yet. In
>> this window, a heap walk or GetObjectsWithTags call would not find an object
>> before the ObjectFree event is posted. This is dealt with in two ways:
>>
>> 1. In the Heap walk, there's an unconditional table walk to post events if
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
>> required, we use the VM thread to post the event.
>>
>> Event posting cannot be done in a JavaThread because the posting needs to be
>> done while holding the table lock, so that the JvmtiEnv state doesn't change
>> before posting is done. ObjectFree callbacks are limited in what they can
>> do as per the JVMTI Specification. The allowed callbacks to the VM already
>> have code to allow NonJava threads.
>>
>> To avoid rehashing, I also tried to use object->identity_hash() but this
>> breaks because entries can be added to the table during heapwalk, where the
>> objects use marking. The starting markWord is saved and restored. Adding a
>> hashcode during this operation makes restoring the former markWord (locked,
>> inflated, etc) too complicated. Plus we don't want all these objects to
>> have hashcodes because locking operations after tagging would have to always
>> use inflated locks.
>>
>> Much of this change is to remove serial weak oop processing for the
>> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
>> jvmti code.
>>
>> It has also been tested with tier1-6.
>>
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:
>
>> 39: // Must be updated when new OopStorages are introduced
>> 40: static const uint strong_count = 4 JVMTI_ONLY(+ 1);
>> 41: static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);
>
> All other uses `+ 1` instead of `+1`.
Fixed, although I think the space looks strange there but I'll go along.
> src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:
>
>> 47: double _phase_times_sec[1];
>> 48: size_t _phase_dead_items[1];
>> 49: size_t _phase_total_items[1];
>
> This should be removed and the associated reset_items
Removed.
> src/hotspot/share/gc/z/zOopClosures.hpp line 64:
>
>> 62: };
>> 63:
>> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {
>
> Seems like you flipped the location of these two. Maybe revert?
Reverted. There was a rebasing conflict here so this was unintentional.
> src/hotspot/share/prims/jvmtiExport.hpp line 405:
>
>> 403:
>> 404: // Delete me and all my callers!
>> 405: static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}
>
> Maybe delete?
Yes, meant to do that.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 131:
>
>> 129: // Operating on the hashmap must always be locked, since concurrent
>> GC threads may
>> 130: // notify while running through a safepoint.
>> 131: assert(is_locked(), "checking");
>
> Maybe move this to the top of the function to make it very clear.
ok.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 133:
>
>> 131: assert(is_locked(), "checking");
>> 132: if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
>> 133: log_info(jvmti, table)("TagMap table needs posting before heap
>> walk");
>
> Not sure about the "before heap walk" since this is also done from
> GetObjectsWithTags, which does *not* do a heap walk but still requires
> posting.
I don't call check_hashmap for GetObjectsWithTags.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 140:
>
>> 138: hashmap()->rehash();
>> 139: _needs_rehashing = false;
>> 140: }
>
> It's not clear to me that it's correct to rehash *after* posting. I think it
> is, because unlink_and_post will use load barriers to fixup old pointers.
I think it's better that the rehashing doesn't encounter null entries and the
WeakHandle.peek() operation is used for both so I hope it would get the same
answer. If not, which seem
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote: > This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. I think I addressed your comments, retesting now. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:34:17 GMT, Stefan Karlsson wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 126: >> >>> 124: // concurrent GCs. So fix it here once we have a lock or are >>> 125: // at a safepoint. >>> 126: // SetTag and GetTag should not post events! >> >> I think it would be good to explain why. Otherwise, this just leaves the >> readers wondering why this is the case. > > Maybe even move this comment to the set_tag/get_tag code. I was trying to explain why there's a boolean there but I can put this comment at both get_tag and set_tag. // Check if we have to processing for concurrent GCs. // GetTag should not post events because the JavaThread has to // transition to native for the callback and this cannot stop for // safepoints with the hashmap lock held. check_hashmap(false); - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8247872: Upgrade HarfBuzz to the latest 2.7.2
On Mon, 2 Nov 2020 04:19:57 GMT, Phil Race wrote: > This upgrades JDK to import the current 2.7.2 version of harfbuzz - an > OpenType text shaping library > > https://bugs.openjdk.java.net/browse/JDK-8247872 > > This has passed building and headless and headful automated tests on all > platforms. Build changes look good. I'm just a bit curious about the added, empty, `src/java.desktop/share/native/libharfbuzz/abc.txt`... If it really is in upstream source, I'm not saying you should remove it. It just looks very odd. It's not a merge artifact? (I could not even add review comments to an empty file in github! 😮) - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/993
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote: > This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. Build changes look good. Not reviewed hotspot changes. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov wrote: > We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an > experimental feature. We shipped Graal as an experimental JIT compiler in JDK > 10. We haven't seen much use of these features, and the effort required to > support and enhance them is significant. We therefore intend to disable these > features in Oracle builds as of JDK 16. > > We'll leave the sources for these features in the repository, in case any one > else is interested in building them. But we will not update or test them. > > We'll continue to build and ship JVMCI as an experimental feature in Oracle > builds. > > Tested changes in all tiers. > > I verified that with these changes I still able to build Graal in open repo > and run graalunit testing: > > `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh > /mydir/graalunit_lib/` > `open$ bash configure --with-debug-level=fastdebug > --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` > `open$ make jdk-image` > `open$ make test-image` > `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v20]
On Mon, 2 Nov 2020 11:59:09 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation (see JEP 393 [1]). This >> iteration focus on improving the usability of the API in 3 main ways: >> >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee that the memory will be >> deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class has been added, which >> defines several useful dereference routines; these are really just thin >> wrappers around memory access var handles, but they make the barrier of >> entry for using this API somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it >> used to be the case that a memory address could (sometimes, not always) have >> a back link to the memory segment which originated it; additionally, memory >> access var handles used `MemoryAddress` as a basic unit of dereference. >> >> This has all changed as per this API refresh; now a `MemoryAddress` is just >> a dumb carrier which wraps a pair of object/long addressing coordinates; >> `MemorySegment` has become the star of the show, as far as dereferencing >> memory is concerned. You cannot dereference memory if you don't have a >> segment. This improves usability in a number of ways - first, it is a lot >> easier to wrap native addresses (`long`, essentially) into a >> `MemoryAddress`; secondly, it is crystal clear what a client has to do in >> order to dereference memory: if a client has a segment, it can use that; >> otherwise, if the client only has an address, it will have to create a >> segment *unsafely* (this can be done by calling >> `MemoryAddress::asSegmentRestricted`). >> >> A list of the API, implementation and test changes is provided below. If >> you have any questions, or need more detailed explanations, I (and the rest >> of the Panama team) will be happy to point at existing discussions, and/or >> to provide the feedback required. >> >> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without >> whom the work on shared memory segment would not have been possible; also >> I'd like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. >> >> Thanks >> Maurizio >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a carrier is one of the usual >> suspect (a Java primitive, minus `boolean`); the access mode can be simple >> (e.g. access base address of given segment), or indexed, in which case the >> accessor takes a segment and either a low-level byte offset,or a high level >> logical index. The classification is reflected in the naming scheme (e.g. >> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which it is easy to derive all >> the other handles using plain var handle combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both `MemoryAddress` and >> `MemorySegme
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v20]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when de
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:25:28 GMT, Stefan Karlsson wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > src/hotspot/share/prims/jvmtiTagMap.cpp line 126: > >> 124: // concurrent GCs. So fix it here once we have a lock or are >> 125: // at a safepoint. >> 126: // SetTag and GetTag should not post events! > > I think it would be good to explain why. Otherwise, this just leaves the > readers wondering why this is the case. Maybe even move this comment to the set_tag/get_tag code. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote:
> This change turns the HashTable that JVMTI uses for object tagging into a
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
> rehashing. Instead of pointing directly to oops so that GC has to walk the
> table to follow oops and then to rehash the table, this table points to
> WeakHandle. GC walks the backing OopStorages concurrently.
>
> The hash function for the table is a hash of the lower 32 bits of the
> address. A flag is set during GC (gc_notification if in a safepoint, and
> through a call to JvmtiTagMap::needs_processing()) so that the table is
> rehashed at the next use.
>
> The gc_notification mechanism of weak oop processing is used to notify Jvmti
> to post ObjectFree events. In concurrent GCs there can be a window of time
> between weak oop marking where the oop is unmarked, so dead (the phantom load
> in peek returns NULL) but the gc_notification hasn't been done yet. In this
> window, a heap walk or GetObjectsWithTags call would not find an object
> before the ObjectFree event is posted. This is dealt with in two ways:
>
> 1. In the Heap walk, there's an unconditional table walk to post events if
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
> required, we use the VM thread to post the event.
>
> Event posting cannot be done in a JavaThread because the posting needs to be
> done while holding the table lock, so that the JvmtiEnv state doesn't change
> before posting is done. ObjectFree callbacks are limited in what they can do
> as per the JVMTI Specification. The allowed callbacks to the VM already have
> code to allow NonJava threads.
>
> To avoid rehashing, I also tried to use object->identity_hash() but this
> breaks because entries can be added to the table during heapwalk, where the
> objects use marking. The starting markWord is saved and restored. Adding a
> hashcode during this operation makes restoring the former markWord (locked,
> inflated, etc) too complicated. Plus we don't want all these objects to have
> hashcodes because locking operations after tagging would have to always use
> inflated locks.
>
> Much of this change is to remove serial weak oop processing for the
> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
> jvmti code.
>
> It has also been tested with tier1-6.
>
> Thank you to Stefan, Erik and Kim for their help with this change.
Commented on nits, and reviewed GC code and tag map code. Didn't look closely
on the hashmap changes.
src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:
> 39: // Must be updated when new OopStorages are introduced
> 40: static const uint strong_count = 4 JVMTI_ONLY(+ 1);
> 41: static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);
All other uses `+ 1` instead of `+1`.
src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:
> 47: double _phase_times_sec[1];
> 48: size_t _phase_dead_items[1];
> 49: size_t _phase_total_items[1];
This should be removed and the associated reset_items
src/hotspot/share/gc/z/zOopClosures.hpp line 64:
> 62: };
> 63:
> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {
Seems like you flipped the location of these two. Maybe revert?
src/hotspot/share/prims/jvmtiExport.hpp line 405:
> 403:
> 404: // Delete me and all my callers!
> 405: static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}
Maybe delete?
src/hotspot/share/prims/jvmtiTagMap.cpp line 126:
> 124: // concurrent GCs. So fix it here once we have a lock or are
> 125: // at a safepoint.
> 126: // SetTag and GetTag should not post events!
I think it would be good to explain why. Otherwise, this just leaves the
readers wondering why this is the case.
src/hotspot/share/prims/jvmtiTagMap.cpp line 131:
> 129: // Operating on the hashmap must always be locked, since concurrent GC
> threads may
> 130: // notify while running through a safepoint.
> 131: assert(is_locked(), "checking");
Maybe move this to the top of the function to make it very clear.
src/hotspot/share/prims/jvmtiTagMap.cpp line 133:
> 131: assert(is_locked(), "checking");
> 132: if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
> 133: log_info(jvmti, table)("TagMap table needs posting before heap
> walk");
Not sure about the "before heap walk" since this is also done from
GetObjectsWithTags, which does *not* do a heap walk but still requires posting.
src/hotspot/share/prims/jvmtiTagMap.cpp line 140:
> 138: hashmap()->rehash();
> 139: _needs_rehashing = false;
> 140: }
It's not clear to me that it's correct to rehash *after* posting. I think it
is, because unlink_and_post will use load barriers to fixup old pointers.
src/hotspot/share/prims/jvmtiTagMap.cpp line 146:
> 144: // The ZDriver may be walking the hashmaps concurrently so all these
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Sun, 1 Nov 2020 16:06:32 GMT, Alan Bateman wrote: > The javadoc for copyFrom isn't changed in this update but I notice it > specifies IndexOutOfBoundException when the source segment is larger than the > receiver, have other exceptions been examined? This exception is consistent with other uses of this exception throughout this API (e.g. when writing a segment out of bounds). - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Sun, 1 Nov 2020 16:06:32 GMT, Alan Bateman wrote: > Now that MemorySegment is AutoCloseable then maybe the term "alive" should be > replaced with "open" or "closed" and isAlive replaced with isOpen is isClosed. While the reason for the method being called "isAlive" are mostly historical (the old Panama pointer API had such a method), I think I still stand behind the current naming scheme. For temporal bounds, I think "isAlive" works better than "isOpened". > MappedMemorySegments. The force method specifies a write back guarantee but > at the same time, the implNote in the class description suggests that the > methods might be a no-op. You might want to adjust the wording to avoid any > suggestion that force might be a no-op. The comment that this operation could be no-op was borrowed from the `MappedByteBuffer` API; looking at the impl, it seems that you are right that, under no circumstances (unless the segment has length zero) this should be a no-op. How do you suggest I proceed? - PR: https://git.openjdk.java.net/jdk/pull/548
