perf: fix ParseWorkflow regression (+29% → -51%) via targeted caching#44994
Conversation
- model_aliases.go: add isBuiltinOnlyAliasMap() using reflect.ValueOf().Pointer() for map identity check, plus builtinOnlyAliasMapID uintptr cache variable; MergeImportedModelAliases returns shared builtin map (no deep copy) when no imports/overrides present - avoids 154 string slice allocations per call - model_alias_validation.go: detectCircularModelAliases() fast-path caches DFS result for builtin-only map via builtinCycleCheckOnce (fixes invalid map==map comparison by using isBuiltinOnlyAliasMap() instead) - tools_parser.go: promote knownTools from local map literal to package-level variable to avoid 14-entry map allocation on every NewTools() call - template_validation.go: add strings.Contains fast-paths to three validation functions so they skip regex entirely for markdown with no template blocks Benchmark: 479μs (regression) → 235μs (local, -51%) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… cached error path - model_aliases.go: replace reflect.ValueOf().Pointer() with mapHeaderPointer() using unsafe.Pointer extraction; avoids reflection overhead on every call; remove redundant getBuiltinOnlyAliasMap() call from isBuiltinOnlyAliasMap() - model_alias_validation.go: pass empty path to builtin cycle check so a hypothetical cached error does not expose a caller's workflow path Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- model_aliases.go: expand mapHeaderPointer comment with Go version stability
note; expand MergeImportedModelAliases doc with prominent immutability warning
using ⚠ symbol
- model_alias_validation.go: use sentinel '<builtin-aliases>' path in builtin
cycle check instead of empty string for clearer hypothetical error messages
- template_validation.go: add comment explaining why '{{#if' is the correct
fast-path substring (templateRegionPattern exclusively matches {{#if blocks)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Draft PR — largest of the perf batch (132 additions, 4 files). Unmark draft and ensure CI passes before merging.
|
🤖 PR Triage
Rationale: Draft. Fixes ParseWorkflow +29% regression via targeted caching. Good approach; batch with other perf PRs for efficient review. Triage run §29183606049
|
🤖 PR Triage
Draft. ParseWorkflow caching improvement. +132/-20. Review in perf-regressions batch.
|
There was a problem hiding this comment.
Pull request overview
Optimizes workflow parsing through targeted caching and template-validation fast paths.
Changes:
- Shares and caches built-in model aliases and cycle validation.
- Reuses the built-in tool-name set.
- Skips template regex scans when conditions are absent.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tools_parser.go |
Reuses the known-tools map. |
pkg/workflow/template_validation.go |
Adds regex fast paths. |
pkg/workflow/model_aliases.go |
Introduces shared built-in aliases. |
pkg/workflow/model_alias_validation.go |
Caches built-in cycle checks. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Medium
| // 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. |
| id := builtinOnlyAliasMapID | ||
| return id != 0 && mapHeaderPointer(m) == id |
| 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, "{{#") { |
| 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, "{{#") { |
| // 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. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel analysis is not applicable. The PR modifies only production code (model_alias_validation.go, model_aliases.go, template_validation.go, tools_parser.go). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Review summary
The performance wins are real and the approach is sound overall, but there are two blocking correctness issues that should be fixed before merge.
Blocking
-
Data race on
builtinOnlyAliasMapID(model_aliases.go:112) — the field is read without synchronization outside thesync.Once, which is a data race under the Go memory model. Fix: useatomic.Uintptr. -
unsafe.Pointermap-header extraction (model_aliases.go:87) — relies on undocumented Go runtime internals.reflect.ValueOf(m).Pointer()is the documented, spec-stable equivalent with identical performance.
Non-blocking
- Shared singleton mutation risk (
model_aliases.go:185) — downstream code could silently corrupt the builtin singleton. The doc comment is a soft guard; consider a concurrentParseWorkflowFiletest run with-raceto enforce the invariant.
The template_validation.go fast-paths and tools_parser.go package-level promotion are clean and unambiguously correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 42.2 AIC · ⌖ 4.74 AIC · ⊞ 4.8K
| // 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 |
There was a problem hiding this comment.
Data race on builtinOnlyAliasMapID
isBuiltinOnlyAliasMap reads builtinOnlyAliasMapID on this line without any synchronization. The write happens inside builtinOnlyAliasMapOnce.Do (line 99), but the Go memory model only guarantees visibility to goroutines that participate in the Once; concurrent goroutines reading the variable directly can observe a stale (zero) value or, on weakly-ordered hardware, a torn read.
Fix: replace the plain uintptr with atomic.Uintptr:
// var block
builtinOnlyAliasMapID atomic.Uintptr
// getBuiltinOnlyAliasMap Once.Do:
builtinOnlyAliasMapID.Store(mapHeaderPointer(data))
// isBuiltinOnlyAliasMap:
id := builtinOnlyAliasMapID.Load()@copilot please address this.
| // 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)) |
There was a problem hiding this comment.
Undocumented runtime internals in mapHeaderPointer
This function deferences the map variable as a *uintptr to extract the internal *runtime.hmap pointer. While this layout has been stable in practice, it is not part of the Go language spec or any exported ABI contract, and the comment's claim that reflect.Value.Pointer() uses "the same technique" is misleading — reflect is part of the runtime and has privileged access.
Risks:
- A future Go version (especially with the register-based calling convention or potential map rewrite in Go 1.24+) could change map value layout, causing a silent wrong result or crash.
- GC-compaction (if ever introduced) could move the hmap, making the stored
uintptrstale.
Prefer using reflect.ValueOf(m).Pointer() — it provides the same identity comparison, is in the standard library, and is explicitly documented to return the hash-table pointer for maps:
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.
| // 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() |
There was a problem hiding this comment.
Shared singleton returned to callers who may mutate it
When both inputs are empty, MergeImportedModelAliases now returns the shared builtinOnlyAliasMap singleton directly. The doc comment warns callers not to mutate it, but the current call site in workflow_builder.go immediately assigns it to workflowData.ModelMappings, which is later assigned to apiProxy.Models (awf_config.go:530). If any code downstream of that ever writes to the map (e.g., apiProxy.Models[key] = value), it will silently corrupt the shared singleton for all subsequent parses.
The doc comment alone is an insufficient guard against this class of bug in a growing codebase. Consider one of:
- Returning a thin read-only wrapper (if the interface allows), or
- Documenting the invariant at both the assignment sites, or
- Adding a
-race-compatible test that callsParseWorkflowFileconcurrently to catch future violations.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on correctness and safety issues.
📋 Key Themes & Highlights
Key Issues
- Data race (
builtinOnlyAliasMapIDis a plainuintptrwritten undersync.Oncebut read without synchronisation — useatomic.Uintptr) - Panic on error (
getBuiltinOnlyAliasMappanics rather than propagating; should cache and return the error) unsafemap identity (mapHeaderPointerextracts*runtime.hmap; same goal achievable more safely withreflect.Value.Pointer()or anatomic.Boolsentinel)- Mutation contract unenforced (shared builtin map returned to callers with no runtime guard; needs at least a test asserting the fast-path identity)
- No regression benchmark guard (the fixed regression has no automated threshold assertion to prevent a future repeat)
Positive Highlights
- ✅ Excellent root-cause analysis and clear PR description with benchmark numbers
- ✅ Template fast-paths (
strings.Contains) are clean and obviously correct - ✅
knownToolspromotion to package-level is a straightforward, safe win - ✅
builtinCycleCheckOncefor DFS caching is well-structured and easy to follow
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 49.3 AIC · ⌖ 5.15 AIC · ⊞ 6.6K
Comment /matt to run again
| builtinOnlyAliasMapOnce.Do(func() { | ||
| data, err := loadBuiltinModelAliases() | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[/diagnosing-bugs] panic in a hot-path singleton hides the error from callers and kills the whole process — even if the caller could recover gracefully.
💡 Suggested fix: return the error instead
Change the signature to (map[string][]string, error) and cache the error:
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 loadBuiltinModelAliases errors are handled elsewhere.
@copilot please address this.
| // 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)) |
There was a problem hiding this comment.
[/diagnosing-bugs] mapHeaderPointer relies on an undocumented Go runtime internal (map values as single-word *hmap pointers). While stable since Go 1.0, this is fragile — a future runtime change (or GC move in a hypothetical moving GC) could silently break the identity check.
💡 Simpler alternative without unsafe
Since getBuiltinOnlyAliasMap is the only place that sets builtinOnlyAliasMap, you can replace mapHeaderPointer + builtinOnlyAliasMapID with an atomic bool:
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 bool sentinel inside the Once and use an explicit pointer stored as unsafe.Pointer via atomic.Pointer[map[string][]string] — all safer than raw uintptr arithmetic.
@copilot please address this.
| // 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 |
There was a problem hiding this comment.
[/diagnosing-bugs] builtinOnlyAliasMapID is a plain uintptr read outside the sync.Once — this is a data race on first concurrent use.
The write on line 99 happens inside builtinOnlyAliasMapOnce.Do, but the read on line 112 (id := builtinOnlyAliasMapID) happens without any synchronisation. The sync.Once guarantees the write is ordered before subsequent Do callers unblock, but isBuiltinOnlyAliasMap can be called concurrently before Do has run — leading to a torn read on 32-bit platforms or under the race detector.
💡 Fix: use atomic.Uintptr
import "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.
| // 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() |
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 result["my-alias"] = ...) will silently corrupt state for all concurrent parsers.
💡 Suggested safeguard
Add 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.
| // 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, "<builtin-aliases>") |
There was a problem hiding this comment.
[/tdd] The PR fixes a performance regression but adds no benchmark assertions or regression guard. The existing BenchmarkParseWorkflow will detect future regressions only if someone remembers to run it with -benchtime and compare — it's a passive check.
💡 Suggested addition
Add a sub-benchmark or TestMain-style baseline guard that fails if ParseWorkflow exceeds a threshold, e.g.:
func BenchmarkParseWorkflow_NoRegression(b *testing.B) {
// Fails CI if median ns/op exceeds the pre-regression baseline (479k ns/op)
// giving a 20% headroom above the new ~240k target.
const maxNsPerOp = 380_000
// ... run benchmark and assert b.Elapsed()/N < maxNsPerOp
}Alternatively, document the expected ns/op threshold in a comment on BenchmarkParseWorkflow so reviewers have context when they see benchmark output in CI.
@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (132 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
REQUEST_CHANGES — correctness and safety issues in the unsafe/singleton optimization
The perf goal is valid and the template/knownTools fast-paths are fine. The model_aliases.go optimizations introduce four interconnected issues that need to be resolved before merging.
Blocking issues (4)
- Data race on
builtinOnlyAliasMapID— plainuintptrread/write withoutatomic, races under concurrentParseWorkflowFilecalls (-racewill flag it) unsafemap header pointer — relies on undocumented Go runtime internals that are not guaranteed stable; Go 1.24+ map redesign (Swiss tables) makes this an active fragility riskpanicinsidesync.Oncepoisons the singleton — after a panicDois marked done butbuiltinOnlyAliasMapisnil; future callers returnnilsilently with no error- Shared mutable map returned to callers — comment-only
MUST NOT mutatecontract is invisible at call sites; any caller mutation causes a data race and corrupts all future parses
All four stem from the same root: using unsafe pointer identity instead of a wrapper type. The fix is straightforward — wrap the singleton in a named struct so identity is just pointer equality with no unsafe needed.
🔎 Code quality review by PR Code Quality Reviewer · 52.3 AIC · ⌖ 5.03 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/model_aliases.go:113
Data race on builtinOnlyAliasMapID: the plain uintptr read in isBuiltinOnlyAliasMap races with the write inside builtinOnlyAliasMapOnce.Do.
<details>
<summary>💡 Details and suggested fix</summary>
isBuiltinOnlyAliasMap reads builtinOnlyAliasMapID via a plain load (line 112) with no synchronization. The write happens inside builtinOnlyAliasMapOnce.Do. sync.Once guarantees happens-before only for goroutines that pass through Do — a goroutine that calls `isBuiltinOnlyAlias…
pkg/workflow/model_aliases.go:91
unsafe map pointer relies on undocumented Go runtime internals: *(*uintptr)(unsafe.Pointer(&m)) deferences the internal map header, which is not part of the Go specification and can break silently on any toolchain upgrade.
<details>
<summary>💡 Why this is fragile and a safer alternative</summary>
The comment says "this layout has been stable since Go 1.0" but that is not a guarantee — it is historical observation. The Go team has discussed changing the map implementation (e.g. Swiss …
pkg/workflow/model_aliases.go:100
panic inside sync.Once permanently poisons the singleton: if loadBuiltinModelAliases() ever returns an error, the panic unwinds through Do, the Once is marked complete, and all future calls to getBuiltinOnlyAliasMap return nil map — silently, with no error.
<details>
<summary>💡 Why this matters</summary>
From the Go docs: if f passed to Once.Do panics, Do considers it to have returned. Future calls to Do return immediately. So after the panic:
- `builtinOnlyAliasMa…
pkg/workflow/model_aliases.go:145
Shared singleton returned to callers with no mutation guard: MergeImportedModelAliases returns the internal builtinOnlyAliasMap directly, but the comment-only contract MUST NOT mutate is unenforceable and not visible at all call sites.
<details>
<summary>💡 Risk and mitigation</summary>
Any caller today or in the future that does:
aliases := MergeImportedModelAliases(nil, nil)
aliases["my-model"] = []string{"gpt-4"} // silently corrupts the shared singleton...will corru…
🤖 PR Triage
Rationale: Fixes
|
|
🎉 This pull request is included in a new release. Release: |
ParseWorkflowregressed from 371μs to 479μs (+29.2%) due to three per-call allocations that were unnecessary for the common case (no imports, no custom model aliases).Root causes & fixes
MergeImportedModelAliases— 5KB deep copy on every call (model_aliases.go)ParseWorkflowFileinvocation, even whenimportedModelsandfrontmatterModelswere both empty (the overwhelmingly common case)getBuiltinOnlyAliasMap()returning a shared read-only singleton;MergeImportedModelAliasesreturns it directly when both inputs are emptymapHeaderPointer()viaunsafe.Pointer+isBuiltinOnlyAliasMap()for zero-overhead map identity comparison (Go disallowsmap == map)detectCircularModelAliases— 52-node DFS on every call (model_alias_validation.go)builtinCycleCheckOnce; runs at most once per process lifetime when the map is the shared builtinNewTools— 14-entry map literal allocated per call (tools_parser.go)knownToolsmap literal was re-created insideNewTools()on every invocationTemplate validation — regex executed on all markdown (
template_validation.go)FindAllStringSubmatchunconditionallystrings.Contains("{{#if")/strings.Contains("{{#")fast-paths to skip regex when no template blocks are presentResult
~50% faster than the regression; ~35% faster than the historical baseline.