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

[Emanuel Peter]

On Thu, 21 Aug 2025 16:33:13 GMT, Manuel Hässig <mhaessig at openjdk.org> wrote:

>> 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:
>>  - [x] Github Actions
>>  - [x] tier1,tier2 plus some internal testing on Oracle supported platforms
>
> Manuel Hässig has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Fix test
>  - Better counting in tests
>  - post processing of flags and documentation

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

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 363:

> 361:     final public TestFramework addCrossProductScenarios(Set<String>... flagSets) {
> 362:         TestFormat.checkAndReport(flagSets != null && Arrays.stream(flagSets).noneMatch(Objects::isNull),
> 363:                 "Flags must not be null");

Suggestion:

        TestFormat.checkAndReport(flagSets != null && Arrays.stream(flagSets).noneMatch(Objects::isNull),
                                  "Flags must not be null");

Optional: indentation of args

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 363:

> 361:     final public TestFramework addCrossProductScenarios(Set<String>... flagSets) {
> 362:         TestFormat.checkAndReport(flagSets != null && Arrays.stream(flagSets).noneMatch(Objects::isNull),
> 363:                 "Flags must not be null");

What about an empty `flagSets`? Is it allowed? Do we have a test for it?

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 367:

> 365:         if (this.scenarioIndices != null && !this.scenarioIndices.isEmpty()) {
> 366:             initIdx = this.scenarioIndices.stream().max(Comparator.comparingInt(Integer::intValue)).get() + 1;
> 367:         }

Nit: you are writing code here that allows previous scenarios, but there is no example / test for that below ;)

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 374:

> 372:                         idx.getAndIncrement(),
> 373:                         flags.stream() // Process flags
> 374:                                 .filter(s -> !s.isEmpty()) // Remove empty flags

Why do you need that? Ah, these are empty strings, right?
Suggestion:

                                .filter(s -> !s.isEmpty()) // Remove empty string flags

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 375:

> 373:                         flags.stream() // Process flags
> 374:                                 .filter(s -> !s.isEmpty()) // Remove empty flags
> 375:                                 .map(s -> Set.of(s.split("[ ]"))) // Split muliple flags in the same string into separate strings

What happens if I enter `"flag_one  flag_two"` with two spaces in the middle? Do I then get an empty string in the middle again? If so: move the empty string filter down.

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 387:

> 385:         if (idx == sets.length) {
> 386:             Set<String> empty = Set.of();
> 387:             return Set.of(empty).stream();

Suggestion:

            return Stream.of(Set.of());

Would this work? Or at least this:
Suggestion:

            Set<String> empty = Set.of();
            return Stream.of(empty);

test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java line 394:

> 392:                             Set<String> newSet = new HashSet<>(set);
> 393:                             newSet.add(setElement);
> 394:                             return newSet;

Not super performant, as it creates a new HashSet at every turn... but oh well we are not making this public anyway ;)

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java line 96:

> 94:             TestFramework t5 = new TestFramework();
> 95:             t5.addCrossProductScenarios(Set.of("", "-XX:TLABRefillWasteFraction=51", "-XX:TLABRefillWasteFraction=53"),
> 96:                                         Set.of("-XX:+UseNewCode", "-XX:+UseNewCode2"));

Now looking at the implementation of `addCrossProductScenarios`: what does it do when it is called without arguments/empty args array? Can you also add a test for that?

-------------

PR Review: https://git.openjdk.org/jdk/pull/26762#pullrequestreview-3143243970
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292817705
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292827988
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292869974
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292855516
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292857113
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292836362
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292852276
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2292825338