8365262: [IR-Framework] Add simple way to add cross-product of flags #26762

mhaessig · 2025-08-13T14:38:01Z

This PR adds the TestFramework::addCrossProductScenarios method to enable more ergonomic testing of the combination of all flag combinations. To illustrate its use, I also converted one test to use the new cross product functionality.

Testing:

Github Actions
tier1,tier2 plus some internal testing on Oracle supported platforms

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-8365262: [IR-Framework] Add simple way to add cross-product of flags (Enhancement - P4)

Reviewers

Benoît Maillard (@benoitmaillard - Author) 🔄 Re-review required (review applies to 0bd8c6a7)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26762

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-13T14:38:58Z

👋 Welcome back mhaessig! 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-08-13T14:40:34Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-08-13T14:41:31Z

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

hotspot-compiler

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 · 2025-08-13T14:45:01Z

Webrevs

galderz

Thanks @mhaessig. Nice API improvement! I'm a bit unsure about the way it's tested though.

galderz · 2025-08-14T12:41:46Z

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java

+                                               "-XX:TLABRefillWasteFraction=53",
+                                               "-XX:TLABRefillWasteFraction=64"));
+            t1.start();
+            Asserts.fail("Should have thrown exception");


Hmmm, why do the tests fail? I'm wondering if a simpler way to test the functionality is possible that doesn't require having to figure out failure modes? Maybe some kind of positive test that counts number of test scenarios run?

Except in the first run, all scenarios fail. That is the only way we currently have to count the scenarios we are executing.

benoitmaillard

Looks good to me, I only have one minor suggestion.

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java

Co-authored-by: Benoît Maillard <[email protected]>

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java

mhaessig · 2025-08-20T16:57:36Z

Thank you for looking at this @eme64. I made the testing a bit more robust and added a case for a pair of arguments.

openjdk · 2025-08-20T16:58:20Z

⚠️ @mhaessig This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java

mhaessig · 2025-08-21T16:27:13Z

@eme64, I completely revamped counting of failures to regex matching and made multiple and no flags in one string work.

eme64

Looks much better already! I now took a closer look at the implementation :)

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java

mhaessig · 2025-08-22T09:33:22Z

Thank you for your detailed review, @eme64. I addressed all of your comments.

eme64 · 2025-08-22T14:05:20Z

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java

+        Stream<List<String>> crossProduct = Arrays.stream(flagSets)
+            .reduce(
+                Stream.of(Collections.<String>emptyList()),
+                (acc, set) ->
+                    acc.flatMap(list ->
+                        set.stream().map(element -> {
+                            List<String> newList = new ArrayList<>(list);
+                            newList.add(element);
+                            return newList;
+                        })
+                    ),
+                (a, b) -> Stream.concat(a, b));


Wow, that's dense. Maybe a little comment could help here.

It ended up being a lottle comment, but I think it does the job.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java

mhaessig added 2 commits August 13, 2025 16:34

Add TestFramework::addCrossProductScenarios

8d564da

Convert test to cross product scenarios

57b3afe

openjdk bot added hotspot-compiler [email protected] rfr Pull request is ready for review labels Aug 13, 2025

galderz suggested changes Aug 14, 2025

View reviewed changes

benoitmaillard reviewed Aug 18, 2025

View reviewed changes

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java Outdated Show resolved Hide resolved

Apply Benoît's suggestion

0bd8c6a

Co-authored-by: Benoît Maillard <[email protected]>

benoitmaillard approved these changes Aug 18, 2025

View reviewed changes

eme64 reviewed Aug 20, 2025

View reviewed changes

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java Show resolved Hide resolved

eme64 reviewed Aug 20, 2025

View reviewed changes

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java Outdated Show resolved Hide resolved

mhaessig added 2 commits August 20, 2025 18:55

Better testing

ed8a9be

Merge branch 'JDK-8365262' of github.com:mhaessig/jdk into JDK-8365262

9cf0f2a

Make the test work

f59e9d9

eme64 suggested changes Aug 21, 2025

View reviewed changes

mhaessig marked this pull request as draft August 21, 2025 08:59

openjdk bot removed the rfr Pull request is ready for review label Aug 21, 2025

mhaessig added 3 commits August 21, 2025 18:04

post processing of flags and documentation

328a00d

Better counting in tests

44556e5

Fix test

273e5f6

mhaessig marked this pull request as ready for review August 21, 2025 16:27

openjdk bot added the rfr Pull request is ready for review label Aug 21, 2025

mhaessig requested a review from eme64 August 21, 2025 16:35

eme64 reviewed Aug 22, 2025

View reviewed changes

Improvements prompted by Emanuel

7bab775

eme64 suggested changes Aug 22, 2025

View reviewed changes

mhaessig added 5 commits August 22, 2025 19:19

Fix copy pasta mistakes

97242a0

Improve comments

4263d61

Fix indentation

f5723e0

Remove excess newline

2e36929

Merge branch 'master' into JDK-8365262

771924f

8365262: [IR-Framework] Add simple way to add cross-product of flags #26762

Are you sure you want to change the base?

8365262: [IR-Framework] Add simple way to add cross-product of flags #26762

Conversation

mhaessig commented Aug 13, 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 Aug 13, 2025

Uh oh!

openjdk bot commented Aug 13, 2025

Uh oh!

openjdk bot commented Aug 13, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

galderz left a comment

Choose a reason for hiding this comment

Uh oh!

galderz Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

mhaessig Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

benoitmaillard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhaessig commented Aug 20, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhaessig commented Aug 21, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhaessig commented Aug 22, 2025

Uh oh!

eme64 Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

mhaessig Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhaessig commented Aug 13, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 13, 2025 •

edited

Loading