8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal #15602

dougxc · 2023-09-06T20:49:05Z

Instead of eagerly initializing JVMCI for -Xcomp, this defers JVMCI compilations until JVMCI is initializable. In the case of libgraal, no compilations are deferred since libgraal is always initializable. For jargraal, it still has to wait until the VM has reached a certain initialization phase so as to avoid deadlocks bewteen the main thread and a compiler thread.

This allows tests such as vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java to succeed on libgraal, even when there is insufficient virtual memory to mmap libgraal. Failing to initialize libgraal in that scenario now results in a bailout instead of exiting the VM.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal (Bug - P4)

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15602/head:pull/15602
$ git checkout pull/15602

Update a local copy of the PR:
$ git checkout pull/15602
$ git pull https://git.openjdk.org/jdk.git pull/15602/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15602

View PR using the GUI difftool:
$ git pr show -t 15602

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15602.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-06T20:50:35Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-06T20:53:08Z

@dougxc The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-09-06T20:57:03Z

Webrevs

01: Full - Incremental (b32a7a3d)
00: Full (038d1521)

tkrodriguez · 2023-09-06T23:15:30Z

This seems a bit unsavory to me. Why can't we set CompileBroker::_initialized to true in the same place that C2 does, but move the comp->is_jvmci() && !JVMCI::can_initialize_JVMCI() stuff to just after the if (!_initialized || comp_level == CompLevel_none) in CompileBroker::compile_method? Wouldn't that give us early enough initialization for Xcomp while still complaining if we actually need a compilation but can't create the isolate?

…tions until JVMCI is initializable

dougxc · 2023-09-07T10:55:36Z

src/hotspot/share/compiler/compileBroker.hpp

@@ -301,7 +301,7 @@ class CompileBroker: AllStatic {
                                 int hot_count,
                                 CompileTask::CompileReason compile_reason,
                                 TRAPS);
-
+private:


Making this helper method avoids anyone outside CompileBroker accidentally calling it.

dougxc · 2023-09-07T11:01:18Z

Thanks for the suggestion Tom. I've pushed a change that implements it.

tkrodriguez · 2023-09-07T18:45:47Z

src/hotspot/share/runtime/threads.cpp

@@ -754,9 +744,8 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
 #endif

 #if INCLUDE_JVMCI
-  if (force_JVMCI_intialization) {
+  if (force_JVMCI_initialization) {
    JVMCI::initialize_compiler(CHECK_JNI_ERR);


Does anything bad happen if force_JVMCI_initialization is true and we end up initializing the compiler from the compile broker thread instead? I guess that would be adding Xcomp to the the other flags that force initialization.

Both JVMCIPrintProperties and JVMCILibDumpJNIConfig explicitly do the equivalent of System.exit() once they've performed their task so I don't think it matters which thread triggers the task. I don't think it will cause any problems for EagerJVMCI either. In fact, I don't think we really need EagerJVMCI any more as it's only purpose was to make JVMCIPrintProperties guaranteed to do something.

tkrodriguez · 2023-09-07T18:54:29Z

src/hotspot/share/jvmci/jvmci.cpp

@@ -56,6 +56,11 @@ const char* JVMCI::_fatal_log_filename = nullptr;
 void jvmci_vmStructs_init() NOT_DEBUG_RETURN;

 bool JVMCI::can_initialize_JVMCI() {


For the non libgraal case is this test really complete? I guess I'm not convinced that the tests in this method are sufficient the safe init point. Maybe it would be simpler to have a JVMCI::_can_be_initialized flag which is set at the correct point during the init steps in the same way that CompileBroker::_initialized is set. Having the order explicitly spelled out there seems better than trying to infer it from other state.

We've been running this code for a long time (Graal unit tests on jargraal, Native Image building of libgraal etc) so I don't see any reason to double-guess its correctness now. I suggest we leave it as is and address problems when/if they show up.

tkrodriguez · 2023-09-07T18:54:56Z

Yes I like that much better.

openjdk · 2023-09-07T19:33:06Z

@dougxc This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal

Reviewed-by: never

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

0c865a7: 8315637: JDK-8314249 broke libgraal
683672c: 8292692: Move MethodCounters inline functions out of method.hpp
9bf3dee: 8314831: NMT tests ignore vm flags
b74805d: 8315863: [GHA] Update checkout action to use v4
1cae0f5: 8315220: Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native
8f7e29b: 8313422: test/langtools/tools/javac 144 test classes uses com.sun.tools.classfile library
8557205: 8312569: RISC-V: Missing intrinsics for Math.ceil, floor, rint
2fd870a: 8315444: Convert test/jdk/tools to Classfile API
81f8c57: 8314632: Intra-case dominance check fails in the presence of a guard
b408a82: 8314260: Unable to load system libraries on Windows when using a SecurityManager
... and 9 more: https://git.openjdk.org/jdk/compare/86a18f5e2e0825dddb77656b2f43f64684f1464c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dougxc · 2023-09-08T11:54:26Z

Thanks for the review Tom.

/integrate

dougxc · 2023-09-08T11:54:38Z

Thanks for the review Tom.

/integrate

openjdk · 2023-09-08T11:55:56Z

Going to push as commit ebc718f.
Since your change was applied there have been 33 commits pushed to the master branch:

4a6bd81: 8315854: G1: Remove obsolete comment in G1ReclaimEmptyRegionsTask
c664f1c: 8307352: AARCH64: Improve itable_stub
8ddf9ea: 8315686: G1: Disallow evacuation of marking regions in a Prepare Mixed gc
7ef059a: 8315605: G1: Add number of nmethods in code roots scanning statistics
825e0ed: 8315774: Enable parallelism in vmTestbase/gc/g1/unloading tests
dac1727: 8308869: C2: use profile data in subtype checks when profile has more than one class
3c258ac: 8315702: jcmd Thread.dump_to_file slow with millions of virtual threads
3a00ec8: 8312075: FileChooser.win32.newFolder is not updated when changing Locale
806ef08: 8315594: Open source few headless Swing misc tests
4b43c25: 8310929: Optimization for Integer.toString
... and 23 more: https://git.openjdk.org/jdk/compare/86a18f5e2e0825dddb77656b2f43f64684f1464c...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-08T11:56:03Z

@dougxc Pushed as commit ebc718f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk · 2023-09-08T11:56:09Z

@dougxc The command integrate can only be used in open pull requests.

dholmes-ora · 2023-09-11T08:38:34Z

@dougxc - please remember that all hotspot changes require at least 2 reviews.

dougxc added 2 commits September 6, 2023 22:43

fix spelling

e7067ee

allow eager JVMCI initialization to fail if only done for -Xcomp

038d152

dougxc marked this pull request as ready for review September 6, 2023 20:50

openjdk bot added the rfr Pull request is ready for review label Sep 6, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 6, 2023

instead of eagerly initializing JVMCI for -Xcomp, defer JVMCI compila…

b32a7a3

…tions until JVMCI is initializable

dougxc commented Sep 7, 2023

View reviewed changes

tkrodriguez reviewed Sep 7, 2023

View reviewed changes

tkrodriguez approved these changes Sep 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 7, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 8, 2023

openjdk bot closed this Sep 8, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 8, 2023

dougxc deleted the JDK-8315818 branch August 20, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal #15602

8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal #15602

dougxc commented Sep 6, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 6, 2023

openjdk bot commented Sep 6, 2023

mlbridge bot commented Sep 6, 2023 •

edited

Loading

tkrodriguez commented Sep 6, 2023

dougxc Sep 7, 2023

dougxc commented Sep 7, 2023

tkrodriguez Sep 7, 2023

dougxc Sep 7, 2023

tkrodriguez Sep 7, 2023

tkrodriguez Sep 7, 2023

dougxc Sep 7, 2023

tkrodriguez Sep 7, 2023

tkrodriguez commented Sep 7, 2023

openjdk bot commented Sep 7, 2023

dougxc commented Sep 8, 2023

dougxc commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

dholmes-ora commented Sep 11, 2023

		@@ -56,6 +56,11 @@ const char* JVMCI::_fatal_log_filename = nullptr;
		void jvmci_vmStructs_init() NOT_DEBUG_RETURN;

		bool JVMCI::can_initialize_JVMCI() {

8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal #15602

8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal #15602

Conversation

dougxc commented Sep 6, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 6, 2023

openjdk bot commented Sep 6, 2023

mlbridge bot commented Sep 6, 2023 • edited Loading

Webrevs

tkrodriguez commented Sep 6, 2023

dougxc Sep 7, 2023

Choose a reason for hiding this comment

dougxc commented Sep 7, 2023

tkrodriguez Sep 7, 2023

Choose a reason for hiding this comment

dougxc Sep 7, 2023

Choose a reason for hiding this comment

tkrodriguez Sep 7, 2023

Choose a reason for hiding this comment

tkrodriguez Sep 7, 2023

Choose a reason for hiding this comment

dougxc Sep 7, 2023

Choose a reason for hiding this comment

tkrodriguez Sep 7, 2023

Choose a reason for hiding this comment

tkrodriguez commented Sep 7, 2023

openjdk bot commented Sep 7, 2023

dougxc commented Sep 8, 2023

dougxc commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

dholmes-ora commented Sep 11, 2023

dougxc commented Sep 6, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 6, 2023 •

edited

Loading