8329103: assert(!thread->in_asgct()) failed during multi-mode profiling #18504

apangin · 2024-03-27T01:02:41Z

This fix makes AsyncGetCallTrace reentrant and async-signal-safe.
Reentrancy is required in the cases when two or more profiling engines are running at the same time, e.g., when CPU and Wall clock profilers work together and therefore one profiler may interrupt another in the middle of getting a stack trace.

Tested with async-profiler:

java -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr

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-8329103: assert(!thread->in_asgct()) failed during multi-mode profiling (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18504

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-27T01:03:22Z

👋 Welcome back apangin! 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-03-27T01:04:00Z

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

8329103: assert(!thread->in_asgct()) failed during multi-mode profiling

Reviewed-by: dholmes, sspitsyn

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

d3fc8df: 8329135: Store Universe::*exception_instance() in CDS archive
a85c849: 8328273: sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java failed with java.rmi.server.ExportException: Port already in use
70c8ff1: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
ecd2b71: 8329354: java/text/Format/MessageFormat/CompactSubFormats.java fails
c2979c1: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
5698f7a: 8329134: Reconsider TLAB zapping
4a14cba: 8329213: Better validation for com.sun.security.ocsp.useget option
9f5464e: 8329353: ResolvedReferencesNotNullTest.java failed with Incorrect resolved references array, quxString should not be archived
3f5b75a: 8328541: Remove or update obsolete comment in JRootPane
1e76e1f: 8328403: Remove applet usage from JColorChooser tests Test6977726
... and 37 more: https://git.openjdk.org/jdk/compare/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19...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 (@dholmes-ora, @sspitsyn) 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-03-27T01:04:26Z

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

hotspot-runtime

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.

mlbridge · 2024-03-27T01:07:30Z

Webrevs

01: Full - Incremental (fc5a4831)
00: Full (af9a57fd)

apangin · 2024-03-27T01:11:04Z

/label add serviceability

openjdk · 2024-03-27T01:11:41Z

@apangin
The serviceability label was successfully added.

dholmes-ora

I have my doubts as to whether AGCT is actually re-entrant in a general sense, but I can see that the ThreadInAsgct RAII object introduced a reentrancy constraint that did not exist prior, and so removing it should not make AGCT any less safe and should allow previous reentrancy cases to continue to work as before.

apangin · 2024-03-28T22:41:03Z

@dholmes-ora Thank you for the review. When looking at the AGCT code, nothing suspicious that could affect reentrancy caught my eye. Also, benchmarks (specJVM, Renaissance) now run fine with two profilers enabled.
So yes, while I don't have 100% proof that AGCT is async-signal-safe, the fix definitely improves the situation.

apangin · 2024-03-28T22:41:11Z

/integrate

openjdk · 2024-03-28T22:41:48Z

@apangin
Your change (at version af9a57f) is now ready to be sponsored by a Committer.

sspitsyn · 2024-03-29T05:55:01Z

src/hotspot/share/runtime/thread.hpp

  ThreadInAsgct(Thread* thread) : _thread(thread) {
    assert(thread != nullptr, "invariant");
-    assert(!thread->in_asgct(), "invariant");
+    // AsyncGetCallTrace is reentrant - save the previous state.


Nit: It is possible to rephrase this comment as follows:

// Allow AsyncGetCallTrace to be reentrant - save the previous state. ``

Thank you for the review - I updated the comment.

sspitsyn · 2024-03-29T13:08:31Z

/sponsor

openjdk · 2024-03-29T13:09:06Z

@sspitsyn The PR has been updated since the change author (@apangin) issued the integrate command - the author must perform this command again.

apangin · 2024-03-29T15:38:31Z

/integrate

openjdk · 2024-03-29T15:39:39Z

@apangin
Your change (at version fc5a483) is now ready to be sponsored by a Committer.

sspitsyn · 2024-04-02T04:29:47Z

/sponsor

openjdk · 2024-04-02T04:30:27Z

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

bc546c2: 8328561: test java/awt/Robot/ManualInstructions/ManualInstructions.java isn't used
af7c6af: 8324808: Manual printer tests have no Pass/Fail buttons, instructions close set 3
d3fc8df: 8329135: Store Universe::*exception_instance() in CDS archive
a85c849: 8328273: sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java failed with java.rmi.server.ExportException: Port already in use
70c8ff1: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
ecd2b71: 8329354: java/text/Format/MessageFormat/CompactSubFormats.java fails
c2979c1: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
5698f7a: 8329134: Reconsider TLAB zapping
4a14cba: 8329213: Better validation for com.sun.security.ocsp.useget option
9f5464e: 8329353: ResolvedReferencesNotNullTest.java failed with Incorrect resolved references array, quxString should not be archived
... and 39 more: https://git.openjdk.org/jdk/compare/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-02T04:30:35Z

@sspitsyn @apangin Pushed as commit 6b1b0e9.

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

8329103: assert(!thread->in_asgct()) failed during multi-mode profiling

af9a57f

openjdk bot added the rfr Pull request is ready for review label Mar 27, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 27, 2024

openjdk bot added the serviceability serviceability-dev@openjdk.org label Mar 27, 2024

dholmes-ora approved these changes Mar 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 28, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 28, 2024

sspitsyn approved these changes Mar 29, 2024

View reviewed changes

Rephrased comment about AsyncGetCallTrace reentrancy

fc5a483

openjdk bot removed the sponsor Pull request is ready to be sponsored label Mar 29, 2024

sspitsyn approved these changes Mar 29, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 29, 2024

dholmes-ora approved these changes Apr 2, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 2, 2024

openjdk bot closed this Apr 2, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 2, 2024

8329103: assert(!thread->in_asgct()) failed during multi-mode profiling #18504

8329103: assert(!thread->in_asgct()) failed during multi-mode profiling #18504

Uh oh!

Conversation

apangin commented Mar 27, 2024 • 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!

bridgekeeper bot commented Mar 27, 2024

Uh oh!

openjdk bot commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 27, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

apangin commented Mar 27, 2024

Uh oh!

openjdk bot commented Mar 27, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

apangin commented Mar 28, 2024

Uh oh!

apangin commented Mar 28, 2024

Uh oh!

openjdk bot commented Mar 28, 2024

Uh oh!

sspitsyn Mar 29, 2024 • 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!

apangin Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

sspitsyn commented Mar 29, 2024

Uh oh!

openjdk bot commented Mar 29, 2024

Uh oh!

apangin commented Mar 29, 2024

Uh oh!

openjdk bot commented Mar 29, 2024

Uh oh!

sspitsyn commented Apr 2, 2024

Uh oh!

openjdk bot commented Apr 2, 2024

Uh oh!

openjdk bot commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

apangin commented Mar 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 27, 2024 •

edited

Loading

mlbridge bot commented Mar 27, 2024 •

edited

Loading

sspitsyn Mar 29, 2024 •

edited

Loading