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.

Sign up for GitHub

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

Jump to bottom

Speed up tpl #11351

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Speed up tpl #11351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db4f330
Speed up `tpl`
greed42 Sep 8, 2022
e2a7c79
Remove the 'reference templates' concept
greed42 Oct 20, 2022
a7d3fd6
Allow a nested `tpl` invocation access to `defines` in a containing one
greed42 Oct 20, 2022
95905f1
Work around template.Clone omitting options
greed42 May 11, 2023
97dbb96
Merge remote-tracking branch 'origin/main' into fast-tpl
greed42 Jul 31, 2023
36d417d
Test update for "Speed up `tpl`"
greed42 Oct 20, 2022
b261a1b
Test update for "Allow a nested `tpl` invocation access to `defines` …
greed42 Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 51 additions & 43 deletions pkg/engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,10 @@ func warnWrap(warn string) string {
return warnStartDelim + warn + warnEndDelim
}

// initFunMap creates the Engine's FuncMap and adds context-specific functions.
func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]renderable) {
funcMap := funcMap()
includedNames := make(map[string]int)

// Add the 'include' function here so we can close over t.
funcMap["include"] = func(name string, data interface{}) (string, error) {
// 'include' needs to be defined in the scope of a 'tpl' template as
// well as regular file-loaded templates.
func includeFun(t *template.Template, includedNames map[string]int) func(string, interface{}) (string, error) {
return func(name string, data interface{}) (string, error) {
var buf strings.Builder
if v, ok := includedNames[name]; ok {
if v > recursionMaxNums {
Expand All @@ -132,33 +129,62 @@ 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")
// 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.Basepath from values inside tpl function: %s", tpl)
return "", errors.Wrapf(err, "cannot clone template")
}

templateName, err := vals.PathValue("Template.Name")
if err != nil {
return "", errors.Wrapf(err, "cannot retrieve Template.Name from values inside tpl function: %s", tpl)
// Re-inject the missingkey option, see text/template issue https://github.com/golang/go/issues/43022
// We have to go by strict from our engine configuration, as the option fields are private in Template.
// TODO: Remove workaround (and the strict parameter) once we build only with golang versions with a fix.
if strict {
t.Option("missingkey=error")
} else {
t.Option("missingkey=zero")
}

templates := map[string]renderable{
templateName.(string): {
tpl: tpl,
vals: vals,
basePath: basePath.(string),
},
// 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)
}

result, err := e.renderWithReferences(templates, referenceTpls)
if err != nil {
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) {
Expand Down Expand Up @@ -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) {
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) {
func (e Engine) render(tpls 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.
Expand All @@ -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]
Expand All @@ -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.
Expand Down
39 changes: 26 additions & 13 deletions pkg/engine/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}`)},
},
}
Expand Down Expand Up @@ -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 {
Expand All @@ -1001,20 +999,30 @@ 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
// partial template defined only in the manifest invoking tpl cannot be accessed
// by that tpl call.
//{Name: "templates/manifest-only", Data: []byte(
// `{{define "manifest-only"}}only-in-manifest{{end}}` +
// `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}}` +
`before: {{include "manifest-only" .}}\n{{tpl .Values.manifestOnlyText .}}\nafter: {{include "manifest-only" .}}`,
)},
{Name: "templates/nested", Data: []byte(
`{{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{
"Values": chartutil.Values{
"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{
Expand All @@ -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/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/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/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 {
Expand Down