8292681: Add JMH for ProtectionDomain #9950

ericcaspole · 2022-08-19T18:48:10Z

Add a JMH for ProtectionDomain related to current work on JDK-8292375. Also, add the InMemoryJavaCompiler to the JMH jar, to generate the classes needed for this test and will be useful for future class loading JMH too.

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-8292681: Add JMH for ProtectionDomain

Reviewers

Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9950

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

Using diff file

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

bridgekeeper · 2022-08-19T18:48:57Z

👋 Welcome back ecaspole! 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 · 2022-08-19T19:20:32Z

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

security

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 · 2022-08-19T19:23:31Z

Webrevs

schlosna · 2022-08-24T05:35:54Z

test/micro/org/openjdk/bench/java/security/ProtectionDomainBench.java

+@Warmup(iterations = 15)
+@Measurement(iterations = 15)


Given the recent work to reduce microbenchmark runtimes, does this actually need this many iterations and time to get a stable microbenchmark?

You are right, I changed it back to throughput mode and 5/2. Also the main work is in the Setup and it can be done per Trial, not per Iteration.

schlosna · 2022-08-24T05:39:29Z

test/micro/org/openjdk/bench/java/security/ProtectionDomainBench.java

+        return new String("public class B" + count + " {"
+                + "   static int intField;"
+                + "   public static void compiledMethod() { "
+                + "       intField++;"
+                + "   }"
+                + "}");


Curious why copying to new String? Is this to try to avoid string deduplication? Could it be simplified?

Suggested change

return new String("public class B" + count + " {"

+ " static int intField;"

+ " public static void compiledMethod() { "

+ " intField++;"

+ " }"

+ "}");

return "public class B" + count + " {"

+ " static int intField;"

+ " public static void compiledMethod() { "

+ " intField++;"

+ " }"

+ "}";

I agree on this, chnaged.

seanjmullan · 2022-08-25T09:14:07Z

test/micro/org/openjdk/bench/java/security/ProtectionDomainBench.java

+
+        for (int i = 0; i < numberOfClasses; i++) {
+            classNames[i] = "B" + i;
+            compiledClasses[i] = InMemoryJavaCompiler.compile(classNames[i], B(i));


Is it possible to use the existing jdk.test.lib.compiler.InMemoryJavaCompiler class instead of writing a new one?

It needs to be added into the benchmarks.jar, and there are already 2 copies of it in different test suites so it seems like this is how it's done.

openjdk · 2022-08-31T13:04:18Z

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

8292681: Add JMH for ProtectionDomain

Reviewed-by: mullan

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

38e6706: 8293050: RISC-V: Remove redundant non-null assertions about macro-assembler
4c90e87: 8290781: Segfault at PhaseIdealLoop::clone_loop_handle_data_uses
9e3176b: 8293035: Cleanup MacroAssembler::movoop code patching logic aarch64 riscv
c42478b: 8293109: Remove unused methods in Compiler
d2eed07: 8293020: jmod should not be treated as "small" tool for large modules
923c952: 8293098: GHA: Harmonize GCC version handling for host and cross builds
38377d5: 8291649: multiple tests failing with -Xcomp after JDK-8290034
2d7792c: 8293065: Zero build failure on AArch64 and RISCV64 after JDK-8293007
c3380c0: 8292995: improve the SA page cache
1cf245d: 8292590: Product JVM crashes with FLAG_SET_XXX on non-product Flag
... and 122 more: https://git.openjdk.org/jdk/compare/7244dd6fab0c516ed76af594593b8378512620c8...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.

ericcaspole · 2022-08-31T13:23:05Z

/integrate

openjdk · 2022-08-31T13:24:47Z

Going to push as commit 5d6779f.
Since your change was applied there have been 132 commits pushed to the master branch:

38e6706: 8293050: RISC-V: Remove redundant non-null assertions about macro-assembler
4c90e87: 8290781: Segfault at PhaseIdealLoop::clone_loop_handle_data_uses
9e3176b: 8293035: Cleanup MacroAssembler::movoop code patching logic aarch64 riscv
c42478b: 8293109: Remove unused methods in Compiler
d2eed07: 8293020: jmod should not be treated as "small" tool for large modules
923c952: 8293098: GHA: Harmonize GCC version handling for host and cross builds
38377d5: 8291649: multiple tests failing with -Xcomp after JDK-8290034
2d7792c: 8293065: Zero build failure on AArch64 and RISCV64 after JDK-8293007
c3380c0: 8292995: improve the SA page cache
1cf245d: 8292590: Product JVM crashes with FLAG_SET_XXX on non-product Flag
... and 122 more: https://git.openjdk.org/jdk/compare/7244dd6fab0c516ed76af594593b8378512620c8...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-31T13:25:02Z

@ericcaspole Pushed as commit 5d6779f.

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

8292681: Add JMH for ProtectionDomain

63e61fe

openjdk bot added the rfr Pull request is ready for review label Aug 19, 2022

openjdk bot added the security security-dev@openjdk.org label Aug 19, 2022

schlosna reviewed Aug 24, 2022

View reviewed changes

Updates from reviewers comments

a77e6a5

seanjmullan reviewed Aug 25, 2022

View reviewed changes

Changes from advice from Sean M

3a83b73

seanjmullan approved these changes Aug 31, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 31, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 31, 2022

openjdk bot closed this Aug 31, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 31, 2022

		@Warmup(iterations = 15)
		@Measurement(iterations = 15)

8292681: Add JMH for ProtectionDomain #9950

8292681: Add JMH for ProtectionDomain #9950

Uh oh!

Conversation

ericcaspole commented Aug 19, 2022 • 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 Aug 19, 2022

Uh oh!

openjdk bot commented Aug 19, 2022

Uh oh!

mlbridge bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

schlosna Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

ericcaspole Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

schlosna Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

ericcaspole Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

seanjmullan Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

ericcaspole Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 31, 2022

Uh oh!

ericcaspole commented Aug 31, 2022

Uh oh!

openjdk bot commented Aug 31, 2022

Uh oh!

openjdk bot commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ericcaspole commented Aug 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 19, 2022 •

edited

Loading