Re: RFR: 8311071: Avoid SoftReferences in LambdaFormEditor and MethodTypeForm when storing heap objects into AOT cache [v3]
On Wed, 18 Sep 2024 03:04:50 GMT, Ioi Lam wrote: >> This is the 6th PR for [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://bugs.openjdk.org/browse/JDK-8315737). >> >> The implementation of java.lang.invoke uses SoftReferences so that unused >> MethodHandles, LambdaForms, etc, can be garbage collected. >> >> However, if we want to store java.lang.invoke objects in the AOT cache >> ([JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336), the final step >> in JEP 493), it's difficult to cache these SoftReferences -- SoftReferences >> in turn point to ReferenceQueues, etc, which have dependencies on the >> current execution state (Threads, etc) which are difficult to cache. >> >> The proposal is to add a new flag: `MethodHandleStatics.NO_SOFT_CACHE`. When >> this flag is true, we avoid using SoftReferences, and store a direct >> reference to the target object instead. >> >> [JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336) stores only >> java.lang.invoke objects that refer to classes loaded by the >> boot/platform/app loaders. These classes are never unloaded, so it's not >> necessary to point to them using SoftReferences. >> >> This RFE modifies only the LambdaFormEditor and MethodTypeForm classes, as >> that's the minimal modification required by >> [JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336). >> >> --- >> See [here](https://bugs.openjdk.org/browse/JDK-8315737) for the sequence of >> dependent RFEs for implementing JEP 483. > > Ioi Lam has updated the pull request with a new target base due to a merge or > a rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - @dholmes-ora review comments > - Merge branch 'jep-483-step-05-8293337-archive-method-handle-intrinsics' of > /jdk3/yam/open into > jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke > - Merge branch 'jep-483-step-05-8293337-archive-method-handle-intrinsics' of > /jdk3/yam/open into > jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke > - Do not use system property to limit usage to only CDS static dumps > - Merge branch 'jep-483-step-05-8293337-archive-method-handle-intrinsics' of > /jdk3/yam/open into > jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke > - Merge branch 'jep-483-step-05-8293337-archive-method-handle-intrinsics' of > /jdk3/yak/open into > jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke > - Merge branch 'jep-483-step-05-8293337-archive-method-handle-intrinsics' of > /jdk3/yak/open into > jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke > - 8311071: Add an option to avoid using SoftReferences in > java.lang.invoke.MethodHandle Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21049#pullrequestreview-2311592121
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow [v7]
On Tue, 17 Sep 2024 11:27:39 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to remove the >> (internal) `SelectVersion()` function from the java launcher and also update >> the code comments in the launcher to match the current implementation? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8340114, the >> `SelectVersion()` function in the launcher used to be relevant when JRE >> selection was a feature. That feature has been removed since Java 9 >> https://bugs.openjdk.org/browse/JDK-8058407. The SelectVersion() in its >> current form isn't relevant anymore and can be removed. >> >> While at it, it was noticed that the current "flowchart" code comments in >> the launcher which attempts to explain the flow in the launcher code are >> outdated. The commit in this PR updates those comments for macosx and unix >> implementation. The windows variant doesn't have a "flowchart", but it too >> deserves a high level comment explaining this flow. I haven't updated the >> windows variant in this PR because that does a few additional things, which >> I need to review and understand better. I plan to take that up in a future >> change. >> >> An existing >> `test/jdk/tools/launcher/MultipleJRERemoved.java/MultipleJRERemoved` test >> had to be updated due to the changes in this PR. That test was launching >> `java` (once) with 3 unsupported JRE selection options and was expecting 3 >> error messages (one each for the unsupported option) for that single launch. >> With the change in this PR, we don't accumulate and throw all those 3 errors >> and instead we fail fast for any of these 3 unsupported JRE selection >> options. The fail fast implementation matches what we do with other similar >> unsupported options. The test had to be updated to not expect all 3 errors >> message in a single launch and instead expect to find one of those error >> messages. Given what this test is for, and the fact that JRE version >> selection options (rightly) continue to raise an error after this change, I >> think, an update to that test should be OK. >> >> No new tests have been introduced in this PR and tier testing is currently >> in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > treat -version: -jre-restrict-search and -jre-no-restrict-search like any > other unknown option Great to see this further simplification. I agree no CSR request needed as all that happens is the error message changes. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20997#pullrequestreview-2311571248
Re: RFR: 8328313: Archived module graph should allow identical --module-path to be specified during dump time and run time
On Tue, 17 Sep 2024 23:44:40 GMT, Calvin Cheung wrote:
> Prior to this patch, if `--module-path` is specified in the command line:
> during CDS dump time, full module graph will not be included in the CDS
> archive;
> during run time, full module graph will not be used.
>
> With this patch, the full module graph will be included in the CDS archive
> with the `--module-path` option. During run time, if the same `--module-path`
> option is specified, the archived module graph will be used.
>
> The checking of module paths between dump time and run time is more lenient
> compared with the checking of class paths; the ordering of the modules is
> unimportant, duplicate module names are ignored.
> E.g. the following is considered a match:
> dump time runtime
> m1,m2 m2,m1
> m1,m2 m1,m2,m2
>
> I included some
> [notes](https://bugs.openjdk.org/browse/JDK-8328313?focusedId=14699275&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14699275)
> in the bug report regarding some changes in the corelib classes.
I've taken an initial pass through and this seems reasonable (a bit more
complicated than the description suggested :) ).
Thanks
src/hotspot/share/cds/filemap.cpp line 931:
> 929: bool FileMapInfo::is_jar_suffix(const char* filename) {
> 930: const char* dot = strrchr(filename, '.');
> 931: if (strcmp(dot + 1, "jar") == 0) {
What if there is no dot? We need a null check.
src/hotspot/share/cds/filemap.hpp line 558:
> 556: unsigned int dumptime_prefix_len,
> 557: unsigned int runtime_prefix_len)
> NOT_CDS_RETURN_(false);
> 558: bool is_jar_suffix(const char* filename);
Suggestion: has_jar_suffix
src/hotspot/share/cds/heapShared.cpp line 884:
> 882: ClassLoaderExt::num_module_paths() > 0) {
> 883: log_info(cds, heap)("is_using_optimized_module_handling %d
> num_module_paths %d jdk.module.main %s",
> 884: CDSConfig::is_using_optimized_module_handling(),
> ClassLoaderExt::num_module_paths(),
> Arguments::get_property("jdk.module.main"));
Why are you printing a bool value as an int? I'm surprised one of the format
checkers doesn't complain about it.
-
PR Review: https://git.openjdk.org/jdk/pull/21048#pullrequestreview-2311418590
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1764261758
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1764267214
PR Review Comment: https://git.openjdk.org/jdk/pull/21048#discussion_r1764268462
Re: RFR: 8311071: Avoid SoftReferences in LambdaFormEditor and MethodTypeForm when storing heap objects into AOT cache
On Tue, 17 Sep 2024 23:48:11 GMT, Ioi Lam wrote: > This is the 6th PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > The implementation of java.lang.invoke uses SoftReferences so that unused > MethodHandles, LambdaForms, etc, can be garbage collected. > > However, if we want to store java.lang.invoke objects in the AOT cache > ([JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336), the final step > in JEP 493), it's difficult to cache these SoftReferences -- SoftReferences > in turn point to ReferenceQueues, etc, which have dependencies on the current > execution state (Threads, etc) which are difficult to cache. > > The proposal is to add a new flag: `MethodHandleStatics.NO_SOFT_CACHE`. When > this flag is true, we avoid using SoftReferences, and store a direct > reference to the target object instead. > > [JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336) stores only > java.lang.invoke objects that refer to classes loaded by the > boot/platform/app loaders. These classes are never unloaded, so it's not > necessary to point to them using SoftReferences. > > This RFE modifies only the LambdaFormEditor and MethodTypeForm classes, as > that's the minimal modification required by > [JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336). > > --- > See [here](https://bugs.openjdk.org/browse/JDK-8315737) for the sequence of > dependent RFEs for implementing JEP 483. This seems quite reasonable for dumping the static archive. A couple of typos. Thanks src/java.base/share/classes/java/lang/invoke/MethodHandleStatics.java line 62: > 60: static final boolean PROFILE_GWT; > 61: static final int CUSTOMIZE_THRESHOLD; > 62: static final boolean NO_SOFT_CACHE; // Don't use SoftReference in > LambdaFormEditor and MethodTypeForm so they can archived by CDS. Suggestion: static final boolean NO_SOFT_CACHE; // Don't use SoftReference in LambdaFormEditor and MethodTypeForm so they can be archived by CDS. src/java.base/share/classes/jdk/internal/misc/CDS.java line 81: > 79: /* > 80: * Wwhen dumping the static archive, CDS is able to archive > MethodHandles. > 81: * However, CDS cannot archive SoftReferences objects, so we need to Suggestion: * However, CDS cannot archive SoftReference objects, so we need to - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21049#pullrequestreview-2311403651 PR Review Comment: https://git.openjdk.org/jdk/pull/21049#discussion_r1764251342 PR Review Comment: https://git.openjdk.org/jdk/pull/21049#discussion_r1764253505
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow [v6]
On Mon, 16 Sep 2024 13:55:49 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to remove the >> (internal) `SelectVersion()` function from the java launcher and also update >> the code comments in the launcher to match the current implementation? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8340114, the >> `SelectVersion()` function in the launcher used to be relevant when JRE >> selection was a feature. That feature has been removed since Java 9 >> https://bugs.openjdk.org/browse/JDK-8058407. The SelectVersion() in its >> current form isn't relevant anymore and can be removed. >> >> While at it, it was noticed that the current "flowchart" code comments in >> the launcher which attempts to explain the flow in the launcher code are >> outdated. The commit in this PR updates those comments for macosx and unix >> implementation. The windows variant doesn't have a "flowchart", but it too >> deserves a high level comment explaining this flow. I haven't updated the >> windows variant in this PR because that does a few additional things, which >> I need to review and understand better. I plan to take that up in a future >> change. >> >> An existing >> `test/jdk/tools/launcher/MultipleJRERemoved.java/MultipleJRERemoved` test >> had to be updated due to the changes in this PR. That test was launching >> `java` (once) with 3 unsupported JRE selection options and was expecting 3 >> error messages (one each for the unsupported option) for that single launch. >> With the change in this PR, we don't accumulate and throw all those 3 errors >> and instead we fail fast for any of these 3 unsupported JRE selection >> options. The fail fast implementation matches what we do with other similar >> unsupported options. The test had to be updated to not expect all 3 errors >> message in a single launch and instead expect to find one of those error >> messages. Given what this test is for, and the fact that JRE version >> selection options (rightly) continue to raise an error after this change, I >> think, an update to that test should be OK. >> >> No new tests have been introduced in this PR and tier testing is currently >> in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > comment improvements Okay I had expected (hoped for?) more simplifications but it seems a lot of the complexity of the launch procedure remains. I think basically this just gets rid of the data model and mJRE stuff and moves the rest out of SelectVersion. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20997#pullrequestreview-2308876241
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow [v6]
On Mon, 16 Sep 2024 14:02:21 GMT, Jaikiran Pai wrote: >> In the context of the 2 platforms - macosx and unix, the recursive >> invocation still continues to happen. >> >> For macosx, the `CreateExecutionEnvironment` unconditionally, through an >> internal `MacOSXStartup` function, spawns a new thread (`pthread_create`). >> That thread is passed a function pointer pointing to the current >> executable/process' `main(...)` function (and thus made to execute afresh). >> That then triggers this recursion where `JLI_Launch` is called again and >> that then calls into `CreateExecutionEnvironment` all the way to >> `MacOSXStartup` function which has the necessary knowledge not to spawn >> another thread the second time. >> >> For unix, in the `CreateExecutionEnvironment` function, there's a specific >> piece of code which determines whether or not `LD_LIBRARY_PATH` environment >> variable needs to be set or updated before loading the JVM. This decision of >> whether or not `LD_LIBRARY_PATH` needs to be updated or set is done in an >> internal function called `RequiresSetenv`. There are several rules which >> determine whether or not to set/update that environment variable. Rules like >> - is musl libc in use; or is the runtime environment AIX; or if the >> `LD_LIBRARY_PATH` is currently set, then whether the value in the >> environment variable matches some well known JVM library path patterns (like >> `lib/client`, `lib/server`). If any of these rules determine that the >> `LD_LIBRARY_PATH` needs to be set/updated, then the >> `CreateExecutionEnvironment` function will do the necessary updates to the >> environment variable value and and exec() the current process with that new >> value. This then triggers the recursion all the way from th e `JLI_Launch` back into this `CreateExecutionEnvironment` (which has the necessary knowledge not to exec() once more). > >> For macosx, the CreateExecutionEnvironment unconditionally, through an >> internal MacOSXStartup function, spawns a new thread (pthread_create). > > I forgot to note that, the reason for spawning this new thread is explained > as a code comment (unrelated to the changes in this PR) on the > `MacOSXStartup` function and it says: > > > /* > * Mac OS X mandates that the GUI event loop run on very first thread of > * an application. This requires that we re-call Java's main() on a new > * thread, reserving the 'main' thread for Cocoa. > */ That aside, the launcher always tries to spawn a new thread because bad stuff can happen on some OS if we try to make the process primordial thread a JavaThread (e.g. you can't enforce stack size limits). But I remain unclear as to why the new thread has to "start again" rather than continuing with the launch process- it is even called ContinueInNewThread. - PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1762683203
Re: RFR: 8286851: Deprecate for removal several of the undocumented java launcher options
On Tue, 17 Sep 2024 06:28:54 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate several > outdated and undocumented java launcher options? This addresses > https://bugs.openjdk.org/browse/JDK-8286851. > > Specifically, the non-standard launcher options `-verbosegc`, `-noclassgc`, > `-verify`, `-verifyremote`, `-ss`, `-ms` and `-mx` will be deprecated for > removal. With this change, a deprecation warning will be printed, if any of > these options are used. > > No new tests have been added for this change. Existing tests in tier1, tier2 > and tier3 continue to pass. > > I'll create a CSR shortly for this change. LGTM! Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21031#pullrequestreview-2308646316
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow [v5]
On Mon, 16 Sep 2024 06:16:56 GMT, Jaikiran Pai wrote: >> src/java.base/share/native/libjli/java.c line 38: >> >>> 36: * One job of the launcher is to remove command line options which the >>> 37: * vm does not understand and will not process. These options include >>> 38: * options which select which style of vm is run (e.g. -client and >> >> Aren't these the only options now? > > Right now, in mainline, the launcher code in `CheckJvmType` function > additionally also checks for the presence of `-XXaltjvm` launcher option. > Just like for `-server`, `-client` options, this `CheckJvmType` function also > strips the `-XXaltjvm` option from the options that are passed along to the > JVM. > > Specifically, in mainline, right now the following appears to be functional: > > > /bin/java -XXaltjvm=/lib/server/ -version > > will load and print the Java 22 VM output: > > > openjdk version "22" 2024-03-19 > OpenJDK Runtime Environment (build 22+36-2370) > OpenJDK 64-Bit Server VM (build 22+36-2370, mixed mode, sharing) > > > i.e. the launcher from mainline JDK launches the altjvm from JDK 22. > `CheckJvmType` in the launcher code, is where this parsing and stripping of > `-XXaltjvm` is currently happening. > > (There's also some hotspot VM side code which appears to be parsing a > `-Dsun.java.launcher.is_altjvm=` option to detect this `-XXaltjvm` usage, but > as far as I can see the launcher no where sets this > `-Dsun.java.launcher.is_altjvm=` when `-XXaltjvm` is used. So, I think, this > needs a separate investigation of its own) The property is used by the gtest launcher - see comments in [JDK-8171508](https://bugs.openjdk.org/browse/JDK-8171508) This is all a lot more complicated than I had thought it was. - PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1761027867
Re: RFR: 8339918: Remove checks for outdated -t -tm -Xfuture -checksource -cs -noasyncgc options from the launcher [v7]
On Mon, 16 Sep 2024 04:44:39 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which cleans up the `java` >> launcher to remove checks/support for outdated options? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8339918, these 6 options >> have been outdated and unsupported for several releases now. 2 of them even >> throw an error currently. The change in this PR removes the code which had >> specific checks for these options and will now consider these options just >> like any other unknown option to the launcher. >> >> tier1, tier2 and tier3 testing passed with these changes. Higher tier >> testing is in progress. >> >> Would this change require a CSR? > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove usage of -ms -mx -ss and -oss from > com.sun.tools.example.debug.tty.TTY Ship it! :) - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20945#pullrequestreview-2305746316
Re: RFR: 8339918: Remove checks for outdated -t -tm -Xfuture -checksource -cs -noasyncgc options from the launcher [v5]
On Mon, 16 Sep 2024 04:15:28 GMT, Jaikiran Pai wrote:
>> src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.java line 991:
>>
>>> 989:// Old-style options (These should remain in place
>>> as long as
>>> 990:// the standard VM accepts them)
>>> 991:token.equals("-prof") ||
>>
>> The `-prof` flag doesn't exist either.
>
> Done. There also was `-v` and `-v:...` in this example which aren't supported
> either (not even in JDK 8). So I've removed those as well.
What about:
token.startsWith("-ms") || token.startsWith("-mx") ||
token.startsWith("-ss") || token.startsWith("-oss") ) {
? `-oss` is gone as of 9. The others are not need as the -X counterpart can be
used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20945#discussion_r1760517430
Re: RFR: 8339918: Remove checks for outdated -t -tm -Xfuture -checksource -cs -noasyncgc options from the launcher [v5]
On Mon, 16 Sep 2024 03:56:39 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this change which cleans up the `java`
>> launcher to remove checks/support for outdated options?
>>
>> As noted in https://bugs.openjdk.org/browse/JDK-8339918, these 6 options
>> have been outdated and unsupported for several releases now. 2 of them even
>> throw an error currently. The change in this PR removes the code which had
>> specific checks for these options and will now consider these options just
>> like any other unknown option to the launcher.
>>
>> tier1, tier2 and tier3 testing passed with these changes. Higher tier
>> testing is in progress.
>>
>> Would this change require a CSR?
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove -noasyncgc from com.sun.tools.example.debug.tty.TTY
Looks good. But another cleanup is also possible
src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.java line 991:
> 989:// Old-style options (These should remain in place as
> long as
> 990:// the standard VM accepts them)
> 991:token.equals("-prof") ||
The `-prof` flag doesn't exist either.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20945#pullrequestreview-2305716139
PR Review Comment: https://git.openjdk.org/jdk/pull/20945#discussion_r1760502796
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow [v2]
On Mon, 16 Sep 2024 01:29:45 GMT, Jaikiran Pai wrote:
>> src/java.base/share/native/libjli/java.c line 1345:
>>
>>> 1343: if (JLI_StrCCmp(arg, "-Djava.class.path=") == 0) {
>>> 1344: _have_classpath = JNI_TRUE;
>>> 1345: } else if (JLI_StrCmp(arg, "-Djava.awt.headless=true") ==
>>> 0) {
>>
>> Was this processing moved from somewhere else?
>
> Hello David, this was in the SelectVersion() method that we removed in this
> PR
> https://github.com/openjdk/jdk/pull/20997/files#diff-c3caaf3e347b1a477e2b7278cb6a35da04a597de5632c13351a6e4e5a2924d21L1154
Okay I initially assumed I knew what `SelectVersion` did, now it seems it did
half a dozen things and we are still doing a large portion of them, but now
moved to other locations in the code: splashscreen, headless ...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760446054
Re: RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow
On Fri, 13 Sep 2024 12:29:26 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which proposes to remove the
> (internal) `SelectVersion()` function from the java launcher and also update
> the code comments in the launcher to match the current implementation?
>
> As noted in https://bugs.openjdk.org/browse/JDK-8340114, the
> `SelectVersion()` function in the launcher used to be relevant when JRE
> selection was a feature. That feature has been removed since Java 9
> https://bugs.openjdk.org/browse/JDK-8058407. The SelectVersion() in its
> current form isn't relevant anymore and can be removed.
>
> While at it, it was noticed that the current "flowchart" code comments in the
> launcher which attempts to explain the flow in the launcher code are
> outdated. The commit in this PR updates those comments for macosx and unix
> implementation. The windows variant doesn't have a "flowchart", but it too
> deserves a high level comment explaining this flow. I haven't updated the
> windows variant in this PR because that does a few additional things, which I
> need to review and understand better. I plan to take that up in a future
> change.
>
> An existing
> `test/jdk/tools/launcher/MultipleJRERemoved.java/MultipleJRERemoved` test had
> to be updated due to the changes in this PR. That test was launching `java`
> (once) with 3 unsupported JRE selection options and was expecting 3 error
> messages (one each for the unsupported option) for that single launch. With
> the change in this PR, we don't accumulate and throw all those 3 errors and
> instead we fail fast for any of these 3 unsupported JRE selection options.
> The fail fast implementation matches what we do with other similar
> unsupported options. The test had to be updated to not expect all 3 errors
> message in a single launch and instead expect to find one of those error
> messages. Given what this test is for, and the fact that JRE version
> selection options (rightly) continue to raise an error after this change, I
> think, an update to that test should be OK.
>
> No new tests have been introduced in this PR and tier testing is currently in
> progress.
Good to see a clean up here but I'm still puzzled by some aspects. And it is a
bit hard to track all the changes especially in relation to splashscreen.
Thanks
src/java.base/macosx/native/libjli/java_md_macosx.m line 83:
> 81: *point of creating a new thread in CreateExecutionEnvironment, the
> CreateExecutionEnvironment will check for
> 82: *the state flag to see if a new thread has already been spawned and
> upon noticing that it has, it will skip
> 83: *spawning any more threads and will return back from
> CreateExecutionEnvironment.
My understanding was that this recursive invocation was only needed when
switching modes/models. I don't see why it would be needed now.
src/java.base/share/native/libjli/java.c line 38:
> 36: * One job of the launcher is to remove command line options which the
> 37: * vm does not understand and will not process. These options include
> 38: * options which select which style of vm is run (e.g. -client and
Aren't these the only options now?
src/java.base/share/native/libjli/java.c line 1345:
> 1343: if (JLI_StrCCmp(arg, "-Djava.class.path=") == 0) {
> 1344: _have_classpath = JNI_TRUE;
> 1345: } else if (JLI_StrCmp(arg, "-Djava.awt.headless=true") ==
> 0) {
Was this processing moved from somewhere else?
src/java.base/share/native/libjli/java.c line 1347:
> 1345: } else if (JLI_StrCmp(arg, "-Djava.awt.headless=true") ==
> 0) {
> 1346: /*
> 1347: * Checking for headless toolkit option in the some way
> as AWT does:
Suggestion:
* Checking for headless toolkit option in the same way as AWT
does:
src/java.base/share/native/libjli/java.c line 1409:
> 1407: }
> 1408: // not in headless mode. we now set a couple of env variables
> that
> 1409: // will be used later by ShowSplashScreen()
Suggestion:
// Not in headless mode. We now set a couple of env variables that
// will be used later by ShowSplashScreen().
src/java.base/share/native/libjli/java.c line 1421:
> 1419: ) {
> 1420:
> 1421: // command line specified "-splash:" takes priority over manifest
> one.
General comment on comment style. Comments should either be written like
sentences and start with a Capital and end with a period; or have neither.
src/java.base/unix/native/libjli/java_md.c line 60:
> 58: * - JLI_Launch function, which is the entry point to the launcher,
> calls CreateExecutionEnvironment
> 59: *
> 60: * - CreateExecutionEnvironment does the following (not necessarily in
> that order):
Suggestion:
* - CreateExecutionEnvironment does the following (not necessarily in this
order):
-
PR Review: https://git.openjdk.org/jdk/pull/20997#pullreque
Re: RFR: 8339918: Remove checks for outdated -t -tm -Xfuture -checksource -cs -noasyncgc options from the launcher [v3]
On Sat, 14 Sep 2024 16:08:00 GMT, Bernd wrote: > What about `-Xdebug` it’s deprecated for remove. It’s not yet moved to the > obsolete section of the manual. And we could also remove it from the -X > command line help. If it is deprecated then it goes in the Deprecated section. The obsoleted section doesn't apply to -X flags but is part of the three-stage removal for Hotspot -XX flags. > src/java.base/share/man/java.1 line 3918: > >> 3916: option \f[V]-XX:-ScavengeBeforeFullGC\f[R]. >> 3917: .TP >> 3918: \f[V]-Xfuture\f[R] > > Maybe also remove the -X help (or at least change its description) in > https://github.com/openjdk/jdk/blob/c91fa278fe17ab204beef0fcef1ada6dd0bc37bb/src/java.base/share/classes/sun/launcher/resources/launcher.properties#L144 Yes this needs to be removed from the help text. - PR Comment: https://git.openjdk.org/jdk/pull/20945#issuecomment-2351149012 PR Review Comment: https://git.openjdk.org/jdk/pull/20945#discussion_r1759814814
Re: RFR: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
On Fri, 13 Sep 2024 06:25:48 GMT, Per Minborg wrote: > This PR fixes a regression introduced by > https://github.com/openjdk/jdk/pull/20848 LGTM! Thanks for the quick fix. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20983#pullrequestreview-2302228727
Re: RFR: 8339769: VM crash during initialization if working directory does not exist [v2]
On Thu, 12 Sep 2024 23:11:31 GMT, Justin Lu wrote: >> Please review this PR which restores the correct exception message when the >> current working directory can not be found during java startup in >> `initPhase1`. >> >> Both MacOS and Linux are expected to fail with `java.lang.Error: Properties >> init: Could not determine current working directory` if the _user.dir_ >> system property cannot be initialized. Currently, MacOS now fails with >> `java.lang.InternalError: platform encoding not initialized` and Linux fails >> with `java.lang.InternalError: null property: user.dir` which are both >> unexpected messages. >> >> In `System.c`, >> `Java_jdk_internal_util_SystemProps_00024Raw_platformProperties` calls >> `GetJavaProperties(JNIEnv *env)` which throws an internal error (with an >> appropriate message) for Unix platforms when the current working directory >> cannot be found. However, this exception is never checked and thus >> unexpected failures occur later. NULL should be returned after the exception >> is thrown, so that the initialization fails with the expected error when the >> return value is checked. >> >> >> // Get the platform specific values >> sprops = GetJavaProperties(env); >> CHECK_NULL_RETURN(sprops, NULL); >> >> >> Testing done locally on both platforms. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > return null in exception site instead Seems like a good fix. This seems to have been working by accident for a long time. IIUC the fix for [JDK-8305746](https://bugs.openjdk.org/browse/JDK-8305746) introduced a Java call that would have encountered this pending exception, but that code then clears any pending exception allowing things to proceed when we failed to setup the encoding properly and so then trigger the unexpected exception. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20975#pullrequestreview-2302107944
Re: RFR: 8299419: Thread.sleep(millis) may throw OOME [v3]
On Wed, 11 Sep 2024 11:42:19 GMT, Viktor Klang wrote: >> This PR applies @AlanBateman's patch from the JBS issue. > > Viktor Klang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Removing ThreadSleepEvent.isTurnedOn from stack trace checks This change exposed an existing bug in the strace tests with a missing constructor - [JDK-8340072](https://bugs.openjdk.org/browse/JDK-8340072) - PR Comment: https://git.openjdk.org/jdk/pull/20923#issuecomment-2347201093
Re: RFR: 8339332: Clean up the java launcher code
On Tue, 10 Sep 2024 06:15:57 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which cleans up the code in the
> `java` launcher? The original motivation for the change was to prevent the
> `java` launcher's C code from parsing a jar's manifest when launching an
> application through `java -jar foo.jar`. The jar parsing code in C currently
> doesn't have the same level of robustness and features as what's available in
> the Java side implementation of `Zip/JarFile` API in `java.util.zip`. Among
> them, one is the lack of ZIP64 support in the launcher's jar parsing code.
> That issue was noticed in https://bugs.openjdk.org/browse/JDK-8328995 and a
> PR (https://github.com/openjdk/jdk/pull/18479) was proposed to enhance this C
> code in the launcher to support ZIP64 jar files. Even before the proposed
> changes in that PR, there already were a few concerns about long term
> maintainability of the jar parsing code in the launcher given that it
> duplicates what we already do in the Java side implementation, so adding new
> C code here isn't prefer
able.
>
> The change in this current PR removes the part where the launcher's C code
> was parsing the jar's manifest file to identify "Main-Class" and
> "SplashScreen-Image" attributes from the manifest of the jar that was being
> launched. This was being done in the C code to support a long outdated "JRE
> selection" feature (mJRE https://bugs.openjdk.org/browse/JDK-8058407) which
> required it to parse the jar's manifest. After that feature was removed, the
> sole reason the jar's manifest was continued to be parsed in this launcher
> code was for convenience.
>
> With the changes proposed in this PR, the launcher will use the jar parsing C
> code only if the jar has a "SplashScreen-Image" in its manifest. The number
> of such jars, based on an experiment against a large corpus of jar files, is
> very minimal (we found just 26 jars in around 900K odd jar files with a
> "SplashScreen-Image"). The updated code in this PR also delegates the
> identification of the "SplashScreen-Image" attribute to the java code (in
> `LauncherHelper`) thus removing the need to parse the manifest in C code. The
> only time the C code will now do the jar parsing is to display a splash
> screen image (if any is present) from the jar file. I think even that can be
> avoided, but I haven't experimented with it and I would like to pursue that
> separately in future, instead of this PR.
>
> Finally, a few of the utility functions that were introduced in the launcher
> C code in a recent JEP to support "Implicitly Declared Classes and Instance
> Main...
Just a few drive-by comments. This is all rather complex from a reviewers
perspective - it is very hard to discern what code change relates to what you
described in the PR. Can it be broken up into smaller chunks perhaps?
Aside: we do an awful lot of Java code execution in the launcher these days
before we even get to load the real main class. I have to wonder how all this
affects startup?
src/java.base/macosx/native/libjli/java_md_macosx.m line 88:
> 86: * the command line or from the manifest of an executable jar file.
> 87: * The vm selection options are not passed to the running
> 88: * virtual machine; they must be screened out by the launcher.
The flowchart below seems to be out-of-date as I'm pretty sure no data-model
selection is performed any more (no -d64).
src/java.base/share/classes/jdk/internal/misc/MethodFinder.java line 105:
> 103: || !Modifier.isPublic(mods)
> 104: || mainMethod.getParameterCount() != 1)
> 105: )) {
This seems a distinct fix rather than a "cleanup". ??
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 607:
> 605: if (mainAttrs == null) {
> 606: abort(null, "java.launcher.jar.error3", jarname);
> 607: }
Why do we no longer need a null check?
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 1046:
> 1044: // carries details about the application's main method and splash
> screen image path,
> 1045: // that will be used by the native code in the launcher.
> 1046: public record MainEntry (Class mainClass, Method mainMethod,
Nit: no space before `(`.
src/java.base/share/native/libjli/java.c line 62:
> 60: * A NOTE TO DEVELOPERS: For performance reasons it is important that
> 61: * the program image remain relatively small until after
> 62: * CreateExecutionEnvironment has finished its possibly recursive
Is recursion still a possibility?
src/java.base/share/native/libjli/java.c line 1138:
> 1136: has_arg = IsOptionWithArgument(argc, argv);
> 1137: if (JLI_StrCCmp(arg, "-version:") == 0) {
> 1138: JLI_ReportErrorMessage(SPC_ERROR1);
Are these error message definitions, `SPC_ERROR1` etc, unused now?
-
PR Review: https://git.openjdk.org/jdk/
Re: RFR: 8339918: Remove checks for outdated -t -tm -Xfuture -checksource -cs -noasyncgc options from the launcher
On Wed, 11 Sep 2024 09:47:24 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which cleans up the `java` launcher > to remove checks/support for outdated options? > > As noted in https://bugs.openjdk.org/browse/JDK-8339918, these 6 options have > been outdated and unsupported for several releases now. 2 of them even throw > an error currently. The change in this PR removes the code which had specific > checks for these options and will now consider these options just like any > other unknown option to the launcher. > > tier1, tier2 and tier3 testing passed with these changes. Higher tier testing > is in progress. > > Would this change require a CSR? Good cleanup. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20945#pullrequestreview-2299031033
Re: RFR: 8299419: Thread.sleep(millis) may throw OOME [v2]
On Tue, 10 Sep 2024 21:01:24 GMT, Viktor Klang wrote: >> This PR applies @AlanBateman's patch from the JBS issue. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Removing ThreadSleepEvent from stack trace checks If these are just methods that might be seen in a stackdump then we still need `ThreadSleepEvent.isEnabled` and `ThreadSleepEvent.`. - PR Comment: https://git.openjdk.org/jdk/pull/20923#issuecomment-2342715153
Re: RFR: 8299419: Thread.sleep(millis) may throw OOME [v2]
On Tue, 10 Sep 2024 21:01:24 GMT, Viktor Klang wrote: >> This PR applies @AlanBateman's patch from the JBS issue. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Removing ThreadSleepEvent from stack trace checks With regards to the nsk tests, are `beforeSleep` and `afterSleep` still potential candidates now that it is impossible for them to throw OOME? - PR Review: https://git.openjdk.org/jdk/pull/20923#pullrequestreview-2294994911
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC [v2]
On Tue, 10 Sep 2024 19:23:16 GMT, Brent Christian wrote: >> From the bug description: >> ForceGC would be improved by moving the Reference.reachabilityFence() calls >> for 'obj' and 'ref'. >> >> Reference.reachabilityFence(obj) is currently placed after 'obj' has been >> set to null, so effectively does nothing. It should occur before obj = null; >> >> For Reference.reachabilityFence(ref): 'ref' is a PhantomReference to 'obj', >> and is registered with 'queue'. ForceGC.waitFor() later remove()s the >> reference from the queue, as an indication that some GC and reference >> processing has taken place (hopefully causing the BooleanSupplier to return >> true). >> >> The code expects the PhantomReference to be cleared and be put on the queue. >> But recall that a Reference refers to its queue, and not the other way >> around. If a Reference becomes unreachable and is garbage collected, it will >> never be enqueued. >> >> I argue that the VM/GC could determine that 'ref' is not used by waitFor() >> and collect it before the call to queue.remove(). Moving >> Reference.reachabilityFence(ref) after the for() loop would prevent this >> scenario. >> >> While this is only a very minor deficiency in ForceGC, I believe it would be >> good to ensure that the code behaves as expected. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > add try-finally block If you hide whitespace this change becomes trivial to review :) LGTM. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20898#pullrequestreview-2294914217
Re: RFR: 8339834: Replace usages of -mx and -ms in some tests [v2]
On Tue, 10 Sep 2024 11:10:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this trivial change which replaces the usages >> of `-mx` and `-ms` to `-Xmx` and `-Xms` in tests and in one code comment? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8339834, these options are >> outdated and support for them will soon be deprecated and removed as part of >> https://bugs.openjdk.org/browse/JDK-8286851. >> >> There are some more tests remaining in client-libs area which too require a >> similar change. I'll be creating a separate JBS issue and PR for that. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > update JImageToolTest too Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20930#pullrequestreview-2293814493
Re: RFR: 8339834: Replace usages of -mx and -ms in some tests [v2]
On Tue, 10 Sep 2024 11:07:30 GMT, Jaikiran Pai wrote: >> Can I please get a review of this trivial change which replaces the usages >> of `-mx` and `-ms` to `-Xmx` and `-Xms` in tests and in one code comment? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8339834, these options are >> outdated and support for them will soon be deprecated and removed as part of >> https://bugs.openjdk.org/browse/JDK-8286851. >> >> There are some more tests remaining in client-libs area which too require a >> similar change. I'll be creating a separate JBS issue and PR for that. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > update JImageToolTest too LGTM! Thanks for cleaning this up. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20930#pullrequestreview-2292111663
Re: RFR: 8339714: Delete tedious bool type define
On Mon, 9 Sep 2024 09:50:59 GMT, SendaoYan wrote: > Hi all, > This PR delete tedious bool type define in > `src/java.base/unix/native/libjsig/jsig.c` and > `src/utils/hsdis/binutils/hsdis-binutils.c`. After JEP > 347([JDK-8246032](https://bugs.openjdk.org/browse/JDK-8246032)), I think we > can "#include " to use bool type directly, like > [string.h](https://github.com/openjdk/jdk/blob/master/src/java.desktop/unix/native/libpipewire/include/spa/utils/string.h#L13) > do. > Make code more concision, the risk is quite low. > > Additional testing: > > - [x] Local build with --with-hsdis=binutils > --with-binutils=$HOME/software/binutils > - [x] Jtreg tests(include tier1/tier2/tier3 etc.) on linux x64 > - [x] Jtreg tests(include tier1/tier2/tier3 etc.) on linux aarch64 Yes I misspoke, I should have said we have required at least C99 for a while now. :) - PR Comment: https://git.openjdk.org/jdk/pull/20909#issuecomment-2340105221
Re: RFR: 8339714: Delete tedious bool type define
On Mon, 9 Sep 2024 09:50:59 GMT, SendaoYan wrote: > Hi all, > This PR delete tedious bool type define in > `src/java.base/unix/native/libjsig/jsig.c` and > `src/utils/hsdis/binutils/hsdis-binutils.c`. After JEP > 347([JDK-8246032](https://bugs.openjdk.org/browse/JDK-8246032)), I think we > can "#include " to use bool type directly, like > [string.h](https://github.com/openjdk/jdk/blob/master/src/java.desktop/unix/native/libpipewire/include/spa/utils/string.h#L13) > do. > Make code more concision, the risk is quite low. > > Additional testing: > > - [x] Local build with --with-hsdis=binutils > --with-binutils=$HOME/software/binutils > - [x] Jtreg tests(include tier1/tier2/tier3 etc.) on linux x64 > - [x] Jtreg tests(include tier1/tier2/tier3 etc.) on linux aarch64 This seems trivially fine to me. The JEP isn't really relevant for this C code as we have C99 as a minimum for a while now. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20909#pullrequestreview-2291801397
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Fri, 6 Sep 2024 19:57:41 GMT, Brent Christian wrote: > From the bug description: > ForceGC would be improved by moving the Reference.reachabilityFence() calls > for 'obj' and 'ref'. > > Reference.reachabilityFence(obj) is currently placed after 'obj' has been set > to null, so effectively does nothing. It should occur before obj = null; > > For Reference.reachabilityFence(ref): 'ref' is a PhantomReference to 'obj', > and is registered with 'queue'. ForceGC.waitFor() later remove()s the > reference from the queue, as an indication that some GC and reference > processing has taken place (hopefully causing the BooleanSupplier to return > true). > > The code expects the PhantomReference to be cleared and be put on the queue. > But recall that a Reference refers to its queue, and not the other way > around. If a Reference becomes unreachable and is garbage collected, it will > never be enqueued. > > I argue that the VM/GC could determine that 'ref' is not used by waitFor() > and collect it before the call to queue.remove(). Moving > Reference.reachabilityFence(ref) after the for() loop would prevent this > scenario. > > While this is only a very minor deficiency in ForceGC, I believe it would be > good to ensure that the code behaves as expected. My understanding is that try/finally is needed to ensure the RF is guaranteed to be seen to be executed at the expected location. Otherwise the RF can in theory be moved around. - PR Comment: https://git.openjdk.org/jdk/pull/20898#issuecomment-2340043873
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Mon, 9 Sep 2024 16:25:15 GMT, Stuart Marks wrote: >> @dholmes-ora Is this really possible? The `obj` ref is passed to the >> PhantomReference constructor, which stores it in a field, the constructed >> PhantomReference is returned, and it's then used in a reachabilityFence call >> below. So `obj` should remain reachable the entire time, right? > > (As an aside, I wasn't able to determine what any of the Reference classes do > if they're created with a null reference. Possibly a spec bug?) @stuart-marks My recollection, which I can't confirm is that this pattern was discussed internally and there was a lot of uncertainty about what was actually needed. Interestingly there was zero discussion of this in the actual PR that added it - https://github.com/openjdk/jdk/pull/8979 Thinking it through now, I tend to agree with you that the RF for `ref` suffices to prevent `obj` from being elided - PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1751000575
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Sat, 7 Sep 2024 00:39:16 GMT, Stuart Marks wrote: >> From the bug description: >> ForceGC would be improved by moving the Reference.reachabilityFence() calls >> for 'obj' and 'ref'. >> >> Reference.reachabilityFence(obj) is currently placed after 'obj' has been >> set to null, so effectively does nothing. It should occur before obj = null; >> >> For Reference.reachabilityFence(ref): 'ref' is a PhantomReference to 'obj', >> and is registered with 'queue'. ForceGC.waitFor() later remove()s the >> reference from the queue, as an indication that some GC and reference >> processing has taken place (hopefully causing the BooleanSupplier to return >> true). >> >> The code expects the PhantomReference to be cleared and be put on the queue. >> But recall that a Reference refers to its queue, and not the other way >> around. If a Reference becomes unreachable and is garbage collected, it will >> never be enqueued. >> >> I argue that the VM/GC could determine that 'ref' is not used by waitFor() >> and collect it before the call to queue.remove(). Moving >> Reference.reachabilityFence(ref) after the for() loop would prevent this >> scenario. >> >> While this is only a very minor deficiency in ForceGC, I believe it would be >> good to ensure that the code behaves as expected. > > test/lib/jdk/test/lib/util/ForceGC.java line 82: > >> 80: PhantomReference ref = new PhantomReference<>(obj, >> queue); >> 81: Reference.reachabilityFence(obj); >> 82: obj = null; > > You're right to question the utility of calling reachabilityFence(obj) after > obj has been nulled out. But I'm still questioning the utility of calling > RF(obj) at all. We don't care when obj is determined to be unreachable; what > we care about is that the GC has done some reference processing. Seems to me > we can simplify the above lines to > > PhantomReference ref = new PhantomReference<>(new Object(), > queue); > > and get rid of the local variable obj entirely. The reason for the explicit reference and RF, as I recall, is to guard against the allocation of the new object being elided entirely, with the `PhantomReference` constructor being passed null (or itself being elided) and no reference processing ever actually happening. - PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1749562854
Re: RFR: 8337753: Target class of upcall stub may be unloaded [v3]
On Wed, 4 Sep 2024 11:55:50 GMT, Jorn Vernee wrote: >> It does return. `ShouldNotReachHere` is used to crash the VM. > > `fatal()` might be better here. I could change it. Yes please do. - PR Review Comment: https://git.openjdk.org/jdk/pull/20479#discussion_r1745356400
Integrated: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths
On Fri, 30 Aug 2024 02:07:54 GMT, David Holmes wrote: > This is the implementation of a new method added to the JNI specification. > > From the CSR request: > > The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) > value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java > string can itself consist of `Integer.MAX_VALUE` characters, each of which > may require more than one byte to represent them in modified UTF-8 format.** > It follows then that this function cannot return the correct answer for all > String values and yet the specification makes no mention of this, nor of any > possible error to report if this situation is encountered. > > **The modified UTF-8 format used by the VM can require up to six bytes to > represent one unicode character, but six byte characters are stored as UTF16 > surrogate pairs. Hence the most bytes per character is 3, and so the maximum > length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to > 2*`Integer.MAX_VALUE`. > > Solution > > Deprecate the existing JNI `GetStringUTFLength` method noting that it may > return a truncated length, and add a new method, JNI > `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. > > --- > > We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni > > There are some incidental whitespace changes in > `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method entries. > > Testing: > - new test added > - tiers 1-3 sanity > > Thanks This pull request has now been integrated. Changeset: 90f3f432 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/90f3f4325772773f1dc1814c56d7326d5389e2c7 Stats: 209 lines in 9 files changed: 182 ins; 1 del; 26 mod 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths Reviewed-by: cjplummer, alanb - PR: https://git.openjdk.org/jdk/pull/20784
Re: RFR: 8337753: Target class of upcall stub may be unloaded
On Tue, 6 Aug 2024 17:26:55 GMT, Jorn Vernee wrote: > As discussed in the JBS issue: > > FFM upcall stubs embed a `Method*` of the target method in the stub. This > `Method*` is read from the `LambdaForm::vmentry` field associated with the > target method handle at the time when the upcall stub is generated. The MH > instance itself is stashed in a global JNI ref. So, there should be a > reachability chain to the holder class: `MH (receiver) -> LF (form) -> > MemberName (vmentry) -> ResolvedMethodName (method) -> Class (vmholder)` > > However, it appears that, due to multiple threads racing to initialize the > `vmentry` field of the `LambdaForm` of the target method handle of an upcall > stub, it is possible that the `vmentry` is updated _after_ we embed the > corresponding `Method`* into an upcall stub (or rather, the latest update is > not visible to the thread generating the upcall stub). Technically, it is > fine to keep using a 'stale' `vmentry`, but the problem is that now the > reachability chain is broken, since the upcall stub only extracts the target > `Method*`, and doesn't keep the stale `vmentry` reachable. The holder class > can then be unloaded, resulting in a crash. > > The fix I've chosen for this is to mimic what we already do in > `MethodHandles::jump_to_lambda_form`, and re-load the `vmentry` field from > the target method handle each time. Luckily, this does not really seem to > impact performance. > > > Performance numbers > x64: > > before: > > Benchmark Mode Cnt Score Error Units > Upcalls.panama_blank avgt 30 69.216 ± 1.791 ns/op > > > after: > > Benchmark Mode Cnt Score Error Units > Upcalls.panama_blank avgt 30 67.787 ± 0.684 ns/op > > > aarch64: > > before: > > Benchmark Mode Cnt Score Error Units > Upcalls.panama_blank avgt 30 61.574 ± 0.801 ns/op > > > after: > > Benchmark Mode Cnt Score Error Units > Upcalls.panama_blank avgt 30 61.218 ± 0.554 ns/op > > > > As for the added TestUpcallStress test, it takes about 800 seconds to run > this test on the dev machine I'm using, so I've set the timeout quite high. > Since it runs for so long, I've dropped it from the default `jdk_foreign` > test suite, which runs in tier2. Instead the new test will run in tier4. > > Testing: tier 1-4 src/hotspot/share/prims/upcallLinker.cpp line 142: > 140: Handle exception_h(Thread::current(), exception); > 141: java_lang_Throwable::print_stack_trace(exception_h, tty); > 142: ShouldNotReachHere(); How does `print_stack_trace` not return here? test/jdk/java/foreign/TestUpcallStress.java line 27: > 25: * @test > 26: * @requires jdk.foreign.linker != "FALLBACK" > 27: * @requires os.arch == "aarch64" & os.name == "Linux" Only for Linux-aarch64 ?? - PR Review Comment: https://git.openjdk.org/jdk/pull/20479#discussion_r1742870369 PR Review Comment: https://git.openjdk.org/jdk/pull/20479#discussion_r1742871513
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v3]
On Tue, 3 Sep 2024 20:50:09 GMT, Chris Plummer wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> The JNI version update was incompete > > Marked as reviewed by cjplummer (Reviewer). Thanks @plummercj ! - PR Comment: https://git.openjdk.org/jdk/pull/20784#issuecomment-2327507194
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v3]
On Tue, 3 Sep 2024 06:01:15 GMT, Alan Bateman wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> The JNI version update was incompete > > Marked as reviewed by alanb (Reviewer). Thanks @AlanBateman ! - PR Comment: https://git.openjdk.org/jdk/pull/20784#issuecomment-2325689077
Re: RFR: 8339364: AIX build fails: various unused variable and function warnings [v2]
On Tue, 3 Sep 2024 05:52:31 GMT, Magnus Ihse Bursie wrote: >> This code is devoid of pretty much all error handling and logging, but I >> agree that a simple fprintf on error would be useful. >> Also doesn't a call like this trigger the warning about ignoring a function >> result, or have we disabled that one? > >> Also doesn't a call like this trigger the warning about ignoring a function >> result, or have we disabled that one? > > Such a warning would be horrible to have enable. Then you would have to read > the value of every function that thought it should return a value, even if it > was just intended as some additional information that perhaps not everyone > wanted to use. (And then you could not just store it into a variable and > ignore it, since that would trigger the "unused variable" warning! ;-)) > > I know IntelliJ can provide warnings when ignoring the return value of a few > select JDK methods that are often used incorrectly, but then they had to > annotate these specially. I don't know if there is any similar kind of > annotation for clang/gcc that can warn about ignoring return values that > really, really shouldn't be ignored, but I'd guess not. At one stage we started using the idiom: (void) someFunc(); to tell the compiler to not warn about the unused result. IIRC that stopped working. - PR Review Comment: https://git.openjdk.org/jdk/pull/20812#discussion_r1741484485
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v3]
> This is the implementation of a new method added to the JNI specification. > > From the CSR request: > > The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) > value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java > string can itself consist of `Integer.MAX_VALUE` characters, each of which > may require more than one byte to represent them in modified UTF-8 format.** > It follows then that this function cannot return the correct answer for all > String values and yet the specification makes no mention of this, nor of any > possible error to report if this situation is encountered. > > **The modified UTF-8 format used by the VM can require up to six bytes to > represent one unicode character, but six byte characters are stored as UTF16 > surrogate pairs. Hence the most bytes per character is 3, and so the maximum > length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to > 2*`Integer.MAX_VALUE`. > > Solution > > Deprecate the existing JNI `GetStringUTFLength` method noting that it may > return a truncated length, and add a new method, JNI > `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. > > --- > > We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni > > There are some incidental whitespace changes in > `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method entries. > > Testing: > - new test added > - tiers 1-3 sanity > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: The JNI version update was incompete - Changes: - all: https://git.openjdk.org/jdk/pull/20784/files - new: https://git.openjdk.org/jdk/pull/20784/files/73174e64..29cfa8ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20784&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20784&range=01-02 Stats: 5 lines in 3 files changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/20784.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20784/head:pull/20784 PR: https://git.openjdk.org/jdk/pull/20784
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
On Fri, 30 Aug 2024 20:45:11 GMT, Chris Plummer wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude test on 32-bit > > Overall it looks good to me, although I don't have experience adding a new > JNI API (the dtrace probes were new to me), but it seems you are following > what is already in place for other functions, and the testing looks good. @plummercj and @AlanBateman could you please re-review as I was missing the main parts of the JNI version update! Thanks - PR Comment: https://git.openjdk.org/jdk/pull/20784#issuecomment-2325518928
Re: RFR: 8339364: AIX build fails: various unused variable and function warnings [v2]
On Mon, 2 Sep 2024 22:32:34 GMT, Magnus Ihse Bursie wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> adjust indentation in X11Color.c > > src/java.desktop/unix/native/libsplashscreen/splashscreen_sys.c line 744: > >> 742: int rslt = pthread_attr_init(&attr); >> 743: if (rslt != 0) return; >> 744: pthread_create(&thr, &attr, SplashScreenThread, (void *) splash); > > You don't think it would be better to check the return code? This code is devoid of pretty much all error handling and logging, but I agree that a simple fprintf on error would be useful. Also doesn't a call like this trigger the warning about ignoring a function result, or have we disabled that one? - PR Review Comment: https://git.openjdk.org/jdk/pull/20812#discussion_r1741354811
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
On Mon, 2 Sep 2024 09:05:17 GMT, Alan Bateman wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude test on 32-bit > > Deprecating the existing function and introducing the new function looks okay. > > The test is really 3 tests in one (GetStringUTFLength returning a truncated > size, GetStringUTFLength with -Xcheck:jni prints a warning, and > GetStringUTFLengthAsLong returns the long size). Personally I would have done > this as 3 test cases rather in one launch with -Xcheck:jni but that's your > choice. Thanks for the review @AlanBateman . I like the test the way it is. - PR Comment: https://git.openjdk.org/jdk/pull/20784#issuecomment-2324302820
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Sun, 1 Sep 2024 16:30:33 GMT, Julian Waters wrote: > This does make me wonder: What if the new method for checking if the VM was > statically linked was inlined? Then the problem comes back yet again as the > object files need to be recompiled once more. This is possible if Link Time > Optimization is switched on, and I don't like the implication that LTO might > be removed as a result just to make this work Wouldn't such link-time "inlining" only appear in the object code stored within the library, not in the original object file? - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2323775705
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
On Fri, 30 Aug 2024 20:45:11 GMT, Chris Plummer wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude test on 32-bit > > Overall it looks good to me, although I don't have experience adding a new > JNI API (the dtrace probes were new to me), but it seems you are following > what is already in place for other functions, and the testing looks good. Thanks for the review @plummercj ! - PR Comment: https://git.openjdk.org/jdk/pull/20784#issuecomment-2323760808
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Fri, 30 Aug 2024 10:51:30 GMT, Magnus Ihse Bursie wrote: >> I understand the cost overhead experienced by any individual Java run may be >> lost in the noise, but it still impacts every single Java run just to save >> some time/resources for the handful of builders of statically linked VMs. I >> am not a fan. > > @dholmes-ora This PR now has three reviewers approving it. You say you are > "not a fan". Does this mean you want to veto this change? Or can you be > willing to accept it, even if you do not like it? @magicus There is no "veto" power in OpenJDK. You have your reviewers, I have to accept it even if I don't like it. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2322873797
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
On Fri, 30 Aug 2024 05:11:30 GMT, Chris Plummer wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude test on 32-bit > > test/hotspot/jtreg/runtime/jni/checked/TestLargeUTF8Length.java line 27: > >> 25: * @bug 8328877 >> 26: * @summary Test warning for GetStringUTFLength and functionality of >> GetStringUTFLengthAsLong >> 27: * @library /test/lib > > Shouldn't this test have: > > `@requires vm.bits == 64 > ` Thanks for taking a look @plummercj . Yep I suppose it should. - PR Review Comment: https://git.openjdk.org/jdk/pull/20784#discussion_r1737947827
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
> This is the implementation of a new method added to the JNI specification. > > From the CSR request: > > The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) > value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java > string can itself consist of `Integer.MAX_VALUE` characters, each of which > may require more than one byte to represent them in modified UTF-8 format.** > It follows then that this function cannot return the correct answer for all > String values and yet the specification makes no mention of this, nor of any > possible error to report if this situation is encountered. > > **The modified UTF-8 format used by the VM can require up to six bytes to > represent one unicode character, but six byte characters are stored as UTF16 > surrogate pairs. Hence the most bytes per character is 3, and so the maximum > length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to > 2*`Integer.MAX_VALUE`. > > Solution > > Deprecate the existing JNI `GetStringUTFLength` method noting that it may > return a truncated length, and add a new method, JNI > `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. > > --- > > We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni > > There are some incidental whitespace changes in > `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method entries. > > Testing: > - new test added > - tiers 1-3 sanity > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: Exclude test on 32-bit - Changes: - all: https://git.openjdk.org/jdk/pull/20784/files - new: https://git.openjdk.org/jdk/pull/20784/files/9a8964b8..73174e64 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20784&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20784&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20784.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20784/head:pull/20784 PR: https://git.openjdk.org/jdk/pull/20784
RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths
This is the implementation of a new method added to the JNI specification. >From the CSR request: The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java string can itself consist of `Integer.MAX_VALUE` characters, each of which may require more than one byte to represent them in modified UTF-8 format.** It follows then that this function cannot return the correct answer for all String values and yet the specification makes no mention of this, nor of any possible error to report if this situation is encountered. **The modified UTF-8 format used by the VM can require up to six bytes to represent one unicode character, but six byte characters are stored as UTF16 surrogate pairs. Hence the most bytes per character is 3, and so the maximum length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to 2*`Integer.MAX_VALUE`. Solution Deprecate the existing JNI `GetStringUTFLength` method noting that it may return a truncated length, and add a new method, JNI `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. --- We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni There are some incidental whitespace changes in `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method entries. Testing: - new test added - tiers 1-3 sanity Thanks - Commit messages: - Test adjustments - 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths - Merge - Initial commit before splitting out UTF8 changes Changes: https://git.openjdk.org/jdk/pull/20784/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20784&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328877 Stats: 203 lines in 7 files changed: 180 ins; 1 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/20784.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20784/head:pull/20784 PR: https://git.openjdk.org/jdk/pull/20784
Re: RFR: 8338017: Add AOT command-line flag aliases [v3]
On Thu, 29 Aug 2024 22:11:36 GMT, Ioi Lam wrote: >> This is the 1st PR for [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://bugs.openjdk.org/browse/JDK-8315737). >> >> Add the following command-line options as specified in JEP 483: >> >> >> - `-XX:AOTMode=off/record/create/auto/on` >> - `-XX:AOTConfiguration=.aotconfig` >> - `-XX:AOTCache=.aot` >> >> These options are implemented as aliases to existing command-line flags such >> as `-Xshare:dump`, `-XX:SharedArchiveFile`, `-XX:DumpLoadedClassesList`, etc. >> >> Please see the CSR (TODO) for detailed specification. >> >> - >> See [here](https://bugs.openjdk.org/browse/JDK-8315737) for the sequence of >> dependent RFEs for implementing JEP 483. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @dholmes-ora comments: do not check for -XX:AOTMode=create in JLI java.c Looks good. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20516#pullrequestreview-2270696172
Re: RFR: 8338017: Add AOT command-line flag aliases [v2]
On Thu, 29 Aug 2024 05:05:58 GMT, David Holmes wrote:
>> Ioi Lam 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:
>>
>> - Fixed copyright dates
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> jep-483-step-01-8338017-add-aot-command-line-aliases
>> - Merge branch 'master' into
>> jep-483-step-01-8338017-add-aot-command-line-aliases
>> - Fixed whitespaces
>> - 8338017: Add AOT command-line flag aliases
>
> src/java.base/share/native/libjli/java.c line 1521:
>
>> 1519: dumpSharedSpaces = JNI_TRUE;
>> 1520: }
>> 1521: if (JLI_StrCmp(arg, "-XX:AOTMode=create") == 0) {
>
> This is inappropriate - the launcher does not, and should not, process
> hotspot -XX options. Any aliasing should happen in the hotspot argument
> processing logic.
I realize this poses a problem with communicating to the launcher that this is
a "terminal" flag. Maybe AOT should have -X flags instead of -XX?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20516#discussion_r1735576562
Re: RFR: 8338017: Add AOT command-line flag aliases [v2]
On Thu, 29 Aug 2024 04:24:02 GMT, Ioi Lam wrote:
>> This is the 1st PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> Add the following command-line options as specified in JEP 483:
>>
>>
>> - `-XX:AOTMode=off/record/create/auto/on`
>> - `-XX:AOTConfiguration=.aotconfig`
>> - `-XX:AOTCache=.aot`
>>
>> These options are implemented as aliases to existing command-line flags such
>> as `-Xshare:dump`, `-XX:SharedArchiveFile`, `-XX:DumpLoadedClassesList`, etc.
>>
>> Please see the CSR (TODO) for detailed specification.
>>
>> -
>> See [here](https://bugs.openjdk.org/browse/JDK-8315737) for the sequence of
>> dependent RFEs for implementing JEP 483.
>
> Ioi Lam 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:
>
> - Fixed copyright dates
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> jep-483-step-01-8338017-add-aot-command-line-aliases
> - Merge branch 'master' into
> jep-483-step-01-8338017-add-aot-command-line-aliases
> - Fixed whitespaces
> - 8338017: Add AOT command-line flag aliases
Seems reasonable but one issue flagged below.
Thanks.
src/java.base/share/native/libjli/java.c line 1521:
> 1519: dumpSharedSpaces = JNI_TRUE;
> 1520: }
> 1521: if (JLI_StrCmp(arg, "-XX:AOTMode=create") == 0) {
This is inappropriate - the launcher does not, and should not, process hotspot
-XX options. Any aliasing should happen in the hotspot argument processing
logic.
-
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20516#pullrequestreview-2267689489
PR Review Comment: https://git.openjdk.org/jdk/pull/20516#discussion_r1735573603
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly I understand the cost overhead experienced by any individual Java run may be lost in the noise, but it still impacts every single Java run just to save some time/resources for the handful of builders of statically linked VMs. I am not a fan. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2309162391
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Sat, 24 Aug 2024 05:12:42 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into snprintf > - Change copyright years and formatting > - Revert Standard Integer type rewrite > - USe os::snprintf in HotSpot > - Obliterate most references to _snprintf in the Windows JDK Still good for me. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2259495896
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Thu, 22 Aug 2024 08:54:56 GMT, Magnus Ihse Bursie wrote: >> Sorry but I don't understand the point of changing build-time constructs >> using `ifdef STATIC_BUILD` into what appear to be runtime checks, but the >> result of which is already determined at build time. These are not really >> runtime checks. > > @dholmes-ora > >> Sorry but I don't understand the point of changing build-time constructs >> using ifdef STATIC_BUILD into what appear to be runtime checks, but the >> result of which is already determined at build time. > > I apologize. I did not express the intent of this change clear enough. > > The background is that we want to build and test statically linked native > libraries. Currently, building a statically linked version requires > recompiling all native source code into a completely new set of .o files, > which are then linked into a static library. > > This is extremely wasteful. Most of the code is completely identical for > static and dynamic libraries. To fix this, me, Jiangli and her team have been > working on a way to get around this. > > By moving the ifdef check to a new file that just contains a single function, > we only need to compile this single file twice -- once for the static > library, and once for the dynamic library. All other .o files is compiled > just once, and then you link "all other files" + "the one special file for > your kind of library" to get what you want. > > Unfortunately, there is also one more blocker before this can be achieved. > That is the reason the corresponding change in the build system is not > included in this patch. (So this is a preparation for these future changes, > but not the complete solution.) The missing part is that the > `[JNI|Agent]_On[Un]Load` functions need to be able to use the static linked > naming scheme, even for dynamically linked libraries. This is trivial per se, > but requires a spec change, which has not yet happened. > > The reason I want to get this partial solution into the mainline right now, > instead of waiting for the spec change and the complete build system fix, is > that these new functions for checking for static/dynamic are needed by > additional changes that Jiangli have created upstream, and that I am trying > to help her get integrated. (The goal of these changes is to make not just > static libraries, but to link these static libraries with the java launcher > into a statically linked launcher, which is a pre-requisite for the rest of > the Hermetic Java story.) @magicus is the final intent here that this one magic file will be compiled first with an inline declaration such that when the other files containing the apparent runtime check get compiled, it can actually be determined at build time and so have the same effects as the old ifdef logic? Otherwise it concerns me that a build-time issue that affects a handful of people becomes a runtime issue that affects every single instance of a running Java program. There are also other source-level solutions possible here by refactoring the code that has static vs dynamic linking dependencies into its own files and the build system can then select which set of files to compile. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2305875408
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly Sorry but I don't understand the point of changing build-time constructs using `ifdef STATIC_BUILD` into what appear to be runtime checks, but the result of which is already determined at build time. These are not really runtime checks. - PR Review: https://git.openjdk.org/jdk/pull/20666#pullrequestreview-2252973892
Re: RFR: 8338417: Explicitly pin a virtual thread before acquiring the JFR string pool monitor [v3]
On Thu, 15 Aug 2024 11:20:23 GMT, Markus Grönlund wrote:
>> Greetings,
>>
>> Explicitly pin a virtual thread before acquiring the JFR string pool monitor
>> because migrating a carrier thread local event writer object onto another
>> carrier thread is impossible.
>>
>> During event commit, the thread is in a critical section because it has
>> loaded a carrier thread local event writer object. For virtual threads, a
>> contended monitor, such as a synchronized block, is a point where a thread
>> could become unmounted.
>>
>> A monitor guards the JFR string pool, but remounting a virtual thread onto
>> another carrier is impossible because of the event writer.
>>
>> Therefore, it's imperative to use explicit pin constructs to prevent
>> unmounting at this location.
>>
>> Testing: jdk_jfr
>>
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one
> additional commit since the last revision:
>
> update test comment
Functionally seems fine. Could be optimised a bit.
src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java line 86:
> 84:
> 85: private static void unpinVirtualThread() {
> 86: if (Thread.currentThread().isVirtual() &&
> ContinuationSupport.isSupported()) {
If you are at all concerned about overhead here then pin could return a boolean
to indicate if the pin happened and oyu could then unpin just by checking that
boolean and avoid doing the isVirtual and isSupported checks again.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20588#pullrequestreview-2240268782
PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1718332618
Re: RFR: 8338398: Trivially fix grammar and typos [v2]
On Thu, 15 Aug 2024 08:58:46 GMT, David Holmes wrote: >> I assume you both mean I should change "elapses" to "elapsed" throughout the >> PR, not just in that particular occurrence. > > No, elsewhere you don't have an existing tense to match - I only want the > sentence to be self-consistent. The question of whether the docs overall > should be expressed in past or future tense is a much bigger issue. Oops I see the others do also have "completed" - my bad. - PR Review Comment: https://git.openjdk.org/jdk/pull/20584#discussion_r1718157359
Re: RFR: 8338398: Trivially fix grammar and typos [v2]
On Thu, 15 Aug 2024 08:24:30 GMT, Pavel Rappo wrote: >> Use "elapsed" here to match "completed". > > I assume you both mean I should change "elapses" to "elapsed" throughout the > PR, not just in that particular occurrence. No, elsewhere you don't have an existing tense to match - I only want the sentence to be self-consistent. The question of whether the docs overall should be expressed in past or future tense is a much bigger issue. - PR Review Comment: https://git.openjdk.org/jdk/pull/20584#discussion_r1718155121
Re: RFR: 8338417: Explicitly pin a virtual thread before acquiring the JFR string pool monitor
On Wed, 14 Aug 2024 20:53:33 GMT, Markus Grönlund wrote:
> Greetings,
>
> Explicitly pin a virtual thread before acquiring the JFR string pool monitor
> because migrating a carrier thread local event writer object onto another
> carrier thread is impossible.
>
> During event commit, the thread is in a critical section because it has
> loaded a carrier thread local event writer object. For virtual threads, a
> contended monitor, such as a synchronized block, is a point where a thread
> could become unmounted.
>
> A monitor guards the JFR string pool, but remounting a virtual thread onto
> another carrier is impossible because of the event writer.
>
> Therefore, it's imperative to use explicit pin constructs to prevent
> unmounting at this location.
>
> Testing: jdk_jfr
>
> Thanks
> Markus
Changes requested by dholmes (Reviewer).
src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java line 93:
> 91: private static long storeString(String s) {
> 92: try {
> 93: pinVirtualThread();
The pin should be outside the try so that the finally can only happen if
pinning was succesful.
-
PR Review: https://git.openjdk.org/jdk/pull/20588#pullrequestreview-2239597266
PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1717852378
Re: RFR: 8338398: Trivially fix grammar and typos
On Wed, 14 Aug 2024 22:11:39 GMT, Pavel Rappo wrote: >> src/java.base/share/classes/java/util/concurrent/ForkJoinTask.java line 1075: >> >>> 1073: /** >>> 1074: * Tries to join this task, returning true if it completed >>> 1075: * (possibly exceptionally) before the given timeout elapses and >> >> Seems like mixed tense now: do we want completes/elapses or >> completed/elapsed? > > I admit I had doubts about the sequence of tenses too. That's a pretty > complex sentence, and I'm a non-native English speaker. It's hard to re-tense > it while retaining its conditional nature. Use "elapsed" here to match "completed". - PR Review Comment: https://git.openjdk.org/jdk/pull/20584#discussion_r1717850485
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 16:11:46 GMT, Viktor Klang wrote: >> 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when >> failing due to a LinkageError or other errors > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Catching both Error and RuntimeException When we have `Catch (Error | RuntimeException ex)` exactly what RuntimeExceptions are we trying to account for - because they should not happen and may break the synchronizer if they do. - PR Comment: https://git.openjdk.org/jdk/pull/20548#issuecomment-2288049212
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp line 84:
> 82:
> 83: if (LockingMode == LM_LIGHTWEIGHT) {
> 84: lightweight_lock(disp_hdr, obj, hdr, temp, rscratch2, slow_case);
Given the declaration:
void MacroAssembler::lightweight_lock(Register basic_lock, Register obj,
Register t1, Register t2, Register t3, Label& slow)
it looks odd to pass `disp_hdr` here - is that variable just mis-named?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716339494
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Tue, 13 Aug 2024 08:02:04 GMT, Alan Bateman wrote: > > It has been a while since I knew this code reasonably well so perhaps I > > have just forgotten this difference between AQS and built-in monitors, but > > it seems that a Condition.await can return by throwing an exception without > > re-acquiring the associated synchronizer. Or is that handled at a > > higher-level? > > The semantics are the same as monitor wait/notify so Condition.await must > guarantee to hold the lock when it returns. If ConditionNode.block were to > throw something like StackOverflowError then there would be an issue (it's a > different park to the one changed in this PR but I think you do have a good > point). AFAICS `await` would call the acquire method that was changed here. I know we have issues with OOME and SOE, but these changes admit more general exception possibilities that would seem to undermine the required guarantee. But perhaps a different `acquire` is involved in the `await` case? - PR Comment: https://git.openjdk.org/jdk/pull/20548#issuecomment-2287218877
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 16:11:46 GMT, Viktor Klang wrote: >> 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when >> failing due to a LinkageError or other errors > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Catching both Error and RuntimeException It has been a while since I knew this code reasonably well so perhaps I have just forgotten this difference between AQS and built-in monitors, but it seems that a Condition.await can return by throwing an exception without re-acquiring the associated synchronizer. Or is that handled at a higher-level? - PR Review: https://git.openjdk.org/jdk/pull/20548#pullrequestreview-2234191725
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 21:37:45 GMT, Doug Lea wrote:
>> src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
>> line 381:
>>
>>> 379: else
>>> 380: break;
>>> 381: } catch (Error | RuntimeException ex) {
>>
>> @DougLea @AlanBateman Changed to Error | RuntimeException here and for AQS
>
> lgtm!
`catch (Throwable ex)` would be consistent with the similar block at line 331.
Though I'm unclear how that compiles without the method declaring `throws
Throwable` ??
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20548#discussion_r1714470560
Re: [jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 08:26:51 GMT, Alan Bateman wrote: >> Before RC we need to update the man pages in the repo from their Markdown >> sources. All pages at a minimum have 23-ea replaced with 23, and publication >> year set to 2024 if needed. >> >> This also picks up the unpublished changes to java.1 from: >> >> - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage >> for obsoletion of RAMFraction flags >> - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage >> for obsoletion of ScavengeBeforeFullGC >> >> And a typo crept in to java.1 from: >> - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the >> Memory-Access Methods in sun.misc.Unsafe for Removal >> >> This also picks up the unpublished change to keytool.1 from: >> >> - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in >> Supported Named Extensions - BasicContraints >> >> This also picks up the unpublished change to javadoc.1 from: >> >> - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should >> default @since for a nested class to that of its enclosing class >> >> and some formatting changes (unclear why - perhaps pandoc version) from the >> combined changeset for: >> >> - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP >> 467: Markdown Documentation Comments >> - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update >> Elements for '///' documentation comments >> >> The javac.1 file has its copyright year reverted to match what is in the >> source file (which should have been updated but wasn't). >> >> Thanks. > > Thanks for the tireless effort to update the man pages at the end of each > release. Thanks for the reviews @AlanBateman and @irisclark ! - PR Comment: https://git.openjdk.org/jdk/pull/20248#issuecomment-2241805932
[jdk23] Integrated: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 05:47:15 GMT, David Holmes wrote: > Before RC we need to update the man pages in the repo from their Markdown > sources. All pages at a minimum have 23-ea replaced with 23, and publication > year set to 2024 if needed. > > This also picks up the unpublished changes to java.1 from: > > - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage > for obsoletion of RAMFraction flags > - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage > for obsoletion of ScavengeBeforeFullGC > > And a typo crept in to java.1 from: > - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the > Memory-Access Methods in sun.misc.Unsafe for Removal > > This also picks up the unpublished change to keytool.1 from: > > - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in > Supported Named Extensions - BasicContraints > > This also picks up the unpublished change to javadoc.1 from: > > - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should > default @since for a nested class to that of its enclosing class > > and some formatting changes (unclear why - perhaps pandoc version) from the > combined changeset for: > > - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP > 467: Markdown Documentation Comments > - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements > for '///' documentation comments > > The javac.1 file has its copyright year reverted to match what is in the > source file (which should have been updated but wasn't). > > Thanks. This pull request has now been integrated. Changeset: 5473e9e4 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/5473e9e488c8fc7d99ea9d97fd0e7dd212636546 Stats: 142 lines in 28 files changed: 51 ins; 52 del; 39 mod 8325280: Update troff manpages in JDK 23 before RC Reviewed-by: alanb, iris - PR: https://git.openjdk.org/jdk/pull/20248
Re: [jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 05:47:15 GMT, David Holmes wrote: > Before RC we need to update the man pages in the repo from their Markdown > sources. All pages at a minimum have 23-ea replaced with 23, and publication > year set to 2024 if needed. > > This also picks up the unpublished changes to java.1 from: > > - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage > for obsoletion of RAMFraction flags > - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage > for obsoletion of ScavengeBeforeFullGC > > And a typo crept in to java.1 from: > - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the > Memory-Access Methods in sun.misc.Unsafe for Removal > > This also picks up the unpublished change to keytool.1 from: > > - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in > Supported Named Extensions - BasicContraints > > This also picks up the unpublished change to javadoc.1 from: > > - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should > default @since for a nested class to that of its enclosing class > > and some formatting changes (unclear why - perhaps pandoc version) from the > combined changeset for: > > - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP > 467: Markdown Documentation Comments > - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements > for '///' documentation comments > > The javac.1 file has its copyright year reverted to match what is in the > source file (which should have been updated but wasn't). > > Thanks. Note this is simply a re-generation of the troff files from their sources. If there are any problems that need fixing then a new JBS issue has to be filed to update the source file and regenerate the troff file. - PR Comment: https://git.openjdk.org/jdk/pull/20248#issuecomment-2238248354
[jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
Before RC we need to update the man pages in the repo from their Markdown sources. All pages at a minimum have 23-ea replaced with 23, and publication year set to 2024 if needed. This also picks up the unpublished changes to java.1 from: - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage for obsoletion of RAMFraction flags - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage for obsoletion of ScavengeBeforeFullGC And a typo crept in to java.1 from: - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal This also picks up the unpublished change to keytool.1 from: - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in Supported Named Extensions - BasicContraints This also picks up the unpublished change to javadoc.1 from: - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should default @since for a nested class to that of its enclosing class and some formatting changes (unclear why - perhaps pandoc version) from the combined changeset for: - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP 467: Markdown Documentation Comments - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements for '///' documentation comments The javac.1 file has its copyright year reverted to match what is in the source file (which should have been updated but wasn't). Thanks. - Commit messages: - 8325280: Update troff manpages in JDK 23 before RC Changes: https://git.openjdk.org/jdk/pull/20248/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20248&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325280 Stats: 142 lines in 28 files changed: 51 ins; 52 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/20248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20248/head:pull/20248 PR: https://git.openjdk.org/jdk/pull/20248
Re: RFR: 8334772: Change Class::signers to an explicit field [v3]
On Thu, 18 Jul 2024 13:48:06 GMT, Chen Liang wrote: >> `Class` has 2 VM-injected fields that can be made explicit: `Object[] >> signers` and `ProtectionDomain protectionDomain`. We make the signers field >> explicit. (The ProtectionDomain can be revisited when SecurityManager is >> removed, as SecurityManager is accessing it via JNI as well.) >> >> Migrate the JNI code to Java. The getter previously had a redundant >> primitive type check, which is dropped in the migrated Java code. The >> `Object[] getSigners` is no longer `native`, thus requiring a CSR record. >> Reviewers please help review the associated CSR. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/class-signers > - Reorder comment of classData to avoid misunderstanding > - 8334772: Change Class::signers to an explicit field I am not a hprof expert but AFAICS the `HPROF_GC_CLASS_DUMP` contains an explicit id for the classloader, signers, and pd, of the class, and then later a list of all fields declared in the class. AFAICS there is no real connection between these, so it doesn't matter if the classloader/signers/pd is an injected field, a regular Java field, or not a field at all. So in that regard it seems `signers` will now be handled the same way as `classloader` and so that should be fine. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20223#pullrequestreview-2186999675
Re: RFR: 8334772: Change Class::signers to an explicit field [v3]
On Thu, 18 Jul 2024 13:48:06 GMT, Chen Liang wrote: >> `Class` has 2 VM-injected fields that can be made explicit: `Object[] >> signers` and `ProtectionDomain protectionDomain`. We make the signers field >> explicit. (The ProtectionDomain can be revisited when SecurityManager is >> removed, as SecurityManager is accessing it via JNI as well.) >> >> Migrate the JNI code to Java. The getter previously had a redundant >> primitive type check, which is dropped in the migrated Java code. The >> `Object[] getSigners` is no longer `native`, thus requiring a CSR record. >> Reviewers please help review the associated CSR. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/class-signers > - Reorder comment of classData to avoid misunderstanding > - 8334772: Change Class::signers to an explicit field On the JVMTI side and the heapDumper ... I see that heapDumper explicitly fills in a slot for the classloader, but that is also an explicit field. Does that mean that the classloader appears twice, or does the fact it is filtered by reflection mean that the heapDumper doesn't see it when dumping fields? If the latter then it suggests to me that we should be doing the same for the signers. Otherwise I don't know what the implications might be for having the field listed twice. - PR Review: https://git.openjdk.org/jdk/pull/20223#pullrequestreview-2186958021
Re: RFR: 8334772: Change Class::signers to an explicit field
On Wed, 17 Jul 2024 19:47:44 GMT, Chen Liang wrote: > `Class` has 2 VM-injected fields that can be made explicit: `Object[] > signers` and `ProtectionDomain protectionDomain`. We make the signers field > explicit. (The ProtectionDomain can be revisited when SecurityManager is > removed, as SecurityManager is accessing it via JNI as well.) > > Migrate the JNI code to Java. The getter previously had a redundant primitive > type check, which is dropped in the migrated Java code. The `Object[] > getSigners` is no longer `native`, thus requiring a CSR record. Reviewers > please help review the associated CSR. The relocation of the field to Java looks good. But I am concerned that the field is now exposed to reflection. - PR Review: https://git.openjdk.org/jdk/pull/20223#pullrequestreview-2184157888
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 102:
> 100: assert(*value != nullptr, "must be");
> 101: return (*value)->object_is_cleared();
> 102: }
The `is_dead` functions seem oddly placed given they do not relate to the
object stored in the wrapper. Why are they here? And what is the difference
between `object_is_cleared` and `object_is_dead` (as used by `LookupMonitor`) ?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 105:
> 103: };
> 104:
> 105: class LookupMonitor : public StackObj {
I'm not understanding why we need this little wrapper class.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680526331
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680526868
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 60:
> 58:
> 59: // ConcurrentHashTable storing links from objects to ObjectMonitors
> 60: class ObjectMonitorWorld : public CHeapObj {
OMWorld describes the project not the hashtable, this should be called
ObjectMonitorTable or some such.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 62:
> 60: class ObjectMonitorWorld : public CHeapObj {
> 61: struct Config {
> 62: using Value = ObjectMonitor*;
Does this alias really help? We don't state the type that many times and it
looks odd to end up with a mix of `Value` and `ObjectMonitor*` in the same code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680508685
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680508801
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/deoptimization.cpp line 1641:
> 1639: assert(fr.is_deoptimized_frame(), "frame must be
> scheduled for deoptimization");
> 1640: if (LockingMode == LM_LEGACY) {
> 1641:
> mon_info->lock()->set_displaced_header(markWord::unused_mark());
In the existing code how is this restricted to the LM_LEGACY case?? It appears
to be unconditional which suggests you are changing the non-UOMT LM_LIGHTWEIGHT
logic. ??
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680500696
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Wed, 17 Jul 2024 06:39:14 GMT, David Holmes wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with 10 >> additional commits since the last revision: >> >> - Remove try_read >> - Add explicit to single parameter constructors >> - Remove superfluous access specifier >> - Remove unused include >> - Update assert message OMCache::set_monitor >> - Fix indentation >> - Remove outdated comment LightweightSynchronizer::exit >> - Remove logStream include >> - Remove strange comment >> - Fix javaThread include > > src/hotspot/share/runtime/basicLock.hpp line 46: > >> 44: // Used as a cache the ObjectMonitor* used when locking. Must either >> 45: // be nullptr or the ObjectMonitor* used when locking. >> 46: volatile uintptr_t _metadata; > > The displaced header/markword terminology was very well known to people, > whereas "metadata" is really abstract - people will always need to go and > find out what it actually refers to. Could we not define a union here to > support the legacy and lightweight modes more explicitly and keep the > existing terminology for the setters/getters for the code that uses it? I should have read ahead. I see you do keep the setters/getters. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680497748
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas wrote: >> When inflating a monitor the `ObjectMonitor*` is written directly over the >> `markWord` and any overwritten data is displaced into a displaced >> `markWord`. This is problematic for concurrent GCs which needs extra care or >> looser semantics to use this displaced data. In Lilliput this data also >> contains the klass forcing this to be something that the GC has to take into >> account everywhere. >> >> This patch introduces an alternative solution where locking only uses the >> lock bits of the `markWord` and inflation does not override and displace the >> `markWord`. This is done by keeping associations between objects and >> `ObjectMonitor*` in an external hash table. Different caching techniques are >> used to speedup lookups from compiled code. >> >> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is >> only supported in combination with the LM_LIGHTWEIGHT locking mode (the >> default). >> >> This patch has been evaluated to be performance neutral when >> `UseObjectMonitorTable` is turned off (the default). >> >> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` >> and `UseObjectMonitorTable` works. >> >> # Cleanups >> >> Cleaned up displaced header usage for: >> * BasicLock >> * Contains some Zero changes >> * Renames one exported JVMCI field >> * ObjectMonitor >> * Updates comments and tests consistencies >> >> # Refactoring >> >> `ObjectMonitor::enter` has been refactored an a >> `ObjectMonitorContentionMark` witness object has been introduced to the >> signatures. Which signals that the contentions reference counter is being >> held. More details are given below in the section about deflation. >> >> The initial purpose of this was to allow `UseObjectMonitorTable` to interact >> more seamlessly with the `ObjectMonitor::enter` code. >> >> _There is even more `ObjectMonitor` refactoring which can be done here to >> create a more understandable and enforceable API. There are a handful of >> invariants / assumptions which are not always explicitly asserted which >> could be trivially abstracted and verified by the type system by using >> similar witness objects._ >> >> # LightweightSynchronizer >> >> Working on adapting and incorporating the following section as a comment in >> the source code >> >> ## Fast Locking >> >> CAS on locking bits in markWord. >> 0b00 (Fast Locked) <--> 0b01 (Unlocked) >> >> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to >> avoid inflating by spinning a bit. >> >> If 0b10 (Inflated) is observed or there is to... > > Axel Boldt-Christmas has updated the pull request incrementally with 10 > additional commits since the last revision: > > - Remove try_read > - Add explicit to single parameter constructors > - Remove superfluous access specifier > - Remove unused include > - Update assert message OMCache::set_monitor > - Fix indentation > - Remove outdated comment LightweightSynchronizer::exit > - Remove logStream include > - Remove strange comment > - Fix javaThread include src/hotspot/share/runtime/basicLock.hpp line 46: > 44: // Used as a cache the ObjectMonitor* used when locking. Must either > 45: // be nullptr or the ObjectMonitor* used when locking. > 46: volatile uintptr_t _metadata; The displaced header/markword terminology was very well known to people, whereas "metadata" is really abstract - people will always need to go and find out what it actually refers to. Could we not define a union here to support the legacy and lightweight modes more explicitly and keep the existing terminology for the setters/getters for the code that uses it? - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680496495
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas wrote: >> When inflating a monitor the `ObjectMonitor*` is written directly over the >> `markWord` and any overwritten data is displaced into a displaced >> `markWord`. This is problematic for concurrent GCs which needs extra care or >> looser semantics to use this displaced data. In Lilliput this data also >> contains the klass forcing this to be something that the GC has to take into >> account everywhere. >> >> This patch introduces an alternative solution where locking only uses the >> lock bits of the `markWord` and inflation does not override and displace the >> `markWord`. This is done by keeping associations between objects and >> `ObjectMonitor*` in an external hash table. Different caching techniques are >> used to speedup lookups from compiled code. >> >> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is >> only supported in combination with the LM_LIGHTWEIGHT locking mode (the >> default). >> >> This patch has been evaluated to be performance neutral when >> `UseObjectMonitorTable` is turned off (the default). >> >> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` >> and `UseObjectMonitorTable` works. >> >> # Cleanups >> >> Cleaned up displaced header usage for: >> * BasicLock >> * Contains some Zero changes >> * Renames one exported JVMCI field >> * ObjectMonitor >> * Updates comments and tests consistencies >> >> # Refactoring >> >> `ObjectMonitor::enter` has been refactored an a >> `ObjectMonitorContentionMark` witness object has been introduced to the >> signatures. Which signals that the contentions reference counter is being >> held. More details are given below in the section about deflation. >> >> The initial purpose of this was to allow `UseObjectMonitorTable` to interact >> more seamlessly with the `ObjectMonitor::enter` code. >> >> _There is even more `ObjectMonitor` refactoring which can be done here to >> create a more understandable and enforceable API. There are a handful of >> invariants / assumptions which are not always explicitly asserted which >> could be trivially abstracted and verified by the type system by using >> similar witness objects._ >> >> # LightweightSynchronizer >> >> Working on adapting and incorporating the following section as a comment in >> the source code >> >> ## Fast Locking >> >> CAS on locking bits in markWord. >> 0b00 (Fast Locked) <--> 0b01 (Unlocked) >> >> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to >> avoid inflating by spinning a bit. >> >> If 0b10 (Inflated) is observed or there is to... > > Axel Boldt-Christmas has updated the pull request incrementally with 10 > additional commits since the last revision: > > - Remove try_read > - Add explicit to single parameter constructors > - Remove superfluous access specifier > - Remove unused include > - Update assert message OMCache::set_monitor > - Fix indentation > - Remove outdated comment LightweightSynchronizer::exit > - Remove logStream include > - Remove strange comment > - Fix javaThread include src/hotspot/share/runtime/basicLock.hpp line 44: > 42: // a sentinel zero value indicating a recursive stack-lock. > 43: // * For LM_LIGHTWEIGHT > 44: // Used as a cache the ObjectMonitor* used when locking. Must either The first sentence doesn't read correctly. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680492976
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Okay for HotSpot, jdk.hotspot.agent, jdk.jdwp.agent and jdk.management. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2181900237
Re: RFR: 8336275: Move common Method and Constructor fields to Executable [v2]
On Wed, 17 Jul 2024 02:57:51 GMT, Chen Liang wrote: >> What would be the terminology for a final field that's set by hotspot, >> against the regular java constrcutor rules? > > I have chosen the wording "all final fields are used by the VM" I don't know of any specific terminology - we typically just add a comment saying the field is set and/or read by the VM. - PR Review Comment: https://git.openjdk.org/jdk/pull/20188#discussion_r1680417581
Re: RFR: 8336275: Move common Method and Constructor fields to Executable [v2]
On Wed, 17 Jul 2024 03:03:23 GMT, Chen Liang wrote: >> Move fields common to Method and Field to executable, which simplifies >> implementation. Removed useless transient modifiers as Method and Field were >> never serializable. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Redundant transient; Update the comments to be more accurate Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20188#pullrequestreview-2181897055
Re: RFR: 8336275: Move common Method and Constructor fields to Executable
On Tue, 16 Jul 2024 03:45:36 GMT, Chen Liang wrote:
> Move fields common to Method and Field to executable, which simplifies
> implementation. Removed useless transient modifiers as Method and Field were
> never serializable.
Hotspot changes look good. Core-libs do too but I will leave that for libs folk
to approve
src/java.base/share/classes/java/lang/reflect/Executable.java line 54:
> 52: public abstract sealed class Executable extends AccessibleObject
> 53: implements Member, GenericDeclaration permits Constructor, Method {
> 54: // fields injected by hotspot
If a field is listed here then it is NOT injected by hotspot.
src/java.base/share/classes/java/lang/reflect/Method.java line 73:
> 71: */
> 72: public final class Method extends Executable {
> 73: // fields injected by hotspot
Again not injected
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20188#pullrequestreview-2181384669
PR Review Comment: https://git.openjdk.org/jdk/pull/20188#discussion_r1680112370
PR Review Comment: https://git.openjdk.org/jdk/pull/20188#discussion_r1680113161
Re: RFR: 8336275: Move common Method and Constructor fields to Executable
On Tue, 16 Jul 2024 03:45:36 GMT, Chen Liang wrote: > Move fields common to Method and Field to executable s/Field/Constructor I was a bit confused about executable fields for a moment. :) - PR Comment: https://git.openjdk.org/jdk/pull/20188#issuecomment-2231889229
Re: RFR: 8336254: Virtual thread implementation + test updates
On Thu, 11 Jul 2024 17:30:21 GMT, Alan Bateman wrote: > Bringover some of the changes accumulated in the loom repo to the main line, > most of these changes are test updates and have been baking in the loom repo > for several months. The motive is partly to reduce the large set of changes > that have accumulated in the loom repo, and partly to improve robustness and > test coverage in the main line. The changes don't include any of the larger > changes in the loom repo that are part of future JEPs. > > Implementation: > - Robustness improvements to not throw OOME when unparking a virtual thread. > - Robustness improvements to reduce class loading when a virtual thread parks > or parks when pinned (jdk.internal.misc.VirtualThreads is removed, > jdk.internal.event.VirtualThreadPinnedEvent is eagerly loaded) > - VirtualThread changes to reduce contention on timer queues when doing > timed-park > > Tests: > - New tests for monitor enter/exit/wait/notify (this is a subset of the tests > in the loom repo, we can't move many tests because they depend on on the > changes to the object monitor implementation) > - New test infrastructure to allow tests use a custom scheduler. This updates > many tests to make use of this infrastructure, the "local" ThreadBuidlers is > removed. > - More test scenarios added to ThreadAPI and JVMTI GetThreadStateTest.java > - New test for ThreadMXBean.getLockedMonitor with synchronized native methods > - Reimplement of JVMTI VThreadEvent test to improve reliability > - Rename some tests to get consistent naming > - Diagnostic output in several stress tests to help trace progress in the > event of a timeout > > Testing: tier1-6 src/java.base/share/classes/java/lang/VirtualThread.java line 273: > 271: // current thread is a ForkJoinWorkerThread so the task > will be pushed > 272: // to the local queue. For other schedulers, it avoids > deadlock that > 273: // would arise due to platform and virtual threads > contenting for a s/contenting/contending/ - PR Review Comment: https://git.openjdk.org/jdk/pull/20143#discussion_r1678800192
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 13:46:46 GMT, Doug Simon wrote: >> This PR addresses intermittent failures in jtreg GC stress tests. The >> failures occur under these conditions: >> 1. Using a libgraal build with assertions enabled as the top tier JIT >> compiler. Such a libgraal build will cause a VM exit if an assertion or >> GraalError occurs in a compiler thread (as this catches more errors in >> testing). >> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) >> to a routine that performs a HotSpot heap allocation that fails. >> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as >> described in 1. >> >> An OOME thrown in these specific conditions should not exit the VM as it not >> related to an OOME in the app or test. Instead, the failure should be >> treated as a bailout and the libgraal compiler should continue. >> >> To accomplish this, libgraal needs to be able to distinguish a GraalError >> caused by an OOME. This PR modifies the exception translation code to make >> this possible. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fixed TestTranslatedException Non JVMCI changes look good. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20083#pullrequestreview-2170778148
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Wed, 10 Jul 2024 05:48:23 GMT, David Holmes wrote: >> I'm not so sure this is in fact a bug. If we are throwing with a cause, but >> we can't actually throw and so will do vm_exit, then the exception of >> interest is the cause not the more generic exception that would otherwise >> contain the cause. >> >> Though I have to wonder why there is not an original `_throw` for the >> "cause" exception, that would have triggered the special_exception handling >> anyway? > > Though I see this is inconsistent with `Exceptions::_throw_msg_cause` Okay I think I see how the logic works. If we were going to abort we would never reach `_throw_cause` as the initial `_throw` would have exited. But for the `!thread->can_call_Java()` case the original `_throw` would replace the intended real exception with the dummy `VM_exception()`, which is then "caught" and we try to replace with a more specific exception to be thrown via `throw_cause`, which will again replace whichever exception is requested with the dummy `VM_exception()` - so the end result is we will throw the dummy regardless of whether the cause or wrapping exception is specified. So your fix here makes sense. - PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671680471
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Wed, 10 Jul 2024 05:46:31 GMT, David Holmes wrote:
>> src/hotspot/share/utilities/exceptions.cpp line 208:
>>
>>> 206: Handle h_loader, Handle
>>> h_protection_domain) {
>>> 207: // Check for special boot-strapping/compiler-thread handling
>>> 208: if (special_exception(thread, file, line, h_cause)) return;
>>
>> This fixes a long standing bug where `special_exception` is being queried
>> with the *cause* of the exception being thrown instead of the *name* of the
>> exception being thrown.
>
> I'm not so sure this is in fact a bug. If we are throwing with a cause, but
> we can't actually throw and so will do vm_exit, then the exception of
> interest is the cause not the more generic exception that would otherwise
> contain the cause.
>
> Though I have to wonder why there is not an original `_throw` for the "cause"
> exception, that would have triggered the special_exception handling anyway?
Though I see this is inconsistent with `Exceptions::_throw_msg_cause`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671653968
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Mon, 8 Jul 2024 19:09:47 GMT, Doug Simon wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed TestTranslatedException
>
> src/hotspot/share/utilities/exceptions.cpp line 208:
>
>> 206: Handle h_loader, Handle
>> h_protection_domain) {
>> 207: // Check for special boot-strapping/compiler-thread handling
>> 208: if (special_exception(thread, file, line, h_cause)) return;
>
> This fixes a long standing bug where `special_exception` is being queried
> with the *cause* of the exception being thrown instead of the *name* of the
> exception being thrown.
I'm not so sure this is in fact a bug. If we are throwing with a cause, but we
can't actually throw and so will do vm_exit, then the exception of interest is
the cause not the more generic exception that would otherwise contain the cause.
Though I have to wonder why there is not an original `_throw` for the "cause"
exception, that would have triggered the special_exception handling anyway?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671652583
Re: RFR: 8334287: Man page update for jstatd deprecation
On Fri, 21 Jun 2024 14:05:51 GMT, Kevin Walls wrote: > Man page update for JDK-8327793 which marked jstatd as deprecated for removal > in JDK 24. Sorry this got left "pending" yesterday src/jdk.jstatd/share/man/jstatd.1 line 49: > 47: future release. > 48: .PP > 49: \f[B]Note:\f[R] This command is experimental and unsupported. Can we combine the new warning with the old note e.g. > This command is deprecated and will be removed in a future release. It was > designated as experimental and unsupported. ? - PR Review: https://git.openjdk.org/jdk/pull/19829#pullrequestreview-2134703147 PR Review Comment: https://git.openjdk.org/jdk/pull/19829#discussion_r1650378180
Re: RFR: 8333962: Obsolete OldSize [v2]
On Sat, 15 Jun 2024 08:51:56 GMT, Albert Mingkun Yang wrote: >> src/hotspot/share/runtime/arguments.cpp line 37: >> >>> 35: #include "gc/shared/gcArguments.hpp" >>> 36: #include "gc/shared/gcConfig.hpp" >>> 37: #include "gc/shared/genArguments.hpp" >> >> Why is this needed? > > `Arguments::set_heap_size` accesses `OldSize`, which is declared in this > header. Got it. Thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1642158519
Re: RFR: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size Marked as reviewed by dholmes (Reviewer). src/hotspot/share/runtime/arguments.cpp line 37: > 35: #include "gc/shared/gcArguments.hpp" > 36: #include "gc/shared/gcConfig.hpp" > 37: #include "gc/shared/genArguments.hpp" Why is this needed? - PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2120074670 PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1640761871
Re: RFR: 8333962: Obsolete OldSize
On Tue, 11 Jun 2024 08:17:02 GMT, Albert Mingkun Yang wrote:
> Obsolete OldSize and related code. An internal variable `OldSize` is kept to
> capture the capacity of old-gen size.
Al seems reasonable.
Thanks.
src/hotspot/share/runtime/arguments.cpp line 543:
> 541: { "UseNeon", JDK_Version::undefined(),
> JDK_Version::jdk(23), JDK_Version::jdk(24) },
> 542: { "ScavengeBeforeFullGC", JDK_Version::undefined(),
> JDK_Version::jdk(23), JDK_Version::jdk(24) },
> 543:
Please remove blank line. but you need to sync with master to get recent
updates to this table.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2117402095
PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1639203788
Re: RFR: 8332400: isspace argument should be a valid unsigned char [v2]
On Tue, 11 Jun 2024 18:07:10 GMT, Robert Toyonaga wrote: >> ### Summary >> This change ensures we don't get undefined behavior when >> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). >> `isspace` accepts an `int` argument that "the application shall ensure is >> a character representable as an unsigned char or equal to the value of the >> macro EOF.". >> >> Previously, there was no checking of the values passed to `isspace`. I've >> replaced direct calls with a new wrapper `os::is_space` that performs a >> range check and prevents the possibility of undefined behavior from >> happening. For instances outside of Hotspot, I've added casts to `unsigned >> char`. >> >> **Testing** >> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check >> `os::is_space` is working correctly. >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Replace wrapper with casts. Okay. I hate the fact we have to do this, but okay. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2111852338
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Mon, 10 Jun 2024 13:36:06 GMT, Robert Toyonaga wrote: > But what about when an int is passed to isspace? The current casts to int are incorrect as a negative value would be sign-extended and then fail the range check. I think we have to cast to unsigned char in all cases in the caller, which renders the wrapper and range-check obsolete AFAICS. - PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2159586765
Integrated: 8330205: Initial troff manpage generation for JDK 24
On Mon, 10 Jun 2024 02:31:00 GMT, David Holmes wrote: > Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - ~~JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC~~ > (covered by merge) > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks This pull request has now been integrated. Changeset: 3a01b47a Author:David Holmes URL: https://git.openjdk.org/jdk/commit/3a01b47ac97714608356ce3faf797c37dc63e9af Stats: 43 lines in 28 files changed: 2 ins; 3 del; 38 mod 8330205: Initial troff manpage generation for JDK 24 Reviewed-by: alanb, iris - PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]
On Mon, 10 Jun 2024 17:27:18 GMT, Iris Clark wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Regenerated after merge > > Marked as reviewed by iris (Reviewer). Thanks for the review @irisclark . I've merged in the latest updates that also brought in part of the missing changes. If anything goes wrong I have another PR in preparation that also updates java.1 - PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2159577044
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]
> Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: Regenerated after merge - Changes: - all: https://git.openjdk.org/jdk/pull/19617/files - new: https://git.openjdk.org/jdk/pull/19617/files/8472c47d..e7563087 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19617&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19617&range=01-02 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v2]
> Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks David Holmes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge - 8330205: Initial troff manpage generation for JDK 24 - Changes: https://git.openjdk.org/jdk/pull/19617/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19617&range=01 Stats: 53 lines in 28 files changed: 12 ins; 3 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Fri, 7 Jun 2024 06:07:17 GMT, Thomas Stuefe wrote: > "To use these functions safely with plain chars (or signed chars), the > argument should first be converted to unsigned char" @tstuefe wow! Okay. That is a surprise to me. A cast to unsigned char doesn't actually do anything. Every char is "representable" as an unsigned char because it holds a bit pattern between 0x00 and 0xff i.e. the function is well defined if the incoming int is either EOF (int -1) or else in the range 0x00 to 0xff. But I did a bit of searching and it seems it comes down to potential arithmetic operations on the "char" the might behave differently depending on the signed-ness. :( - PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2157677944
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Mon, 10 Jun 2024 07:15:51 GMT, Alan Bateman wrote: >> Sets the version to 24/24-ea and the copyright year to 2025. >> >> Content changes related to the start of release (e.g. for removed options in >> java.1) are handled separately. >> >> This initial generation also picks up the unpublished changes from: >> >> - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC >> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints >> - JDK-8324342: Doclet should default `@since` for a nested class to that of >> its enclosing class >> >> Thanks > > Marked as reviewed by alanb (Reviewer). Thanks for the review @AlanBateman ! - PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2157606095
RFR: 8330205: Initial troff manpage generation for JDK 24
Sets the version to 24/24-ea and the copyright year to 2025. Content changes related to the start of release (e.g. for removed options in java.1) are handled separately. This initial generation also picks up the unpublished changes from: - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC - JDK-8284500: Typo in Supported Named Extensions - BasicContraints - JDK-8324342: Doclet should default @since for a nested class to that of its enclosing class Thanks - Commit messages: - 8330205: Initial troff manpage generation for JDK 24 Changes: https://git.openjdk.org/jdk/pull/19617/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19617&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330205 Stats: 66 lines in 28 files changed: 12 ins; 13 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga wrote:
> ### Summary
> This change ensures we don't get undefined behavior when
> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html).
> `isspace` accepts an `int` argument that "the application shall ensure is a
> character representable as an unsigned char or equal to the value of the
> macro EOF.".
>
> Previously, there was no checking of the values passed to `isspace`. I've
> replaced direct calls with a new wrapper `os::is_space` that performs a range
> check and prevents the possibility of undefined behavior from happening. For
> instances outside of Hotspot, I've added casts to `unsigned char`.
>
> **Testing**
> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check
> `os::is_space` is working correctly.
> - tier1
Sorry I think this is well-intentioned but unnecessary in nearly all cases. If
you pass a char there is no potential problem. Only passing an actual int could
be a problem.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 664:
> 662: char after_key = line[key_len];
> 663: if (strncmp(line, key, key_len) == 0
> 664: && os::is_space(after_key) != 0
I think this is excessive. The value is a char so cannot be a problem. The only
calls to isspace that need checking are those that actually pass an int, and
even then there could be adequate safeguards in place.
src/hotspot/os/linux/os_linux.cpp line 1356:
> 1354: if (s) {
> 1355: // Skip blank chars
> 1356: do { s++; } while (s && os::is_space(*s));
Again not needed.
-
PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2103234054
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630265925
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630266490
Re: RFR: 8331977: Crash: SIGSEGV in dlerror()
On Mon, 3 Jun 2024 14:38:03 GMT, Alexey Semenyuk wrote: >> I am yet to see anything that actually explains the cause of the `dlerror` >> crash here ??? > > @dholmes-ora there is no fix for the cause of the `dlerror` crash in this PR. > The PR fixes jpackage tests to rerun a launcher if it crashes. This > workaround for jpackage tests was first introduced in > [JDK-8269403](https://bugs.openjdk.org/browse/JDK-8269403) fix but > MainClassTest was left unfixed back then. This PR complements > [JDK-8269403](https://bugs.openjdk.org/browse/JDK-8269403). @alexeysemenyukoracle But now there is no bug to investigate the dlerrror crash! If you wanted to make the test more robust a new JBS issue should have been filed for that. - PR Comment: https://git.openjdk.org/jdk/pull/19502#issuecomment-2146669916
