Skip to content
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

[23.0] Allow built-in periodic JFR events when using JFR API to make recordings #573

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

roberttoyonaga
Copy link
Collaborator

Summary

Built-in periodic JFR events were previously erroneously disabled unless JFR is started from the command line when executing your app. This means that users using the JFR API would not have access to those periodic events. The fix is to always register the built-in periodic events if JFR is in the image, regardless of whether a recording is started upon application execution.

Original fix: oracle#7252

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 18, 2023
@roberttoyonaga roberttoyonaga added affects/JDK17 JDK17 related backport affects/23.0 OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. affects/JDK17 JDK17 related labels Sep 18, 2023
@roberttoyonaga roberttoyonaga changed the title Allow built-in periodic JFR events when using JFR API to make recordings [23.0] Allow built-in periodic JFR events when using JFR API to make recordings Sep 18, 2023
Copy link
Collaborator

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM and safe to backport.

@roberttoyonaga do you think it would be possible to backport the TestThreadCPULoadEvent test as well?

@zakkak
Copy link
Collaborator

zakkak commented Sep 19, 2023

Nit: Ideally you should use git cherry-pick -x for backporting. This will preserve the same commit message and set of commits, while also adding a reference to the commit SHA you are cherry-picking/backporting, which makes tracking things a bit easier.

@zakkak zakkak requested a review from jerboaa September 19, 2023 06:37
@jerboaa
Copy link
Collaborator

jerboaa commented Sep 19, 2023

@roberttoyonaga do you think it would be possible to backport the TestThreadCPULoadEvent test as well?

Not Robert, but it doesn't make much sense to backport the test changes as the test got introduced with oracle#6588 which introduced the ThreadCPULoadEvent.

@jerboaa
Copy link
Collaborator

jerboaa commented Sep 19, 2023

@roberttoyonaga Could you please pin the workflow file to commit 5c44969 (rather than default) as it looks like the newer changes broke the CI workflow.

Something like this (in a separate commit):

diff --git a/.github/workflows/mandrel.yml b/.github/workflows/mandrel.yml
index d21d1e3d16a3..ff7dee95c5cf 100644
--- a/.github/workflows/mandrel.yml
+++ b/.github/workflows/mandrel.yml
@@ -29,7 +29,7 @@ concurrency:
 jobs:
   q-main-ea:
     name: "Q main M 23.0 EA"
-    uses: graalvm/mandrel/.github/workflows/base.yml@default
+    uses: graalvm/mandrel/.github/workflows/base.yml@5c44969947cbeff84ca9ccedc127cb6730b82415
     with:
       quarkus-version: "main"
       repo: ${{ github.repository }}
@@ -38,7 +38,7 @@ jobs:
       jdk: "17/ea"
   q-main-ea-win:
     name: "Q main M 23.0 windows EA"
-    uses: graalvm/mandrel/.github/workflows/base-windows.yml@default
+    uses: graalvm/mandrel/.github/workflows/base-windows.yml@5c44969947cbeff84ca9ccedc127cb6730b82415
     with:
       quarkus-version: "main"
       repo: ${{ github.repository }}
@@ -47,7 +47,7 @@ jobs:
       jdk: "17/ea"
   q-main-ea-20:
     name: "Q main M 23.0 EA Java 20"
-    uses: graalvm/mandrel/.github/workflows/base.yml@default
+    uses: graalvm/mandrel/.github/workflows/base.yml@5c44969947cbeff84ca9ccedc127cb6730b82415
     with:
       quarkus-version: "main"
       repo: ${{ github.repository }}
@@ -56,7 +56,7 @@ jobs:
       jdk: "20/ea"
   q-main-ea-win-20:
     name: "Q main M 23.0 windows EA Java 20"
-    uses: graalvm/mandrel/.github/workflows/base-windows.yml@default
+    uses: graalvm/mandrel/.github/workflows/base-windows.yml@5c44969947cbeff84ca9ccedc127cb6730b82415
     with:
       quarkus-version: "main"
       repo: ${{ github.repository }}

Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Cherry-picking doesn't really make sense in this case as it would conflict. Please be sure to pin the workflow file so that we get better pre-integration CI results. Thanks!

@roberttoyonaga
Copy link
Collaborator Author

do you think it would be possible to backport the TestThreadCPULoadEvent test as well?

@zakkak I decided it might be risky to also backport jdk.ThreadCPULoadEvent because it involves VM operations, which is why I didn't also backport that test. I can instead make a new test that is specific to this fix. Would that be better?

Also, how can I run the native unittests on Mandrel? Do I need to do something in order to usemx native-unittest?

@zakkak
Copy link
Collaborator

zakkak commented Sep 19, 2023

do you think it would be possible to backport the TestThreadCPULoadEvent test as well?

@zakkak I decided it might be risky to also backport jdk.ThreadCPULoadEvent because it involves VM operations, which is why I didn't also backport that test. I can instead make a new test that is specific to this fix. Would that be better?

No, I believe it's not worth it (especially given we can't run unittests on Mandrel).

Also, how can I run the native unittests on Mandrel? Do I need to do something in order to usemx native-unittest?

Unfortunately we can't, see #516 (comment)

Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

LGTM

@zakkak
Copy link
Collaborator

zakkak commented Sep 20, 2023

@roberttoyonaga now that #577 is merged could you please change the build type in the mandrel.yml workflow to mandrel-source-nolocalmvn and keep the @default pinning?

You can do so by adding the following line below with: in each job:

     build-type: "mandrel-source-nolocalmvn"

@zakkak
Copy link
Collaborator

zakkak commented Sep 21, 2023

The mandrel IT failures are due to quarkusio/quarkus#36058

@zakkak zakkak merged commit 9d9b7e7 into graalvm:mandrel/23.0 Sep 21, 2023
137 of 141 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/23.0 backport OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants