-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8344942: Template-Based Testing Framework #24217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
|
@eme64 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 81 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/contributor add @TobiHartmann @tobiasholenstein @maasaid @theoweidmannoracle |
|
@eme64 Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add @TobiHartmann |
|
/contributor add @tobiasholenstein |
|
@eme64 |
|
/contributor add @maasaid |
|
/contributor add @theoweidmannoracle |
|
@eme64 |
|
@eme64 Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
@eme64 |
Nice work @eme64 ! |
galderz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great though I'm not too familiar with the code to be able to do a reasonable review, but I had a question:
Have you got any practical use case that can show where you've used this and show what it takes to build such a use case? VectorReduction2 or similar type of microbenchmarks would be great to see auto generated using this?
The reason I ask this is because I feel that something that is missing in this PR is a small practical use case where this framework is put into action to actually generate some jtreg/IR/microbenchmark test and see how it runs as part of the CI in the PR. WDYT?
|
@galderz Thanks for your questions!
Well the code is all brand new, so really anybody could review ;)
I actually have a list of experiments in this branch (it is linked in the PR description): #23418 Ah, but there was this test:
I also have a few tests in this PR that just generate regular JTREG tests, without the IR framework, did you see those?
I don't yet have a solution for microbenchmarks. It's mostly an issue of including the The patch is already quite large, and so I wanted to just publish the basic framework. Do you think that is ok? |
Right, what I meant is that developers that have past history with this work will be able to provide a more thorough review :)
Yes it is. Sorry I missed the linked PR when I read the description. The examples there look great, it's what I was looking for.
Yeah I've seen them now.
Yeah sure. |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work Emanuel! This is a very valuable framework to improve our testing coverage.
Here are some first comments after skimming some of the code and comments. I will deep dive more into the code and documentation later.
|
|
||
| The basic functionalities of the Template Framework are described in the [Template Class](./Template.java), together with some examples. More examples can be found in [TestSimple.java](../../../testlibrary_tests/template_framework/examples/TestSimple.java) and [TestTutorial.java](../../../testlibrary_tests/template_framework/examples/TestTutorial.java). | ||
|
|
||
| The [Template Library](../template_library/README.md) provides a large number of Templates which can be used to create anything from simple regression tests to complex fuzzers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template_library/README.md does not exist.
Another thought: Should template_library be a subfolder in template_framework? Otherwise, it could suggest to be something separate from the Template Framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing that line, and moving the library to a subfolder.
| * String Templates to inject these arguments into the strings. But since String Templates are not (yet) available, | ||
| * the {@link Template}s provide <strong>hashtag replacements</strong> in the Strings: the {@link Template} argument | ||
| * names are captured, and the argument values automatically replace any {@code "#name"} in the Strings. See the | ||
| * different overloads of {@link make} for examples. Additional hashtag replacements can be defined with {@link let}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. As we saw offline, javadoc does not need the hashtag, but some editors do, and it seems to be common practice to have the hashtag. I'll add them for every applicable link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed all, but I cannot easily confirm. Can you check if there are any left?
test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Outdated
Show resolved
Hide resolved
| * {@link Template}s are used to generate code, based on {@link Token} which are rendered to {@link String}. | ||
| * | ||
| * <p> | ||
| * A {@link Template} can have zero or more arguments, and for each number of arguments there is an implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the README refers to this file for more information (which is perfectly fine to avoid repetition), I naturally started to read here at the top. But in this paragraph, we are already explaining details of the implementation without giving a more general introduction and motivation for the Template Framework which can leave readers without background confused. I think it could be worth to spend some more time in the README (which you can then also just refer to when suggesting to use the framework in PRs). You could cover: Why is the framework useful/why should I care about it, some leading example/testing scenario and why it is really hard to cover that without the framework (i.e. what we've done so far until today), how does the framework roughly work, how easy is it to write tests (you can then reference example tests from there) etc.
You can still do many references from the README to classes and examples.
Side note: I admit that I could have extended the IR framework README with a more motivational introduction as well - maybe I will add that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an additional test, and copied some snippets to the java docs.
I also added some more motivation.
Let me know if that is better, or if you have any more suggestions :)
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Almost there, some last comments and then we're good to go :-)
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java
Outdated
Show resolved
Hide resolved
| if (w < 0) { | ||
| throw new RuntimeException("Negative weight not allowed: " + w); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought zero is also not allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that should have been filtered out already earlier, when we added the individual names. Now we could have a total weight of zero if we have no names. Then we just return null here, and then throw an exception a little further up the use case chain, e.g. DataName.sample -> throw new RendererException("No variable: " + mutability + msg1 + msg2 + ".");
This here is just a sanity check, hence I throw a RuntimeException, and not a RendererException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As asked for offline: added some more comments here.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java
Outdated
Show resolved
Hide resolved
| int hashtag = s.indexOf("#", start); | ||
| // If the character was not found, we want to have the rest of the | ||
| // String s, so instead of "-1" take the end/length of the String. | ||
| dollar = (dollar == -1) ? s.length() : dollar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.length() could be called once before the loop and then reused inside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean as a performance optimization? Is that not something we let the compiler do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline: I made s final, so we are sure that it is not mutated and the length should be moved out of the loop by the compiler. It would also only be a very small performance impact, as we are doing things like indexOf here, which are much more expensive anyway.
Co-authored-by: Christian Hagedorn <[email protected]>
|
@chhagedorn Thanks for this batch of comments, they are all addressed! |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for addressing everything and all the interesting and insightful discussions - also learnt quite a lot :-) There is nothing left to say other than: Ship it! 🚀 (if the other reviewers also agree with the latest changes)
|
@chhagedorn Thank you very much! |
mhaessig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at the changes since my last review. They look excellent.
Especially good to see the tutorial improving even further.
|
@mhaessig Thank you very much for having another look! |
robcasloz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just have a few language suggestions. Thanks for driving this Emanuel!
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestTutorial.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestTutorial.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Roberto Castañeda Lozano <[email protected]>
|
@robcasloz Thanks for making another pass and for the suggestions! 😊 |
|
@chhagedorn @robcasloz @mhaessig Thanks a lot for all the time you invested to see this through, I know it took a lot of effort to review this, so I am very thankful 😊 |
|
I want to thank all the contributors here again before integration: |
|
/integrate |
|
Going to push as commit 248341d.
Your commit was automatically rebased without conflicts. |


Goal
We want to generate Java source code:
Note: with the Template Library draft I was already able to find a list of bugs.
How to get started
When reviewing, please start by looking at:
jdk/test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestSimple.java
Lines 60 to 76 in d21a8aa
We have a Template with two arguments. They are typed (Integer and String). We then apply the arguments
template.withArgs(42, "7"), producing aTemplateWithArgs. This can then berendered to a String. And then that can be compiled and executed with the CompileFramework.Second, look at this advanced test:
jdk/test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestAdvanced.java
Lines 102 to 119 in 7707980
And then for a "tutorial", look at:
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestTutorial.javaIt shows these features:
bodyof a Template is essentially a list ofTokens that are concatenated.TemplateWithArgsis also aToken.#namereplacements to directly format values into the String. If we had proper String Templates in Java, we would not need this feature.$varto make variable names unique: if we applied the same template twice, we would get variable collisions.$varis then replaced with e.g.var_7in one template use andvar_42in the other template use.Hooks to insert code into outer (earlier) code locations. This is useful, for example, to insert fields on demand.fuelto limit the recursion.Names: useful to register field and variable names in code scopes.Next, look at the documentation in. This file is the heart of the Template Framework, and describes all the important features.
jdk/test/hotspot/jtreg/compiler/lib/template_framework/Template.java
Lines 31 to 76 in d21a8aa
For a better experience, you may want to generate the
javadocs:javadoc -sourcepath test/hotspot/jtreg:./test/lib compiler.lib.template_frameworkHistory
@TobiHartmann and I have played with code generators for a while, and have had the dream of doing that in a more principled way. And to hopefully make it more accessible for other VM developers, with the goal of improving test coverage.
@TobiHartmann started with an initial prototype. @tobiasholenstein took over, and worked with our intern @maasaid. Their approach was to take old tests and templetize them (draft PR). Their templates had holes for replacements, and a
$prefix for variables name replacement.Once @tobiasholenstein left, I took over the project, and focused on nested template use, where templates could be passed arguments. I kept it string based, which worked, but the resulting syntax was a little cryptic. Debugging was difficult, as I had to produce custom stack traces, print available variables, etc. Here is the string syntax based version.
@theoweidmannoracle reviewed my draft, and had some very good feedback. He was frustrated with the complexity, and also the string syntax. Over several iterations, we kept most of the complexity (because code generation is a little complex), but changed the approach to use Java Generics. I took one of his prototypes and fleshed it out with all the other necessary features.
Related Work
There is lots of related work, for test generation:
Verify.java: verify results.Generators.java: generate random inputs from "interesting distributions".Compile Framework: take string source code, compile and class-load it for execution.Note: with the Template Library draft I was already able to find a list of bugs.
Progress
Issue
Reviewers
Contributors
<[email protected]><[email protected]><[email protected]><[email protected]><[email protected]><[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24217/head:pull/24217$ git checkout pull/24217Update a local copy of the PR:
$ git checkout pull/24217$ git pull https://git.openjdk.org/jdk.git pull/24217/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24217View PR using the GUI difftool:
$ git pr show -t 24217Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24217.diff
Using Webrev
Link to Webrev Comment