RFR: 8367531: Template Framework: use scopes and tokens instead of misbehaving immediate-return-queries [v14]
Christian Hagedorn
chagedorn at openjdk.org
Thu Nov 6 10:17:45 UTC 2025
On Wed, 5 Nov 2025 12:51:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> I got some feedback from users of the Template Framework, especially @galderz . And personally, I already was slightly unsatisfied by some of the issues described below, but did not expect it to be as bad as it is.
>>
>> So I'm sorry, but I think we need to do a significant re-design. It is now still early enough, and only trivial changes are required for the "real" uses of the framework. Only the framework internal tests require significant changes.
>>
>> Many thanks to @galderz for trying out the framework, and reporting the issues. And thanks to @chhagedorn for spending a few hours in an offline meeting discussing the issue.
>>
>> **Major issue with Template Framework: lambda vs token order**
>>
>> The template rendering involves some state, such as keeping track of hashtag replacements, names and fuel cost.
>> Some methods have side-effects (`addDataName`, `let`, ...) and others are simple queries (`sample`, ...).
>> Sadly, the first version of the template framework was not very consistent, and created tokens (deferred evaluation, during token evaluation) for some, and for others it queried the state and returned the result immediately (during lambda execution). One nasty consequence is that an immediately returning query can "float" above a state affecting token. For example, `addDataName` generated a token (so that we know if it is to be added for the template frame or a hook anchoring frame), but answered sampling queries immediately (because that means we can use the returned value immediately and make decisions based on it immediately, which is nice). Looking at the example below, this had the confusing result that `addDataName` only generates a token at first, then `sample` does not have that name available yet, and only later during token evaluation is the name actually added.
>>
>> var testTemplate = Template.make(() -> body(
>> ...
>> addDataName("name", someType, MUTABLE),
>> let("name", dataNames(MUTABLE).exactOf(someType).sample().name()),
>> ...
>> ));
>>
>>
>> **Two possible solutions: all-in on lambda execution or all-in on tokens**
>>
>> First, I thought I want to go all-in on lambda execution, and have everything have immediate effect and return results immediately. This would have the nice effect that the user feels like they are directly in control of the execution order. But I did not find a good way without exposing too many internals to the user, or getting rid of the nice "token lists" we currently have inside Templates (the...
>
> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
>
> - for Christian
> - Apply suggestions from code review
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
Dumping some more comments as I'm refreshing my memory on how the internal classes interact with each other by jumping around the code. Will pick it up again after lunch.
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 32:
> 30:
> 31: /**
> 32: * The {@link CodeFrame} represents a frame (i.e. scope) of code, appending {@link Code} to the {@code 'codeList'}
Needed to jump back and forth between different classes to refresh my memory about internals. I was a little bit confused about the difference between `TemplateFrame` and `CodeFrame` in the context where it was used, so I ended up studying the class comments of these classes. IIUC, the latter (this class here) is actually about the generated code. Maybe it helps when adding here "generated"?
Suggestion:
* The {@link CodeFrame} represents a frame (i.e. scope) of generated code by appending {@link Code} to the {@code 'codeList'}
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 34:
> 32: * The {@link CodeFrame} represents a frame (i.e. scope) of code, appending {@link Code} to the {@code 'codeList'}
> 33: * as {@link Token}s are rendered, and adding names to the {@link NameSet}s with {@link Template#addStructuralName}/
> 34: * {@link Template#addDataName}. {@link Hook}s can be added to a frame, which allows code to be inserted at that
Since we have template and code frames it might help to be more precise here:
Suggestion:
* {@link Template#addDataName}. {@link Hook}s can be added to a code frame, which allows code to be inserted at that
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 50:
> 48: * <p>
> 49: * Note, that {@link CodeFrame}s and {@link TemplateFrame}s often go together, but can also
> 50: * diverge.
Can you give an example here where/when this is the case? If there are not many variants, it might be worth to list them here.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 27:
> 25:
> 26: import java.util.List;
> 27: import java.util.function.Function;
The IDE reports that this is unused.
Suggestion:
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 88:
> 86: *
> 87: * <p>
> 88: * Instead, the user should create a {@link TemplateToken} from the inner {@link Template}, and
Read this through again to refresh my memory an noticed this: "Should" sounds like good advise but below it clearly mention it's forbidden. Would it make sense to swap the paragraphs? Then it makes it more clear that this is not only advise but the only way to go.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 117:
> 115: static Renderer getCurrent() {
> 116: if (renderer == null) {
> 117: throw new RendererException("A Template method such as '$', 'fuel', etc. was called outside a template rendering.");
Call?
Suggestion:
throw new RendererException("A Template method such as '$', 'fuel', etc. was called outside a template rendering call.");
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 145:
> 143: // Ensure CodeFrame consistency.
> 144: if (baseCodeFrame != currentCodeFrame) {
> 145: throw new RuntimeException("Internal error: Renderer did not end up at base CodeFrame.");
Optionally, something for another RFE (there are more "Internal error message"): You could turn these runtime exception into `TemplateFrameworkException`s add a hint to report a bug.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 237:
> 235: // If the ScopeToken is transparent to Names, then the Template is transparent to names.
> 236: ScopeToken st = templateToken.instantiate();
> 237: renderScopeToken(st, () -> {});
Just a suggestion: You seem to use `() -> {}` quite often when calling `renderScopeToken()`. Might be worth to overload it and provide one `renderScopeToken()` version with just a `ScopeToken`.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 246:
> 244:
> 245: private void renderScopeToken(ScopeToken st, Runnable preamble) {
> 246: if (!(st instanceof ScopeTokenImpl sti)) {
Wasn't aware of that but seems quite convenient: The IDE suggests to use a record pattern:
if (!(st instanceof ScopeTokenImpl(List<Token> tokens, boolean nestedNamesAreLocal,
boolean nestedHashtagsAreLocal, boolean nestedSetFuelCostAreLocal
))) {
Then you can directly access the fields below:
sti.nestedNamesAreLocal() -> nestedNamesAreLocal
etc.
test/hotspot/jtreg/compiler/lib/template_framework/ScopeTokenImpl.java line 34:
> 32: *
> 33: * Note: we want the tokens to be package private, so we create this Impl
> 34: * record.
Can you extend this comment why it matters? Is it that users just don't need to know/worry about this or is it to actually avoid that someone is trying to write code that relies on this class and possibly prevent future internal changes?
test/hotspot/jtreg/compiler/lib/template_framework/ScopeTokenImpl.java line 39:
> 37: boolean nestedNamesAreLocal,
> 38: boolean nestedHashtagsAreLocal,
> 39: boolean nestedSetFuelCostAreLocal) implements ScopeToken, Token {}
Could be confusing what "local" means. Does it mean local to the scope or local to the nested scope? IIUC, it means to the nested scope. Maybe "nestedXAreTransparent" (or "nestedXAreNonTransparent" ) is clearer and adheres to the scope transparency definitions. What do you think?
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 30:
> 28:
> 29: /**
> 30: * The {@link TemplateFrame} is the frame for a {@link Template} and its inner
unique?
Suggestion:
* The {@link TemplateFrame} is the frame for a unique {@link Template} and its inner
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 34:
> 32: * {@link #id} used to deconflict names using {@link Template#$}. It also has a set of hashtag
> 33: * replacements, which combine the key-value pairs from the template argument and the
> 34: * {@link Template#let} definitions. Inner scopes of a {@link Template} have access to
`Inner scopes of a {@link Template} [...]`
Should be obvious but since it's a class comment you could add something here that we only mean scopes that are not template themselves.
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 36:
> 34: * {@link Template#let} definitions. Inner scopes of a {@link Template} have access to
> 35: * the outer scope hashtag replacements, and any hashtag replacement defined inside an
> 36: * inner scope is local and disapears once we leave the scope. The {@link #parent} relationship
Suggestion:
* inner scope is local and disappears once we leave the scope. The {@link #parent} relationship
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 38:
> 36: * inner scope is local and disapears once we leave the scope. The {@link #parent} relationship
> 37: * provides a trace for the use chain of templates and their inner scopes. The {@link #fuel}
> 38: * is reduced over this chain, to give a heuristic on how much time is spent on the code
I don't have a better suggestion but the first part mentioning "time" is not very clear since it's only about nesting depth?
Suggestion:
* is reduced over this chain to give a heuristic on how much time is spent on the code
-------------
PR Review: https://git.openjdk.org/jdk/pull/27255#pullrequestreview-3426605511
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498309359
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498331002
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498335738
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2497775079
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2497797514
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2497782957
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498027072
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498047072
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498053874
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498147231
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498168918
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498192200
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498197996
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498229138
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2498245702
More information about the hotspot-compiler-dev
mailing list