8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes #3732

alexmenkov · 2021-04-27T21:30:09Z

Class loading may cause loading of some other system/internal classes (for example loading of java.util.concurrent classes when an other thread loads some classes concurrently).
The fix updates ClassPrepare test so it skip events for "unexpected" classes.
Also it adds a testcase to verify that classes which are explicitly loaded on other thread do not generate ClassPrepare event on the test thread.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes

Reviewers

Chris Plummer (@plummercj - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3732/head:pull/3732
$ git checkout pull/3732

Update a local copy of the PR:
$ git checkout pull/3732
$ git pull https://git.openjdk.java.net/jdk pull/3732/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3732

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3732.diff

bridgekeeper · 2021-04-27T21:31:27Z

👋 Welcome back amenkov! 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 · 2021-04-27T21:33:21Z

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

serviceability

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 · 2021-04-27T21:38:27Z

Webrevs

plummercj

I don't see any mention in the CR of the test failing due to this issue. How was it discovered? How is the fix being verified?

plummercj · 2021-04-27T23:12:36Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassPrepare/classprep001/classprep001.cpp

@@ -62,6 +62,12 @@ static class_info classes[] = {
    { "Lnsk/jvmti/ClassPrepare/classprep001$TestInterface;", EXP_STATUS, 2, 1, 0 },
    { "Lnsk/jvmti/ClassPrepare/classprep001$TestClass;", EXP_STATUS, 3, 2, 1 }
 };
+// This classes are loaded on a different thread.


"These classes..."

plummercj · 2021-04-27T23:14:29Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassPrepare/classprep001/classprep001.cpp

@@ -266,24 +289,16 @@ jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
 }

 JNIEXPORT void JNICALL
-Java_nsk_jvmti_ClassPrepare_classprep001_getReady(JNIEnv *env, jclass cls) {
+Java_nsk_jvmti_ClassPrepare_classprep001_getReady(JNIEnv *env, jclass cls, jthread thread) {


It's not clear to me why you now pass in the thread instead of calling GetCurrentThread, when the thread passed in is still always the current thread. Same thing for the check() method.

I think it makes the code more flexible and easier to extend

alexmenkov · 2021-04-28T19:16:34Z

I don't see any mention in the CR of the test failing due to this issue. How was it discovered? How is the fix being verified?

I discovered it researching JCK failure on a platform with UsageTracker enabled.
JCK test does exactly the same as this test, but I was not able to reproduce the failure for this test (but this is a race, so that's normal)

I ensured the fix works during development when created otherTread before getReady and had printdump on.
It produced a lot of ClassPrepare event for Thread and lambda-related classes.

mlbridge · 2021-04-28T20:47:29Z

Mailing list message from David Holmes on serviceability-dev:

On 29/04/2021 5:21 am, Alex Menkov wrote:

On Tue, 27 Apr 2021 23:15:35 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

I don't see any mention in the CR of the test failing due to this issue. How was it discovered? How is the fix being verified?

I discovered it researching JCK failure on a platform with UsageTracker enabled.
JCK test does exactly the same as this test, but I was not able to reproduce the failure for this test (but this is a race, so that's normal)

I ensured the fix works during development when created otherTread before getReady and had printdump on.

otherTread -> otherThread ?

Cheers,
David
-----

alexmenkov · 2021-04-28T21:44:38Z

otherTread -> otherThread ?

fixed

openjdk · 2021-04-28T22:09:04Z

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

8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes

Reviewed-by: cjplummer, sspitsyn

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

82768d9: 8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java fails with: com.sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][]
b71f85a: 8264398: BevelBorderUIResource(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color
b172555: 8266171: -Warray-bounds happens in imageioJPEG.c
8bcebe7: 8265505: findsym does not work on remote debug server
b88785d: 8266038: Move newAddress() to JVMDebugger
2c53654: 8266179: [macos] jpackage should specify architecture for produced pkg files
d282799: 8255566: Add size validation when parsing values from VersionProps
61365d5: 8266465: Add wildcard to JTwork/JTreport exclude in jib-profiles.js
f00b70e: 8266527: RandomTestCoverage.java failing due to API removal
c53dee7: 8266227: Fix help text for --mac-signing-keychain
... and 229 more: https://git.openjdk.java.net/jdk/compare/f45d46095ac89000c090c2bc6370884860477ee1...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.

sspitsyn

Hi Alex,
The fix looks good to me. The logics is more clear now.
Thanks,
Serguei

alexmenkov · 2021-05-06T18:37:06Z

/integrate

openjdk · 2021-05-06T18:38:33Z

@alexmenkov Since your change was applied there have been 273 commits pushed to the master branch:

52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
2dcbedf: 8266044: Nested class summary should show kind of class or interface
e840597: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
fcedfc8: 8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java & test/jdk/java/sql/testng/util/TestPolicy.java
c665dba: 8266561: Remove Compile::_save_argument_registers
47d4438: 8266426: ZHeapIteratorOopClosure does not handle native access properly
2438498: 8252758: Lanai: Optimize index calculation while copying glyphs
eb3b96d: 8266496: WBIsKlassAliveClosure.do_klass() fails for hidden classes
... and 263 more: https://git.openjdk.java.net/jdk/compare/f45d46095ac89000c090c2bc6370884860477ee1...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0ca86da.

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

ClassPrepare test fix

23d5ec6

openjdk bot added the rfr Pull request is ready for review label Apr 27, 2021

openjdk bot added the serviceability serviceability-dev@openjdk.org label Apr 27, 2021

plummercj suggested changes Apr 27, 2021

View reviewed changes

Updated comment

4cebb5d

fixed type in otherThread variable name

9929298

plummercj approved these changes Apr 28, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 28, 2021

sspitsyn approved these changes May 5, 2021

View reviewed changes

openjdk bot closed this May 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 6, 2021

alexmenkov deleted the ClassPrepTest branch May 10, 2021 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes #3732

8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes #3732

alexmenkov commented Apr 27, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

mlbridge bot commented Apr 27, 2021 •

edited

Loading

plummercj left a comment

plummercj Apr 27, 2021

alexmenkov Apr 28, 2021

plummercj Apr 27, 2021

alexmenkov Apr 28, 2021

alexmenkov commented Apr 28, 2021

mlbridge bot commented Apr 28, 2021

alexmenkov commented Apr 28, 2021

openjdk bot commented Apr 28, 2021 •

edited

Loading

sspitsyn left a comment

alexmenkov commented May 6, 2021

openjdk bot commented May 6, 2021

8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes #3732

8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes #3732

Conversation

alexmenkov commented Apr 27, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

mlbridge bot commented Apr 27, 2021 • edited Loading

Webrevs

plummercj left a comment

Choose a reason for hiding this comment

plummercj Apr 27, 2021

Choose a reason for hiding this comment

alexmenkov Apr 28, 2021

Choose a reason for hiding this comment

plummercj Apr 27, 2021

Choose a reason for hiding this comment

alexmenkov Apr 28, 2021

Choose a reason for hiding this comment

alexmenkov commented Apr 28, 2021

mlbridge bot commented Apr 28, 2021

alexmenkov commented Apr 28, 2021

openjdk bot commented Apr 28, 2021 • edited Loading

sspitsyn left a comment

Choose a reason for hiding this comment

alexmenkov commented May 6, 2021

openjdk bot commented May 6, 2021

alexmenkov commented Apr 27, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 27, 2021 •

edited

Loading

openjdk bot commented Apr 28, 2021 •

edited

Loading