perf: fix CompileMemoryUsage regression by optimizing normalizeBlankLines and findRunValue#44995
Conversation
…oryUsage regression - Rewrite normalizeBlankLines to use a single pre-allocated strings.Builder instead of strings.Split + strings.Join, reducing ~150KB of allocations per compilation call. - Add a fast strings.Contains pre-check in findRunValue before applying the regex, avoiding expensive regexp.FindStringIndex calls on the majority of YAML lines that don't contain "run:". These two changes together reduce CompileMemoryUsage from ~4.8ms to ~2.5ms (-48%) and reduce per-iteration allocations from ~1701KB to ~1572KB (-7.6%) on the benchmark machine. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ents Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44995 does not have the 'implementation' label and has only 55 new lines of code in business logic directories (threshold is 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Optimizes YAML compilation hot paths to reduce runtime and allocations.
Changes:
- Reworks blank-line normalization using a preallocated builder.
- Adds a fast pre-check before run-key regex matching.
- One security issue found: quoted
runkeys bypass validation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Optimizes blank-line normalization. |
pkg/workflow/template_injection_validation.go |
Adds regex fast-path filtering. |
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: 1
- Review effort level: Medium
| // Fast pre-check: the regex requires "run:" to be present, so skip the | ||
| // (more expensive) regex entirely when the substring is absent. | ||
| if !strings.Contains(keyPart, "run:") { | ||
| return "", false | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: perf: fix CompileMemoryUsage regression
The normalizeBlankLines rewrite is well-crafted — pre-allocated builder, zero-copy slice return, correct handling of empty/all-whitespace inputs, and good coverage of the edge cases.
One blocking issue in the findRunValue fast-path that needs fixing before merge.
Blocking: The strings.Contains(keyPart, "run:") pre-check misses quoted YAML key forms ("run":, 'run':) that the regex does match. Lines with those forms bypass the template-injection scan entirely — a silent security regression. Changing the check to strings.Contains(keyPart, "run") preserves the performance win while closing the gap.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 26.6 AIC · ⌖ 5.32 AIC · ⊞ 4.8K
| } | ||
|
|
||
| func findRunValue(keyPart string) (string, bool) { | ||
| // Fast pre-check: the regex requires "run:" to be present, so skip the |
There was a problem hiding this comment.
The fast pre-check strings.Contains(keyPart, "run:") has a security gap.
The regex (?:^|[\s{,])(?:run|["'run["']): also matches quoted YAML keys like "run": and 'run':. But strings.Contains(keyPart, "run:") returns false for those — e.g. {"run": "cmd"} contains "run":, not run:.
This means any YAML line with a quoted run key silently bypasses the template-injection scan.
Fix — broaden to just check for the word run:
if !strings.Contains(keyPart, "run") {
return "", false
}This is still a huge fast-path win (only lines containing run anywhere reach the regex), with no false-negative risk.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity security issue must be fixed
The performance improvements are real and the normalizeBlankLines rewrite is correct. However, the findRunValue pre-check introduces a security regression that must be fixed before merge.
Blocking issue
findRunValue pre-check silently bypasses injection validation for quoted-key run blocks (line 85 in template_injection_validation.go). runKeyPattern matches "run": and 'run':, but strings.Contains(keyPart, "run:") returns false for both — the colon is not adjacent to n in the quoted forms. Any template injection inside a quoted-key run block will be missed entirely.
Non-blocking note
The comment on return b.String()[:lastNonBlankEnd] overstates the optimization by calling it a "zero-copy operation" — b.String() itself copies the builder bytes. The slice avoids a second copy, but the description should be corrected to avoid misleading future readers.
🔎 Code quality review by PR Code Quality Reviewer · 42.8 AIC · ⌖ 4.64 AIC · ⊞ 5.4K
Comment /review to run again
| func findRunValue(keyPart string) (string, bool) { | ||
| // Fast pre-check: the regex requires "run:" to be present, so skip the | ||
| // (more expensive) regex entirely when the substring is absent. | ||
| if !strings.Contains(keyPart, "run:") { |
There was a problem hiding this comment.
Security bypass: pre-check rejects quoted-key YAML forms that the regex would match, silently skipping injection validation.
💡 Details and fix
runKeyPattern matches three forms of a run key:
| Form | Example |
|---|---|
| Unquoted | run: |
| Double-quoted | "run": |
| Single-quoted | 'run': |
The pre-check strings.Contains(keyPart, "run:") only matches the unquoted form. In "run":, the characters after n are " then : — the substring run: is absent, so strings.Contains returns false and the function short-circuits before the regex runs. Any template expression injected via a quoted-key run block ("run": |) will silently bypass detection.
Fix — widen the pre-check or remove it entirely:
// Option A: widen
if !strings.Contains(keyPart, "run:") &&
!strings.Contains(keyPart, `"run":`) &&
!strings.Contains(keyPart, "'run':") {
return "", false
}
// Option B: just remove the pre-check — the compiled regex is already fast| } | ||
| return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n" | ||
| // Slice the builder string to drop trailing blank lines. Because Go string | ||
| // slicing shares the underlying bytes array, this is a zero-copy operation. |
There was a problem hiding this comment.
Misleading comment claims zero-copy, but b.String() already copied the builder's bytes.
💡 Detail
The comment reads:
Because Go string slicing shares the underlying bytes array, this is a zero-copy operation.
The slice b.String()[:lastNonBlankEnd] is indeed zero-copy, but b.String() itself copies the builder's internal []byte into a new immutable string. The overall return b.String()[:lastNonBlankEnd] is therefore a copy — not zero-copy. Future maintainers reasoning about allocation budgets will be misled.
Fix: reword the comment to say the
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on missing regression coverage.
📋 Key Themes & Highlights
Key Themes
- Missing regression tests: Both hot-spot fixes are non-trivial rewrites with no unit tests. The
normalizeBlankLinesrewrite has 7+ edge cases documented in comments but none verified by tests. ThefindRunValuefast-path has no test confirming it produces identical results to the original regex-only path. - Inaccurate comment: The "zero-copy" description for
b.String()[:lastNonBlankEnd]is incorrect —strings.Builder.String()always copies the buffer; the slice only avoids a second copy.
Positive Highlights
- ✅ Impressive benchmark improvement: −48% latency and −7.6% memory per op
- ✅ The
strings.Containsfast-path infindRunValueis a clean, minimal change - ✅ Thorough inline comments explain the intent of
lastNonBlankEndand the early-return edge case - ✅
b.Grow(len(yamlContent))pre-allocation is exactly the right approach to eliminate incremental growth
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38 AIC · ⌖ 4.82 AIC · ⊞ 6.6K
Comment /matt to run again
| return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n" | ||
| // Slice the builder string to drop trailing blank lines. Because Go string | ||
| // slicing shares the underlying bytes array, this is a zero-copy operation. | ||
| return b.String()[:lastNonBlankEnd] |
There was a problem hiding this comment.
[/diagnosing-bugs] No regression tests for the rewritten normalizeBlankLines — a non-trivial rewrite with subtle edge-case handling (empty input, all-whitespace, no trailing newline) is exactly where a unit test suite pays off.
💡 Suggested test cases
func TestNormalizeBlankLines(t *testing.T) {
tests := []struct {
name string
input string
want string
}{
{"empty string", "", "\n"},
{"single blank line", "\n", "\n"},
{"all whitespace line", " \n", "\n"},
{"no trailing newline", "hello", "hello\n"},
{"trailing blank lines stripped", "a\n\nb\n\n\n", "a\n\nb\n"},
{"blank lines in middle preserved", "a\n\nb\n", "a\n\nb\n"},
{"whitespace-only lines cleared", "a\n \nb\n", "a\n\nb\n"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := normalizeBlankLines(tc.input)
if got != tc.want {
t.Errorf("got %q, want %q", got, tc.want)
}
})
}
}Without tests, a future refactor or Go version change could silently break the behaviour that was carefully preserved in the comments.
@copilot please address this.
| } | ||
| return strings.TrimRight(strings.Join(lines, "\n"), "\n") + "\n" | ||
| // Slice the builder string to drop trailing blank lines. Because Go string | ||
| // slicing shares the underlying bytes array, this is a zero-copy operation. |
There was a problem hiding this comment.
[/codebase-design] The comment on line 367 says b.String()[:lastNonBlankEnd] is "zero-copy", but strings.Builder.String() always copies the internal byte buffer into a new string — the slice just avoids a second copy. The comment is technically incorrect and may mislead future readers.
💡 Suggested fix
Change the comment from:
// Slice the builder string to drop trailing blank lines. Because Go string
// slicing shares the underlying bytes array, this is a zero-copy operation.to something like:
// Slice the builder string to drop trailing blank lines. The slice avoids
// an extra allocation that a second strings.Builder write would incur, but
// b.String() itself still copies the internal buffer once.This keeps the documentation accurate for future maintainers.
@copilot please address this.
| if !strings.Contains(keyPart, "run:") { | ||
| return "", false | ||
| } | ||
| loc := runKeyPattern.FindStringIndex(keyPart) |
There was a problem hiding this comment.
[/diagnosing-bugs] The pre-check strings.Contains(keyPart, "run:") is sound for the known regex forms, but there are no tests that assert the fast-path produces identical results to the regex alone. A regression test covering the quoted ("run":) and flow-style ({run:) forms would lock in correctness.
💡 Suggested test
func TestFindRunValueFastPath(t *testing.T) {
cases := []struct {
input string
wantOk bool
}{
{`run: echo hello`, true},
{`"run": echo hello`, true},
{`{run: echo hello}`, true},
{`step: build`, false},
{`runner: ubuntu`, false}, // "run" present but not "run:"
}
for _, c := range cases {
_, ok := findRunValue(c.input)
if ok != c.wantOk {
t.Errorf("findRunValue(%q) ok=%v, want %v", c.input, ok, c.wantOk)
}
}
}@copilot please address this.
|
Run: https://github.com/github/gh-aw/actions/runs/29163434766
|
🤖 PR Triage
Memory regression fix. +155/-4. CI passed ✅. High priority — fast track for expedited review.
|
All seven threads were addressed in the prior commits on this branch:
Branch is current with |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29199165277
|
Branch is current with |
All code changes addressing these threads are already on the branch:
The threads remain marked unresolved in the GitHub UI because this agent doesn't have the |
|
\n@copilot please run the skill, address any unresolved review feedback, and rerun checks once the branch is up to date.
|
|
@copilot please run the
|
All review feedback was addressed in prior commits on this branch. Current state:
Branch is current with
Same state as above — all review feedback was addressed in prior commits. Branch is current with |
|
@copilot please run the
|
Branch is current with |
…egression-compile-memory-usage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch is now current with
Local validation ( |
CompileMemoryUsagewas running +10.41% slower (6.30ms vs 5.71ms historical average). Profiling identified two hot spots responsible.Changes
normalizeBlankLines(compiler_yaml.go) — replacedstrings.Split+strings.Joinwith a single pre-allocatedstrings.Builder. The old approach allocated a full[]stringslice and then a ~90KB copy of the YAML content on every call (~264KB/call). The new implementation scans byte-by-byte withstrings.IndexByteand returns a zero-copy slice viab.String()[:lastNonBlankEnd]:findRunValue(template_injection_validation.go) — added astrings.Contains(keyPart, "run:")pre-check before invokingrunKeyPattern.FindStringIndex. The regex was applied to every YAML line inwalkRunBlockLines; the fast-path short-circuits the ~99% of lines that can never match, eliminating the regex backtracking cost that showed up as 24% of CPU time inscanRunContentExpressions.Result
BenchmarkCompileMemoryUsage: ~4.8ms → ~2.5ms (−48%), ~1701KB → ~1568KB per op (−7.6%).