8354257: xctracenorm profiler not working with JDK JMH benchmarks

[galderz]

Avoid filtering out xml files at the root of the JMH folder, in order to get the default.instruments.template.xml file bundled in the JMH core jar to support xtrace profiler.

---

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

Warning

 ⚠️ Found leading lowercase letter in issue title for 8354257: xctracenorm profiler not working with JDK JMH benchmarks

Issue

• JDK-8354257: xctracenorm profiler not working with JDK JMH benchmarks (Bug - P4)

Reviewers

• Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24571

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back galder! 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]

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

8354257: xctracenorm profiler not working with JDK JMH benchmarks

Reviewed-by: ihse

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

• 0eb680c: 8350308: [s390x] Relativize last_sp (and top_frame_sp) in interpreter frames
• 762423d: 8355561: [macos] Build failure with Xcode 16.3
• acad0b4: 8356188: RISC-V: Cleanup effect of vmaskcmp_fp
• ... and 441 more: https://git.openjdk.org/jdk/compare/6c2667018a49ac78c3a01dc4d52ff6cdf39b7647...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 (@magicus) 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]

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

• build

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]

Webrevs

• 00: Full (a2b73be2)

[erikj79]

The jmh-core jar file published in Maven central (https://repo.maven.apache.org/maven2/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar), which is the one referenced from the devkit generator script, does not contain this default.instruments.template.xml file. The rm command seems to be aimed at checkstyle.xml and findbugs.xml, both of which I would assume to be irrelevant, but should also be benign, but I can't help but wonder why those files are included in the JMH distribution. In what kind of build configuration do you get default.instruments.template.xml bundled?

Unfortunately, the original author of the JDK build logic for micro benchmarks isn't available atm. I don't know enough about JMH to say if this is a reasonable change. Maybe @shipilev can fill in with some knowledge?

[shipilev]

xtraceasm was not released yet in any JMH version, it is going to be in 1.38. So what is the problem here? @galderz, have you dropped the snapshot JMH JARs, and it fails to run with -prof xtraceasm?

[galderz]

@shipilev Yes I'm using the latest JMH source code snapshot to try out xtrace profiler locally. -prof xctraceasm works fine as is, but -prof xctracenorm fails. The problem is explained in JDK-8354257.

[magicus]

My understanding here is that we unpack four different jars: jmh-core, jmh-generator-annprocess, jopt-simple and commons-math3. The removal of *.xml is done on the single directory where all of the jars were unpacked into, so the intended target could be xml files in any of these, not just jmh-core .

[magicus]

With that said, it is very much unclear why this line is here at all. The removal of xml files is from the original integration of microbenchmarks, JDK-8061281, and the reason behind it is lost in time.

But if this works, and does not break any existing microbenchmark tests, I don't see any problem.

[erikj79]

With that said, it is very much unclear why this line is here at all. The removal of xml files is from the original integration of microbenchmarks, JDK-8061281, and the reason behind it is lost in time.

As I wrote above:

The rm command seems to be aimed at checkstyle.xml and findbugs.xml, both of which I would assume to be irrelevant, but should also be benign, but I can't help but wonder why those files are included in the JMH distribution.

That was based on inspecting all 4 jars. I highly doubt there was anything more sinister or dangerous hidden in any of the jar files historically. These seemingly irrelevant files seem enough to have warranted this file removal to me at least. I do think it's a bug that JMH is bundling its checkstyle and findbugs configurations in its distribution jar, so perhaps we can suggest to a JMH maintainer to stop doing that. :)

Regardless, the files should be benign so removing the rm line seems fine to me.

[galderz]

I do think it's a bug that JMH is bundling its checkstyle and findbugs configurations in its distribution jar, so perhaps we can suggest to a JMH maintainer to stop doing that. :)

I think that makes sense to me and I can try to do that. Both files could be moved to src/test/resources, modify the jmh-core pom.xml to files there instead. The combination of both would stop those xml from ending in the distribution jar.

[galderz]

I do think it's a bug that JMH is bundling its checkstyle and findbugs configurations in its distribution jar, so perhaps we can suggest to a JMH maintainer to stop doing that. :)

I think that makes sense to me and I can try to do that. Both files could be moved to src/test/resources, modify the jmh-core pom.xml to files there instead. The combination of both would stop those xml from ending in the distribution jar.

I've created CODETOOLS-7903998 to track this.

[magicus]

I apparently missed to approve this. I think this fix looks sane, regardless of any upstream changes.

[galderz]

/integrate

[openjdk]

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

[magicus]

/sponsor

[openjdk]

Going to push as commit 772c970.
Since your change was applied there have been 445 commits pushed to the master branch:

• 4e3ec3c: 8356208: Remove obsolete code in PSPrinterJob for plugin printing
• 0eb680c: 8350308: [s390x] Relativize last_sp (and top_frame_sp) in interpreter frames
• 762423d: 8355561: [macos] Build failure with Xcode 16.3
• ... and 442 more: https://git.openjdk.org/jdk/compare/6c2667018a49ac78c3a01dc4d52ff6cdf39b7647...master

Your commit was automatically rebased without conflicts.

[openjdk]

@magicus @galderz Pushed as commit 772c970.

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

[galderz]

I do think it's a bug that JMH is bundling its checkstyle and findbugs configurations in its distribution jar, so perhaps we can suggest to a JMH maintainer to stop doing that. :)

I think that makes sense to me and I can try to do that. Both files could be moved to src/test/resources, modify the jmh-core pom.xml to files there instead. The combination of both would stop those xml from ending in the distribution jar.

I've created CODETOOLS-7903998 to track this.

FYI ^ is now fixed in JMH