8350051: [JMH] Several tests fails NPE #23625

sendaoYan · 2025-02-14T07:21:27Z

Hi all,

Several JMH tests fails 'Cannot invoke "java.io.InputStream.available()" because "is" is null', because the file 'build/linux-x86_64-server-release/images/test/micro/benchmarks.jar' missing the required xml input file defined by test/micro/org/openjdk/bench/javax/xml/AbstractXMLMicro.java. This PR copy the required xml file to benchmarks.jar, and remove two unexist xml input file.

After this PR, below JMH tests will run passes.

org.openjdk.bench.javax.xml.DOM.testBuild
org.openjdk.bench.javax.xml.DOM.testModify
org.openjdk.bench.javax.xml.DOM.testWalk
org.openjdk.bench.javax.xml.SAXUsingJDK.testParse
org.openjdk.bench.javax.xml.STAX.testParse

Test command:

rm -rf build/jmh-result/ ; mkdir -p build/jmh-result/ ; time for test in `cat list.txt` ; do time make test TEST="micro:$test" MICRO="FORK=1;WARMUP_ITER=2" CONF=release &> build/jmh-result/$test.log ; done

Change has been verified locally, no risk.

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

Issue

JDK-8350051: [JMH] Several tests fails NPE (Bug - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer)
Claes Redestad (@cl4es - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23625

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-14T07:22:05Z

👋 Welcome back syan! 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 · 2025-02-14T07:22:16Z

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

8350051: [JMH] Several tests fails NPE

Reviewed-by: erikj, redestad

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

7734f8e: 8349664: HEX dump should always use ASCII or ISO_8859_1
7631984: 8349923: Refactor StackMapTable constructor and StackMapReader
3487f8c: 8350102: Decouple jpackage test-lib Executor.Result and Executor classes
70a6c0b: 8346094: Harden X509CertImpl.getExtensionValue for NPE cases
efbad00: 8349688: G1: Wrong initial optional region index when selecting candidates from retained regions
c6e47fd: 8348171: Refactor GenerationCounters and its subclasses
577ff98: 8347946: Add API note that caller should validate/trust signers to the getCertificates and getCodeSigners methods of JarEntry and JarURLConnection
014701a: 8350260: Improve HTML instruction formatting in PassFailJFrame
59810ad: 8350201: Out of bounds access on Linux aarch64 in os::print_register_info
2353f3e: 8350210: CTW: Use stackless exceptions
... and 46 more: https://git.openjdk.org/jdk/compare/ff52859d2ad65b97c56dd19323213a0d07be47ae...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-02-14T07:22:46Z

@sendaoYan 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.

sendaoYan · 2025-02-14T07:25:20Z

/label add xml

openjdk · 2025-02-14T07:25:30Z

@sendaoYan
The label xml is not a valid label.
These labels are valid:

graal
serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

mlbridge · 2025-02-14T07:25:40Z

Webrevs

01: Full - Incremental (f553bf77)
00: Full (ba555448)

sendaoYan · 2025-02-14T07:26:39Z

/label add net

openjdk · 2025-02-14T07:26:43Z

@sendaoYan
The net label was successfully added.

sendaoYan · 2025-02-14T07:49:10Z

/label add core-libs

sendaoYan · 2025-02-14T07:49:19Z

/label remove net

openjdk · 2025-02-14T07:50:03Z

@sendaoYan
The core-libs label was successfully added.

openjdk · 2025-02-14T07:50:27Z

@sendaoYan
The net label was successfully removed.

erikj79

I'm not sure if this is the right way to solve this. I would really like someone more familiar with the microbenchmarks to weigh in on how this was originally meant to work.

make/test/BuildMicrobenchmark.gmk

sendaoYan · 2025-02-19T01:54:32Z

I'm not sure if this is the right way to solve this. I would really like someone more familiar with the microbenchmarks to weigh in on how this was originally meant to work.

Hi, @cl4es, Could you take took this PR.

cl4es · 2025-02-19T05:05:17Z

The issue here looks like an oversight (of mine) in the migration from the standalone JMH repo. There we simply held a copy of the needed XML files, e.g. https://github.com/openjdk/jmh-jdk-microbenchmarks/blob/master/micros-jdk8/src/main/resources/org/openjdk/bench/javax/xml/msgAttach.xml

As we're now in-tree I guess it makes sense to copy these over at build time as per this fix - though it creates an undocumented dependency. If anyone changes these tests on the functional side we'd silently break. Perhaps needs a unit test to guard and document the dependency?

Taking a step back: If these particular micros have all been broken since JEP 230 was integrated in JDK 12 and no-one noticed until now - maybe they aren't really worth their weight and should instead be removed?

sendaoYan · 2025-02-19T07:05:17Z

If anyone changes these tests on the functional side we'd silently break.

After this PR, if the dependency XML files been removed will make make test-image fails. So maybe we do not need a new unit test to guard it.

Perhaps needs a unit test to guard and document the dependency?

Where should I document the dependency, does anyone could give some suggestions. OTOH maybe we should remove this JMH test?

cl4es · 2025-02-19T08:02:07Z

If you have manually verified that the build fails if any of these files are removed then I'm happy. No new tests or documentation needed, really.

I could cast my vote towards removing these particular micros. But as the work is done I say let's accept your change, (re-)evaluate their usefulness, and file a RFE to have them removed if they aren't. We have been building out the set of micros we regularly test in nightly / promotion testing, but due to resource constraints only a fraction is run on a regular basis - and javax.xml has not been a priority.

sendaoYan · 2025-02-19T08:49:17Z

have manually verified that the build fails if any of these files are removed.

Yes. I have verified that. If I remove the dependency XML file, then make test-image CONF=release JOBS=1 will report below failure.

gmake[3]: *** No rule to make target '/home/yansendao/git/jdk-ysd/test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/message_12.xml', needed by '/home/yansendao/git/jdk-ysd/build/linux-x86_64-server-release/support/test/micro/classes/org/openjdk/bench/javax/xml/message_12.xml'.  Stop.
gmake[2]: *** [make/Main.gmk:788: build-microbenchmark] Error 2

ERROR: Build failed for target 'test-image' in configuration 'linux-x86_64-server-release' (exit code 2)

sendaoYan · 2025-02-20T02:50:29Z

Thanks @cl4es @erikj79 for the suggestions and reviews.

/integrate

openjdk · 2025-02-20T02:51:26Z

Going to push as commit 00d4e4a.
Since your change was applied there have been 61 commits pushed to the master branch:

0131c1b: 8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true"
3ebed78: 8349943: [JMH] Use jvmArgs consistently
92efab9: 8350344: Cross-build failure: _vptr name conflict
4fb70c7: 8229012: When single stepping, the debug agent can cause the thread to remain in interpreter mode after single stepping completes
4e60c2d: 8349699: XSL transform fails with certain UTF-8 characters on 1024 byte boundaries
7734f8e: 8349664: HEX dump should always use ASCII or ISO_8859_1
7631984: 8349923: Refactor StackMapTable constructor and StackMapReader
3487f8c: 8350102: Decouple jpackage test-lib Executor.Result and Executor classes
70a6c0b: 8346094: Harden X509CertImpl.getExtensionValue for NPE cases
efbad00: 8349688: G1: Wrong initial optional region index when selecting candidates from retained regions
... and 51 more: https://git.openjdk.org/jdk/compare/ff52859d2ad65b97c56dd19323213a0d07be47ae...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-20T02:51:35Z

@sendaoYan Pushed as commit 00d4e4a.

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

8350051: [JMH] Several tests fails NPE

ba55544

openjdk bot added the rfr Pull request is ready for review label Feb 14, 2025

openjdk bot added the build build-dev@openjdk.org label Feb 14, 2025

openjdk bot added the net net-dev@openjdk.org label Feb 14, 2025

openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 14, 2025

openjdk bot removed the net net-dev@openjdk.org label Feb 14, 2025

erikj79 reviewed Feb 18, 2025

View reviewed changes

make/test/BuildMicrobenchmark.gmk Outdated Show resolved Hide resolved

Use SetupCopyFiles macro to copy the dependency files

f553bf7

erikj79 approved these changes Feb 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 19, 2025

cl4es approved these changes Feb 19, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 20, 2025

openjdk bot closed this Feb 20, 2025

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

8350051: [JMH] Several tests fails NPE #23625

8350051: [JMH] Several tests fails NPE #23625

Uh oh!

Conversation

sendaoYan commented Feb 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Feb 14, 2025

Uh oh!

sendaoYan commented Feb 14, 2025

Uh oh!

openjdk bot commented Feb 14, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

sendaoYan commented Feb 14, 2025

Uh oh!

openjdk bot commented Feb 14, 2025

Uh oh!

sendaoYan commented Feb 14, 2025

Uh oh!

sendaoYan commented Feb 14, 2025

Uh oh!

openjdk bot commented Feb 14, 2025

Uh oh!

openjdk bot commented Feb 14, 2025

Uh oh!

erikj79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sendaoYan commented Feb 19, 2025

Uh oh!

cl4es commented Feb 19, 2025

Uh oh!

sendaoYan commented Feb 19, 2025

Uh oh!

cl4es commented Feb 19, 2025

Uh oh!

sendaoYan commented Feb 19, 2025

Uh oh!

sendaoYan commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

sendaoYan commented Feb 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 14, 2025 •

edited

Loading

mlbridge bot commented Feb 14, 2025 •

edited

Loading