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

[Manuel Hässig]

On Fri, 22 Aug 2025 06:31:11 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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
>
> 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?

It is allowed, nothing happens. However, I still added a short circuit and a test.

> 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 ;)

Added a test.

> 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.

I moved it down for the sake of defensiveness.

> 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 ;)

In general, creating new collections in streams is a necessary evil. But I rewrote the cross product as a reduction with an ArrayList, which should be a bit more performant.

> 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?

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2293170188
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2293168880
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2293168286
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2293172614
PR Review Comment: https://git.openjdk.org/jdk/pull/26762#discussion_r2293173254