perf: eliminate strings.Split allocation in validateExpressionSizes#45282
Conversation
Replace the two-pass approach (strings.Split + per-line loop + validateBlockScalarExpressionSizes loop) with a single strings.SplitSeq pass in validateExpressionSizesSinglePass. This removes the 1273-element []string allocation (~21KB/op) that strings.Split was creating for each compiled workflow YAML. Benchstat with 6+ runs shows a statistically significant -2.10% reduction in B/op (p=0.004) with no regression in throughput (p=0.177). The separate validateBlockScalarExpressionSizes([]string, int) function is preserved for backward compatibility with its unit tests. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage (run 29275650494)\n\nCategory: perf | Risk: low | Score: 45/100 | Action: batch_review | Batch: draft-quality\n\nScore: Impact 20/50 + Urgency 12/30 + Quality 13/20\n\nEliminates strings.Split allocation in validateExpressionSizes via SplitSeq. Single file, +83/-24. DRAFT. Can be reviewed alongside #45280 and #45298.
|
There was a problem hiding this comment.
Pull request overview
Optimizes expression-size validation by replacing an allocating, two-pass scan with a single strings.SplitSeq pass.
Changes:
- Combines per-line and block-scalar validation.
- Avoids allocating a YAML line slice.
- Retains the legacy block-scalar helper for existing tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/runtime_validation.go |
Implements single-pass expression-size validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Medium
| if colonIdx := strings.Index(trimmed, ":"); colonIdx > 0 { | ||
| afterColon := strings.TrimSpace(trimmed[colonIdx+1:]) | ||
| if afterColon == "|" || afterColon == ">" || strings.HasPrefix(afterColon, "|-") || strings.HasPrefix(afterColon, ">-") { | ||
| inBlock = true |
There was a problem hiding this comment.
Addressed in commit 40d9367: extracted isBlockScalarHeader that covers all YAML-spec block scalar variants — |, >, |-, >-, |+, >+, |2–|9, |2-, |-2, >+3, etc. Both validateExpressionSizesSinglePass and validateBlockScalarExpressionSizes now use it. New test cases verify the previously-missed chomping/indent-indicator variants are caught.
|
@copilot please run the
|
- Add isBlockScalarHeader helper to cover |+, >+, |2, |2-, |-2 etc. in both validateExpressionSizesSinglePass and validateBlockScalarExpressionSizes (previously only |- and >- were caught; keep-chomping and explicit-indent indicators were silently missed, letting oversized expression blocks bypass validation) - Add TestValidateExpressionSizesSinglePass with parity cases against the legacy helper plus per-line size check and EOF-block coverage - Add chomping/indent-indicator cases to TestValidateBlockScalarExpressionSizes Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Two issues addressed in commits Thread 1 (line 77 — missing tests for Thread 2 (lines 149-152 — chomping/indent indicators missed): Extracted an All |
|
Run: https://github.com/github/gh-aw/actions/runs/29289965782
|
validateExpressionSizeswas callingstrings.Split(yamlContent, "\n")on every compilation, allocating a ~21KB[]stringslice for the compiled YAML and then scanning it twice (per-line length check + block-scalar check).Changes
runtime_validation.go: Replaces the two-pass approach withvalidateExpressionSizesSinglePass, a singlestrings.SplitSeqpass that combines per-line and block-scalar validation, eliminating the slice allocation entirely.validateBlockScalarExpressionSizes([]string, int)is preserved unchanged for its unit tests.Benchmark (benchstat, 6+ runs)
Memory reduction is statistically significant; timing is within noise. A
walkRunBlockLinesfast-path was also evaluated (adding an earlystrings.Containsskip for non-run-block lines) but reverted after profiling showed the Contains overhead on ~913 lines/compilation exceeded the savings from skippingTrimLeft+findRunValue.