Skip to content

Commit

Permalink
Merge pull request #11351 from greed42/fast-tpl
Browse files Browse the repository at this point in the history 
Speed up `tpl`
  • Loading branch information
@sabre1041
sabre1041 committed Jan 8, 2024
2 parents 762a1c7 + b261a1b commit 77d54d7
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 56 deletions.
94 changes: 51 additions & 43 deletions pkg/engine/engine.go
View file
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
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

0 comments on commit 77d54d7

Please sign in to comment.