8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm #15986

lkorinth · 2023-09-29T13:44:26Z

Minor testing done, I will test more later with other fixes.

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-8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm (Enhancement - P4)

Reviewers

Stefan Johansson (@kstefanj - Reviewer) ⚠️ Review applies to e0dde6f1
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15986

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-29T13:46:36Z

👋 Welcome back lkorinth! 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 · 2023-09-29T13:57:08Z

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

hotspot-gc

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 · 2023-09-29T14:39:20Z

Webrevs

lkorinth · 2023-10-09T08:51:30Z

I have tested this with tier1-5 on x86.

lkorinth · 2023-10-13T08:44:41Z

Added the @key flag-sensitive after conversation with @kstefanj

kstefanj

Some small comments but I'm ok with the change as is.

I'm not sure how we should handle additional flags we know fail, but aren't part of the CI testing. For example: G1HeapRegionSize=8m or larger is not good for TestG1HeapSizeFlags.java. So feel free to add this if you think that is the correct way to go.

kstefanj · 2023-10-13T10:01:50Z

test/hotspot/jtreg/gc/arguments/TestG1HeapSizeFlags.java

@@ -26,8 +26,9 @@
 /*
 * @test TestG1HeapSizeFlags
 * @bug 8006088
- * @requires vm.gc.G1
+ * @requires vm.gc.G1 & vm.opt.x.Xmx == null & vm.opt.x.Xms == null & vm.opt.MinHeapSize == null & vm.opt.MaxHeapSize == null & vm.opt.InitialHeapSize == null


Please move the requires below the @key to be consistent with the other tests.

kstefanj · 2023-10-13T10:05:31Z

test/jtreg-ext/requires/VMProps.java

@@ -135,6 +140,7 @@ public Map<String, String> call() {
        map.put("jdk.containerized", this::jdkContainerized);
        map.put("vm.flagless", this::isFlagless);
        map.put("jdk.foreign.linker", this::jdkForeignLinker);
+        map.putAll(xOptFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )


Does it have to be vm.opt.x.* or could we just add those to vm.opt and use for example: vm.opt.Xmx == null? Or do we have -X and -XX: options that collide?

I know of no colliding options ATM. They could be colliding in the future (especially if we chose to parse extra options not starting with a -X). Also, I think it is incredibly confusing to reuse the vm.opt namespace that is used by vanilla JTREG by our extension code in VMProps. I would object to put both in the vm.opt namespace.

I agree it is a bit ugly and the point of this being an extension is good. An alternative would be to use vm.xopt.Xmx to even less clobber the vm.opt namespace, but I guess we already do by adding the vm.opt.final details.

I'm ok with the current way, just wanted to raise possible alternatives.

vm.xopt was my first choice, but it works badly because of other reasons, so lets keep it as it is now.

The reason is that we need to use a namespace that starts with vm.opt, because that string is special in JTREG. If the property starts with that prefix, there is no need to register all possible values manually --- something that would be hard to maintain. However, reusing parts of namespaces have prior art in for example vm.gc.* so although maybe not optimal, it should not be problematic. It follows standard practice.

kstefanj · 2023-10-13T10:36:33Z

test/jtreg-ext/requires/VMProps.java

+    private Map<String, String> xOptFlags() {
+        return allFlags()
+            .filter(s -> s.startsWith("-X") && !s.startsWith("-XX:") && !s.equals("-X"))
+            .map(s -> s.replaceFirst("-*", ""))


Is there a need to replace "-*", should you be able to just do:

Suggested change

.map(s -> s.replaceFirst("-*", ""))

.map(s -> s.replaceFirst("-", ""))

I will remove it, it is needed if I want to parse extra options starting with two dashes, but I chose to keep it simple and just parse extra options starting with -X.

openjdk · 2023-10-13T11:17:43Z

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

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm

Reviewed-by: sjohanss, ayang

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

5521560: 8310673: [JVMCI] batch compilation for libgraal should work the same way as for C2
e4329a8: 8317696: Fix compilation with clang-16
504b0bd: 8318078: ADLC: pass ASSERT and PRODUCT flags
7b39172: 8318086: [jvmci] RISC-V: Reuse target config from TargetDescription
aed1e81: 8317285: Misspellings in hprof test lib
741ae06: 8313613: Use JUnit in langtools/lib tests
8c1bb2b: 8268916: Tests for AffirmTrust roots
2d38495: 8316309: AArch64: VMError::print_native_stack() crashes on Java native method frame
0d09168: 8312527: (ch) Re-examine use of sun.nio.ch.Invoker.myGroupAndInvokeCount
1a7fd5d: 8317687: (fs) FileStore.supportsFileAttributeView("posix") incorrectly returns 'true' for FAT32 volume on macOS
... and 271 more: https://git.openjdk.org/jdk/compare/c24c66db97a52371875a63862f85ea5c2010d5a7...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.

kstefanj · 2023-10-16T09:50:59Z

test/hotspot/jtreg/TEST.ROOT

@@ -32,7 +32,7 @@
 # intermittent:         flaky test, known to fail intermittently
 # randomness:           test uses randomness, test cases differ from run to run
 # cgroups:              test uses cgroups
-keys=stress headful intermittent randomness cgroups
+keys=stress headful intermittent randomness cgroups flag-sensitive


Maybe add a comment above about the key, similar to the others. Something like:

# flag-sensitive: test is sensitive to certain flags and might fail when flags are passed in

Thanks, I reworded it just slightly

…en flags are passed using -vmoptions and -javaoptions

lkorinth · 2023-10-17T09:21:31Z

Thanks Stefan and Albert!!!

/integrate

openjdk · 2023-10-17T09:22:05Z

Going to push as commit 7ca0ae9.
Since your change was applied there have been 281 commits pushed to the master branch:

5521560: 8310673: [JVMCI] batch compilation for libgraal should work the same way as for C2
e4329a8: 8317696: Fix compilation with clang-16
504b0bd: 8318078: ADLC: pass ASSERT and PRODUCT flags
7b39172: 8318086: [jvmci] RISC-V: Reuse target config from TargetDescription
aed1e81: 8317285: Misspellings in hprof test lib
741ae06: 8313613: Use JUnit in langtools/lib tests
8c1bb2b: 8268916: Tests for AffirmTrust roots
2d38495: 8316309: AArch64: VMError::print_native_stack() crashes on Java native method frame
0d09168: 8312527: (ch) Re-examine use of sun.nio.ch.Invoker.myGroupAndInvokeCount
1a7fd5d: 8317687: (fs) FileStore.supportsFileAttributeView("posix") incorrectly returns 'true' for FAT32 volume on macOS
... and 271 more: https://git.openjdk.org/jdk/compare/c24c66db97a52371875a63862f85ea5c2010d5a7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-17T09:22:12Z

@lkorinth Pushed as commit 7ca0ae9.

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

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm

c85a18b

openjdk bot added the rfr Pull request is ready for review label Sep 29, 2023

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Sep 29, 2023

add @key flag-sensitive

e0dde6f

kstefanj approved these changes Oct 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 13, 2023

lkorinth added 2 commits October 13, 2023 15:30

fix improvments requested by Stefan

f0aaa67

add flag-sensitive key to TEST.ROOT

a5b8791

kstefanj reviewed Oct 16, 2023

View reviewed changes

add explanation: test is sensitive to certain flags and might fail wh…

8491a67

…en flags are passed using -vmoptions and -javaoptions

albertnetymk approved these changes Oct 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 17, 2023

openjdk bot closed this Oct 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 17, 2023

dholmes-ora mentioned this pull request Nov 27, 2023

8320750: Allow a testcase to run with multiple -Xlog #16824

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm #15986

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm #15986

lkorinth commented Sep 29, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 29, 2023

openjdk bot commented Sep 29, 2023

mlbridge bot commented Sep 29, 2023 •

edited

Loading

lkorinth commented Oct 9, 2023

lkorinth commented Oct 13, 2023

kstefanj left a comment

kstefanj Oct 13, 2023

lkorinth Oct 13, 2023

kstefanj Oct 13, 2023

lkorinth Oct 13, 2023

kstefanj Oct 13, 2023

lkorinth Oct 13, 2023

lkorinth Oct 13, 2023

kstefanj Oct 13, 2023

lkorinth Oct 13, 2023

openjdk bot commented Oct 13, 2023 •

edited

Loading

kstefanj Oct 16, 2023

lkorinth Oct 16, 2023

lkorinth commented Oct 17, 2023

openjdk bot commented Oct 17, 2023

openjdk bot commented Oct 17, 2023

	.map(s -> s.replaceFirst("-*", ""))
	.map(s -> s.replaceFirst("-", ""))

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm #15986

8317228: GC: Make TestXXXHeapSizeFlags use createTestJvm #15986

Conversation

lkorinth commented Sep 29, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 29, 2023

openjdk bot commented Sep 29, 2023

mlbridge bot commented Sep 29, 2023 • edited Loading

Webrevs

lkorinth commented Oct 9, 2023

lkorinth commented Oct 13, 2023

kstefanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkorinth commented Oct 17, 2023

openjdk bot commented Oct 17, 2023

openjdk bot commented Oct 17, 2023

lkorinth commented Sep 29, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 29, 2023 •

edited

Loading

openjdk bot commented Oct 13, 2023 •

edited

Loading