proposal: text/template: revise execution model

[bep]

Go's template packages (text/template, html/template) do in general work really well. But there are some issues that are hard/impossible to work around.

In the current implementation, a Template struct is mirrored in both packages, and while there is some separation of parsing and execution to allow for concurrent execution, it is very much intermingled.

This proposal suggests adding a new "template executer" which would:

1. Prepare and execute the template
2. Store and resolve any template functions
3. Provide some user-defined hook interface to allow for a custom evaluation of method/func/map lookups etc.

The above may look like different things, candidates for different proposals. But these issues are connected and I'm convinced that it helps to look at them as a whole, even if this proposal is only partly accepted.

The benefits of the above would be:

2. Stateful functions. One common example would be a i18n translation func. You could make it a method ({{ .I18n "hello" }}) or make it take the language as the first argument ({{ i18n .Language "hello" }}), neither very practical. The current workaround is to clone the template set for each language, which is both resource wasteful and hard to get right in the non-trivial setups. I assume that these functions are added early to get parse-time signature validation, a good thing, but I don't see a reason why not to use another set for execution, making the function lookup lock-free a great performance bonus.
3. Some examples include case insensitive map lookups, adding some execution context to methods that support it (https://github.com/golang/go/issues/31107), allow range of funcs that return map/slice (https://github.com/golang/go/issues/20503), allow a custom variant of the built-in and rather opinionated version of IsTrue (https://github.com/golang/go/issues/28391)

Doing this also removes some performance bottlenecks. A relevant benchmark in Hugo before/after we made these changes:

name                            old time/op    new time/op    delta
SiteNew/Many_HTML_templates-16    55.6ms ± 2%    42.9ms ± 1%  -22.81%  (p=0.008 n=5+5)

name                            old alloc/op   new alloc/op   delta
SiteNew/Many_HTML_templates-16    22.5MB ± 0%    17.6MB ± 0%  -21.99%  (p=0.008 n=5+5)

name                            old allocs/op  new allocs/op  delta
SiteNew/Many_HTML_templates-16      341k ± 0%      247k ± 0%  -27.48%  (p=0.008 n=5+5)

Implementation outline

In the Hugo project we have worked around the problems outlined above, some workaround less pretty than others. But recently we stumbled into a hurdle we didn't find a way around, so we closed our eyes and created a scripted fork of the text/template and html/template packages. We will pull in upstream fixes, and the ultimate goal is for that fork to go away.

But this means that there is a working implementation of this proposal. The implementation is a little bit coloured by trying to do the least amount of changes to the existing code, but I think it outlines a possible API. And it is backwards compatible.

// Executer executes a given template.
type Executer interface {
    Execute(p Preparer, wr io.Writer, data interface{}) error
}

// Preparer prepares the template before execution.
type Preparer interface {
    Prepare() (*Template, error)
}

// ExecHelper allows some custom eval hooks.
type ExecHelper interface {
    GetFunc(tmpl Preparer, name string) (reflect.Value, bool)
    GetMethod(tmpl Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value)
    GetMapValue(tmpl Preparer, receiver, key reflect.Value) (reflect.Value, bool)
    IsTrue(val reflect.Value) bool
}

Hugo's fork can be found here https://github.com/gohugoio/hugo/tree/master/tpl/internal/go_templates (all the patches are placed in hugo_*.go files). Changes are marked with the comment Added for Hugo.. The script that maintains the fork can be found at https://github.com/gohugoio/hugo/tree/master/scripts/fork_go_templates.

Some related Go issues

• Add ExecuteContext methods https://github.com/golang/go/issues/31107
• Document ability to modify FuncMap after template parse by calling Funcs again https://github.com/golang/go/issues/34680
• Allow callers to override IsTrue https://github.com/golang/go/issues/28391
• Methods vs funcs discrepancy https://github.com/golang/go/issues/20503
• index should return nil instead of index out of range error https://github.com/golang/go/issues/14751

[gour]

Hugo is contributing a lot to the glory of Go language, so it would be nice to release the burden of maintaining separate fork from Hugo devs since the proposal addresses issues which cab be useful in wider scope.

[mvdan]

My first impression of this proposal is that it intends to be an overhaul of the API, to also make the packages more powerful. It sounds more like a v2 module, or even a separate module, instead of a small set of incremental changes we could do to the existing packages.

stateful functions. [...] you could make it a method [...] but it's not very practical

Could you expand on this? I think a method is probably the right solution here for most people.

adding some execution context to methods that support it, [...] allow a custom variant of the built-in and rather opinionated version of IsTrue

The first proposal seems likely to be accepted in some form, and the second had a proposed solution that hasn't been implemented yet. Is there a reason you prefer this larger proposal over those two smaller, incremental proposals?

Granted that neither have advanced recently, but that doesn't mean they are rejected or unlikely to happen. Our efforts here could be focused there.

Doing this also removes some performance bottlenecks.

Has anyone investigated whether we could get any performance gains without touching the API? For example, there are dozens of encoding/json alternatives out there with very different APIs, but in recent versions of Go we simply made internal incremental changes to improve performance by 10-30%. I consider API changes to be the last resort when one wants to improve performance.

[bep]

Could you expand on this? I think a method is probably the right solution here for most people.

These functions represent what I would call cross cutting concerns (string translation, logging, caching ...). In Go templates, the method API is limited to the "dot" you pass into Execute and then the "dot" you pass on to any nested template definition. In Hugo, we have interfaces such as Page, Image, Site etc., and while I guess it may be possible in the simple case to embed some I18nHelper or something into all of these, I don't see how it would be practical/possible/pretty. You need to consider templates calling other templates that would then need to somehow wrap the original context ... Even I get dizzy thinking about this, then think about Average Joe.

Is there a reason you prefer this larger proposal over those two smaller, incremental proposals?

Two reasons: 1) I'm convinced that looking at this problem as a whole will give an overall much better and more flexible design. I'm not saying my outlined design is fantastic, but it is at least a coherent starting point. 2) These smaller incremental proposals have a tendency to move very slowly to their resolution. Sitting back and watch these slowly fall into place from Go 1.10 to Go 1.20 is not great if you really need them.

As to the specific IsTrue discussion. If I remember correctly, the proposed solution was to make it a template function that could be overridden. In my opinion, these functions/methods are not meant to be called from the templates, so putting them there sounds like a convenience thing, and to me, it sounds much better to define one or more interfaces that would have proper GoDoc etc.

I consider API changes to be the last resort when one wants to improve performance.

"Almost every performance problem can and should be solved by redesigning, rather than optimizing. Optimization techniques should be the absolute last tool(s) you reach for, not the first." - Quote Peter Bourgon on Twitter. I do not agree with everything he says, but this I do.

[rogpeppe]

@bep I took at look at the API you've made in your fork and the documentation seems a little lacking. For example, what does this function do?

It would be helpful if you could provide a full summary of the entire API that you're proposing in this issue, so we don't have to look into the code to try to see what the changes are and what they do.

Thanks.

[rogpeppe]

@bep AFAICS a significant portion of your concerns (with the exception of #28391, which already has an accepted solution, and #20503, which has a good workaround in any case) could be solved if #31107 was implemented to allow "silent" passing of context.Context through to any function that has a context as a first argument.

Would that be a reasonable characterisation? If there are more use cases that you have in mind, it might be good to list them here.

[networkimprov]

@bep have you considered publishing your forked packges in their own repo?

Attracting users is the best way to prove your point re better design, and convincing the Go team to fix/enhance stdlib APIs is, erm... time consuming :-p

[bep]

@rogpeppe I think you're missing out on the main topic of this issue: A stricter separation of parsing and execution to allow for stateful functions, which also would get rid of a fairly expensive RLock/RUnlock construct, which wouldn't get any better if you implement #28391 as a template function -- which would mean that every conditional (if, with etc.) would be protected by a mutex. A proposal to do similar with Go's if statement would, I assume, be rejected.

I will try to find time to do better GoDoc of Hugo's fork in the next few days. But the custom code should be between 50-100 lines of Go code in 2 files, so it should be doable to understand it even without.

@networkimprov I have put this into an internal package in Hugo for a reason. Putting it "out there" would 1) Make it harder to get back to 1 source of truth (which this issue is about) and 2) It would add work that I currently do not have time for. But I would welcome it if this proposal could maybe lead to an alternative design that allowed for the template execution to live in a package outside stdlib.

[rogpeppe]

On Fri, 10 Jan 2020, 23:03 Bjørn Erik Pedersen, notifications@github.com wrote:

@rogpeppe https://github.com/rogpeppe I think you're missing out on the main topic of this issue: A stricter separation of parsing and execution to allow for stateful functions

Presumably by stateful functions, you mean functions that can have different implicit state each time the template runs? How is that not catered for by an implicit context argument (albeit at the cost of a dynamic type conversion) ?

, which also would get rid of a fairly expensive RLock/RUnlock construct,

which wouldn't not get any better if you implement #28391 https://github.com/golang/go/issues/28391 as a template function -- which would mean that every conditional execution (if, with, not etc.) would be protected by a mutex.

I don't understand that, I'm afraid.

A proposal to do similar with Go's if statement would, I assume, be rejected.

I will try to find time to do better GoDoc of Hugo's fork in the next few days. But the custom code should be between 50-100 lines of Go code in 2 files, so it should be doable to understand it even without.

@networkimprov https://github.com/networkimprov I have put this into an internal package in Hugo for a reason. Putting it "out there" would 1) Make it harder to get back to 1 source of truth (which this issue is about) and 2) It would add work that I currently do not have time for.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/36462?email_source=notifications&email_token=AAAQHO4WBVTPPRSDJI7KNWTQ5D5E5A5CNFSM4KEM2LCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIVPDAQ#issuecomment-573239682, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQHOYALF7AEBZL5WNW54LQ5D5E5ANCNFSM4KEM2LCA .