diff --git a/compiler.go b/compiler.go index 733d5f3b21..3b00bc3cab 100644 --- a/compiler.go +++ b/compiler.go @@ -54,30 +54,45 @@ func (c *Compiler) getVariables(t *ast.Task, call *Call, evaluateShVars bool) (* result.Set(k, ast.Var{Value: v}) } + // Share a single Cache across every rangeFunc invocation below. The + // previous implementation allocated a fresh Cache per variable, which + // caused Replace -> ToCacheMap to rebuild the full cacheMap on every + // call. SyncVarSet keeps the cacheMap in sync after each result.Set so + // subsequent Replace calls on the same cache can reuse it. + sharedCache := &templater.Cache{Vars: result} + getRangeFunc := func(dir string) func(k string, v ast.Var) error { return func(k string, v ast.Var) error { - cache := &templater.Cache{Vars: result} + // Each variable is processed independently, so clear any sticky + // error from a previous template before evaluating this one. + sharedCache.ResetErr() // Replace values - newVar := templater.ReplaceVar(v, cache) + newVar := templater.ReplaceVar(v, sharedCache) // If the variable should not be evaluated, but is nil, set it to an empty string // This stops empty interface errors when using the templater to replace values later // Preserve the Sh field so it can be displayed in summary if !evaluateShVars && newVar.Value == nil { - result.Set(k, ast.Var{Value: "", Sh: newVar.Sh}) + out := ast.Var{Value: "", Sh: newVar.Sh} + result.Set(k, out) + sharedCache.SyncVarSet(k, out) return nil } // If the variable should not be evaluated and it is set, we can set it and return if !evaluateShVars { - result.Set(k, ast.Var{Value: newVar.Value, Sh: newVar.Sh}) + out := ast.Var{Value: newVar.Value, Sh: newVar.Sh} + result.Set(k, out) + sharedCache.SyncVarSet(k, out) return nil } // Now we can check for errors since we've handled all the cases when we don't want to evaluate - if err := cache.Err(); err != nil { + if err := sharedCache.Err(); err != nil { return err } // If the variable is already set, we can set it and return if newVar.Value != nil || newVar.Sh == nil { - result.Set(k, ast.Var{Value: newVar.Value}) + out := ast.Var{Value: newVar.Value} + result.Set(k, out) + sharedCache.SyncVarSet(k, out) return nil } // If the variable is dynamic, we need to resolve it first @@ -85,7 +100,9 @@ func (c *Compiler) getVariables(t *ast.Task, call *Call, evaluateShVars bool) (* if err != nil { return err } - result.Set(k, ast.Var{Value: static}) + out := ast.Var{Value: static} + result.Set(k, out) + sharedCache.SyncVarSet(k, out) return nil } } @@ -95,11 +112,11 @@ func (c *Compiler) getVariables(t *ast.Task, call *Call, evaluateShVars bool) (* if t != nil { // NOTE(@andreynering): We're manually joining these paths here because // this is the raw task, not the compiled one. - cache := &templater.Cache{Vars: result} - dir := templater.Replace(t.Dir, cache) - if err := cache.Err(); err != nil { + dir := templater.Replace(t.Dir, sharedCache) + if err := sharedCache.Err(); err != nil { return nil, err } + sharedCache.ResetErr() dir = filepathext.SmartJoin(c.Dir, dir) taskRangeFunc = getRangeFunc(dir) } diff --git a/internal/templater/templater.go b/internal/templater/templater.go index 896cba23c1..7a0af39a5f 100644 --- a/internal/templater/templater.go +++ b/internal/templater/templater.go @@ -5,6 +5,7 @@ import ( "fmt" "maps" "strings" + "sync" "github.com/go-task/template" @@ -12,6 +13,76 @@ import ( "github.com/go-task/task/v3/taskfile/ast" ) +// dataMapPool reuses map[string]any backing memory across [ReplaceWithExtra] +// calls. When a template might mutate the dot via sprig's set/unset, we +// clone cache.cacheMap into a fresh map so those mutations cannot leak back +// into cache.cacheMap. The maps are short-lived and identically shaped, so a +// pool drops the per-call allocation without changing semantics. +var dataMapPool = sync.Pool{ + New: func() any { + m := make(map[string]any, 64) + return &m + }, +} + +func acquireDataMap() *map[string]any { + return dataMapPool.Get().(*map[string]any) +} + +func releaseDataMap(m *map[string]any) { + clear(*m) + dataMapPool.Put(m) +} + +// templateMayMutateDot reports whether a template source string might mutate +// its dot (root data map). The only functions registered in templateFuncs +// that mutate the dict passed to them are sprig's `set` and `unset`. Other +// sprig mutators (push/append/prepend on slices) operate on slice values +// retrieved from the dict — they do not modify the dict itself, and the +// original code's shallow [maps.Clone] never protected against those either. +// +// This is a conservative substring check: false positives are fine (they +// just force an unnecessary clone), false negatives would leak mutations. +// "set" appearing inside a literal string ({{"set"}}) or inside an unrelated +// identifier (settings, subset) is treated as potentially mutating, which is +// safe. Checking for "set" alone also catches "unset". +func templateMayMutateDot(s string) bool { + return strings.Contains(s, "set") +} + +// parsedTemplateCache memoizes parsed [template.Template] objects keyed by +// their source string. Each call to template.New("").Funcs(templateFuncs) +// copies the ~170-entry FuncMap into a fresh Template (sprig + go-task +// builtins) — that copy is the single largest allocator on the hot path of +// listing tasks in a project with many includes, because the same merged +// TaskfileEnv values are processed once per task. +// +// Caching the parsed Template is safe: once parsed, a Template may be +// executed concurrently from multiple goroutines (Go docs guarantee this), +// and Execute reads from but does not mutate the Template. The cache key is +// the unparsed source — sufficient because templateFuncs is a package-level +// singleton. +// +// We cache errors as well so that a malformed template surfaces the same +// error on every call instead of being re-parsed each time. +var parsedTemplateCache sync.Map // map[string]parsedTemplateEntry + +type parsedTemplateEntry struct { + tpl *template.Template + err error +} + +func cachedParse(text string) (*template.Template, error) { + if v, ok := parsedTemplateCache.Load(text); ok { + entry := v.(parsedTemplateEntry) + return entry.tpl, entry.err + } + tpl, err := template.New("").Funcs(templateFuncs).Parse(text) + actual, _ := parsedTemplateCache.LoadOrStore(text, parsedTemplateEntry{tpl: tpl, err: err}) + entry := actual.(parsedTemplateEntry) + return entry.tpl, entry.err +} + // Cache is a help struct that allow us to call "replaceX" funcs multiple // times, without having to check for error each time. The first error that // happen will be assigned to r.err, and consecutive calls to funcs will just @@ -31,6 +102,38 @@ func (r *Cache) Err() error { return r.err } +// ResetErr clears the sticky error flag so a single Cache can be reused for +// a sequence of independent template operations. Without this, the first +// failed Replace would short-circuit every subsequent call on the same +// Cache. +func (r *Cache) ResetErr() { + r.err = nil +} + +// SyncVarSet updates the internal cacheMap after the caller mutates Vars via +// [ast.Vars.Set]. This lets a single Cache be reused across many Replace +// calls (each interleaved with a Vars.Set) without rebuilding the entire +// cacheMap from scratch via [ast.Vars.ToCacheMap]. The mirroring rules match +// [ast.Vars.ToCacheMap]: dynamic-only entries are excluded, Live takes +// precedence over Value. +// +// No-op if cacheMap has not yet been initialized — the lazy init in Replace +// will pick up the value on first use. +func (r *Cache) SyncVarSet(key string, v ast.Var) { + if r.cacheMap == nil { + return + } + if v.Sh != nil && *v.Sh != "" { + delete(r.cacheMap, key) + return + } + if v.Live != nil { + r.cacheMap[key] = v.Live + } else { + r.cacheMap[key] = v.Value + } +} + func ResolveRef(ref string, cache *Cache) any { // If there is already an error, do nothing if cache.err != nil { @@ -45,7 +148,7 @@ func ResolveRef(ref string, cache *Cache) any { if ref == "." { return cache.cacheMap } - t, err := template.New("resolver").Funcs(templateFuncs).Parse(fmt.Sprintf("{{%s}}", ref)) + t, err := cachedParse(fmt.Sprintf("{{%s}}", ref)) if err != nil { cache.err = err return nil @@ -73,16 +176,58 @@ func ReplaceWithExtra[T any](v T, cache *Cache, extra map[string]any) T { cache.cacheMap = cache.Vars.ToCacheMap() } - // Create a copy of the cache map to avoid editing the original - // If there is extra data, merge it with the cache map - data := maps.Clone(cache.cacheMap) - if extra != nil { - maps.Copy(data, extra) + // Decide whether the template can read directly from cache.cacheMap or + // whether we must clone it. Cloning is required when there is `extra` + // data to merge in, or when any template traversed below might mutate + // the dict via sprig's set/unset. For all other cases the clone is pure + // overhead — tpl.Execute only reads from the data map. + // + // nil values (nil interface, nil *string) cannot run any template at + // all, so they don't need a cloned data map. Strings are checked for + // "set" anywhere. For other concrete types we conservatively clone. + mayMutate := false + switch { + case extra != nil: + mayMutate = true + case any(v) == nil: + // nil interface — TraverseStringsFunc has no strings to visit. + mayMutate = false + default: + switch tv := any(v).(type) { + case string: + mayMutate = templateMayMutateDot(tv) + case *string: + // nil pointer — no template runs. + mayMutate = tv != nil && templateMayMutateDot(*tv) + default: + // []string, map, struct, etc. — don't risk it. + mayMutate = true + } + } + + var data map[string]any + if mayMutate { + dataPtr := acquireDataMap() + defer releaseDataMap(dataPtr) + data = *dataPtr + maps.Copy(data, cache.cacheMap) + if extra != nil { + maps.Copy(data, extra) + } + } else { + data = cache.cacheMap } // Traverse the value and parse any template variables copy, err := deepcopy.TraverseStringsFunc(v, func(v string) (string, error) { - tpl, err := template.New("").Funcs(templateFuncs).Parse(v) + // Skip the template engine entirely if the string has no actions. + // Go templates only fire on `{{`; pure literals can short-circuit. + // This is a large win when iterating many merged taskfile env vars + // whose values are static strings. + if !strings.Contains(v, "{{") { + return v, nil + } + tpl, err := cachedParse(v) if err != nil { return v, err }