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

8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics #13054

Closed
wants to merge 6 commits into from

Conversation

sspitsyn
Copy link
Contributor

@sspitsyn sspitsyn commented Mar 16, 2023

This is needed for future performance/scalability improvements in JVMTI support of virtual threads.
The update includes the following:

  1. Refactored the VirtualThread native methods:
    notifyJvmtiMountBegin and notifyJvmtiMountEnd => notifyJvmtiMount
    notifyJvmtiUnmountBegin and notifyJvmtiUnmountEnd => notifyJvmtiUnmount
  2. Still useful implementation of old native methods is moved from jvm.cpp to jvmtiThreadState.cpp:
    JVM_VirtualThreadMountStart => VTMS_mount_begin
    JVM_VirtualThreadMountEnd => VTMS_mount_end
    JVM_VirtualThreadUnmountStart = > VTMS_unmount_begin
    JVM_VirtualThreadUnmountEnd => VTMS_mount_end
  3. Intrinsified the VirtualThread native methods: notifyJvmtiMount, notifyJvmtiUnmount, notifyJvmtiHideFrames.
  4. Removed theVirtualThread static boolean state variable notifyJvmtiEvents and its support in javaClasses.
  5. Added static boolean state variable _VTMS_notify_jvmti_events to the jvmtiVTMSTransitionDisabler class as a replacement of the VirtualThread notifyJvmtiEvents variable.

Implementing the same methods as C1 intrinsics can be needed in the future but is a low priority for now.

Testing:

  • Ran mach5 tiers 1-6. No regressions were found.

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-8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13054

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 16, 2023

👋 Welcome back sspitsyn! 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 Mar 16, 2023

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

  • build
  • core-libs
  • hotspot
  • serviceability

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.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 16, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 16, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 16, 2023

Webrevs

Copy link
Contributor

@iwanowww iwanowww left a comment

Choose a reason for hiding this comment

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

Overall, compiler changes look good.

Any performance numbers to justify the intrinsification?

@openjdk
Copy link

openjdk bot commented Mar 17, 2023

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

8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics

Reviewed-by: vlivanov, lmesnik

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

  • e339e18: 7016187: javac -h could generate conflict .h for inner class and class name with '_'
  • 033c0b1: 8304437: ProblemList com/sun/jdi/ThreadMemoryLeadTest.java with ZGC
  • 7503ecc: 8304138: [JVMCI] Test FailedSpeculation existence before appending.
  • f8482c2: 8297638: Memory leak in case of many started-dead threads
  • c56f011: 8298995: tools/jpackage/share/AddLauncherTest.java#id1 failed "AddLauncherTest.test; checks=53"
  • 254288a: 8014021: TreeMaker.Params behaves inconsistently when the owning method has the same number of parameters as the number of parameter types requested
  • 8f5bb53: 8015831: Add lint check for calling overridable methods from a constructor
  • b085ab9: 8180387: com.sun.source.util.JavacTask should have a protected constructor.
  • bfb812a: 8292818: replace 96-bit representation for field metadata with variable-sized streams
  • 932be35: 8298469: Obsolete legacy parallel class loading workaround for non-parallel-capable class loaders
  • ... and 62 more: https://git.openjdk.org/jdk/compare/7bbc5e0efbcbf97e8c1d4e889bd06c33c5f4eaa5...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 Mar 17, 2023
@sspitsyn
Copy link
Contributor Author

sspitsyn commented Mar 17, 2023

Overall, compiler changes look good.
Any performance numbers to justify the intrinsification?

Thank you for review, your guidance and help with C2 intrinsification!
My goal was to move the notifyJvmtiEvents checks from Java to VM side without a performance penalty.
It is needed to always/unconditionally set the VTMS transition bit in the current JavaThread.
I do not observe any performance degradation with customized Skynet benchmark executing 5 million virtual threads.
Used time utility to measure total execution time (in milliseconds) of 10 runs without JVMTI agent on Oracle Linux server:

  • without intrinsics: 6083, 5405, 5270, 5700, 5004, 5402, 5536, 5031, 4902, 5124
  • with intrinsics: 5904, 5287, 5470, 5672, 5298, 5053, 6154, 4992, 6237, 5155

Similarly, I see no differences when run the Skynet benchmark with a JVMTI agent but it is much less important now because current performance/scalability with JVMTI agent is very poor and needs significant improvements. This fix is a good starting point for them.

@AlanBateman
Copy link
Contributor

The most important case is when there is no JVMTI env. If I read the changes correctly, the overhead for park/continue changes from one volatile-read (notifyJvmtiEvents) to two plain-writes (JavaThread::_is_in_VTMS_transition).

If a JVMTI env has been created then there is no benefit for the moment as there is still a call into the runtime to interact with JvmtiVTMSTransitionDisabler. So I think you are saying that is for follow-on PRs.

Copy link
Member

@lmesnik lmesnik left a comment

Choose a reason for hiding this comment

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

I haven't reviewed C2 changes, all other changes look good to me.

@sspitsyn
Copy link
Contributor Author

The most important case is when there is no JVMTI env. If I read the changes correctly, the overhead for park/continue changes from one volatile-read (notifyJvmtiEvents) to two plain-writes (JavaThread::_is_in_VTMS_transition).

If a JVMTI env has been created then there is no benefit for the moment as there is still a call into the runtime to interact with JvmtiVTMSTransitionDisabler. So I think you are saying that is for follow-on PRs.
@AlanBateman Yes, your conclusion is correct.

@sspitsyn
Copy link
Contributor Author

Thank you for review, Leonid!

@sspitsyn
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 20, 2023

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

  • 2d0d057: 8304016: Add BitMap find_last suite of functions
  • 42723dc: 8304420: Regression ~11% with Javac-Generates on all platforms in b14
  • 19f2edd: 8304541: Modules THROW_MSG_ should return nullptr instead of JNI_FALSE
  • 622f239: 8304163: Move jdk.internal.module.ModuleInfoWriter to the test library
  • 4c8c993: 8304364: [AIX] Build erroneously determines build disk is non-local when using GNU-utils df on AIX
  • 4ed7350: 8304393: Provide method to iterate over regions of humongous object in G1
  • eb73fa8: 8301715: CDS should be disabled in exploded JDK
  • 80e9797: 8304433: cleanup sentence breaker code in DocTreeMaker
  • c396f1e: 8304443: bootcycle builds fail after JDK-8015831
  • ded6a81: 8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
  • ... and 75 more: https://git.openjdk.org/jdk/compare/7bbc5e0efbcbf97e8c1d4e889bd06c33c5f4eaa5...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Mar 20, 2023

@sspitsyn Pushed as commit bc0ed73.

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

@sspitsyn sspitsyn deleted the br24 branch March 20, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
4 participants