RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v2]
Christian Hagedorn
chagedorn at openjdk.java.net
Mon Apr 19 12:49:46 UTC 2021
On Fri, 16 Apr 2021 17:41:08 GMT, Igor Ignatyev <iignatyev at openjdk.org> wrote:
>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Adjust whitelist
>
> 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.
The problem with having a separate class for it is that we would need to create a new instance somehow in a test and store it in a field to access it in subsequent iterations of a test. On the other hand, one could argue that we do not really need to guarantee that the first invocation of `toggleBoolean()` always returns `false` as the common use-case of this method is just to execute some if-else paths (roughly) equally often (regardless with which one we start) . Then we could also just move the method to `TestFramework::toggleBoolean()` with a static boolean field. We are always only executing a single test at a time, so `toggleBoolean()` reliably returns a different boolean each time it is called in a single test. What do you think?
> 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.
I think you're right. I first had the linked hash map to keep some sort of ordering for the reported IR rule failures. But in the current version, the compilations would only be sorted by type of test which probably does not add much benefit. I changed it back to `HashMap`.
> 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?
Yes it should. Would this be equivalent to `Paths.get("")`? It seems to work like that.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 107:
>
>> 105: */
>> 106: public static final Set<String> JTREG_WHITELIST_FLAGS = new HashSet<>(
>> 107: Arrays.asList(
>
> it doesn't seem to be a comprehensive list of flags that don't affect IR verification work, e.g. different GCs. I think it might be easy to go with the blacklist instead, and possibly warning people if there are any flags in `test.*.opts`.
Maybe first some general background about having a whitelist/blacklist in the first place. When a user writes an `@IR` rule, we do not want him to think about any possible VM flag combination (which he is not specifically using in his test, for example with `addFlags`) that could break his `@IR` rule and then requiring him to restrict the `@IR` rule with `IR::applyIfXX()` for these flags.
I agree that the whitelist is probably not complete and could be improved (but I think we should not whitelist different GCs as these affect the barrier code of the IR). About whitelist vs. blacklist, I had discussions about it with @TobiHartmann. We eventually decided to use a whitelist which seemed easier to maintain and is probably a lot shorter than a blacklist. In addition, if a new flag is introduced or removed, we most likely do not need to worry about updating the whitelist as the (currently) whitelisted flags will most likely remain unchanged. This first whitelist version is also just a best guess solution. We might miss on some other flags that will not affect the IR but we have the confidence that we do not run into surprising failures due to forgetting to blacklist a particular flag. But this is open for discussion of course. It would be interesting to hear how people think about it.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 146:
>
>> 144: private List<Class<?>> helperClasses = null;
>> 145: private List<Scenario> scenarios = null;
>> 146: private final List<String> flags = new ArrayList<>();
>
> why did you decide to eagerly create list for `flags`, but not for `scenarios` / `helpersClasses`?
Good catch. I adapted it to also create it lazily.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 166:
>
>> 164: if (VERBOSE) {
>> 165: System.out.println("Test class: " + testClass);
>> 166: }
>
> this c-tor can be replaced by:
> Suggestion:
>
> this(StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE).getCallerClass());
>
>
> and actually, I don't see it being used (outside of the tests for this testlibrary) and I don't think it will ever be used by actual tests that would use this framework, so I think we can safely remove it.
I think we should keep it for the cases when a user wants a more specific setup for his tests in the calling test class. For example, if he wants to define a new default warm-up for all of his tests (using `setDefaultWarmup`) or if he needs to combine different options for which there is no `runXX` method (helper classes, scenarios, and/or flags). In this case, he needs to create a `TestFramework()` builder and then call the different builder methods. We could argue that a single constructor is enough in which the test class needs to be specified. But I think most of the time, the user wants to test the calling class for which this additional constructor is quite useful.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 211:
>
>> 209: }
>> 210:
>> 211: /**
>
> I'm not sure how much of the benefits all these different `run.*` bring. I fully understand the desire to simplify the most common use-case (i.e. no-arg `run`), but for the rest I'd assume the users will be capable of writing, for example, `new TestFramework(testClass).addScenarios(scenarios).start();` instead of `TestFramework .runWithScenarios(testClass, scenarios);`
I think you are probably right. I added this builder approach later during the development of the framework. It might be better to keep the `TestFramework` interface simple(r). However, I will wait for others to comment on that before changing/removing all of these.
If we're gonna simplify it, I suggest to keep `run()` and maybe also `runWithFlags()` as (probably?) most common use-case.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 269:
>
>> 267: * <ul>
>> 268: * <li><p>If a helper class is not in the same file as the test class, make sure that JTreg compiles it by using
>> 269: * {@literal @}compile in the JTreg header comment block.</li>
>
> you don't need `@compile` to compile a helper class, 1st of all, you shouldn't use `@compile` when you want to compile a class in your test, you should use `@build`, 2nd, in this particular case, the class will be automatically built as part of your test b/c jtreg (or rather javac) will be able to statically detect it as a dependency of the code that calls `runWithHelperClasses(Class, Class...)`
Okay, thanks for clearing that up! I removed that comment and related ones.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 407:
>
>> 405: start(null);
>> 406: } catch (TestVMException e) {
>> 407: System.err.println("\n" + e.getExceptionInfo());
>
> you shouldn't use "\n", as it might not be the right line-separator. you can either do:
> Suggestion:
>
> System.err.println();
> System.err.println(e.getExceptionInfo());
>
> or
> Suggestion:
>
> System.err.printf("%n%s%n", e.getExceptionInfo());
I will address all `\n` in a separate commit as there are a lot of them. Could I also just use `System.lineSeparator()` anywhere where I'm currently using `\n` like `System.err.println(System.lineSeparator() + e.getExceptionInfo()`)? Might be easier when I'm using `\n` with a `StringBuilder`, for example.
> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 674:
>
>> 672: + "Compilation(s) of failed matche(s):");
>> 673: System.out.println(irException.getCompilations());
>> 674: builder.append(errorMsg).append("\n").append(irException.getExceptionInfo()).append(e.getMessage());
>
> as `builder.toString` will be printed out to cout/cerr, you should use platform-specific line-separator instead of `\n`
See earlier comment.
> 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?
I guess it's not necessary. Removed it.
> 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)
I wanted to distinguish the following cases (but I'm open to use a different approach):
- `TestFormat::check` (using `TestFormatException`): The format of any test/helper class does not conform with the format the framework is expecting. The user needs to fix this (for example, using a wrong annotation). I report these kind of failures before actually running any of the tests [here](https://github.com/openjdk/jdk/blob/7ed789dc98691d7c7fc2295e045a9f54b9fa6277/test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java#L265).
- `TestRun::check` (using `TestRunException`): Thrown while the framework is [executing a test](https://github.com/openjdk/jdk/blob/7ed789dc98691d7c7fc2295e045a9f54b9fa6277/test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java#L799), there is nothing wrong with the format of a test.
- `TestFramework::check` (using `TestFrameworkException`): If such a check fails there is a bug in the framework itself (like internal framework assertions). The user cannot really do anything about it without fixing the framework itself.
I have to double check again if I'm using the right kind of exception everywhere.
> 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?
Fixed.
> 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.
+1 for `AutoClosable`. I wanted to avoid passing the socket object to all the different methods and thus tried to use the `socket` field like that. I agree that this not nice. Is delegating the socket object from `TestFramework::start` to the different methods the only solution or are there any other recommended ways?
-------------
PR: https://git.openjdk.java.net/jdk/pull/3508
More information about the hotspot-compiler-dev
mailing list