Skip to content

Conversation

@mgronlun
Copy link

@mgronlun mgronlun commented Mar 28, 2025

Greetings,

This is the implementation of JEP JDK-8350338 Cooperative JFR Sampling.

Implementations in this change set are provided and have been tested on the following platforms:

  • windows-x64
  • windows-x64-debug
  • linux-x64
  • linux-x64-debug
  • macosx-x64
  • macosx-x64-debug
  • linux-aarch64
  • linux-aarch64-debug
  • macosx-aarch64
  • macosx-aarch64-debug

Testing: tier1-6, jdk_jfr, stress testing.

Platform porters note:
Some platform-specific code needs to be provided, mainly in the interpreter. Take a look at the following files for changes:

  • src/hotspot/cpu/x86/frame_x86.cpp
  • src/hotspot/cpu/x86/interp_masm_x86.cpp
  • src/hotspot/cpu/x86/interp_masm_x86.hpp
  • src/hotspot/cpu/x86/javaFrameAnchor_x86.hpp
  • src/hotspot/cpu/x86/macroAssembler_x86.cpp
  • src/hotspot/cpu/x86/macroAssembler_x86.hpp
  • src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
  • src/hotspot/cpu/x86/templateTable_x86.cpp
  • src/hotspot/os_cpu/linux_x86/javaThread_linux_x86.hpp

Thanks
Markus


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
  • Change requires a JEP request to be targeted

Issues

  • JDK-8352251: Implement JEP 518: JFR Cooperative Sampling (Sub-task - P2)
  • JDK-8350338: JEP 518: JFR Cooperative Sampling (JEP)

Reviewers

Contributors

  • Aleksey Shipilev <shade@openjdk.org>
  • Erik Österlund <eosterlund@openjdk.org>
  • Boris Ulasevich <bulasevich@openjdk.org>
  • Patricio Chilano Mateo <pchilanomate@openjdk.org>
  • Martin Doerr <mdoerr@openjdk.org>
  • Fei Yang <fyang@openjdk.org>
  • Amit Kumar <amitkumar@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24296

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 28, 2025

👋 Welcome back mgronlun! 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 28, 2025

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

8352251: Implement JEP 518: JFR Cooperative Sampling

Co-authored-by: Aleksey Shipilev <shade@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Boris Ulasevich <bulasevich@openjdk.org>
Co-authored-by: Patricio Chilano Mateo <pchilanomate@openjdk.org>
Co-authored-by: Martin Doerr <mdoerr@openjdk.org>
Co-authored-by: Fei Yang <fyang@openjdk.org>
Co-authored-by: Amit Kumar <amitkumar@openjdk.org>
Reviewed-by: eosterlund, egahlin

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 rfr Pull request is ready for review label Mar 28, 2025
@openjdk
Copy link

openjdk bot commented Mar 28, 2025

@mgronlun 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 Mar 28, 2025
@mlbridge
Copy link

mlbridge bot commented Mar 28, 2025

@mgronlun
Copy link
Author

/label add hotspot-jfr

@openjdk openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Mar 28, 2025
@openjdk
Copy link

openjdk bot commented Mar 28, 2025

@mgronlun
The hotspot-jfr label was successfully added.

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

A drive-by comment, since I am cleaning up some of x86 code after x86_32 removals:

@mgronlun
Copy link
Author

mgronlun commented Apr 7, 2025

Friendly reminder/heads-up to platform and port maintainers:

Please review the necessary platform changes in advance so that your port will be ready once this integration is complete. Alternatively, you can send me your change sets for them to be incorporated into this PR.

Thanks
Markus

@openjdk
Copy link

openjdk bot commented Apr 18, 2025

⚠️ @mgronlun This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@shipilev
Copy link
Member

Zero does not use JFR, jfr build feature flag is not enabled. So we only need to make sure it compiles. It currently does not, because last_Java_fp returns ZeroFrame*, and not generic intptr_t*. Here: jfr-cooperative-zero-1.txt

@mgronlun
Copy link
Author

mgronlun commented Apr 23, 2025

Zero does not use JFR, jfr build feature flag is not enabled. So we only need to make sure it compiles. It currently does not, because last_Java_fp returns ZeroFrame*, and not generic intptr_t*. Here: jfr-cooperative-zero-1.txt

Thanks alot @shipilev . It was precisely this "ZeroFrame*" that tripped me up.

@mgronlun
Copy link
Author

/contributor add @shipilev

@openjdk
Copy link

openjdk bot commented Apr 23, 2025

@mgronlun
Contributor Aleksey Shipilev <shade@openjdk.org> successfully added.

@TheRealMDoerr
Copy link
Contributor

Rebased version is here: TheRealMDoerr@e98c21e
I think I have all parts which have been implemented for other platforms. I can continue testing the whole PR after it is integrated.

@mgronlun
Copy link
Author

Rebased version is here: TheRealMDoerr@e98c21e I think I have all parts which have been implemented for other platforms. I can continue testing the whole PR after it is integrated.

Great. Integrated.

@openjdk openjdk bot removed the jep label May 26, 2025
Copy link
Contributor

@fisk fisk left a comment

Choose a reason for hiding this comment

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

I see you fixed the JVMTI issue. This looks good to me now.

@mgronlun
Copy link
Author

I see you fixed the JVMTI issue. This looks good to me now.

Thank you very much, Erik!

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 26, 2025
@offamitkumar
Copy link
Member

Hi @mgronlun ,

Please include the minimal build fix for s390x from here: offamitkumar@916efec . I need to port some other enhancement before I can complete this one. So I am planning it for later.

@mgronlun
Copy link
Author

Hi @mgronlun ,

Please include the minimal build fix for s390x from here: offamitkumar@916efec . I need to port some other enhancement before I can complete this one. So I am planning it for later.

Thanks Amit. Integrated.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label May 26, 2025
@mgronlun
Copy link
Author

/contributor add @offamitkumar

@mgronlun mgronlun requested review from egahlin and fisk May 26, 2025 16:24
@openjdk
Copy link

openjdk bot commented May 26, 2025

@mgronlun
Contributor Amit Kumar <amitkumar@openjdk.org> successfully added.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 26, 2025
@mgronlun
Copy link
Author

Thank you, @fisk and @egahlin, for your reviews!

@mgronlun
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented May 26, 2025

Going to push as commit bbceab0.
Since your change was applied there has been 1 commit pushed to the master branch:

  • e8eff4d: 8357530: C2 SuperWord: Diagnostic flag AutoVectorizationOverrideProfitability

Your commit was automatically rebased without conflicts.

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

openjdk bot commented May 26, 2025

@mgronlun Pushed as commit bbceab0.

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

@mgronlun
Copy link
Author

Thank you all for contributing, reviewing, providing recommendations, and supporting getting this feature ready for JDK 25.

Many thanks
Markus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hotspot hotspot-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.