8352738: Implement JEP 520: JFR Method Timing and Tracing #25306

egahlin · 2025-05-19T15:24:42Z

Could I have a review of this enhancement that will add tracing capabilities to JFR? There are opportunities for performance improvements in the implementation, but I would rather add them later and separately.

Testing: tier 1-3, test/jdk/jdk/jfr

Thanks
Erik

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8356994 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a JEP request to be targeted

Issues

JDK-8352738: Implement JEP 520: JFR Method Timing and Tracing (Enhancement - P2)
JDK-8328610: JEP 520: JFR Method Timing & Tracing (JEP)
JDK-8356994: Implement JEP 520: JFR Method Timing and Tracing (CSR)

Reviewers

Markus Grönlund (@mgronlun - Reviewer) Review applies to 53787227
Aleksey Shipilev (@shipilev - Reviewer)

Contributors

Markus Grönlund <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25306

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-19T15:25:40Z

👋 Welcome back egahlin! 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.

egahlin · 2025-05-19T15:26:46Z

/contributor add @mgronlun

openjdk · 2025-05-19T15:27:14Z

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

8352738: Implement JEP 520: JFR Method Timing and Tracing

Co-authored-by: Markus Grönlund <[email protected]>
Reviewed-by: shade, mgronlun

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

d43f588: 8357955: java.lang.classfile.Signature.ArrayTypeSig.of IAE not thrown for dims > 255
04e0fe0: 8356049: Need a simple way to play back a sound clip
83a2804: 8357408: runtime/interpreter/CountBytecodesTest.java should be flagless
... and 12 more: https://git.openjdk.org/jdk/compare/63d0e7ff117537bf4768b88c43a0231a14ed1512...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 · 2025-05-19T15:27:59Z

@egahlin
Contributor Markus Grönlund <[email protected]> successfully added.

openjdk · 2025-05-19T15:28:39Z

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

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.

egahlin · 2025-05-19T15:28:51Z

/label remove serviceability

egahlin · 2025-05-19T15:29:14Z

/label add hotspot-jfr

openjdk · 2025-05-19T15:29:42Z

@egahlin
The serviceability label was successfully removed.

openjdk · 2025-05-19T15:30:20Z

@egahlin
The hotspot-jfr label was successfully added.

mgronlun

Great work, Erik!

mlbridge · 2025-05-19T15:42:59Z

Webrevs

shipilev · 2025-05-22T10:40:31Z

May want to say /jep JEP-520 as well :)

shipilev

Impressive work! I read it a bit superficially, here are comments/questions:

src/hotspot/share/jfr/instrumentation/jfrClassTransformer.cpp

src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp

src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java.rej

src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataRepository.java

src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/PlatformTracer.java

src/jdk.jfr/share/classes/jdk/jfr/tracing/MethodTracer.java

egahlin · 2025-05-22T13:30:43Z

/jep JEP-520

openjdk · 2025-05-22T13:31:24Z

@egahlin
This pull request will not be integrated until the JEP-520 has been targeted.

liach

Mainly looked at bytecode transformation; those look good.

src/jdk.jfr/share/classes/jdk/jfr/internal/JDKEvents.java

src/jdk.jfr/share/classes/jdk/jfr/internal/LogTag.java

src/hotspot/share/jfr/support/methodtracer/jfrMethodTracer.cpp

src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp

openjdk · 2025-05-26T19:35:16Z

@egahlin this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jep-method-timing-and-tracing
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

src/hotspot/share/jfr/support/methodtracer/jfrMethodTracer.cpp

mgronlun

Looks good.

shipilev

Looks like some of my comments were not fully addressed, these ones that I caught on second read:

src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp

src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp

src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/PlatformTracer.java

egahlin · 2025-05-28T21:21:18Z

Looks like some of my comments were not fully addressed, these ones that I caught on second read:

Sorry about that. I remember fixing synchronized, but maybe it was in the wrong branch.

shipilev

Looks OK to me, thanks.

egahlin · 2025-05-29T08:14:52Z

Thanks for the reviews, @liach, @mgronlun, and @shipilev

egahlin · 2025-05-29T08:30:43Z

/integrate

openjdk · 2025-05-29T08:31:17Z

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

d43f588: 8357955: java.lang.classfile.Signature.ArrayTypeSig.of IAE not thrown for dims > 255
04e0fe0: 8356049: Need a simple way to play back a sound clip
83a2804: 8357408: runtime/interpreter/CountBytecodesTest.java should be flagless
... and 12 more: https://git.openjdk.org/jdk/compare/63d0e7ff117537bf4768b88c43a0231a14ed1512...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-29T08:31:30Z

@egahlin Pushed as commit 07f5b76.

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

Initial

2e5ade8

openjdk bot added the csr Pull request needs approved CSR before integration label May 19, 2025

openjdk bot added serviceability [email protected] hotspot [email protected] labels May 19, 2025

openjdk bot removed the serviceability [email protected] label May 19, 2025

openjdk bot added the hotspot-jfr [email protected] label May 19, 2025

mgronlun approved these changes May 19, 2025

View reviewed changes

egahlin marked this pull request as ready for review May 19, 2025 15:36

openjdk bot added the rfr Pull request is ready for review label May 19, 2025

shipilev reviewed May 22, 2025

View reviewed changes

openjdk bot added the jep label May 22, 2025

liach reviewed May 22, 2025

View reviewed changes

src/jdk.jfr/share/classes/jdk/jfr/internal/JDKEvents.java Outdated Show resolved Hide resolved

src/jdk.jfr/share/classes/jdk/jfr/internal/LogTag.java Outdated Show resolved Hide resolved

mgronlun reviewed May 23, 2025

View reviewed changes

src/hotspot/share/jfr/support/methodtracer/jfrMethodTracer.cpp Outdated Show resolved Hide resolved

mgronlun reviewed May 23, 2025

View reviewed changes

src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp Outdated Show resolved Hide resolved

egahlin added 2 commits May 24, 2025 00:18

Reviewer feedback

489fd83

Atomic::load_acquire(&_method_tracer_state);

c274da4

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed jep labels May 26, 2025

Merge

585a929

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label May 26, 2025

Remove METHOD_FLAG_USED_PREVIOUS_EPOCH_BIT(method)

5f5a42a

mgronlun reviewed May 27, 2025

View reviewed changes

src/hotspot/share/jfr/support/methodtracer/jfrMethodTracer.cpp Outdated Show resolved Hide resolved

magicus mentioned this pull request May 27, 2025

8357920: Add .rej and .orig to .gitignore #25474

Closed

3 tasks

Improve comment

4b28ae9

openjdk bot removed the csr Pull request needs approved CSR before integration label May 28, 2025

Fix comment

5378722

mgronlun approved these changes May 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 28, 2025

Merge

2ca64a1

openjdk bot removed the ready Pull request is ready to be integrated label May 28, 2025

shipilev reviewed May 28, 2025

View reviewed changes

egahlin added 2 commits May 28, 2025 22:48

Add back sticky bit

73025f1

Fix indentation and synchronized

06ad6fa

Atomic::release_store

31d602b

shipilev approved these changes May 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 29, 2025

openjdk bot added the integrated Pull request has been integrated label May 29, 2025

openjdk bot closed this May 29, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 29, 2025

8352738: Implement JEP 520: JFR Method Timing and Tracing #25306

8352738: Implement JEP 520: JFR Method Timing and Tracing #25306

Conversation

egahlin commented May 19, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented May 19, 2025

Uh oh!

egahlin commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

egahlin commented May 19, 2025

Uh oh!

egahlin commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

mgronlun left a comment

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev commented May 22, 2025

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

egahlin commented May 22, 2025

Uh oh!

openjdk bot commented May 22, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented May 26, 2025

Uh oh!

Uh oh!

mgronlun left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

egahlin commented May 28, 2025

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

egahlin commented May 19, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 19, 2025 •

edited

Loading

mlbridge bot commented May 19, 2025 •

edited

Loading