8358666: [REDO] Implement JEP 509: JFR CPU-Time Profiling#25654
8358666: [REDO] Implement JEP 509: JFR CPU-Time Profiling#25654parttimenerd wants to merge 147 commits intoopenjdk:masterfrom
Conversation
… with a thread-local critical section sampling interrupt mask.
| if (isCPUTimeMethodSampling) { | ||
| this.cpuRate = rate; | ||
| if (isEnabled()) { | ||
| JVM.setCPUThrottle(rate.rate(), rate.autoAdapt()); |
There was a problem hiding this comment.
but we need to set the throttle somewhere? Else changes are not propagated?
|
That call was what caused the issues to begin with, so I'm slightly nervous about it. |
|
Perhaps it's correct, but again.... |
|
I think you need a REDO issue. |
|
I tested it and it works as expected. This is similar to what the normal JFR profiler does. We have two scenarios:
|
I reopened it, isn't this enough? |
|
No, you can't reopen it. Its already integrated. |
parttimenerd
changed the title
|
I created a new issue and linked it |
parttimenerd
changed the title
|
The settings change looks reasonable. |
|
I updated the JIRA issue to capitalized [REDO] which seems to be the more common process. Can you please update the PR title accordingly? |
openjdk
bot
changed the title
|
Skara did it automatically |
mgronlun
left a comment
mgronlun
left a comment
There was a problem hiding this comment.
I see that this PR includes my fixes for the issues I found during this night of debugging, as detailed in the backout issue: https://bugs.openjdk.org/browse/JDK-8358628
I am approving for that reason, else all that work would have been in vain.
|
I'm waiting for the last test to finish (just in case) |
|
/integrate |
|
The JEP is targeted for JDK-25, if you are planning to integrate this today then I think it is too rushed. There is no need to rush this into JDK-25 instead of deferring it to JDK-26. |
|
You need two reviewers! sigh... |
TheRealMDoerr
left a comment
TheRealMDoerr
left a comment
There was a problem hiding this comment.
As mentioned in #25302, I think this is good enough for an experimental feature assuming the the tests are fine this time :-)
|
Going to push as commit ace70a6.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@parttimenerd Pushed as commit ace70a6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
It seems the new test TestCPUTimeSampleMultipleRecordings fails most of the time on the systems with 90+G of memory. |
I can't reproduce this on a machine with 128G of RAM, can you give me more details? |
8 participants
This is the code for the JEP 509: CPU Time based profiling for JFR.
Currently tested using this test suite. This runs profiles the Renaissance benchmark with
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25654/head:pull/25654$ git checkout pull/25654Update a local copy of the PR:
$ git checkout pull/25654$ git pull https://git.openjdk.org/jdk.git pull/25654/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25654View PR using the GUI difftool:
$ git pr show -t 25654Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25654.diff
Using Webrev
Link to Webrev Comment