RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v2]

Igor Ignatyev iignatyev at openjdk.java.net
Fri Apr 16 18:27:39 UTC 2021 

On Thu, 15 Apr 2021 13:19:56 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This RFE provides an IR test framework to perform regex-based checks on the C2 IR shape of test methods emitted by the VM flags `-XX:+PrintIdeal` and `-XX:+PrintOptoAssembly`. The framework can also be used for other non-IR matching (and non-compiler) tests by providing easy to use annotations for commonly used testing patterns and compiler control flags.
>> 
>> The framework is based on the ideas of the currently present IR test framework in [Valhalla](https://github.com/openjdk/valhalla/blob/e9c78ce4fcfd01361c35883e0d68f9ae5a80d079/test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypeTest.java) (mainly implemented by @TobiHartmann) which is being used with great success. This new framework aims to replace the old one in Valhalla at some point.
>> 
>> A detailed description about how this new IR test framework works and how it is used is provided in the [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) file and in the [Javadocs](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc/jdk/test/lib/hotspot/ir_framework/package-summary.html) written for the framework classes.
>> 
>> To finish a first version of this framework for JDK 17, I decided to leave some improvement possibilities and ideas to be followed up on in additional RFEs. Some ideas are mentioned in "Future Work" in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md) and were also created as subtasks of this RFE.
>> 
>> Testing (also described in "Internal Framework Tests in [README.md](https://github.com/chhagedorn/jdk/blob/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/README.md)):
>> There are various tests to verify the correctness of the test framework which can be found as JTreg tests in the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests) folder. Additional testing was performed by converting all compiler Inline Types test of project Valhalla (done by @katyapav in [JDK-8263024](https://bugs.openjdk.java.net/browse/JDK-8263024)) that used the old framework to the new framework. This provided additional testing for the framework itself. We ran the converted tests with all the flag settings used in hs-tier1-9 and hs-precheckin-comp. For sanity checking, this was also done with a sample IR test in mainline.
>> 
>> Some stats about the framework code added to [ir_framework](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework):
>> 
>> -  without the [Javadocs files](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/doc) : 60 changed files, 13212 insertions, 0 deletions.
>>    - without the [tests](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/tests)  and [examples](https://github.com/chhagedorn/jdk/tree/aa005f384a4567c6c0b5f08f7c5df57f705dc540/test/lib/jdk/test/lib/hotspot/ir_framework/examples) folder: 40 files changed, 6781 insertions
>>       - comments: 2399 insertions (calculated with `git diff --cached !(tests|examples) | grep -c -E "(^[+-]\s*(/)?*)|(^[+-]\s*//)"`)
>>       - which leaves 4382 lines of code inserted 
>> 
>> Big thanks to:
>> - @TobiHartmann for all his help by discussing the new framework and for providing insights from his IR test framework in Valhalla.
>> - @katyapav for converting the Valhalla tests to use the new framework which found some harder to catch bugs in the framework and also some actual C2 bugs.
>> - @iignatev for helping to simplify the framework usage with JTreg and with the framework internal VM calling structure.
>> - and others who provided valuable feedback.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adjust whitelist

next portion of my comments, stay tuned for more to come :)

-- Igor

PS I must say that by some reason github's 'file changed' tab is unbelievably slow for me in this particular PR, so it's somewhat painful to even just find the place I want to associate my comment with (not to speak about actually using it to browse/read the code)

test/lib/jdk/test/lib/hotspot/ir_framework/AbstractInfo.java line 41:

> 39:  */
> 40: abstract public class AbstractInfo {
> 41:     private static final Random random = new Random();

you shouldn't use Random w/o predefined seed as it might make it harder to reproduce, please consider using `jdk.test.lib.Utils.getRandomInstance` or `jdk.test.lib.RandomFactory.getRandom` here

test/lib/jdk/test/lib/hotspot/ir_framework/AbstractInfo.java line 57:

> 55:      * @return an inverted boolean of the result of the last invocation of this method.
> 56:      */
> 57:     public boolean toggleBoolean() {

I don't think `toggleBoolean` really belongs to `AbstractInfo`, I'd rather move it into a (separate) aux class.

test/lib/jdk/test/lib/hotspot/ir_framework/ArgumentValue.java line 35:

> 33:  */
> 34: class ArgumentValue {
> 35:     private static final Random random = new Random();

the same comment as in `AbstractInfo`, please use the reproducible rng here.

test/lib/jdk/test/lib/hotspot/ir_framework/IREncodingPrinter.java line 213:

> 211: 
> 212:     private boolean checkBooleanFlag(String flag, String value, Boolean actualFlagValue) {
> 213:         boolean actualBooleanFlagValue = actualFlagValue;

as `actualFlagValue` can't be null, you don't need to use box here: 
Suggestion:

    private boolean checkBooleanFlag(String flag, String value, boolean actualBooleanFlagValue) {


the same for `checkLongFlag`, `checkDoubleFlag`

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 47:

> 45: 
> 46:     public IRMatcher(String hotspotPidFileName, String irEncoding, Class<?> testClass) {
> 47:         this.compilations =  new LinkedHashMap<>();

why do we use LinkedHashMap here (as opposed to HashMap)? I haven't found the place where you need to traverse it in the insertion order.

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 88:

> 86:         Map<String, Integer[]> irRulesMap = new HashMap<>();
> 87:         String patternString = "(?<=" + IREncodingPrinter.START + "\\R)[\\s\\S]*(?=" + IREncodingPrinter.END + ")";
> 88:         Pattern pattern = Pattern.compile(patternString);

`patternString` is effectively a constant here, but you will compile it into `j.u.Pattern` on each invocation of `parseIREncoding`, it might be a better idea to introduce a static field that holds a precompiled patter.

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 98:

> 96:             String[] splitComma = line.split(",");
> 97:             if (splitComma.length < 2) {
> 98:                 throw new TestFrameworkException("Invalid IR match rule encoding");

will it make sense to include the line-offender into the exception message here?

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 101:

> 99:             }
> 100:             String testName = splitComma[0];
> 101:             Integer[] irRulesIdx = new Integer[splitComma.length - 1];

you can actually use an array of int here, so there will be less wasted memory and no unboxing later on

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 116:

> 114:     private void parseHotspotPidFile() {
> 115:         Map<Integer, String> compileIdMap = new HashMap<>();
> 116:         try (BufferedReader br = new BufferedReader(new FileReader(new File(System.getProperty("user.dir") + File.separator + hotspotPidFileName)))) {

more idiomatic/modern version would be
Suggestion:

try (BufferedReader br = Files.newBufferedReader(Paths.get(System.getProperty("user.dir"), hotspotPidFileName))) {


I actually not sure if you really need `$user.dir`, won't hotspot_pid file get generated in the current dir?

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 207:

> 205:     private void addTestMethodCompileId(Map<Integer, String> compileIdMap, String line) {
> 206:         Pattern pattern = Pattern.compile("compile_id='(\\d+)'.*" + Pattern.quote(testClass.getCanonicalName()) + " (\\S+)");
> 207:         Matcher matcher = pattern.matcher(line);

similarly to `parseIREncoding`, `pattern` here can be precomputed in `IRMatcher::IRMatcher` and stored into a final instance field.

test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 243:

> 241:     private String getMethodName(Map<Integer, String> compileIdMap, String line) {
> 242:         Pattern pattern = Pattern.compile("compile_id='(\\d+)'");
> 243:         Matcher matcher = pattern.matcher(line);

again, precompiled pattern can be saved into a (in this case) static field and reused.

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 639:

> 637:             TestFormat.check(!scenarioIndices.contains(scenarioIndex),
> 638:                              "Cannot define two scenarios with the same index " + scenarioIndex);
> 639:             scenarioIndices.add(scenarioIndex);

you can use `Set::add` to verify that the element isn't in a set:
Suggestion:

            TestFormat.check(scenarioIndices.add(scenarioIndex),
                             "Cannot define two scenarios with the same index " + scenarioIndex);

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 795:

> 793:     private void checkFlagVMExitCode(OutputAnalyzer oa) {
> 794:         String flagVMOutput = oa.getOutput();
> 795:         final int exitCode = oa.getExitValue();

nit: there is no need for this `final`

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 804:

> 802:             System.err.println("--- OUTPUT TestFramework flag VM ---");
> 803:             System.err.println(flagVMOutput);
> 804:             throw new RuntimeException("\nTestFramework flag VM exited with " + exitCode);

what's the reason for `\n` in the begging of this exception message?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 958:

> 956:         throw new TestFrameworkException("Internal Test Framework exception - please file a bug:\n" + failureMessage, e);
> 957:     }
> 958: }

I am not convinced that we really these guys when we already have `TestFormat::check` and `TestRun::check` (I'm actually not 100% convinced that we need check/fail in both TestFormat and TestRun)

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 1027:

> 1025:     }
> 1026: 
> 1027:     public static String getRerunHint() {

why is this a method and not just a public final static String?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 1043:

> 1041:  * Dedicated socket to send data from the flag and test VM back to the driver VM.
> 1042:  */
> 1043: class TestFrameworkSocket {

could you please move it to a separate .java file?

test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 1043:

> 1041:  * Dedicated socket to send data from the flag and test VM back to the driver VM.
> 1042:  */
> 1043: class TestFrameworkSocket {

I guess it should implement AutoClosable, and then you try-catch-finally in `TestFramework::start` could be replaced by `try-w-resource`. 

btw, I don't the fact that `socket` is a field of `TestFramework` with the lifetime bounded to `start` method.

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

Changes requested by iignatyev (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3508

More information about the hotspot-dev mailing list