Reduce BenchmarkValidation overhead by caching safe-output field order#44090
Conversation
|
Hey This PR is still a fresh draft with no code changes yet, so a couple of things to keep in mind as it takes shape:
When ready, here's a prompt you can use to drive the implementation:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 Triage Assessment
Summary: Caches safe-output field order to reduce BenchmarkValidation overhead. Draft. Small diff (+27/−4, 3 files). No CI triggered. Next: Convert to ready-for-review when benchmarks verified.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 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 #44090 does not have the 'implementation' label and has only 27 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR reduces CPU time and allocations in the safe-outputs sample validation/replay hot paths by caching the deterministic field iteration order (sorted safeOutputFieldMapping keys) behind a sync.Once helper, avoiding repeated sorting per validation/replay pass.
Changes:
- Added a
sync.Once-initialized cache for sorted safe-output field names and used it invalidateSafeOutputsSamples. - Updated sample replay entry collection to reuse the same cached field ordering for determinism without per-call sorting.
- Added a unit test intended to verify cached ordering matches
sliceutil.SortedKeysand that repeated calls reuse the cached backing slice.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/samples_validation.go | Adds cached sorted safe-output field names helper and uses it in validation to avoid repeated sorting. |
| pkg/workflow/samples_validation_test.go | Adds a unit test covering ordering correctness and intended cache reuse behavior. |
| pkg/workflow/samples_replay.go | Switches replay entry collection to use the cached field ordering helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| if assert.NotEmpty(t, first) && assert.NotEmpty(t, second) { | ||
| assert.Equal(t, &first[0], &second[0], "expected cached field names to reuse the same backing slice") | ||
| } |
| sortedSafeOutputFieldNamesOnce.Do(func() { | ||
| sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping) | ||
| }) | ||
| return sortedSafeOutputFieldNames | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Analysis
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The caching approach is correct and well-targeted.
sync.Onceinitialization is safe for concurrent use.- Both callers (
validateSafeOutputsSamplesandcollectSampleEntries) onlyrangeover the returned slice — no mutation — so returning the shared backing slice directly is safe. - The pointer-identity assertion in
TestGetSortedSafeOutputFieldNames_MatchesSortedKeysis the right way to verify caching without relying on slice equality alone. safeOutputFieldMappingis initialized once at package load and never reassigned, so the cached result is stable for the process lifetime.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 43.5 AIC · ⌖ 5.63 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test contract is too tight: The pointer-identity check (
&first[0] == &second[0]) couples the test tosync.Onceinternals rather than the observable contract (idempotent sorted order). A future defensive-copy refactor would break the test falsely. - Mutation safety is implicit:
getSortedSafeOutputFieldNamesreturns the live cached slice without documenting the "do not mutate" contract. All current call sites are safe, but new callers have no guidance. - No regression benchmark: The fix addresses a
BenchmarkValidationregression but adds no benchmark that would detect a future reintroduction of the per-call sort. varblock grouping: The two new cache variables are appended to thecompiledToolSchemasblock without a separating comment, making the cache topology slightly harder to read.
Positive Highlights
- ✅ Correct use of
sync.Once— the pattern is idiomatic and consistent with the existingcompiledToolSchemascache. - ✅ Both hot paths (validation and replay) updated in a single PR — no drift.
- ✅ Existing test verifies ordering correctness and cache idempotency.
- ✅
sliceutil.SortedKeysimport correctly removed fromsamples_replay.go.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 56.1 AIC · ⌖ 5.61 AIC · ⊞ 6.6K
Comment /matt to run again
| assert.Equal(t, expected, first) | ||
| assert.Equal(t, expected, second) | ||
| if assert.NotEmpty(t, first) && assert.NotEmpty(t, second) { | ||
| assert.Equal(t, &first[0], &second[0], "expected cached field names to reuse the same backing slice") |
There was a problem hiding this comment.
[/tdd] The pointer-identity assertion (&first[0] == &second[0]) is an implementation detail of sync.Once, not a public contract of getSortedSafeOutputFieldNames. If the implementation ever switches to returning a defensive copy, this test breaks even though the behaviour is correct.
💡 Suggested alternative
The meaningful invariant is idempotent sorted order, which value-equality already captures:
// first two asserts already cover this
assert.Equal(t, first, second)If you want to document the zero-alloc property, a BenchmarkGetSortedSafeOutputFieldNames with b.ReportAllocs() is a more meaningful and durable guarantee than a pointer check.
@copilot please address this.
| compiledToolSchemasErr error | ||
|
|
||
| sortedSafeOutputFieldNamesOnce sync.Once | ||
| sortedSafeOutputFieldNames []string |
There was a problem hiding this comment.
[/codebase-design] The new sync.Once variables are grouped with compiledToolSchemas* in the same var block, but they serve a logically different concern (field-name ordering vs. schema compilation). Consider adding a short comment separating them, following the pattern already used for compiledToolSchemas.
💡 Suggestion
var (
compiledToolSchemasOnce sync.Once
compiledToolSchemas map[string]toolSchemaEntry
compiledToolSchemasErr error
)
// sortedSafeOutputFieldNames caches the deterministic key order of
// safeOutputFieldMapping, computed once and reused across validation passes.
var (
sortedSafeOutputFieldNamesOnce sync.Once
sortedSafeOutputFieldNames []string
)This mirrors the doc-comment convention already in place for compiledToolSchemas (line 58) and makes each cache group independently navigable.
@copilot please address this.
| sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping) | ||
| }) | ||
| return sortedSafeOutputFieldNames | ||
| } |
There was a problem hiding this comment.
[/codebase-design] getSortedSafeOutputFieldNames returns the mutable cached slice directly. Any caller that appends to or sorts the returned slice will silently corrupt the cache for all future callers.
💡 Suggested fix
Return a copy, or document the contract that callers must not mutate the result:
// getSortedSafeOutputFieldNames returns the sorted keys of safeOutputFieldMapping.
// The returned slice MUST NOT be modified by the caller.
func getSortedSafeOutputFieldNames() []string {
sortedSafeOutputFieldNamesOnce.Do(func() {
sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping)
})
return sortedSafeOutputFieldNames
}The existing call sites only range over the slice, so they are safe today — but this is an implicit contract that new callers can easily violate. A doc comment is low-cost insurance.
@copilot please address this.
| ) | ||
|
|
||
| func TestGetSortedSafeOutputFieldNames_MatchesSortedKeys(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
[/tdd] The test asserts cache correctness but not the benchmark regression it was designed to fix. There is no regression test or benchmark that would catch a future reintroduction of the per-call sort.
💡 Suggestion
Add a BenchmarkGetSortedSafeOutputFieldNames that calls b.ReportAllocs() alongside the existing BenchmarkValidation in compiler_performance_benchmark_test.go. A zero-alloc assertion is both more meaningful and more durable than the current pointer-equality check:
func BenchmarkGetSortedSafeOutputFieldNames(b *testing.B) {
b.ReportAllocs()
for b.Loop() {
_ = getSortedSafeOutputFieldNames()
}
// After warm-up the cache is primed; allocs should be 0
}This would have caught the original regression directly.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations — no blocking issues found.
### Findings summary
The caching approach is correct and safe. safeOutputFieldMapping is a package-level var initialized once at startup and never mutated, so caching its sorted keys via sync.Once is sound.
Two non-blocking observations:
-
getSortedSafeOutputFieldNames()returns a shared alias (samples_validation.go:107). Current callers only range-iterate, so no bug today — but the lack of a defensive copy means any future caller that writes to an element corrupts the cache permanently for the process. -
The
&first[0] == &second[0]assertion (samples_validation_test.go:82) enshrines the aliasing as a test contract, discouraging safe refactors. The content-equality assertions on the lines above already fully validate correctness; this pointer-address check adds risk without adding value.
Both are low-severity maintainability concerns that can be addressed independently of merging.
🔎 Code quality review by PR Code Quality Reviewer · 114.7 AIC · ⌖ 6.84 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/samples_validation.go:107
Returned slice aliases the cached backing array — callers who mutate elements will silently corrupt future results from getSortedSafeOutputFieldNames().
<details>
<summary>💡 Details and suggested fix</summary>
getSortedSafeOutputFieldNames returns sortedSafeOutputFieldNames directly with no defensive copy:
func getSortedSafeOutputFieldNames() []string {
sortedSafeOutputFieldNamesOnce.Do(func() {
sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping)
})…
</details>
<details><summary>pkg/workflow/samples_validation_test.go:82</summary>
**Pointer-equality assertion leaks implementation details into the test contract** — this assertion makes the mutable-alias the expected behavior, which is a maintenance trap.
<details>
<summary>💡 Why this matters</summary>
```go
assert.Equal(t, &first[0], &second[0], "expected cached field names to reuse the same backing slice")This assertion verifies that first and second share the same underlying array — i.e., they are the same slice header pointing to the same memory. That is a…
pkg/workflow/samples_validation_test.go:82
assert.Equal on pointer values does not test pointer identity — the caching invariant this assertion claims to verify is never actually checked.
<details>
<summary>💡 Details and fix</summary>
assert.Equal(t, &first[0], &second[0], "expected cached field names to reuse the same backing slice")assert.Equal uses reflect.DeepEqual under the hood, which dereferences *string pointers and compares the string values, not the memory addresses. This assertion passes even if…
|
🎉 This pull request is included in a new release. Release: |
BenchmarkValidation regressed due to repeated work in validation hot paths. The dominant cost was re-sorting safe-output field mappings on each validation/replay pass, inflating CPU and allocations.
Root-cause reduction in validation path
sync.Once-backed cache for sortedsafeOutputFieldMappingkeys insamples_validation.go.validateSafeOutputsSampleswith cached lookup.Keep replay path consistent
collectSampleEntriesinsamples_replay.goto use the same cached field-order helper, preserving deterministic ordering while removing duplicate sort work.Targeted coverage
TestGetSortedSafeOutputFieldNames_MatchesSortedKeysto confirm: