perf: cache builtin alias map and DFS cycle-check to fix CompileMCPWorkflow regression#44996
Conversation
…Workflow speedup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Draft PR — part of perf regression batch. Review together with #44994 and #44995.
|
🤖 PR Triage
Rationale: Draft. Caches builtin alias map and DFS cycle-check to fix CompileMCPWorkflow +14% regression. Small change. Batch with other perf PRs. Triage run §29183606049
|
🤖 PR Triage
Draft. Caches builtin alias map and DFS cycle-check. +72/-1. Review as part of perf-regressions batch.
|
There was a problem hiding this comment.
Pull request overview
Optimizes model-alias compilation by caching immutable builtin data and cycle-check results.
Changes:
- Caches the builtin-only alias map.
- Adds a cached DFS fast path for builtin aliases.
- Logs builtin-loading failures before fallback validation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/model_aliases.go |
Adds builtin alias-map caching. |
pkg/workflow/model_alias_validation.go |
Adds cached builtin cycle validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| // isBuiltinOnlyAliasMap reports whether aliasMap has exactly the same set of keys as | ||
| // rawBuiltins. Because MergeImportedModelAliases never modifies existing builtin keys | ||
| // (it only appends new ones), equal key sets mean equal content. | ||
| func isBuiltinOnlyAliasMap(aliasMap, rawBuiltins map[string][]string) bool { | ||
| if len(aliasMap) != len(rawBuiltins) { | ||
| return false | ||
| } | ||
| for k := range aliasMap { | ||
| if _, exists := rawBuiltins[k]; !exists { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
| if len(importedModels) == 0 && len(frontmatterModels) == 0 { | ||
| result := builtinOnlyAliasMap() | ||
| modelAliasesLog.Printf("Fast path: returning shared builtin alias map (%d entries)", len(result)) | ||
| return result |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44996 does not have the 'implementation' label and has only 72 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness issues must be fixed before merge
The performance goal is valid, but the implementation introduces two correctness bugs that need addressing:
-
Shared mutable map (
model_aliases.go):MergeImportedModelAliases(nil, nil)now returns a package-level singleton with no compile-time mutation guard. Any future caller that writes to the result will silently corrupt all subsequent compilations. The fix is trivial: returnBuiltinModelAliases()(a copy) in the nil/nil fast path — the allocation cost is negligible for 52 entries. -
Key-set equality heuristic for cycle-check caching (
model_alias_validation.go):isBuiltinOnlyAliasMapuses key-count + key-membership to infer the map is the builtin singleton. This produces a wrong result for any map built with the same key set but different values (e.g., hand-constructed in tests). Pointer identity (aliasMap == builtinOnlyAliasMap()) is O(1), guaranteed-correct, and eliminates both the heuristic and the separateloadBuiltinModelAliases()call for non-builtin callers.
Non-blocking observation
The overhead comment (comment 3) is informational: the O(n) key scan on every non-builtin call is a minor regression for custom-model workflows, but fixable as a side effect of adopting pointer identity.
🔎 Code quality review by PR Code Quality Reviewer · 49.7 AIC · ⌖ 4.97 AIC · ⊞ 5.4K
Comment /review to run again
| if len(importedModels) == 0 && len(frontmatterModels) == 0 { | ||
| result := builtinOnlyAliasMap() | ||
| modelAliasesLog.Printf("Fast path: returning shared builtin alias map (%d entries)", len(result)) | ||
| return result |
There was a problem hiding this comment.
Shared map with no mutation enforcement: MergeImportedModelAliases now returns a cached singleton in the fast path; any caller that writes to the map will silently corrupt all future compilations.
💡 Details and suggested fix
The fast path at lines 143–151 returns builtinOnlyAliasMap(), which is the package-level singleton mergedBuiltinOnlyMap. Go maps are reference types — assigning the returned value to a struct field and then writing to it (even transitively, e.g. via maps.Copy) mutates the singleton.
Current callers look safe:
workflow_builder.go:154stores the result inWorkflowData.ModelMappings, which is then read-only byawf_config.go:530.
But the contract is enforced only by a comment, and MergeImportedModelAliases is a public function. Any new caller that does:
result := MergeImportedModelAliases(nil, nil)
result["my-model"] = []string{"acme/model"} // silently corrupts the singletonwill corrupt every subsequent compilation without any test catching it.
Fix options (in order of preference):
- Return a copy always in the nil/nil case (cheap: 52 entries, already the pre-diff behaviour).
- Introduce a typed read-only wrapper so the compiler enforces immutability at the call site.
- At minimum, add a test that calls
MergeImportedModelAliases(nil, nil)twice and verifies modifications to one result do not affect the other.
Option 1 is one line and eliminates the whole class of bug without a fragile API contract.
| } | ||
|
|
||
| // isBuiltinOnlyAliasMap reports whether aliasMap has exactly the same set of keys as | ||
| // rawBuiltins. Because MergeImportedModelAliases never modifies existing builtin keys |
There was a problem hiding this comment.
Fragile key-equality assumption can return a wrong cached cycle-check result: the comment claims equal key sets imply identical content, but this is only true as long as callers go through MergeImportedModelAliases. Any direct construction of an alias map with the same keys but different values will hit the cached builtin DFS result instead of running a fresh check.
💡 Details and suggested fix
The logic is:
- Load the raw builtins.
- If
aliasMaphas the same key set → it must be the builtin map → return cached DFS result.
The invariant relies on MergeImportedModelAliases never overriding existing builtin keys. That's documented in the current code, but:
- It is a cross-function invariant with no compile-time enforcement.
- Any test or future code path that builds an alias map directly (e.g. in unit tests constructing
WorkflowDataby hand) with the same key count as the builtins will silently skip cycle detection. - If a bug in
MergeImportedModelAliasesever does overwrite a key, the cycle check becomes wrong with no visible failure.
Safer approach: Instead of a key-set equality heuristic, use pointer identity. Cache a pointer to the builtin map from builtinOnlyAliasMap() and compare:
if aliasMap == builtinOnlyAliasMap() { // pointer equality — exact same map
...
}This is O(1), guaranteed-correct, and eliminates the indirect reasoning about key sets entirely. The fast path in MergeImportedModelAliases already returns the same singleton, so this will fire exactly when intended.
| // are immutable, so we run the DFS once and cache the result for all future calls. | ||
| rawBuiltins, err := loadBuiltinModelAliases() | ||
| if err != nil { | ||
| modelAliasValidationLog.Printf("Warning: could not load builtin model aliases for cycle-check fast path, falling back to full validation: %v", err) |
There was a problem hiding this comment.
loadBuiltinModelAliases() + O(n) key scan runs on every call even for non-builtin maps: the fast-path check itself adds overhead for the common case of workflows with custom models.
💡 Details
For every compilation that has custom models (i.e., non-nil importedModels or frontmatterModels), detectCircularModelAliases now does:
loadBuiltinModelAliases()— returns cached data, cheap but not free (sync.Once unlock, two pointer reads).isBuiltinOnlyAliasMap()— iterates all N keys ofaliasMapchecking map membership in rawBuiltins.
For the builtin-only fast path this is a net win. For the customModels path, it is pure overhead added before the actual DFS.
If this function is on the hot path for custom-model compilations too, the scan should be gated behind the pointer-identity check suggested in the other comment (which is O(1) and requires no iteration):
if aliasMap == builtinOnlyAliasMap() {
// run/return cached DFS
}
// fall through to full DFS — no loadBuiltinModelAliases call at allThis eliminates both the loadBuiltinModelAliases() call and the O(n) key scan for all non-builtin callers.
There was a problem hiding this comment.
Review: perf — cache builtin alias map and DFS cycle-check
Good performance win with clear motivation. Two non-blocking suggestions surfaced (see inline comments).
Key observations
model_aliases.go fast path — The MergeImportedModelAliases(nil,nil) shortcut is sound. The risk is that the read-only contract is enforced only by convention. Any future caller that appends to a slice value (e.g. aliases["sonnet"] = append(...)) will silently corrupt the shared singleton.
isBuiltinOnlyAliasMap O(n) key comparison — The equal-key-sets-imply-equal-values assumption is correct today but fragile. Since MergeImportedModelAliases(nil,nil) returns the exact singleton from builtinOnlyAliasMap(), a pointer-identity check would be O(1) and structurally correct.
builtinCycleCheckErr race safety — The write inside sync.Once.Do and the read after are correctly ordered. No race.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46.3 AIC · ⌖ 4.53 AIC · ⊞ 4.8K
| } | ||
| } | ||
| return true | ||
| } |
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The key-set equality check in isBuiltinOnlyAliasMap is O(n) and relies on the invariant that equal key-sets imply equal values. This is a fragile assumption that will silently break if MergeImportedModelAliases ever allows a caller to shadow a builtin key without adding a new key. Since MergeImportedModelAliases(nil,nil) now returns the exact singleton produced by builtinOnlyAliasMap(), a simpler and more reliable identity check is possible using a pointer sentinel stored when the once fires, removing both the O(n) iteration and the brittleness. @copilot please address this.
| // from avoiding the O(n) deep copy on every compilation. | ||
| if len(importedModels) == 0 && len(frontmatterModels) == 0 { | ||
| result := builtinOnlyAliasMap() | ||
| modelAliasesLog.Printf("Fast path: returning shared builtin alias map (%d entries)", len(result)) |
There was a problem hiding this comment.
The fast path returns a shared map, but the doc-comment change only adds a sentence at the top — the function signature still returns map[string][]string with no indication to users of the public API that it can return a read-only reference. Callers outside this package (e.g., model_aliases_import_test.go) receive this map and the read-only contract is only communicated via the updated comment. Consider returning the map wrapped in a defensive copy check in tests, or adding a // go:noinline sentinel comment to at least surface the caveat in godoc prominently. This is low severity but could silently corrupt the singleton if an external caller ever ranges over the map and appends to a slice value. @copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness/safety issues and one missing regression guard.
📋 Key Themes & Highlights
Key Themes
- Shared-map mutation risk (highest priority):
MergeImportedModelAliases(nil, nil)now returns a shared reference with no write protection. Any caller writing intoworkflowData.ModelMappingswill silently corrupt the cache for all subsequent compilations in the same process. - Missing regression tests: The two
sync.Oncecaching paths have no dedicated tests. Removing or breaking either cache causes a silent regression with nothing to catch it. - Residual overhead in DFS fast path:
loadBuiltinModelAliases()plus an O(n) key-set scan still run on everydetectCircularModelAliasescall. Pointer-identity comparison would eliminate this.
Positive Highlights
- ✅ Clean two-layer
sync.Oncepattern — alias-map cache and DFS-result cache are nicely separated. - ✅ Fallback logging on
loadBuiltinModelAliaseserror is a real improvement over the previous silent discard. - ✅
detectCircularModelAliasesImplextraction is a well-named seam that makes fast paths testable. - ✅ Benchmark results are compelling (~30% improvement, 80 fewer allocs/op).
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55 AIC · ⌖ 4.86 AIC · ⊞ 6.6K
Comment /matt to run again
| // Callers that only read the result (WorkflowData.ModelMappings consumers) benefit | ||
| // from avoiding the O(n) deep copy on every compilation. | ||
| if len(importedModels) == 0 && len(frontmatterModels) == 0 { | ||
| result := builtinOnlyAliasMap() |
There was a problem hiding this comment.
[/diagnosing-bugs] MergeImportedModelAliases(nil, nil) now returns a shared cached map, but nothing in Go prevents a caller from mutating workflowData.ModelMappings and corrupting the cache for all subsequent compilations in the same process.
💡 Suggested mitigation
Add a test that verifies mutating the returned map does not affect subsequent calls:
func TestMergeImportedModelAliases_CachedMapIsolation(t *testing.T) {
m1 := MergeImportedModelAliases(nil, nil)
originalLen := len(m1)
// Simulate an accidental write by a caller
for k := range m1 { delete(m1, k); break }
m2 := MergeImportedModelAliases(nil, nil)
require.Len(t, m2, originalLen, "mutating returned map must not corrupt the shared cache")
}This test will currently fail — making the risk concrete. The fix could be returning a read-only wrapper, or defensive copying in builtinOnlyAliasMap for all write-capable callers.
@copilot please address this.
| // IS the builtin map (MergeImportedModelAliases only ever adds new keys — it never | ||
| // removes or overrides them — so equal key sets imply identical content). Builtins | ||
| // are immutable, so we run the DFS once and cache the result for all future calls. | ||
| rawBuiltins, err := loadBuiltinModelAliases() |
There was a problem hiding this comment.
[/diagnosing-bugs] loadBuiltinModelAliases() is still called on every invocation of detectCircularModelAliases, even when the fast path fires. The DFS is cached, but the key-set comparison and the loadBuiltinModelAliases call itself add overhead on each compilation.
💡 Suggested optimisation
Use pointer identity instead of key-set equality: since builtinOnlyAliasMap() always returns the same pointer, you can compare map headers:
// At package level (model_alias_validation.go)
var builtinAliasMapPtr map[string][]string
func init() { builtinAliasMapPtr = builtinOnlyAliasMap() }
func detectCircularModelAliases(aliasMap map[string][]string, markdownPath string) error {
if isBuiltinOnly := reflect.ValueOf(aliasMap).Pointer() == reflect.ValueOf(builtinAliasMapPtr).Pointer(); isBuiltinOnly {
// ... cached DFS
}
return detectCircularModelAliasesImpl(aliasMap, markdownPath)
}Or more idiomatically, pass a bool isBuiltinOnly flag from the call site (validateModelAliasMap) to avoid loading builtins altogether in the fast path.
@copilot please address this.
| // Pass an empty markdownPath: the cached result is shared across all | ||
| // compilations, so any path embedded in an error message would be | ||
| // misleading for callers other than the first one. | ||
| builtinCycleCheckErr = detectCircularModelAliasesImpl(rawBuiltins, "") |
There was a problem hiding this comment.
[/tdd] No regression test exists for the builtinCycleCheckOnce caching path. If the sync.Once wrapper is accidentally removed or the fast path condition breaks, compilation will silently regress without any failing test.
💡 Suggested test
func TestDetectCircularModelAliases_BuiltinFastPath(t *testing.T) {
builtins := BuiltinModelAliases()
// First call — populates the cache
err1 := detectCircularModelAliases(builtins, "/path/a.md")
require.NoError(t, err1)
// Second call — must use cache (same result, no panic)
err2 := detectCircularModelAliases(builtins, "/path/b.md")
require.NoError(t, err2)
// Non-builtin map — must NOT use cache
custom := map[string][]string{"custom": {"gpt-4o"}}
for k, v := range builtins { custom[k] = v }
err3 := detectCircularModelAliases(custom, "/path/c.md")
require.NoError(t, err3)
}This guards the fast-path branch and ensures the key-set equality check behaves correctly for both the hit and miss cases.
@copilot please address this.
🤖 PR Triage
Rationale: Caches builtin alias map and DFS cycle-check to fix
|
BenchmarkCompileMCPWorkflowregressed +14% (432µs vs 379µs). Two hot paths were doing redundant work on every compilation:BuiltinModelAliases()deep-copied the 52-entry builtin map on every call, anddetectCircularModelAliasesran a full DFS over all 52 builtin keys even though builtins are immutable and cycle-free.model_aliases.gobuiltinOnlyAliasMap()backed bysync.Oncereturning a cached shared reference to the merged builtin mapMergeImportedModelAliases(nil, nil)— the common case for MCP workflows with no imports — now returns the cached reference instead of deep-copyingmodel_alias_validation.godetectCircularModelAliasesuses a newisBuiltinOnlyAliasMaphelper (key-set equality, not just length) to detect when the input is the builtin-only mapsync.Onceand the result is cached for all subsequent callsloadBuiltinModelAliases()errors are now logged with a fallback to full DFS instead of being silently discardedmarkdownPathto avoid embedding a stale caller path in a shared errorResult: ~291–306µs vs reported 432µs (~30% faster); 80 fewer allocs/op, ~10KB less memory per compilation.