-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8338417: Explicitly pin a virtual thread before acquiring the JFR string pool monitor #20588
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mgronlun! A progress list of the required criteria for merging this PR into |
@mgronlun 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:
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 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally seems fine. Could be optimised a bit.
} | ||
|
||
private static void unpinVirtualThread() { | ||
if (Thread.currentThread().isVirtual() && ContinuationSupport.isSupported()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are at all concerned about overhead here then pin could return a boolean to indicate if the pin happened and oyu could then unpin just by checking that boolean and avoid doing the isVirtual and isSupported checks again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to create a boolean in the EventWriter that indicates if it is associated with a carrier thread or a normal thread (which can never be virtual) and then have two methods.
long l = this.carrierThread ? StringPool.addPinnedString(s) : StringPool.addString(s);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread.currentThread() has an intrinsic, and isVirtual is just a type check. ContinuationSupport.isSupported reads a static final so will disappear once compiled. The pattern we are using in other areas is for the pin to return a boolean (like David suggested).
Greetings,
Explicitly pin a virtual thread before acquiring the JFR string pool monitor because migrating a carrier thread local event writer object onto another carrier thread is impossible.
During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object. For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.
A monitor guards the JFR string pool, but remounting a virtual thread onto another carrier is impossible because of the event writer.
Therefore, it's imperative to use explicit pin constructs to prevent unmounting at this location.
Testing: jdk_jfr
Thanks
Markus
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20588/head:pull/20588
$ git checkout pull/20588
Update a local copy of the PR:
$ git checkout pull/20588
$ git pull https://git.openjdk.org/jdk.git pull/20588/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20588
View PR using the GUI difftool:
$ git pr show -t 20588
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20588.diff
Webrev
Link to Webrev Comment