8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads #16383

dougxc · 2023-10-26T17:39:46Z

This PR reduces the context in which a libjvmci CompilerThread can make a Java call. By default, a CompileThread for a JVMCI compiler can make Java calls (as jarjvmci only works that way). When libjvmci calls into the VM via a CompilerToVM native method, it enters a scope where Java calls are disabled until either the call returns or a nested scope is entered that re-enables Java calls. The latter is required for the Truffle use case described in JDK-8318694 as well as for a few other VM entry points where libgraal currently still requires the ability to make Java calls (e.g. to load the Java classes used for boxing primitives). We may be able to eventually eliminate all need for libgraal to make Java calls but this PR is a first step in the right direction.

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-8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads (Bug - P2)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to b7181d70
Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to b7181d70

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16383

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

Using diff file

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

Webrev

Link to Webrev Comment

dougxc · 2023-10-26T17:40:53Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

 #define JNI_THROW(caller, name, msg) do {                                         \
    jint __throw_res = env->ThrowNew(JNIJVMCI::name::clazz(), msg);               \
    if (__throw_res != JNI_OK) {                                                  \
-      tty->print_cr("Throwing " #name " in " caller " returned %d", __throw_res); \


The VM should prefer event logging over printing to the console.

dougxc · 2023-10-26T17:41:50Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

    if (strstr(val, "<trace>") != nullptr) {
      tty->print_cr("CompilerToVM.lookupType: %s", str);
-    } else if (strstr(val, str) != nullptr) {
+    } else if (strstr(str, val) != nullptr) {


This fixes an existing bug: the test is meant to be whether val is a substring of str, not the other way around.

bridgekeeper · 2023-10-26T17:47:31Z

👋 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-10-26T17:48:42Z

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

graal
hotspot

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

dougxc · 2023-10-26T21:42:52Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

 C2V_END

 C2V_VMENTRY_NULL(jobject, lookupType, (JNIEnv* env, jobject, jstring jname, ARGUMENT_PAIR(accessing_klass), jint accessing_klass_loader, jboolean resolve))
+  CompilerThreadCanCallJava canCallJava(thread, resolve); // Resolution requires Java calls


This is currently required by libgraal - it may be fixable in future.

… for libjvmci

mlbridge · 2023-10-27T11:20:54Z

Webrevs

dougxc · 2023-10-27T18:16:25Z

src/hotspot/share/classfile/javaClasses.cpp

      print_stack_element_to_stream(st, bte._mirror, bte._method_id, bte._version, bte._bci, bte._name);
    }
-    {
+    if (THREAD->can_call_java()) {


This allows java_lang_Throwable::print_stack_trace to be used in contexts where Java calls cannot be made.

…ead cannot call Java

openjdk · 2023-10-29T20:35:10Z

@dougxc Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

openjdk · 2023-10-29T20:36:40Z

@dougxc Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

dholmes-ora

Can't comment on all the details of the changes, but I don't see anything untoward in general.

Thanks.

openjdk · 2023-10-30T08:00:20Z

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

8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads

Reviewed-by: dholmes, 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 86 new commits pushed to the master branch:

ab19348: 8318647: Serial: Refactor BlockOffsetTable
b4f5379: 8304939: os::win32::exit_process_or_thread should be marked noreturn
0461d9a: 8318016: Per-compilation memory ceiling
2a76ad9: 8318683: compiler/c2/irTests/TestPhiDuplicatedConversion.java "Failed IR Rules (2) of Methods (2)"
b3fec6b: 8306980: Generated docs should contain correct Legal Documents
1139482: 8316132: CDSProtectionDomain::get_shared_protection_domain should check for exception
2182c93: 8313643: Update HarfBuzz to 8.2.2
613a3cc: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library
613d32c: 8169475: WheelModifier.java fails by timeout
f1e8787: 8317609: Classfile API fails to verify /jdk.jcmd/sun/tools/jstat/Alignment.class
... and 76 more: https://git.openjdk.org/jdk/compare/d2d1592dd94e897fae6fc4098e43b4fffb6d6750...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.

tkrodriguez

This looks better than I'd imagined.

tkrodriguez · 2023-10-31T14:07:51Z

Should we convert any assert only can_call_java checks into guarantees?

dougxc · 2023-10-31T14:25:09Z

Should we convert any assert only can_call_java checks into guarantees?

Makes sense to me. There are only 3 such assertions and none of them are on performance critical paths:

jdk/src/hotspot/share/classfile/systemDictionary.cpp

Line 614 in 3e39d7b

assert(THREAD->can_call_java(),
jdk/src/hotspot/share/classfile/systemDictionary.cpp

Line 2059 in 3e39d7b

assert(THREAD->can_call_java() ,"");
jdk/src/hotspot/share/prims/upcallLinker.cpp

Line 82 in 3e39d7b

assert(thread->can_call_java(), "must be able to call Java");

What do you think @dholmes-ora @vnkozlov ?

turbanoff · 2023-10-31T19:36:42Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java

        JavaType fieldHolder = lookupType(holderIndex, opcode);

-        if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl) {
+        if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl)  {


Suggested change

if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl) {

if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl) {

dholmes-ora · 2023-11-01T02:08:56Z

What do you think @dholmes-ora @vnkozlov ?

Normally we use asserts for things that are absolutely expected to be uncovered during testing - things that are purely internal VM concerns and for which a failure is a VM coding error. So I don't think it is necessary to change these to guarantees provided we have sufficient test coverage. The guarantee may not hurt but the same could be said for many other assertions.

dougxc · 2023-11-01T08:00:46Z

With GraalVM, we're doing a lot more testing with product builds than fastdebug builds as the majority of checks are done at the Java level and fastdebug just slows everything down. In that testing context, guarantees are much more useful. Given the importance of the "can_call_java" invariant, would you agree that converting these 3 specific assertions to guarantees is justified?

dholmes-ora · 2023-11-01T12:01:54Z

The majority of hotspot testing is done on fastdebug and performance is not considered an issue there. But I don't have a hard objection to these three asserts becoming guarantees.

dougxc · 2023-11-01T16:24:42Z

Thanks for all the reviews and useful input.

/integrate

openjdk · 2023-11-01T16:27:06Z

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

c86592d: 8319046: Execute tests in source/class-file order in JavadocTester
3660a90: 8319139: Improve diagnosability of JavadocTester output
7f47c51: 8316025: Use testUI() method of PassFailJFrame.Builder in FileChooserSymLinkTest.java
36de19d: 8317048: VerifyError with unnamed pattern variable and more than one components
ab19348: 8318647: Serial: Refactor BlockOffsetTable
b4f5379: 8304939: os::win32::exit_process_or_thread should be marked noreturn
0461d9a: 8318016: Per-compilation memory ceiling
2a76ad9: 8318683: compiler/c2/irTests/TestPhiDuplicatedConversion.java "Failed IR Rules (2) of Methods (2)"
b3fec6b: 8306980: Generated docs should contain correct Legal Documents
1139482: 8316132: CDSProtectionDomain::get_shared_protection_domain should check for exception
... and 80 more: https://git.openjdk.org/jdk/compare/d2d1592dd94e897fae6fc4098e43b4fffb6d6750...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-01T16:27:12Z

@dougxc Pushed as commit d354141.

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

dcubed-ojdk · 2023-11-07T20:18:49Z

With GraalVM, we're doing a lot more testing with product builds than fastdebug
builds as the majority of checks are done at the Java level and fastdebug just
slows everything down. In that testing context, guarantees are much more useful.
Given the importance of the "can_call_java" invariant, would you agree that
converting these 3 specific assertions to guarantees is justified?

Hmmm... I hope this doesn't mean that the GraalVM project has changed the
Tier[1-3] task definitions to focus on 'release' bits testing instead of 'fastdebug'
bits testing.

dougxc · 2023-11-07T21:13:34Z

Hmmm... I hope this doesn't mean that the GraalVM project has changed the
Tier[1-3] task definitions to focus on 'release' bits testing instead of 'fastdebug'
bits testing.

No, we did not change this in the tier testing. I was referring to the Graal CI.

dcubed-ojdk · 2023-11-07T21:43:16Z

No, we did not change this in the tier testing. I was referring to the Graal CI.

Thanks for the info. Does this mean that the Graal CI is running its own Tier[1-8]
definitions and not the same Tier[1-8] as the main JDK repo?

dcubed-ojdk · 2023-11-07T21:47:47Z

I meant does that mean that Graal CI is running its own Graal-Tier[1-N] and it not
running the same Tier[1-8] definitions as the main JDK repo at all?

dougxc · 2023-11-07T21:51:43Z

The Graal CI system is completely disjoint from mach5-based JDK testing.

dougxc commented Oct 26, 2023

View reviewed changes

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Oct 26, 2023

dougxc force-pushed the JDK-8318694_v2 branch 3 times, most recently from 4df5494 to 3e70e6e Compare October 26, 2023 21:40

dougxc commented Oct 26, 2023

View reviewed changes

dougxc force-pushed the JDK-8318694_v2 branch 2 times, most recently from 5457928 to 0c0b192 Compare October 27, 2023 08:19

[skip ci] update CompilerThread::can_call_java to be false more often…

9fc5ad9

… for libjvmci

dougxc force-pushed the JDK-8318694_v2 branch from 0c0b192 to 9fc5ad9 Compare October 27, 2023 08:26

dougxc marked this pull request as ready for review October 27, 2023 11:15

openjdk bot added the rfr Pull request is ready for review label Oct 27, 2023

CompilerThreadCanCallJava scope must enclose the JVMCIEnv scope

7f2285e

dougxc commented Oct 27, 2023

View reviewed changes

dougxc added 3 commits October 27, 2023 20:26

improved error message

af42062

removed unused method: JVMCIRuntime::get_field_by_index

0370178

avoid class loading in HotSpotConstantPool.lookupField if current thr…

a646620

…ead cannot call Java

dougxc force-pushed the JDK-8318694_v2 branch from f47527a to cc78b7b Compare October 29, 2023 20:34

allow JavaCalls in HotSpotConstantPool.callSystemExit

b7181d7

dougxc force-pushed the JDK-8318694_v2 branch from cc78b7b to b7181d7 Compare October 29, 2023 20:35

dholmes-ora approved these changes Oct 30, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2023

tkrodriguez approved these changes Oct 30, 2023

View reviewed changes

turbanoff reviewed Oct 31, 2023

View reviewed changes

convert assertions about can_call_java to guarantees

832a591

openjdk bot added the integrated Pull request has been integrated label Nov 1, 2023

openjdk bot closed this Nov 1, 2023

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

dougxc deleted the JDK-8318694_v2 branch August 20, 2024 06:22

	if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl) {
	if (fieldHolder instanceof HotSpotResolvedObjectTypeImpl) {

8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads #16383

8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads #16383

Uh oh!

Conversation

dougxc commented Oct 26, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

dougxc Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Oct 26, 2023

Uh oh!

openjdk bot commented Oct 26, 2023

Uh oh!

dougxc Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dougxc Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 29, 2023

Uh oh!

openjdk bot commented Oct 29, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Oct 31, 2023

Uh oh!

dougxc commented Oct 31, 2023

Uh oh!

turbanoff Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Nov 1, 2023

Uh oh!

dougxc commented Nov 1, 2023

Uh oh!

dholmes-ora commented Nov 1, 2023

Uh oh!

dougxc commented Nov 1, 2023

Uh oh!

openjdk bot commented Nov 1, 2023

Uh oh!

openjdk bot commented Nov 1, 2023

Uh oh!

dcubed-ojdk commented Nov 7, 2023

Uh oh!

dougxc commented Nov 7, 2023

Uh oh!

dcubed-ojdk commented Nov 7, 2023

Uh oh!

dcubed-ojdk commented Nov 7, 2023

Uh oh!

dougxc commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

dougxc commented Oct 26, 2023 •

edited by openjdk bot

Loading

dougxc Oct 26, 2023 •

edited

Loading

mlbridge bot commented Oct 27, 2023 •

edited

Loading

openjdk bot commented Oct 30, 2023 •

edited

Loading