Merge pull request #11351 from greed42/fast-tpl

Speed up `tpl`

pkg/engine/engine.go

@@ -112,13 +112,10 @@ func warnWrap(warn string) string {
 	return warnStartDelim + warn + warnEndDelim
 }
 
-// initFunMap creates the Engine's FuncMap and adds context-specific functions.
+// 'include' needs to be defined in the scope of a 'tpl' template as
-func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]renderable) {
+// well as regular file-loaded templates.
-	funcMap := funcMap()
+func includeFun(t *template.Template, includedNames map[string]int) func(string, interface{}) (string, error) {
-	includedNames := make(map[string]int)
+	return func(name string, data interface{}) (string, error) {
-
-	// Add the 'include' function here so we can close over t.
-	funcMap["include"] = func(name string, data interface{}) (string, error) {
 		var buf strings.Builder
 		if v, ok := includedNames[name]; ok {
 			if v > recursionMaxNums {
@@ -132,34 +129,63 @@ func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]render
 		includedNames[name]--
 		return buf.String(), err
 	}
-
-	// Add the 'tpl' function here
-	funcMap["tpl"] = func(tpl string, vals chartutil.Values) (string, error) {
-		basePath, err := vals.PathValue("Template.BasePath")
-		if err != nil {
-			return "", errors.Wrapf(err, "cannot retrieve Template.Basepath from values inside tpl function: %s", tpl)
 }
 
-		templateName, err := vals.PathValue("Template.Name")
+// As does 'tpl', so that nested calls to 'tpl' see the templates
+// defined by their enclosing contexts.
+func tplFun(parent *template.Template, includedNames map[string]int, strict bool) func(string, interface{}) (string, error) {
+	return func(tpl string, vals interface{}) (string, error) {
+		t, err := parent.Clone()
 		if err != nil {
-			return "", errors.Wrapf(err, "cannot retrieve Template.Name from values inside tpl function: %s", tpl)
+			return "", errors.Wrapf(err, "cannot clone template")
 		}
 
-		templates := map[string]renderable{
+		// Re-inject the missingkey option, see text/template issue https://github.com/golang/go/issues/43022
-			templateName.(string): {
+		// We have to go by strict from our engine configuration, as the option fields are private in Template.
-				tpl:      tpl,
+		// TODO: Remove workaround (and the strict parameter) once we build only with golang versions with a fix.
-				vals:     vals,
+		if strict {
-				basePath: basePath.(string),
+			t.Option("missingkey=error")
-			},
+		} else {
+			t.Option("missingkey=zero")
 		}
 
-		result, err := e.renderWithReferences(templates, referenceTpls)
+		// Re-inject 'include' so that it can close over our clone of t;
+		// this lets any 'define's inside tpl be 'include'd.
+		t.Funcs(template.FuncMap{
+			"include": includeFun(t, includedNames),
+			"tpl":     tplFun(t, includedNames, strict),
+		})
+
+		// We need a .New template, as template text which is just blanks
+		// or comments after parsing out defines just addes new named
+		// template definitions without changing the main template.
+		// https://pkg.go.dev/text/template#Template.Parse
+		// Use the parent's name for lack of a better way to identify the tpl
+		// text string. (Maybe we could use a hash appended to the name?)
+		t, err = t.New(parent.Name()).Parse(tpl)
 		if err != nil {
+			return "", errors.Wrapf(err, "cannot parse template %q", tpl)
+		}
+
+		var buf strings.Builder
+		if err := t.Execute(&buf, vals); err != nil {
 			return "", errors.Wrapf(err, "error during tpl function execution for %q", tpl)
 		}
-		return result[templateName.(string)], nil
+
+		// See comment in renderWithReferences explaining the <no value> hack.
+		return strings.ReplaceAll(buf.String(), "<no value>", ""), nil
+	}
 }
 
+// initFunMap creates the Engine's FuncMap and adds context-specific functions.
+func (e Engine) initFunMap(t *template.Template) {
+	funcMap := funcMap()
+	includedNames := make(map[string]int)
+
+	// Add the template-rendering functions here so we can close over t.
+	funcMap["include"] = includeFun(t, includedNames)
+	funcMap["tpl"] = tplFun(t, includedNames, e.Strict)
+
 	// Add the `required` function here so we can use lintMode
 	funcMap["required"] = func(warn string, val interface{}) (interface{}, error) {
 		if val == nil {
@@ -210,13 +236,7 @@ func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]render
 }
 
 // render takes a map of templates/values and renders them.
-func (e Engine) render(tpls map[string]renderable) (map[string]string, error) {
+func (e Engine) render(tpls map[string]renderable) (rendered map[string]string, err error) {
-	return e.renderWithReferences(tpls, tpls)
-}
-
-// renderWithReferences takes a map of templates/values to render, and a map of
-// templates which can be referenced within them.
-func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) (rendered map[string]string, err error) {
 	// Basically, what we do here is start with an empty parent template and then
 	// build up a list of templates -- one for each file. Once all of the templates
 	// have been parsed, we loop through again and execute every template.
@@ -238,12 +258,11 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable)
 		t.Option("missingkey=zero")
 	}
 
-	e.initFunMap(t, referenceTpls)
+	e.initFunMap(t)
 
 	// We want to parse the templates in a predictable order. The order favors
 	// higher-level (in file system) templates over deeply nested templates.
 	keys := sortTemplates(tpls)
-	referenceKeys := sortTemplates(referenceTpls)
 
 	for _, filename := range keys {
 		r := tpls[filename]
@@ -252,17 +271,6 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable)
 		}
 	}
 
-	// Adding the reference templates to the template context
-	// so they can be referenced in the tpl function
-	for _, filename := range referenceKeys {
-		if t.Lookup(filename) == nil {
-			r := referenceTpls[filename]
-			if _, err := t.New(filename).Parse(r.tpl); err != nil {
-				return map[string]string{}, cleanupParseError(filename, err)
-			}
-		}
-	}
-
 	rendered = make(map[string]string, len(keys))
 	for _, filename := range keys {
 		// Don't render partials. We don't care out the direct output of partials.

pkg/engine/engine_test.go

@@ -948,8 +948,6 @@ func TestRenderTplTemplateNames(t *testing.T) {
 			{Name: "templates/default-name", Data: []byte(`{{tpl "{{ .Template.Name }}" .}}`)},
 			{Name: "templates/modified-basepath", Data: []byte(`{{tpl "{{ .Template.BasePath }}" .Values.dot}}`)},
 			{Name: "templates/modified-name", Data: []byte(`{{tpl "{{ .Template.Name }}" .Values.dot}}`)},
-			// Current implementation injects the 'tpl' template as if it were a template file, and
-			// so only BasePath and Name make it through.
 			{Name: "templates/modified-field", Data: []byte(`{{tpl "{{ .Template.Field }}" .Values.dot}}`)},
 		},
 	}
@@ -979,7 +977,7 @@ func TestRenderTplTemplateNames(t *testing.T) {
 		"TplTemplateNames/templates/default-name":      "TplTemplateNames/templates/default-name",
 		"TplTemplateNames/templates/modified-basepath": "path/to/template",
 		"TplTemplateNames/templates/modified-name":     "name-of-template",
-		"TplTemplateNames/templates/modified-field":    "",
+		"TplTemplateNames/templates/modified-field":    "extra-field",
 	}
 	for file, expect := range expects {
 		if out[file] != expect {
@@ -1001,13 +999,17 @@ func TestRenderTplRedefines(t *testing.T) {
 				`{{define "manifest"}}original-in-manifest{{end}}` +
 					`before: {{include "manifest" .}}\n{{tpl .Values.manifestText .}}\nafter: {{include "manifest" .}}`,
 			)},
-			// The current implementation replaces the manifest text and re-parses, so a
+			{Name: "templates/manifest-only", Data: []byte(
-			// partial template defined only in the manifest invoking tpl cannot be accessed
+				`{{define "manifest-only"}}only-in-manifest{{end}}` +
-			// by that tpl call.
+					`before: {{include "manifest-only" .}}\n{{tpl .Values.manifestOnlyText .}}\nafter: {{include "manifest-only" .}}`,
-			//{Name: "templates/manifest-only", Data: []byte(
+			)},
-			//	`{{define "manifest-only"}}only-in-manifest{{end}}` +
+			{Name: "templates/nested", Data: []byte(
-			//		`before: {{include "manifest-only" .}}\n{{tpl .Values.manifestOnlyText .}}\nafter: {{include "manifest-only" .}}`,
+				`{{define "nested"}}original-in-manifest{{end}}` +
-			//)},
+					`{{define "nested-outer"}}original-outer-in-manifest{{end}}` +
+					`before: {{include "nested" .}} {{include "nested-outer" .}}\n` +
+					`{{tpl .Values.nestedText .}}\n` +
+					`after: {{include "nested" .}} {{include "nested-outer" .}}`,
+			)},
 		},
 	}
 	v := chartutil.Values{
@@ -1015,6 +1017,12 @@ func TestRenderTplRedefines(t *testing.T) {
 			"partialText":      `{{define "partial"}}redefined-in-tpl{{end}}tpl: {{include "partial" .}}`,
 			"manifestText":     `{{define "manifest"}}redefined-in-tpl{{end}}tpl: {{include "manifest" .}}`,
 			"manifestOnlyText": `tpl: {{include "manifest-only" .}}`,
+			"nestedText": `{{define "nested"}}redefined-in-tpl{{end}}` +
+				`{{define "nested-outer"}}redefined-outer-in-tpl{{end}}` +
+				`before-inner-tpl: {{include "nested" .}} {{include "nested-outer" . }}\n` +
+				`{{tpl .Values.innerText .}}\n` +
+				`after-inner-tpl: {{include "nested" .}} {{include "nested-outer" . }}`,
+			"innerText": `{{define "nested"}}redefined-in-inner-tpl{{end}}inner-tpl: {{include "nested" .}} {{include "nested-outer" . }}`,
 		},
 		"Chart": c.Metadata,
 		"Release": chartutil.Values{
@@ -1028,9 +1036,14 @@ func TestRenderTplRedefines(t *testing.T) {
 	}
 
 	expects := map[string]string{
-		"TplRedefines/templates/partial":  `before: original-in-partial\ntpl: original-in-partial\nafter: original-in-partial`,
+		"TplRedefines/templates/partial":       `before: original-in-partial\ntpl: redefined-in-tpl\nafter: original-in-partial`,
 		"TplRedefines/templates/manifest":      `before: original-in-manifest\ntpl: redefined-in-tpl\nafter: original-in-manifest`,
-		//"TplRedefines/templates/manifest-only": `before: only-in-manifest\ntpl: only-in-manifest\nafter: only-in-manifest`,
+		"TplRedefines/templates/manifest-only": `before: only-in-manifest\ntpl: only-in-manifest\nafter: only-in-manifest`,
+		"TplRedefines/templates/nested": `before: original-in-manifest original-outer-in-manifest\n` +
+			`before-inner-tpl: redefined-in-tpl redefined-outer-in-tpl\n` +
+			`inner-tpl: redefined-in-inner-tpl redefined-outer-in-tpl\n` +
+			`after-inner-tpl: redefined-in-tpl redefined-outer-in-tpl\n` +
+			`after: original-in-manifest original-outer-in-manifest`,
 	}
 	for file, expect := range expects {
 		if out[file] != expect {