RFR: 8367531: Template Framework: use scopes and tokens instead of misbehaving immediate-return-queries [v26]
Christian Hagedorn
chagedorn at openjdk.org
Wed Nov 12 14:31:33 UTC 2025
On Tue, 11 Nov 2025 16:21:00 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 one additional commit since the last revision:
>
> add missing comma from suggestion application
It was difficult to grasp the implementation in its entirety. I focused more on the descriptions and how things are connected. I added some last comments but then it looks good from my side! Thanks for your patience and again: Great work! :-)
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 32:
> 30:
> 31: /**
> 32: * The {@link CodeFrame} represents a frame (i.e. scope) of generated code by appending {@link Code} to the {@code 'codeList'}
Suggestion:
* The {@link CodeFrame} represents a frame (i.e. scope) of generated code by appending {@link Code} to the {@link #codeList}
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 45:
> 43: * the execution jumps from the current (caller) {@link CodeFrame} scope to the scope of the
> 44: * {@link Hook#anchor}. This ensures that the {@link Name}s of the anchor scope are accessed,
> 45: * and not of the ones from the caller scope. Once the {@link Hook#insert}ion is complete, we
Suggestion:
* {@link Hook#anchor}. This ensures that the {@link Name}s of the anchor scope are accessed,
* and not the ones from the caller scope. Once the {@link Hook#insert}ion is complete, we
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 60:
> 58: * Below, we look at a few examples, and show the use of CodeFrames (c) and TemplateFrames (t).
> 59: *
> 60: * Example1: anchoring and insertion in the same Template
I think this example is already powerful enough to get a feeling of what is going on - thanks for adding it! From my side, no more examples are required for now but feel free to add more examples as you see fit.
If we stick with one example:
Suggestion:
* Below, we look at an example, and show the use of CodeFrames (c) and TemplateFrames (t).
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 101:
> 99: * t1 c1 t2 c2b ... t3 c3 <-- TemplateFrame nesting ---t4 c4
> 100: * t1 c1 t2 c2b ... t3 c3 with hashtag t4 c4
> 101: * t1 c1 t2 c2b ... t3 c3 and setFuelCost t4 c4
Probably clear but for completness for explanation below:
Suggestion:
* t1 c1 t2 c2b ... t3 c3 with hashtag t4 c4 // t: Concerns Template Frame
* t1 c1 t2 c2b ... t3 c3 and setFuelCost t4 c4 // c: Concerns Code Frame
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 111:
> 109: * t1 c1 t2 c2b ... t3 c3 t4 c4
> 110: * t1 c1 t2 c2b ... t3 c3 t4 c4 addDataName(...) -> c: names escape to the caller scope because
> 111: * t1 c1 t2 c2b ... t3 c3 t4 c4 insertion scope is transparent
The suggestions below include:
- Since you gave the scopes proper names in the example, let's stress this here.
- Use numbers `c3` etc.?
- For `let()`: hashtag "definition"?
- Maybe add a small extra comment for `dataNames()` code frame nesting?
Suggestion:
* t1 c1 t2 c2b ... t3 c3 t4 c4 "use hashtag #x" -> t: hashtag queried in Insertion (t4) and Caller Scope (t3)
* t1 c1 t2 c2b ... t3 c3 t4 c4 c: code added to Anchoring Scope (c2a)
* t1 c1 t2 c2b ... t3 c3 t4 c4
* t1 c1 t2 c2b ... t3 c3 t4 c4 let("x", 42) -> t: hashtag definition escapes to Caller Scope (t3) because
* t1 c1 t2 c2b ... t3 c3 t4 c4 Insertion Scope is transparent
* t1 c1 t2 c2b ... t3 c3 t4 c4
* t1 c1 t2 c2b ... t3 c3 t4 c4 dataNames(...)...sample() -> c: sample from Insertion (c4) and Anchoring Scope (c2a)
* t1 c1 t2 c2b ... t3 c3 t4 c4 (CodeFrame nesting: c2a -> c4)
* t1 c1 t2 c2b ... t3 c3 t4 c4 addDataName(...) -> c: names escape to the Caller Scope (c3) because
* t1 c1 t2 c2b ... t3 c3 t4 c4 Insertion Scope is transparent
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 119:
> 117:
> 118: // Wrap the FilteredSet as a Predicate.
> 119: private static record DataNamePredicate(FilteredSet fs) implements NameSet.Predicate {
IDE reports that `static` is redundant for inner records:
Suggestion:
private record DataNamePredicate(FilteredSet fs) implements NameSet.Predicate {
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 205:
> 203: */
> 204: public Token sample(Function<DataName, ScopeToken> function) {
> 205: return new NameSampleToken<DataName>(predicate(), null, null, function);
Suggestion:
return new NameSampleToken<>(predicate(), null, null, function);
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 329:
> 327: */
> 328: public Token toList(Function<List<DataName>, ScopeToken> function) {
> 329: return new NamesToListToken(predicate(), function);
Suggestion:
return new NamesToListToken<>(predicate(), function);
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 337:
> 335: *
> 336: * @param function The {@link Function} that is called to create the inner {@link ScopeToken}s
> 337: * for each of the {@link DataName}s in the filtereds set.
Suggestion:
* for each of the {@link DataName}s in the filtered set.
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 343:
> 341: */
> 342: public Token forEach(Function<DataName, ScopeToken> function) {
> 343: return new NameForEachToken<DataName>(predicate(), null, null, function);
Suggestion:
return new NameForEachToken<>(predicate(), null, null, function);
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 367:
> 365: */
> 366: public Token forEach(String name, String type, Function<DataName, ScopeToken> function) {
> 367: return new NameForEachToken<DataName>(predicate(), name, type, function);
Suggestion:
return new NameForEachToken<>(predicate(), name, type, function);
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 287:
> 285:
> 286: // Tear down CodeFrame nesting. If no nesting happened, the code is already
> 287: // in the currendCodeFrame.
Suggestion:
// in the currentCodeFrame.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 346:
> 344: currentCodeFrame.addName(name);
> 345: }
> 346: case ScopeToken st -> {
For this and below, can you use full names (i.e. `scopeToken` etc.)? This makes it a little easier to read instead of the abbreviations.
test/hotspot/jtreg/compiler/lib/template_framework/ScopeToken.java line 27:
> 25:
> 26: import java.util.List;
> 27:
Unused:
Suggestion:
test/hotspot/jtreg/compiler/lib/template_framework/ScopeTokenImpl.java line 32:
> 30: * hashtag replacements and {@link Template#setFuelCost} are local, or escape to
> 31: * outer scopes.
> 32: *
Suggestion:
*
* <p>
test/hotspot/jtreg/compiler/lib/template_framework/ScopeTokenImpl.java line 34:
> 32: *
> 33: * Note: We want the {@link ScopeToken} to be public, but the internals of the
> 34: * record should be private. One way too solve this is with a public interface
Suggestion:
* record should be private. One way to solve this is with a public interface
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 94:
> 92:
> 93: // Wrap the FilteredSet as a Predicate.
> 94: private static record StructuralNamePredicate(FilteredSet fs) implements NameSet.Predicate {
Suggestion:
private record StructuralNamePredicate(FilteredSet fs) implements NameSet.Predicate {
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 178:
> 176: */
> 177: public Token sample(Function<StructuralName, ScopeToken> function) {
> 178: return new NameSampleToken<StructuralName>(predicate(), null, null, function);
Suggestion:
return new NameSampleToken<>(predicate(), null, null, function);
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 248:
> 246: */
> 247: public Token toList(Function<List<StructuralName>, ScopeToken> function) {
> 248: return new NamesToListToken(predicate(), function);
Suggestion:
return new NamesToListToken<>(predicate(), function);
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 262:
> 260: */
> 261: public Token forEach(Function<StructuralName, ScopeToken> function) {
> 262: return new NameForEachToken<StructuralName>(predicate(), null, null, function);
Suggestion:
return new NameForEachToken<>(predicate(), null, null, function);
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 286:
> 284: */
> 285: public Token forEach(String name, String type, Function<StructuralName, ScopeToken> function) {
> 286: return new NameForEachToken<StructuralName>(predicate(), name, type, function);
Suggestion:
return new NameForEachToken<>(predicate(), name, type, function);
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 289:
> 287: }
> 288: }
> 289: }
Feels like we have a lot of duplication in `DataName` and `StructuralName`. But I'm not sure if/how we could share it somehow. Anyhow, not something for today.
test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 774:
> 772: *
> 773: * <p>
> 774: * In some cases, it can be helpful to have different {@link setFuelCost} within
Suggestion:
* In some cases, it can be helpful to have different {@link #setFuelCost} within
test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 791:
> 789: * // depth, and recursive template uses should not get
> 790: * // as much fuel as in CODE1.
> 791: * )
Suggestion:
* ),
test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 883:
> 881: * "System.out.println(\"Use a and b as capture variables:\"" + a + " and " + b + ");\n"
> 882: * ))
> 883: * )));
Suggestion:
* ));
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 55:
> 53: * of the current {@link Template}. Inner scopes of a {@link Template} have access to
> 54: * the outer scope hashtag replacements, and any hashtag replacement defined inside an
> 55: * inner scope is local and disappears once we leave the scope.
We could explicitly mention here that we do not mean inner scopes that are templates themselves?
Suggestion:
* of the current {@link Template}. Inner scopes of a {@link Template}, that are not
* templates themselves, have access to the outer scope hashtag replacements, and any
* hashtag replacement defined inside an inner scope is local and disappears once we
* leave the scope.
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 60:
> 58: * The {@link #parent} relationship provides a trace for the use chain of templates and
> 59: * their inner scopes. The {@link #fuel} is reduced over this chain to give a heuristic
> 60: * on how much time is spent on the code from the template corresponding to the frame,
"time" sounds misleading. What about: [...] on how many times we already nested the template corresponding to the frame recursively [...]?
test/hotspot/jtreg/compiler/lib/template_framework/TemplateFrame.java line 171:
> 169: if (this.isTransparentForFuel) {
> 170: this.parent.setFuelCost(fuelCost);
> 171: }
`this` can be omitted:
Suggestion:
if (isTransparentForFuel) {
parent.setFuelCost(fuelCost);
}
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27255#pullrequestreview-3453229086
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518192324
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518272771
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518264127
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518237849
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518227687
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518469550
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518471976
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518479356
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518482072
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518482740
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518485286
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518413436
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518427367
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518430513
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518446122
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518445484
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518488456
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518489768
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518494322
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518494855
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518495329
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518506571
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518150213
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518149435
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518151125
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518174448
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518186319
PR Review Comment: https://git.openjdk.org/jdk/pull/27255#discussion_r2518154504
More information about the hotspot-compiler-dev
mailing list