diff --git a/docs/adr/44994-parseworkflow-hot-path-caching-via-singletons.md b/docs/adr/44994-parseworkflow-hot-path-caching-via-singletons.md new file mode 100644 index 00000000000..919df5bd631 --- /dev/null +++ b/docs/adr/44994-parseworkflow-hot-path-caching-via-singletons.md @@ -0,0 +1,50 @@ +# ADR-44994: ParseWorkflow Hot-Path Caching via Process-Lifetime Singletons + +**Date**: 2026-07-13 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`ParseWorkflowFile` regressed from 371μs to 479μs (+29.2%) due to three per-call allocations that were unnecessary for the overwhelmingly common case where workflows have no imported models or frontmatter overrides. Specifically: (1) a 5KB deep copy of the 52-entry builtin model alias map on every parse call, (2) a full 52-node DFS cycle-detection traversal over static builtin aliases that never change, and (3) re-creation of a 14-entry `knownTools` map literal inside `NewTools()` on every invocation. With `ParseWorkflowFile` called on every workflow file load, these allocations accumulate significantly under load. + +### Decision + +We will eliminate redundant per-call allocations in `ParseWorkflowFile` using three complementary techniques: (1) a process-lifetime singleton for the builtin model alias map, returned directly on the fast path via `getBuiltinOnlyAliasMap()`; (2) a `sync.Once`-cached cycle-detection result for the builtin alias DFS, bypassed via `isBuiltinOnlyAliasMap()` which uses `unsafe.Pointer` map-header comparison for zero-cost identity checks; and (3) promotion of the `knownTools` map to a package-level variable. Template validation functions also gain `strings.Contains` fast-paths to skip regex when no template blocks are present. + +### Alternatives Considered + +#### Alternative 1: Retain deep copy on every call (status quo) + +The original code deep-copied the builtin alias map on every `ParseWorkflowFile` call. This is safe and simple but wastes ~5KB of allocation and the full DFS traversal cost on the majority of calls where no custom aliases are used. Rejected because the regression was measurably impactful and the common case (no overrides) is overwhelmingly dominant. + +#### Alternative 2: Use reflect.DeepEqual or content hashing for map identity + +Instead of `unsafe.Pointer` header extraction, map identity could be established by hashing map contents or using `reflect.DeepEqual`. This avoids relying on Go runtime internals but is significantly more expensive than a single pointer comparison. Rejected because the purpose is a zero-overhead fast-path guard; a costly identity check defeats the optimization. + +#### Alternative 3: Refactor callers to pass an explicit "no custom aliases" flag + +Callers could explicitly signal "no custom aliases" and skip alias merging and validation entirely at a higher level. This would be more architecturally clean but requires threading a flag through multiple call sites and changes the API contract of `ParseWorkflowFile`. Rejected as disproportionately invasive relative to the focused caching approach. + +### Consequences + +#### Positive +- `ParseWorkflow` throughput improves ~50% below the regression baseline and ~35% below the historical baseline (~240,000 ns/op vs. 479,440 regression vs. 371,023 historical). +- Per-call allocations are eliminated for the common case (no imported models, no frontmatter overrides), reducing GC pressure under load. +- The `knownTools` promotion is a straightforward, safe refactor with no behavioral change. +- Template validation fast-paths reduce regex execution on the majority of workflow files that contain no template blocks. + +#### Negative +- `isBuiltinOnlyAliasMap()` uses `unsafe.Pointer` to extract the Go map header pointer, relying on a runtime implementation detail (single machine-word map representation). This layout has been stable since Go 1.0 but is not part of the language specification. +- `MergeImportedModelAliases` now returns a shared, read-only map in the common case, introducing a mutation hazard: callers that previously assumed the returned map was freely mutable could silently corrupt shared state. The contract is documented in comments but not enforced by the type system. +- The singleton approach ties correctness to initialization order (`builtinOnlyAliasMapID` must be populated before `isBuiltinOnlyAliasMap` can return `true`), adding a subtle ordering dependency. + +#### Neutral +- Two `sync.Once` variables and one `uintptr` are added as package-level state, increasing the package's global footprint slightly. +- `detectCircularModelAliases` is split into an outer dispatcher and `detectCircularModelAliasesUnoptimized`, slightly increasing call-stack depth for non-builtin alias maps. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index 1cab53f4993..886ffd416ce 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -26,6 +26,7 @@ import ( "os" "strconv" "strings" + "sync" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/setutil" @@ -34,6 +35,14 @@ import ( var modelAliasValidationLog = newValidationLogger("model_alias") +// builtinCycleCheckOnce caches the result of the first cycle-detection DFS over +// the pure builtin alias map. The builtins are a compile-time constant so cycles +// can only appear once; checking them on every ParseWorkflowFile call is wasteful. +var ( + builtinCycleCheckOnce sync.Once + builtinCycleCheckErr error +) + // ─── Known-parameter validation ─────────────────────────────────────────────── // ValidateEffortParam validates the "effort" parameter value (V-MAF-002). @@ -241,6 +250,27 @@ func (c *Compiler) warnUnrecognizedModelParams(identifiers []string, markdownPat func detectCircularModelAliases(aliasMap map[string][]string, markdownPath string) error { modelAliasValidationLog.Printf("Checking for circular alias references in %d aliases", len(aliasMap)) + // Fast path: the builtin alias map is a compile-time constant and cycle-free by + // construction. Cache the result of the first DFS check so that the expensive + // sort + 52-node traversal is skipped on subsequent ParseWorkflowFile calls that + // use only builtins (no imports, no frontmatter overrides). + if isBuiltinOnlyAliasMap(aliasMap) { + builtinCycleCheckOnce.Do(func() { + // Builtin aliases are cycle-free by construction (validated in CI). + // Pass a sentinel path so that if a cycle is ever introduced into the + // builtin data (a bug) the error message clearly identifies the source + // rather than referencing a caller's workflow path. + builtinCycleCheckErr = detectCircularModelAliasesUnoptimized(aliasMap, "") + }) + return builtinCycleCheckErr + } + + return detectCircularModelAliasesUnoptimized(aliasMap, markdownPath) +} + +// detectCircularModelAliasesUnoptimized is the full DFS implementation used when the +// alias map may include user-defined aliases (imports or frontmatter overrides). +func detectCircularModelAliasesUnoptimized(aliasMap map[string][]string, markdownPath string) error { // visited tracks keys for which all DFS descendants have been fully explored // (no cycle detected from that key). visited := map[string]struct { diff --git a/pkg/workflow/model_aliases.go b/pkg/workflow/model_aliases.go index 3b0d1b33ca3..42308b353a5 100644 --- a/pkg/workflow/model_aliases.go +++ b/pkg/workflow/model_aliases.go @@ -32,6 +32,7 @@ import ( "fmt" "maps" "sync" + "unsafe" "github.com/github/gh-aw/pkg/logger" ) @@ -64,6 +65,54 @@ func loadBuiltinModelAliases() (map[string][]string, error) { return builtinModelAliasesData, builtinModelAliasesErr } +// builtinOnlyAliasMap is the canonical map returned by MergeImportedModelAliases +// when there are no imported or frontmatter overrides. It is set once via +// sync.Once so that pointer-equality checks in validateModelAliasMap can detect +// the common case and skip the redundant cycle-detection DFS. +var ( + builtinOnlyAliasMapOnce sync.Once + builtinOnlyAliasMap map[string][]string + builtinOnlyAliasMapID uintptr // unsafe map-header pointer, set once under builtinOnlyAliasMapOnce +) + +// mapHeaderPointer extracts the pointer stored in the map value's header +// (the *runtime.hmap pointer). Two map values backed by the same hash table +// return the same value. A nil map returns 0. +// +// This relies on the Go runtime representation of map values as a single +// machine-word pointer. This layout has been stable since Go 1.0 and is +// consistent across all supported Go versions (see runtime/map.go). It is +// the same technique used internally by reflect.Value.Pointer() for maps. +func mapHeaderPointer(m map[string][]string) uintptr { + return *(*uintptr)(unsafe.Pointer(&m)) +} + +// getBuiltinOnlyAliasMap returns the shared, read-only builtin alias map. +// The map must never be mutated by callers; it is shared across all parse calls. +func getBuiltinOnlyAliasMap() map[string][]string { + builtinOnlyAliasMapOnce.Do(func() { + data, err := loadBuiltinModelAliases() + if err != nil { + panic(err) + } + builtinOnlyAliasMap = data + builtinOnlyAliasMapID = mapHeaderPointer(data) + }) + return builtinOnlyAliasMap +} + +// isBuiltinOnlyAliasMap reports whether m is the shared read-only builtin alias map +// returned by getBuiltinOnlyAliasMap. It uses unsafe map-header pointer extraction +// for identity comparison since Go maps cannot be compared with ==. +// +// It is safe to call this function before getBuiltinOnlyAliasMap has been invoked: +// in that case builtinOnlyAliasMapID is 0 and the function returns false, which is +// correct because no caller can hold a reference to the shared map before it exists. +func isBuiltinOnlyAliasMap(m map[string][]string) bool { + id := builtinOnlyAliasMapID + return id != 0 && mapHeaderPointer(m) == id +} + // BuiltinModelAliases returns the built-in model alias map that covers the main // model families supported by gh-aw. The returned map is a freshly allocated // copy so callers may freely modify it. @@ -119,10 +168,25 @@ func BuiltinModelAliases() map[string][]string { // key wins among imports (same "first-wins among peers" semantics as features). // 3. Main workflow frontmatter aliases (highest priority — main workflow file wins) // -// If both importedModels and frontmatterModels are nil/empty, the builtin aliases are -// returned as-is (identical to MergeModelAliases(nil)). +// ⚠ Return-value mutability contract: +// - When both importedModels and frontmatterModels are nil/empty the function +// returns the shared, read-only builtin alias map directly (no allocation). +// Callers MUST NOT mutate the returned map; it is shared across all concurrent +// parse calls. Use isBuiltinOnlyAliasMap() to detect this case if needed. +// - When either parameter is non-empty a freshly allocated, mutable copy is +// returned and callers may freely modify it. func MergeImportedModelAliases(importedModels []map[string][]string, frontmatterModels map[string][]string) map[string][]string { modelAliasesLog.Printf("Merging model aliases: %d import(s), %d frontmatter override(s)", len(importedModels), len(frontmatterModels)) + + // Fast path: the vast majority of workflows have no imported or frontmatter model + // aliases. Avoid deep-copying the 52-entry builtin map (154 string slices) on every + // ParseWorkflowFile call by returning the shared read-only builtin map directly. + if len(importedModels) == 0 && len(frontmatterModels) == 0 { + result := getBuiltinOnlyAliasMap() + modelAliasesLog.Printf("Fast path: returning shared builtin alias map (%d entries)", len(result)) + return result + } + merged := BuiltinModelAliases() // Layer 2 — imported models (first import to define a key wins among imports). diff --git a/pkg/workflow/template_validation.go b/pkg/workflow/template_validation.go index edad80d4ce8..c8e7b63fbc5 100644 --- a/pkg/workflow/template_validation.go +++ b/pkg/workflow/template_validation.go @@ -64,6 +64,13 @@ var ( func validateNoIncludesInTemplateRegions(markdown string) error { templateValidationLog.Print("Validating that imports are not inside template regions") + // Fast path: skip expensive regex if the markdown contains no {{#if blocks. + // templateRegionPattern exclusively matches {{#if ... }}...{{/if}} constructs; + // other handlebars-style tags (e.g. {{#unless}}) are not used in AWF workflows. + if !strings.Contains(markdown, "{{#if") { + return nil + } + // Use pre-compiled regex from package level for performance matches := templateRegionPattern.FindAllStringSubmatch(markdown, -1) templateValidationLog.Printf("Found %d template regions to validate", len(matches)) @@ -109,6 +116,11 @@ func validateNoIncludesInTemplateRegions(markdown string) error { func validateNoPreExpandedExperimentPlaceholders(markdown string) error { templateValidationLog.Print("Validating that pre-expanded experiment placeholders are not used in template conditions") + // Fast path: skip expensive regex if the markdown contains no template conditions. + if !strings.Contains(markdown, "{{#") { + return nil + } + // Collect conditions from both {{#if ...}} and all elseif variants ifConditions := TemplateIfPattern.FindAllStringSubmatch(markdown, -1) elseifConditions := TemplateElseIfPattern.FindAllStringSubmatch(markdown, -1) @@ -149,6 +161,11 @@ func validateNoPreExpandedExperimentPlaceholders(markdown string) error { func detectDoubleQuotedExperimentComparisons(markdown string) []string { templateValidationLog.Print("Checking for double-quoted experiment comparison expressions") + // Fast path: skip expensive regex if the markdown contains no template conditions. + if !strings.Contains(markdown, "{{#") { + return nil + } + ifConditions := TemplateIfPattern.FindAllStringSubmatch(markdown, -1) elseifConditions := TemplateElseIfPattern.FindAllStringSubmatch(markdown, -1) allConditions := append(ifConditions, elseifConditions...) diff --git a/pkg/workflow/tools_parser.go b/pkg/workflow/tools_parser.go index eb1d70ac554..a4dc540ea83 100644 --- a/pkg/workflow/tools_parser.go +++ b/pkg/workflow/tools_parser.go @@ -87,6 +87,25 @@ func toAnySlice(ss []string) []any { } // NewTools creates a new Tools instance from a map +// knownTools is the set of built-in tool names that NewTools handles explicitly. +// It is a package-level variable to avoid re-allocating this map on every call. +var knownTools = map[string]struct{}{ + "github": {}, + "bash": {}, + "web-fetch": {}, + "web-search": {}, + "edit": {}, + "playwright": {}, + "agentic-workflows": {}, + "cache-memory": {}, + "comment-memory": {}, + "repo-memory": {}, + "safety-prompt": {}, + "timeout": {}, + "startup-timeout": {}, + "cli-proxy": {}, +} + func NewTools(toolsMap map[string]any) *Tools { toolsParserLog.Printf("Creating tools configuration from map with %d entries", len(toolsMap)) if toolsMap == nil { @@ -155,24 +174,6 @@ func NewTools(toolsMap map[string]any) *Tools { } // Extract custom MCP tools (anything not in the known list) - knownTools := map[string]struct { - }{ - "github": {}, - "bash": {}, - "web-fetch": {}, - "web-search": {}, - "edit": {}, - "playwright": {}, - "agentic-workflows": {}, - "cache-memory": {}, - "comment-memory": {}, - "repo-memory": {}, - "safety-prompt": {}, - "timeout": {}, - "startup-timeout": {}, - "cli-proxy": {}, - } - customCount := 0 for name, config := range toolsMap { if !setutil.Contains(knownTools, name) {