Skip to content

perf: fix double regex pass in UnquoteYAMLKey causing ~2x compile slowdown#22459

Merged
pelikhan merged 4 commits intomainfrom
copilot/fix-regression-compile-memory-usage
Mar 23, 2026
Merged

perf: fix double regex pass in UnquoteYAMLKey causing ~2x compile slowdown#22459
pelikhan merged 4 commits intomainfrom
copilot/fix-regression-compile-memory-usage

Conversation

Copy link
Contributor

Copilot AI commented Mar 23, 2026

BenchmarkCompileMemoryUsage regressed +103.7% (6.4ms → 13.1ms) due to UnquoteYAMLKey performing two regex operations per match: ReplaceAllStringFunc to iterate matches, then FindStringSubmatch inside the callback to retrieve capture groups. This function is called 10+ times per compilation via extractTopLevelYAMLSection.

Change

Replace the two-pass approach with ReplaceAllString using capture group references — a single-pass operation:

// Before: two regex passes per match
return re.ReplaceAllStringFunc(yamlStr, func(match string) string {
    submatches := re.FindStringSubmatch(match) // ← redundant second pass
    return submatches[1] + submatches[2] + key + ":"
})

// After: one pass
return re.ReplaceAllString(yamlStr, "${1}${2}"+key+":")

Benchmark after fix: ~7.6ms (restored to near historical baseline of ~6.4ms).

Testing

Added BenchmarkUnquoteYAMLKey to pkg/workflow/yaml_test.go to guard against future regressions of the double regex pass pattern. The benchmark uses a realistic workflow YAML string and confirms the single-pass implementation performance.


⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.

…-pass regex replacement

The UnquoteYAMLKey function was using ReplaceAllStringFunc with a callback
that called re.FindStringSubmatch inside - performing two regex passes per
match. This caused a ~103% performance regression in BenchmarkCompileMemoryUsage.

Replace with ReplaceAllString using capture group references (${1}${2}key:)
for a single-pass O(n) replacement, restoring performance from ~13ms back
to ~7.6ms (near historical average of ~6.4ms).

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/892822a4-0baa-4043-a447-cb4ead4ac131
Copilot AI changed the title [WIP] Fix regression in CompileMemoryUsage performance perf: fix double regex pass in UnquoteYAMLKey causing ~2x compile slowdown Mar 23, 2026
Copilot AI requested a review from pelikhan March 23, 2026 16:38
@pelikhan
Copy link
Contributor

@copilot add tests

Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot add tests

Added BenchmarkUnquoteYAMLKey to pkg/workflow/yaml_test.go (commit 17d774e). It benchmarks the single-pass ReplaceAllString implementation on a realistic workflow YAML, providing a regression guard for the double-pass antipattern this PR fixes.

@pelikhan pelikhan marked this pull request as ready for review March 23, 2026 16:51
Copilot AI review requested due to automatic review settings March 23, 2026 16:51
@pelikhan pelikhan merged commit b9374bf into main Mar 23, 2026
102 checks passed
@pelikhan pelikhan deleted the copilot/fix-regression-compile-memory-usage branch March 23, 2026 16:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves workflow compilation performance by eliminating a redundant second regex pass in UnquoteYAMLKey, and adds a benchmark to track the regression going forward.

Changes:

  • Refactor UnquoteYAMLKey to use ReplaceAllString with capture group references (single regex pass).
  • Add BenchmarkUnquoteYAMLKey to measure/track UnquoteYAMLKey performance over realistic input.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/workflow/yaml.go Switches UnquoteYAMLKey to a single-pass ReplaceAllString implementation to reduce compile-time overhead.
pkg/workflow/yaml_test.go Adds a benchmark covering the UnquoteYAMLKey hot path to help detect future performance regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +149 to +151
// Use ReplaceAllString with capture group references for a single-pass replacement.
// ${1} = line start (^ or \n), ${2} = optional whitespace
return re.ReplaceAllString(yamlStr, "${1}${2}"+key+":")
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regexp.Regexp.ReplaceAllString treats $ in the replacement string as a capture-group expansion. Since key is concatenated directly into the replacement, keys containing $ (e.g. user-provided frontmatter keys) would be corrupted by unintended backref expansion. Escape $ in key before building the replacement (e.g. replace $ with $$), or use an API that performs literal insertion for the dynamic portion.

Suggested change
// Use ReplaceAllString with capture group references for a single-pass replacement.
// ${1} = line start (^ or \n), ${2} = optional whitespace
return re.ReplaceAllString(yamlStr, "${1}${2}"+key+":")
// Escape '$' in the key so it is treated literally in the replacement string,
// not as a backreference marker by regexp.ReplaceAllString.
escapedKey := strings.ReplaceAll(key, "$", "$$")
// Use ReplaceAllString with capture group references for a single-pass replacement.
// ${1} = line start (^ or \n), ${2} = optional whitespace
return re.ReplaceAllString(yamlStr, "${1}${2}"+escapedKey+":")

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[performance] Regression in CompileMemoryUsage: +103.7% slower

3 participants