Skip to content
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

text/template: optimize Clone+Funcs+Execute sequence #38114

Closed
rsc opened this issue Mar 27, 2020 · 9 comments
Closed

text/template: optimize Clone+Funcs+Execute sequence #38114

rsc opened this issue Mar 27, 2020 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Mar 27, 2020

In #31107 it came up that maybe most of the requested new functionality would be doable outside the package if it were possible to install new functions on a per-execution basis. One option is to add a new Execute that takes a second FuncMap, but I think we should consider not adding new API and instead make Clone+Funcs+Execute run faster.

It might be that all that's needed is making Clone make the actual template backing data copy-on-write. Then Clone is cheap. Then Funcs would copy only the map, or maybe even start a second map, also cheap. And Execute would be unchanged.

This issue is to think about whether this is possible to do without visible semantic changes, and if so, to do it.

/cc @bep @robpike @empijei

@empijei
Copy link
Contributor

empijei commented Mar 27, 2020

Thanks for opening this. It would indeed address my need to seamlessly provide a per-execution nonce on the framework side.

Both adding other functions and cloning would work.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 27, 2020
@andybons andybons added this to the Unplanned milestone Mar 27, 2020
@bep
Copy link
Contributor

bep commented Mar 27, 2020

I assume you still would need to synchronize the clone? What is the cost compared to doing it the proper way (adding the funcs to the execution struct where they belong)?

@empijei
Copy link
Contributor

empijei commented Mar 29, 2020

What is the cost

This could be a lazy copy so potentially very little. It would just mark the template as being in use by more than one user and thus turning on a copy-on-write flag. I am similarly worried about this but Russ said this could be done with very little overhead.

doing it the proper way

Are you considering as an alternative to add a FuncMap to the Execute call? I would personally prefer it too but it would require an API change.

If your concern is about performance, would you be ok with expressing this semantic as Clone+Funcs+Execute (which has the perk of not changing the API) if it had a similar runtime cost?

@empijei
Copy link
Contributor

empijei commented May 2, 2020

@rsc I am still trying to wrap my head around this.

How could this be acceptably efficient to clone the templates every time we need to render them?

Most of our servers require to render the same template tens of times with different data and different nonces at the same time.

Can you please share a bit of technical details on what you had in mind to implement this so I can maybe start taking a look?

@zeripath
Copy link

zeripath commented Aug 13, 2022

One option is to add a new Execute that takes a second FuncMap, but I think we should consider not adding new API and instead make Clone+Funcs+Execute run faster.

I think I have to disagree here. The number of changes required to make Clone+Funcs+Execute faster will be immense.

Here's a simple patch that does ExecuteFuncMaps:

Patch
diff --git a/src/html/template/template.go b/src/html/template/template.go
index 30b64dff04..d473d54a7f 100644
--- a/src/html/template/template.go
+++ b/src/html/template/template.go
@@ -124,6 +124,20 @@ func (t *Template) Execute(wr io.Writer, data any) error {
 	return t.text.Execute(wr, data)
 }
 
+// ExecuteFuncMap applies a parsed template to the specified data object,
+// FuncMap overlay and writes the output to wr.
+// If an error occurs executing the template or writing its output,
+// execution stops, but partial results may already have been written to
+// the output writer.
+// A template may be executed safely in parallel, although if parallel
+// executions share a Writer the output may be interleaved.
+func (t *Template) ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error {
+	if err := t.escape(); err != nil {
+		return err
+	}
+	return t.text.ExecuteFuncMap(wr, data, funcs)
+}
+
 // ExecuteTemplate applies the template associated with t that has the given
 // name to the specified data object and writes the output to wr.
 // If an error occurs executing the template or writing its output,
@@ -139,6 +153,21 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data any) error {
 	return tmpl.text.Execute(wr, data)
 }
 
+// ExecuteTemplateFuncMap applies the template associated with t that has the given
+// name to the specified data object and FuncMap overlay writing the output to wr.
+// If an error occurs executing the template or writing its output,
+// execution stops, but partial results may already have been written to
+// the output writer.
+// A template may be executed safely in parallel, although if parallel
+// executions share a Writer the output may be interleaved.
+func (t *Template) ExecuteTemplateFuncMap(wr io.Writer, name string, data any, funcs FuncMap) error {
+	tmpl, err := t.lookupAndEscapeTemplate(name)
+	if err != nil {
+		return err
+	}
+	return tmpl.text.ExecuteFuncMap(wr, data, funcs)
+}
+
 // lookupAndEscapeTemplate guarantees that the template with the given name
 // is escaped, or returns an error if it cannot be. It returns the named
 // template.
diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index 37984cf91a..9a43edb08f 100644
--- a/src/text/template/exec.go
+++ b/src/text/template/exec.go
@@ -37,6 +37,7 @@ type state struct {
 	node  parse.Node // current node, for errors
 	vars  []variable // push-down stack of variable values.
 	depth int        // the height of the stack of executing templates.
+	funcs map[string]reflect.Value
 }
 
 // variable holds the dynamic value of a variable such as $, $x etc.
@@ -187,6 +188,21 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data any) error {
 	return tmpl.Execute(wr, data)
 }
 
+// ExecuteTemplateFuncMap applies the template associated with t that has the given name
+// to the specified data object and writes the output to wr.
+// If an error occurs executing the template or writing its output,
+// execution stops, but partial results may already have been written to
+// the output writer.
+// A template may be executed safely in parallel, although if parallel
+// executions share a Writer the output may be interleaved.
+func (t *Template) ExecuteTemplateFuncMap(wr io.Writer, name string, data interface{}, funcs FuncMap) error {
+	tmpl := t.Lookup(name)
+	if tmpl == nil {
+		return fmt.Errorf("template: no template %q associated with template %q", name, t.name)
+	}
+	return tmpl.ExecuteFuncMap(wr, data, funcs)
+}
+
 // Execute applies a parsed template to the specified data object,
 // and writes the output to wr.
 // If an error occurs executing the template or writing its output,
@@ -198,10 +214,24 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data any) error {
 // If data is a reflect.Value, the template applies to the concrete
 // value that the reflect.Value holds, as in fmt.Print.
 func (t *Template) Execute(wr io.Writer, data any) error {
-	return t.execute(wr, data)
+	return t.execute(wr, data, nil)
 }
 
-func (t *Template) execute(wr io.Writer, data any) (err error) {
+// ExecuteFuncMap applies a parsed template to the specified data object,
+// FuncMap overlay and writes the output to wr.
+// If an error occurs executing the template or writing its output,
+// execution stops, but partial results may already have been written to
+// the output writer.
+// A template may be executed safely in parallel, although if parallel
+// executions share a Writer the output may be interleaved.
+//
+// If data is a reflect.Value, the template applies to the concrete
+// value that the reflect.Value holds, as in fmt.Print.
+func (t *Template) ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error {
+	return t.execute(wr, data, funcs)
+}
+
+func (t *Template) execute(wr io.Writer, data any, funcs FuncMap) (err error) {
 	defer errRecover(&err)
 	value, ok := data.(reflect.Value)
 	if !ok {
@@ -211,7 +241,9 @@ func (t *Template) execute(wr io.Writer, data any) (err error) {
 		tmpl: t,
 		wr:   wr,
 		vars: []variable{{"$", value}},
+                funcs: map[string]reflect.Value{},
 	}
+	addValueFuncs(state.funcs, funcs)
 	if t.Tree == nil || t.Root == nil {
 		state.errorf("%q is an incomplete or empty template", t.Name())
 	}
@@ -436,6 +468,7 @@ func (s *state) walkTemplate(dot reflect.Value, t *parse.TemplateNode) {
 	newState.tmpl = tmpl
 	// No dynamic scoping: template invocations inherit no variables.
 	newState.vars = []variable{{"$", dot}}
+	newState.funcs = s.funcs
 	newState.walk(dot, tmpl.Root)
 }
 
@@ -594,9 +627,14 @@ func (s *state) evalFieldChain(dot, receiver reflect.Value, node parse.Node, ide
 func (s *state) evalFunction(dot reflect.Value, node *parse.IdentifierNode, cmd parse.Node, args []parse.Node, final reflect.Value) reflect.Value {
 	s.at(node)
 	name := node.Ident
-	function, isBuiltin, ok := findFunction(name, s.tmpl)
-	if !ok {
-		s.errorf("%q is not a defined function", name)
+	isBuiltin := false
+	function := s.funcs[name]
+	if !function.IsValid() {
+		ok := false
+		function, isBuiltin, ok = findFunction(name, s.tmpl)
+		if !ok {
+			s.errorf("%q is not a defined function", name)
+		}
 	}
 	return s.evalCall(dot, function, isBuiltin, cmd, name, args, final)
 }

As you can see this very small API change makes for a very clean and highly efficient way of providing execution FuncMap overlays.

zeripath added a commit to zeripath/go that referenced this issue Aug 13, 2022
There have been several requests for the ability to add or overlay
functions at time of template execution. One suggestion was to optimize
the template Clone feature to avoid creating a new function or API
however, this would be very complicated to do. (golang#38114)

This PR suggests a simpler solution.

Add the following new functions to text/template and html/template:

```go
func ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error { ... }

func ExecuteTemplateFuncMap(wr io.Writer, name string, data any, funcs FuncMap) error { ... }
```

These functions then simply allow one to overlay a new FuncMap over the
already parsed functions. This requires a function map to be added to
the execution state which is then further passed to sub-template
executions.

As you can see the required change is quite simple and avoids the need
for complex locking changes or parent-child relationships in Template.

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/go that referenced this issue Aug 13, 2022
There have been several requests for the ability to add or overlay
functions at time of template execution. One suggestion was to optimize
the template Clone feature to avoid creating a new function or API
however, this would be very complicated to do. (golang#38114)

This PR suggests a simpler solution.

Add the following new functions to text/template and html/template:

```go
func ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error { ... }

func ExecuteTemplateFuncMap(wr io.Writer, name string, data any, funcs FuncMap) error { ... }
```

These functions then simply allow one to overlay a new FuncMap over the
already parsed functions. This requires a function map to be added to
the execution state which is then further passed to sub-template
executions.

As you can see the required change is quite simple and avoids the need
for complex locking changes or parent-child relationships in Template.

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/go that referenced this issue Aug 13, 2022
There have been several requests for the ability to add or overlay
functions at time of template execution. One suggestion was to optimize
the template Clone feature to avoid creating a new function or API
however, this would be very complicated to do. (golang#38114)

This PR suggests a simpler solution.

Add the following new functions to text/template and html/template:

```go
func ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error { ... }

func ExecuteTemplateFuncMap(wr io.Writer, name string, data any, funcs FuncMap) error { ... }
```

These functions then simply allow one to overlay a new FuncMap over the
already parsed functions. This requires a function map to be added to
the execution state which is then further passed to sub-template
executions.

As you can see the required change is quite simple and avoids the need
for complex locking changes or parent-child relationships in Template.
@gopherbot
Copy link

Change https://go.dev/cl/423554 mentions this issue: text/template: add ExecuteFuncMap and ExecuteTemplateFuncMap

@wxiaoguang
Copy link

I think the PR text/template: add ExecuteFuncMap and ExecuteTemplateFuncMap #54432 is really helpful, and good enough.

How to push the proposal & progress? Thank you very much.

@wxiaoguang
Copy link

wxiaoguang commented Apr 10, 2023

Clone doesn't work according to my test.

For a site with 400 templates:

  1. It wastes too much memory , 55M (no clone) vs 3.7G (clone)
  2. It's quite slow, each clone takes ~ 10-20ms, the program startup time increases from nearly zero to 10s.

I think "proposal: text/template, html/template: Add ExecuteFuncMap and ExecuteTemplateFuncMap #54450" and "text/template: add ExecuteFuncMap and ExecuteTemplateFuncMap #54432" are the right approach.

@rsc
Copy link
Contributor Author

rsc commented Jul 18, 2023

Right, I was suggestion an optimized Clone, not the current Clone. There is no need to make the copy of everything eagerly. It can all be done lazily. That said, it is undeniably less work to add ExecuteFuncs instead, and it's more convenient for callers too, so I've changed my mind to do that. Closing this.

@rsc rsc closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants