8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests #3508
Conversation
…in JTreg compiler tests
|
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
|
@chhagedorn The following label will be automatically applied to this pull request:
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. |
|
/label remove javadoc |
|
@chhagedorn |
|
/label add hotspot-compiler |
|
@chhagedorn |
|
@chhagedorn |
|
/contributor add @TobiHartmann |
|
@chhagedorn |
Webrevs
|
|
Hi Christian, kudos to you, Tobias, Katya, and all involved, I have really high hopes that this framework will improve the quality of both JVM and our testing. I'll look at the code later, a few general comments:
Thanks, |
| * <li><p>{@link DontCompile @DontCompile}: Prevents any compilation of the associated helper method.</li> | ||
| * </ul> | ||
| */ | ||
| ANY(-2), |
There was a problem hiding this comment.
This will change (-2 -> -1) after AOT is removed (8264805).
There was a problem hiding this comment.
I'll change that as soon as it is integrated.
There was a problem hiding this comment.
should we also add a comment suggesting to keep in sync w/ compiler/compilerDefinitions.hpp?
| * </ul> | ||
| */ | ||
| ANY(-2), | ||
| /** |
There was a problem hiding this comment.
For completeness may be add value NONE(0) for Interpreter only case.
There was a problem hiding this comment.
I agree that it makes sense for completeness. But I'm not sure if this is useful to have as NONE would not be supported by the framework for the time being and would just require additional handling, informing the user that this has no effect.
| /** | ||
| * Helper class to store information about a method that needs to be IR matched. | ||
| */ | ||
| class IRMethod { |
There was a problem hiding this comment.
Can you move this class IRMethod into a separate file? This file is already big.
| @@ -0,0 +1,3 @@ | |||
| # Minimal TEST.ROOT file to run the examples tests as if the examples would have been placed inside | |||
There was a problem hiding this comment.
Missing Copyright header.
| @@ -0,0 +1,12 @@ | |||
| # Minimal TEST.ROOT file to run the internal framework tests as if they would have been placed inside | |||
There was a problem hiding this comment.
Missing Copyright header.
| Additional testing should be performed with the converted Valhalla tests (see [JDK-8263024](https://bugs.openjdk.java.net/browse/JDK-8263024)) to make sure a changeset is correct (these are part of the Valhalla CI). | ||
|
|
There was a problem hiding this comment.
I don't think you should reference particular RFE which will be resolved eventually. The statement itself is fine.
| @@ -0,0 +1,160 @@ | |||
| <h1>IR Test Framework</h1> | |||
There was a problem hiding this comment.
Just curious: why HTML instead of markdown?
There was a problem hiding this comment.
There is also a README.md file in the same folder from which I generated this HTML file from. But as others have suggested, I will remove this html file again and only leave the markdown file.
there is an RFE to rename |
| StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); | ||
| this.testClass = walker.getCallerClass(); | ||
| if (VERBOSE) { | ||
| System.out.println("Test class: " + testClass); | ||
| } |
There was a problem hiding this comment.
this c-tor can be replaced by:
| StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); | |
| this.testClass = walker.getCallerClass(); | |
| if (VERBOSE) { | |
| System.out.println("Test class: " + testClass); | |
| } | |
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But I think most of the time, the user wants to test the calling class for which this additional constructor is quite useful.
I concur that in most cases the users would want to use the caller as a testClass, yet we already have run and friends to cover that (most common) use-case, users who would need to construct fancier TestFramework, most probably, won't be doing it in their testClass, and if they would, the overhead of writing new TestFramework(getClass()) is just 10 keystrokes and is neglatible (given the assumption of a more complicated use case)
There was a problem hiding this comment.
That is true. The entire test will probably look more complex in this case so it's probably reasonable to only provide the constructor with a class argument. However, if we are going to remove most of the (current) runXX() methods as discussed in a comment below, then keeping this parameterless constructor might be justified again? Anyhow, I'm fine with both. (Nit: when used from main() we would need to use MyTestClass.class instead of getClass())
| * the entries of this list as a substring (partial match). | ||
| */ | ||
| public static final Set<String> JTREG_WHITELIST_FLAGS = new HashSet<>( | ||
| Arrays.asList( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll need to think about it a bit more, but my first impression after reading your comment is that we don't actually want to allow any flags, so we might better off just using @requies vm.flagless or reusing the underlying code to check that we don't have any flags that can "significantly" change behavior.
There was a problem hiding this comment.
Our idea was to only restrict the IR verification done after the test VM terminates with the whitelist (or blacklist). But the framework should still run each test with any flag combination. This showed to be quite useful in Valhalla to find bugs.
| * <li><p>If a helper class is not in the same file as the test class, make sure that JTreg compiles it by using | ||
| * {@literal @}compile in the JTreg header comment block.</li> |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
Okay, thanks for clearing that up! I removed that comment and related ones.
| run(walker.getCallerClass()); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I suggest to keep run() and maybe also runWithFlags() as (probably?) most common use-case.
I guess you are right, run() and runWithFlags(String[]) will be most-commonly used ones.
There was a problem hiding this comment.
Sounds good. Will do these changes if others agree.
| private List<Class<?>> helperClasses = null; | ||
| private List<Scenario> scenarios = null; | ||
| private final List<String> flags = new ArrayList<>(); |
There was a problem hiding this comment.
why did you decide to eagerly create list for flags, but not for scenarios / helpersClasses?
There was a problem hiding this comment.
Good catch. I adapted it to also create it lazily.
| try { | ||
| start(null); | ||
| } catch (TestVMException e) { | ||
| System.err.println("\n" + e.getExceptionInfo()); |
There was a problem hiding this comment.
you shouldn't use "\n", as it might not be the right line-separator. you can either do:
| System.err.println("\n" + e.getExceptionInfo()); | |
| System.err.println(); | |
| System.err.println(e.getExceptionInfo()); |
or
| System.err.println("\n" + e.getExceptionInfo()); | |
| System.err.printf("%n%s%n", e.getExceptionInfo()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sure you case use System.lineSeparator(), it's just a matter of personal choice/style
| VERIFY_IR = hasIRAnnotations(); | ||
| if (!VERIFY_IR) { | ||
| System.out.println("IR verification disabled due to test " + testClass + " not specifying any @IR annotations"); | ||
| return; | ||
| } | ||
|
|
||
| VERIFY_IR = Platform.isDebugBuild() && !Platform.isInt() && !Platform.isComp(); | ||
| if (!VERIFY_IR) { | ||
| System.out.println("IR verification disabled due to not running a debug build (required for PrintIdeal" + | ||
| "and PrintOptoAssembly), running with -Xint, or -Xcomp (use warm-up of 0 instead)"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'd reorder them as "platform" checks are faster than hasIRAnnotations check and, I'd guess, will be a more common culprit to disable IR verification.
| /** | ||
| * Disable IR verification completely in certain cases. | ||
| */ | ||
| private void maybeDisableIRVerificationCompletely() { |
There was a problem hiding this comment.
nit:
| private void maybeDisableIRVerificationCompletely() { | |
| private void disableIRVerificationIfNotFeasible() { |
| if (e instanceof IRViolationException) { | ||
| IRViolationException irException = (IRViolationException) e; |
There was a problem hiding this comment.
nit:
| if (e instanceof IRViolationException) { | |
| IRViolationException irException = (IRViolationException) e; | |
| if (e instanceof IRViolationException irException) { |
| System.out.println((scenario != null ? "Scenario #" + scenario.getIndex() + " - " : "") | ||
| + "Compilation(s) of failed matche(s):"); | ||
| System.out.println(irException.getCompilations()); | ||
| builder.append(errorMsg).append("\n").append(irException.getExceptionInfo()).append(e.getMessage()); |
There was a problem hiding this comment.
as builder.toString will be printed out to cout/cerr, you should use platform-specific line-separator instead of \n
There was a problem hiding this comment.
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)
| TestFormat.check(!scenarioIndices.contains(scenarioIndex), | ||
| "Cannot define two scenarios with the same index " + scenarioIndex); | ||
| scenarioIndices.add(scenarioIndex); |
There was a problem hiding this comment.
you can use Set::add to verify that the element isn't in a set:
| TestFormat.check(!scenarioIndices.contains(scenarioIndex), | |
| "Cannot define two scenarios with the same index " + scenarioIndex); | |
| scenarioIndices.add(scenarioIndex); | |
| TestFormat.check(scenarioIndices.add(scenarioIndex), | |
| "Cannot define two scenarios with the same index " + scenarioIndex); |
There was a problem hiding this comment.
also, shouldn't this check be done as part of addScenarios?
There was a problem hiding this comment.
Right, I moved it to addScenarios.
| /** | ||
| * Dedicated socket to send data from the flag and test VM back to the driver VM. | ||
| */ | ||
| class TestFrameworkSocket { |
There was a problem hiding this comment.
could you please move it to a separate .java file?
| /** | ||
| * Dedicated socket to send data from the flag and test VM back to the driver VM. | ||
| */ | ||
| class TestFrameworkSocket { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+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?
| if (exitCode != 0) { | ||
| System.err.println("--- OUTPUT TestFramework flag VM ---"); | ||
| System.err.println(flagVMOutput); | ||
| throw new RuntimeException("\nTestFramework flag VM exited with " + exitCode); |
There was a problem hiding this comment.
what's the reason for \n in the begging of this exception message?
There was a problem hiding this comment.
I guess it's not necessary. Removed it.
There was a problem hiding this comment.
👍 . I noticed that you have leading \n in other exceptions, do you plan to remove it from there as well?
There was a problem hiding this comment.
Sometimes, I found the resulting format of the error message nicer with the additional new lines. For example the exception at L940:
JavaTest Message: Test threw exception: jdk.test.lib.hotspot.ir_framework.TestFormatException:
Violations (1)
--------------
- Cannot use explicit compile command annotations ...
vs. no new lines:
JavaTest Message: Test threw exception: jdk.test.lib.hotspot.ir_framework.TestFormatException: Violations (1)
--------------
- Cannot use explicit compile command annotations ...
I will check the other exceptions again if they really need the new lines or not.
|
|
||
| private void checkFlagVMExitCode(OutputAnalyzer oa) { | ||
| String flagVMOutput = oa.getOutput(); | ||
| final int exitCode = oa.getExitValue(); |
There was a problem hiding this comment.
nit: there is no need for this final
| Pattern pattern = Pattern.compile("compile_id='(\\d+)'.*" + Pattern.quote(testClass.getCanonicalName()) + " (\\S+)"); | ||
| Matcher matcher = pattern.matcher(line); |
There was a problem hiding this comment.
similarly to parseIREncoding, pattern here can be precomputed in IRMatcher::IRMatcher and stored into a final instance field.
| Pattern pattern = Pattern.compile("compile_id='(\\d+)'"); | ||
| Matcher matcher = pattern.matcher(line); |
There was a problem hiding this comment.
again, precompiled pattern can be saved into a (in this case) static field and reused.
| String line = lines[i].trim(); | ||
| String[] splitComma = line.split(","); | ||
| if (splitComma.length < 2) { | ||
| throw new TestFrameworkException("Invalid IR match rule encoding"); |
There was a problem hiding this comment.
will it make sense to include the line-offender into the exception message here?
| throw new TestFrameworkException("Invalid IR match rule encoding"); | ||
| } | ||
| String testName = splitComma[0]; | ||
| Integer[] irRulesIdx = new Integer[splitComma.length - 1]; |
There was a problem hiding this comment.
you can actually use an array of int here, so there will be less wasted memory and no unboxing later on
| */ | ||
| private void parseHotspotPidFile() { | ||
| Map<Integer, String> compileIdMap = new HashMap<>(); | ||
| try (BufferedReader br = new BufferedReader(new FileReader(new File(System.getProperty("user.dir") + File.separator + hotspotPidFileName)))) { |
There was a problem hiding this comment.
more idiomatic/modern version would be
| try (BufferedReader br = new BufferedReader(new FileReader(new File(System.getProperty("user.dir") + File.separator + hotspotPidFileName)))) { | |
| 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?
There was a problem hiding this comment.
Yes it should. Would this be equivalent to Paths.get("")? It seems to work like that.
There was a problem hiding this comment.
Paths.get(hotspotPidFileName) returns you a Path to hotspotPidFileName in cwd.
…otspot/jtreg/testlibrary_tests/compiler/lib/ir_framework
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:+PrintIdealand-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 (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 file and in the Javadocs 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 and were also created as subtasks of this RFE.
Testing (also described in "Internal Framework Tests in README.md):
There are various tests to verify the correctness of the test framework which can be found as JTreg tests in the tests folder. Additional testing was performed by converting all compiler Inline Types test of project Valhalla (done by @katyapav in 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:
git diff --cached !(tests|examples) | grep -c -E "(^[+-]\s*(\/)?\*)|(^[+-]\s*\/\/)")Big thanks to:
Thanks,
Christian
Progress
Issue
Reviewers
Contributors
<chagedorn@openjdk.org><thartmann@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3508/head:pull/3508$ git checkout pull/3508Update a local copy of the PR:
$ git checkout pull/3508$ git pull https://git.openjdk.java.net/jdk pull/3508/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3508View PR using the GUI difftool:
$ git pr show -t 3508Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3508.diff