perf: fix ParseWorkflow regression (+29% → -51%) via targeted caching #44994
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)) | ||
|
Contributor
There was a problem hiding this comment. Undocumented runtime internals in This function deferences the map variable as a Risks:
Prefer using import "reflect"
func mapHeaderPointer(m map[string][]string) uintptr {
return reflect.ValueOf(m).Pointer()
}This is zero additional allocation (reflect.Value is stack-allocated for non-interface values) and removes the dependency on undocumented layout. @copilot please address this.
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] 💡 Simpler alternative without unsafeSince var builtinOnlyAliasMapSet atomic.Bool
func getBuiltinOnlyAliasMap() map[string][]string {
builtinOnlyAliasMapOnce.Do(func() {
data, _ := loadBuiltinModelAliases() // handle error appropriately
builtinOnlyAliasMap = data
builtinOnlyAliasMapSet.Store(true)
})
return builtinOnlyAliasMap
}
func isBuiltinOnlyAliasMap(m map[string][]string) bool {
// A caller can only hold the shared map after getBuiltinOnlyAliasMap returns it.
// If the set flag is false, no caller can hold it yet.
return builtinOnlyAliasMapSet.Load() && m != nil && len(m) == len(builtinOnlyAliasMap) &&
// cheap pointer check via reflect for read-only assertion
reflect.ValueOf(m).Pointer() == reflect.ValueOf(builtinOnlyAliasMap).Pointer()
}Or more simply, just keep a @copilot please address this. |
||
| } | ||
|
|
||
| // 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) | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] 💡 Suggested fix: return the error insteadChange the signature to func getBuiltinOnlyAliasMap() (map[string][]string, error) {
builtinOnlyAliasMapOnce.Do(func() {
data, err := loadBuiltinModelAliases()
if err != nil {
builtinOnlyAliasMapErr = err
return
}
builtinOnlyAliasMap = data
builtinOnlyAliasMapID = mapHeaderPointer(data)
})
return builtinOnlyAliasMap, builtinOnlyAliasMapErr
}This keeps error surfacing consistent with how @copilot please address this. |
||
| } | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Data race on
Fix: replace the plain // var block
builtinOnlyAliasMapID atomic.Uintptr
// getBuiltinOnlyAliasMap Once.Do:
builtinOnlyAliasMapID.Store(mapHeaderPointer(data))
// isBuiltinOnlyAliasMap:
id := builtinOnlyAliasMapID.Load()@copilot please address this.
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] The write on line 99 happens inside 💡 Fix: use atomic.Uintptrimport "sync/atomic"
var builtinOnlyAliasMapID atomic.Uintptr
// in Once.Do:
builtinOnlyAliasMapID.Store(mapHeaderPointer(data))
// in isBuiltinOnlyAliasMap:
id := builtinOnlyAliasMapID.Load()
return id != 0 && mapHeaderPointer(m) == id@copilot please address this. |
||
| return id != 0 && mapHeaderPointer(m) == id | ||
|
Comment on lines
+112
to
+113
|
||
| } | ||
|
|
||
| // 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) | ||
| // | ||
| // ⚠ 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. | ||
|
Comment on lines
+173
to
+175
|
||
| // - 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() | ||
|
Contributor
There was a problem hiding this comment. Shared singleton returned to callers who may mutate it When both inputs are empty, The doc comment alone is an insufficient guard against this class of bug in a growing codebase. Consider one of:
@copilot please address this.
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] The returned shared map has no mutation guard — any caller that accidentally writes to it (e.g., a test doing 💡 Suggested safeguardAdd a test that verifies mutation attempts on the returned map are caught early: func TestMergeImportedModelAliases_NoMutationOfBuiltinMap(t *testing.T) {
result := MergeImportedModelAliases(nil, nil)
before := len(result)
// Deliberately mutate — should be safe only because callers won't do this,
// but document that it would corrupt state:
// result["should-not-be-here"] = []string{"x"} // this would break other tests
_ = before
// At minimum, assert the returned map equals the builtin map by reference
assert.True(t, isBuiltinOnlyAliasMap(result), "fast-path should return the shared builtin map")
}If enforcing immutability at runtime is critical, consider wrapping in a type that panics on write rather than relying on documentation alone. @copilot please address this. |
||
| 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). | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The PR fixes a performance regression but adds no benchmark assertions or regression guard. The existing
BenchmarkParseWorkflowwill detect future regressions only if someone remembers to run it with-benchtimeand compare — it's a passive check.💡 Suggested addition
Add a sub-benchmark or
TestMain-style baseline guard that fails ifParseWorkflowexceeds a threshold, e.g.:Alternatively, document the expected ns/op threshold in a comment on
BenchmarkParseWorkflowso reviewers have context when they see benchmark output in CI.@copilot please address this.