Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8289094: [JVMCI] reduce JNI overhead and other VM rounds trips in JVMCI #9305

Closed
wants to merge 1 commit into from

Conversation

dougxc
Copy link
Member

@dougxc dougxc commented Jun 28, 2022

The interface between HotSpot and libjvmci is implemented via JNI. Every time HotSpot C++ code needs to access libjvmci objects, it requires 4 VM transitions:

  1. HotSpot: VM to native (_thread_in_vm -> _thread_in_native)
  2. SVM: Native to VM (enter SVM)
  3. SVM: VM to native (exit SVM)
  4. HotSpot: Native to VM (_thread_in_native -> _thread_in_vm)

When processing a HotSpotCompiledCode object during code installation (i.e. CodeInstaller::install), the overhead of all these transitions is significant.
This PR changes the way code installation is done by serializing a HotSpotCompiledCode object to a byte array (in malloc'ed memory). The C++ code then deserializes this format to install the final code in the code cache. The bulk of this change is in jdk.vm.ci.hotspot.HotSpotCompiledCodeStream and jvmciCodeInstaller.cpp.

There are other smaller changes to reduce JNI overhead included in this PR:

  • Pass raw VM values alongside JVMCI wrapper objects to the VM to avoid JNI upcall to unbox in C++. For example, the getBytecode native method passed a HotSpotResolvedJavaMethodImpl to the VM and the VM then subsequently needs a JNI upcall to extract the Method* value from the HotSpotResolvedJavaMethodImpl.methodHandle field. With PR, getBytecode now passes an argument pair (i.e. getBytecode(HotSpotResolvedJavaMethodImpl method, long methodPointer);) that removes the unboxing upcall. This is done for all JVMCI objects that wrap a C++ object pointer. The box object is still passed as it protects the validity of the C++ pointer value.
  • Cache HotSpotConstantPool holder in HotSpotConstantPool.getHolder().
  • Remove VM round trip for converting a Klass* value to a ResolvedJavaType.
  • Avoid eager String creation in JVMCIEnv::get_jvmci_type.

This change is primarily in JVMCI-only code. It has been extensively tested in GraalVM on JDK 17. On the octane JavaScript benchmark, this PR reduces total time spent by GraalVM EE in JVMCI code installation from 648 s to 382 s.


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-8289094: [JVMCI] reduce JNI overhead and other VM rounds trips in JVMCI

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9305

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

Using diff file

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

* serialize HotSpotCompiledCode for use in CompilerToVM.installCode
* pass raw VM values instead of JVMCI wrapper objects to the VM to avoid unboxing in C++
* restrict unboxing of VM raw values to CompilerToVM and pass box along with raw value into VM to guarantee box stays alive as long as the raw value is in use
* cache HotSpotConstantPool holder
* remove VM round trip for converting Klass* value to a ResolvedJavaType
* avoid eager String creation in JVMCIEnv::get_jvmci_type
@@ -38,6 +38,7 @@ class elapsedTimer {
public:
elapsedTimer() { _active = false; reset(); }
void add(elapsedTimer t);
void add_nanoseconds(jlong ns);
Copy link
Member Author

@dougxc dougxc Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required so that code installation work done in libjvmci can be properly attributed to a VM based timer.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 28, 2022

👋 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
Copy link

openjdk bot commented Jun 28, 2022

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Jun 28, 2022
@@ -100,7 +100,7 @@ class outputStream : public ResourceObj {
void print_raw(const char* str, size_t len) { write(str, len); }
void print_raw_cr(const char* str) { write(str, strlen(str)); cr(); }
void print_raw_cr(const char* str, size_t len){ write(str, len); cr(); }
void print_data(void* data, size_t len, bool with_ascii);
void print_data(void* data, size_t len, bool with_ascii, bool rel_addr=true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows for hex dumps to show absolute addresses which is important for comparing a buffer written in Java and read in C++.

@dougxc dougxc changed the title [JVMCI] reduce JNI overhead and other VM rounds trips in JVMCI 8289094: [JVMCI] reduce JNI overhead and other VM rounds trips in JVMCI Jun 28, 2022
@dougxc dougxc marked this pull request as ready for review June 28, 2022 13:20
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 28, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 28, 2022

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine. Please add link to mach5 testing results to RFE.

@openjdk
Copy link

openjdk bot commented Jun 28, 2022

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

8289094: [JVMCI] reduce JNI overhead and other VM rounds trips in JVMCI

Reviewed-by: kvn, dlong

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

  • af00880: 8284640: CollectorImpl class could be a record class
  • c67149b: 8288961: jpackage: test MSI installation fix
  • 1f36ed1: 8288013: jpackage: test utility Windows registry enhancement
  • 88fe19c: 8289071: Compute allocation sizes of stubs and nmethods outside of lock protection
  • d4eeeb8: 8271252: os::reserve_memory should not use mtOther as default NMT flag
  • 549c6c2: 8287818: Shenandoah: adapt nmethod arming from Loom
  • aa43824: 8289138: G1: Remove redundant is-marking-active checks in C1 barrier
  • b4ab5fe: 8288396: Always create reproducible builds
  • 3336971: 8289258: ProblemList some failing custom loader tests with ZGC
  • 784fa0a: 8282036: Change java/util/zip/ZipFile/DeleteTempJar.java to stop HttpServer cleanly in case of exceptions
  • ... and 39 more: https://git.openjdk.org/jdk/compare/c8cc94a38423c0cef597986fb51938a26dc20b51...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 28, 2022
Copy link
Member

@dean-long dean-long left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a quick look and didn't find any problems. It's clever how the tag values are synchronized.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Doug,

I just looked at the general runtime changes and they seem okay.

Thanks.

@dougxc
Copy link
Member Author

dougxc commented Jun 29, 2022

Thanks @vnkozlov @dean-long and @dholmes-ora for the review. I'm doing a little more GraalVM testing before merging.

@dougxc
Copy link
Member Author

dougxc commented Jun 29, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jun 29, 2022

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

  • 0709a6a: 8284942: Proxy building can just iterate superinterfaces once
  • 2961b7e: 8285364: Remove REF_ enum for java.lang.ref.Reference
  • 167ce4d: 8289421: No-PCH build for Minimal VM was broken by JDK-8287001
  • 108cd69: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long
  • b96ba19: 8289182: NMT: MemTracker::baseline should return void
  • 779b4e1: 8287001: Add warning message when fail to load hsdis libraries
  • 910053b: 8280235: Deprecated flag FlightRecorder missing from VMDeprecatedOptions test
  • 7b3bf97: 8289401: Add dump output to TestRawRSACipher.java
  • 86dc760: Merge
  • 1504804: 8289398: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 again
  • ... and 64 more: https://git.openjdk.org/jdk/compare/c8cc94a38423c0cef597986fb51938a26dc20b51...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 29, 2022
@openjdk openjdk bot closed this Jun 29, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 29, 2022
@openjdk
Copy link

openjdk bot commented Jun 29, 2022

@dougxc Pushed as commit ba670ec.

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

@dougxc dougxc deleted the JDK-8289094 branch May 23, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
4 participants