8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread #21285

tzezula · 2024-10-01T10:57:58Z

JDK-8318694 limited the ability for JVMCI CompilerThreads to make Java upcalls. This is to mitigate against deadlock when an upcall does class loading. Class loading can easily create deadlock situations in -Xcomp or -Xbatch mode.

However, for Truffle, upcalls are unavoidable if Truffle partial evaluation occurs as part of JIT compilation inlining. This occurs when the Graal inliner sees a constant Truffle AST node which allows a Truffle-specific inlining extension to perform Truffle partial evaluation (PE) on the constant. Such PE involves upcalls to the Truffle runtime (running in Java).

This PR provides the escape hatch such that Truffle specific logic can put a compiler thread into "allow Java upcall" mode during the scope of the Truffle logic.

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-8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to 81b59f7e
Doug Simon (@dougxc - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21285

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-01T10:58:53Z

👋 Welcome back tzezula! 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 · 2024-10-01T10:58:58Z

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

8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread

Reviewed-by: dnsimon, kvn

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 114 new commits pushed to the master branch:

e7c5bf4: 8341722: Fix some warnings as errors when building on Linux with toolchain clang
36fca5d: 8341882: [BACKOUT] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
780de00: 8051591: Test javax/swing/JTabbedPane/8007563/Test8007563.java fails
313f4a9: 8340809: Open source few more AWT PopupMenu tests
9d621d3: 8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
475f8f9: 8341859: Optimize ClassFile Benchmark Write
c850ecb: 8341755: Optimize argNames in InnerClassLambdaMetafactory
172f744: 8340985: Open source some Desktop related tests
e7045e9: 8341684: Typo in External Specifications link of java.util.Currency
49c7148: 8341366: Suspicious check in Locale.getDisplayName(Locale inLocale)
... and 104 more: https://git.openjdk.org/jdk/compare/0b467e902d591ae9feeec1669918d1588987cd1c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @dougxc) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-10-01T10:59:47Z

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

graal
hotspot-compiler

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.

mlbridge · 2024-10-01T11:02:55Z

Webrevs

vnkozlov

`/compiler' part of changes is fine.

openjdk · 2024-10-01T16:04:23Z

⚠️ @tzezula the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8340733
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

…Java

openjdk · 2024-10-04T15:01:46Z

@tzezula 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.

tzezula · 2024-10-04T15:16:07Z

I have simplified the _can_call_java transitions. The only feature in the libjvmci compiler that requires Java calls is Truffle compiler, which utilizes JNI to invoke the Truffle runtime methods. Given that we now have CompilerThreadCanCallJavaScope, which Truffle can use to explicitly enable Java calls, we can safely disable Java calls by default for the libjvmci compiler.

For the Java JVMCI compiler, we still need to permit Java calls to accommodate upcalls to the Graal compiler and for InterpreterRuntime while running the Java JVMCI compiler.

The simplification eliminates the need for TriBool for _can_call_java; it can remain a bool.

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

dougxc

Looks good to me.

dougxc · 2024-10-10T07:29:46Z

src/hotspot/share/compiler/compilerThread.cpp

@@ -55,8 +55,12 @@ CompilerThread::~CompilerThread() {
 }

 void CompilerThread::set_compiler(AbstractCompiler* c) {
-  // Only jvmci compiler threads can call Java
-  _can_call_java = c != nullptr && c->is_jvmci();
+  /*


The comment could be a little shorter:

/* * Compiler threads need to make Java upcalls to the jargraal compiler. * Java upcalls are also needed by the InterpreterRuntime when using jargraal. */

openjdk bot added the rfr Pull request is ready for review label Oct 1, 2024

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Oct 1, 2024

vnkozlov approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 1, 2024

dougxc and others added 3 commits October 3, 2024 11:20

added CompilerThreadCanCallJavaScope

81eb1ed

rename changeCompilerThreadCanCallJava to updateCompilerThreadCanCall…

fc960a8

…Java

Libgraal does not allow _can_call_java.

dfd7249

tzezula force-pushed the JDK-8340733 branch from 81b59f7 to dfd7249 Compare October 4, 2024 15:00

openjdk bot removed the ready Pull request is ready to be integrated label Oct 4, 2024

UseJVMCINativeLibrary check must be guarded by JVMCI_ONLY.

f687c82

dougxc reviewed Oct 4, 2024

View reviewed changes

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp Outdated Show resolved Hide resolved

Tomas Zezula added 2 commits October 4, 2024 18:03

Set __block_can_call_java to true for non compiler threads.

346f898

Simplified C2V_BLOCK.

e07d444

dougxc approved these changes Oct 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread #21285

8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread #21285

tzezula commented Oct 1, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024 •

edited

Loading

openjdk bot commented Oct 1, 2024

mlbridge bot commented Oct 1, 2024 •

edited

Loading

vnkozlov left a comment

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 4, 2024

tzezula commented Oct 4, 2024

dougxc left a comment

dougxc Oct 10, 2024

8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread #21285

Are you sure you want to change the base?

8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread #21285

Conversation

tzezula commented Oct 1, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024 • edited Loading

openjdk bot commented Oct 1, 2024

mlbridge bot commented Oct 1, 2024 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 4, 2024

tzezula commented Oct 4, 2024

dougxc left a comment

Choose a reason for hiding this comment

dougxc Oct 10, 2024

Choose a reason for hiding this comment

tzezula commented Oct 1, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 1, 2024 •

edited

Loading

mlbridge bot commented Oct 1, 2024 •

edited

Loading