-
Notifications
You must be signed in to change notification settings - Fork 454
perf: eliminate strings.Split allocation in validateExpressionSizes #45282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+308
−26
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
bc23b2f
Initial plan
Copilot db57da2
perf: eliminate strings.Split allocation in validateExpressionSizes
Copilot 7787b6b
fix: clarify blockStartLine comment in validateExpressionSizesSinglePass
Copilot 40d9367
fix: extend block-scalar header detection and add single-pass tests
Copilot f3c8e86
fix: correct test name keep chomping (|-) -> (|+)
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,6 @@ | |
| package workflow | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "strings" | ||
|
|
@@ -61,47 +60,140 @@ var runtimeValidationLog = newValidationLogger("runtime") | |
| // many lines but are parsed by GitHub Actions as a single string value. When such a | ||
| // block contains at least one ${{ }} expression AND its total length exceeds 21,000 | ||
| // characters, GitHub Actions rejects the workflow with "Exceeded max expression length". | ||
| // | ||
| // Performance: uses a single strings.SplitSeq pass to avoid allocating a []string slice | ||
| // for the YAML content and to eliminate the double-pass overhead of the original | ||
| // implementation. | ||
| func (c *Compiler) validateExpressionSizes(yamlContent string) error { | ||
| lines := strings.Split(yamlContent, "\n") | ||
| runtimeValidationLog.Printf("Validating expression sizes: yaml_lines=%d, max_size=%d", len(lines), MaxExpressionSize) | ||
| maxSize := MaxExpressionSize | ||
| runtimeValidationLog.Printf("Validating expression sizes: max_size=%d", maxSize) | ||
| return validateExpressionSizesSinglePass(yamlContent, maxSize) | ||
| } | ||
|
|
||
| // validateExpressionSizesSinglePass combines the per-line size check and the block-scalar | ||
| // size check into a single strings.SplitSeq pass over the YAML content. This avoids | ||
| // allocating a []string slice for the YAML lines and eliminates the redundant second | ||
| // scan that validateBlockScalarExpressionSizes would otherwise perform. | ||
| func validateExpressionSizesSinglePass(yamlContent string, maxSize int) error { | ||
| // Block scalar tracking state (mirrors validateBlockScalarExpressionSizes). | ||
| inBlock := false | ||
| blockKey := "" | ||
| blockStartLine := 0 | ||
| blockIndent := -1 | ||
| blockSize := 0 | ||
| blockHasExpression := false | ||
|
|
||
| checkBlock := func() error { | ||
| if inBlock && blockHasExpression && blockSize > maxSize { | ||
| actualSize := console.FormatFileSize(int64(blockSize)) | ||
| maxSizeFormatted := console.FormatFileSize(int64(maxSize)) | ||
| return fmt.Errorf("expression value for %q (%s) exceeds maximum allowed size (%s) starting at line %d. "+ | ||
| "GitHub Actions has a 21KB limit for YAML values that contain template expressions (${{ }}). "+ | ||
| "Split the step into separate run: blocks so that no single block containing ${{ }} expressions exceeds the limit", | ||
| blockKey, actualSize, maxSizeFormatted, blockStartLine+1) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| for lineNum, line := range lines { | ||
| // Check the line length (actual content that will be in the YAML) | ||
| lineNum := 0 | ||
| for line := range strings.SplitSeq(yamlContent, "\n") { | ||
| lineNum++ | ||
|
|
||
| // Per-line size check: a single YAML line value exceeding maxSize is rejected. | ||
| if len(line) > maxSize { | ||
| // Extract the key/value for better error message | ||
| trimmed := strings.TrimSpace(line) | ||
| key := "" | ||
| if colonIdx := strings.Index(trimmed, ":"); colonIdx > 0 { | ||
| key = strings.TrimSpace(trimmed[:colonIdx]) | ||
| } | ||
|
|
||
| // Format sizes for display | ||
| actualSize := console.FormatFileSize(int64(len(line))) | ||
| maxSizeFormatted := console.FormatFileSize(int64(maxSize)) | ||
|
|
||
| var errorMsg string | ||
| if key != "" { | ||
| errorMsg = fmt.Sprintf("expression value for %q (%s) exceeds maximum allowed size (%s) at line %d. GitHub Actions has a 21KB limit for expression values including environment variables. Consider chunking the content or using artifacts instead.", | ||
| key, actualSize, maxSizeFormatted, lineNum+1) | ||
| } else { | ||
| errorMsg = fmt.Sprintf("line %d (%s) exceeds maximum allowed expression size (%s). GitHub Actions has a 21KB limit for expression values.", | ||
| lineNum+1, actualSize, maxSizeFormatted) | ||
| return fmt.Errorf("expression value for %q (%s) exceeds maximum allowed size (%s) at line %d. GitHub Actions has a 21KB limit for expression values including environment variables. Consider chunking the content or using artifacts instead", | ||
| key, actualSize, maxSizeFormatted, lineNum) | ||
| } | ||
| return fmt.Errorf("line %d (%s) exceeds maximum allowed expression size (%s). GitHub Actions has a 21KB limit for expression values", | ||
| lineNum, actualSize, maxSizeFormatted) | ||
| } | ||
|
|
||
| // Block scalar tracking: detect multi-line block scalars and accumulate their | ||
| // sizes so that run: | blocks containing ${{ }} can be checked against maxSize. | ||
| trimmed := strings.TrimLeft(line, " \t") | ||
| indent := len(line) - len(trimmed) | ||
|
|
||
| return errors.New(errorMsg) | ||
| if inBlock { | ||
| if strings.TrimSpace(line) == "" { | ||
| blockSize += len(line) + 1 // +1 for the newline | ||
| continue | ||
| } | ||
| if indent > blockIndent { | ||
| blockSize += len(line) + 1 | ||
| if strings.Contains(line, "${{") { | ||
| blockHasExpression = true | ||
| } | ||
| continue | ||
| } | ||
| // Indentation dropped back – the block has ended. | ||
| if err := checkBlock(); err != nil { | ||
| return err | ||
| } | ||
| inBlock = false | ||
| blockKey = "" | ||
| blockSize = 0 | ||
| blockHasExpression = false | ||
| blockIndent = -1 | ||
| } | ||
| } | ||
|
|
||
| // Check multi-line YAML block scalars that contain template expressions. | ||
| // A run: | or any other block-scalar value is a single string from GitHub Actions' | ||
| // perspective; if it contains ${{ }} AND is longer than 21,000 characters the | ||
| // runner rejects it with "Exceeded max expression length". | ||
| if err := validateBlockScalarExpressionSizes(lines, maxSize); err != nil { | ||
| return err | ||
| // Detect the start of a block scalar: a YAML key whose value is | or > | ||
| // e.g. " run: |" or " script: >" or " run: |+" or " run: |2-" | ||
| if colonIdx := strings.Index(trimmed, ":"); colonIdx > 0 { | ||
| afterColon := strings.TrimSpace(trimmed[colonIdx+1:]) | ||
| if isBlockScalarHeader(afterColon) { | ||
| inBlock = true | ||
|
Comment on lines
+149
to
+152
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in commit |
||
| blockKey = strings.TrimSpace(trimmed[:colonIdx]) | ||
| blockStartLine = lineNum - 1 // 0-indexed; checkBlock reports it as blockStartLine+1 | ||
| blockIndent = indent | ||
| blockSize = 0 | ||
| blockHasExpression = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| // Check any block that was still open at EOF. | ||
| return checkBlock() | ||
| } | ||
|
|
||
| // isBlockScalarHeader reports whether s (the trimmed string after a YAML colon) | ||
| // is a valid YAML block scalar indicator, covering all YAML-spec variants: | ||
| // | ||
| // bare: "|" ">" | ||
| // strip chomping: "|-" ">-" | ||
| // keep chomping: "|+" ">+" | ||
| // indent only: "|2" ">3" (digit 1–9) | ||
| // indent+strip: "|2-" ">3-" (also "|-2", ">-3") | ||
| // indent+keep: "|2+" ">3+" (also "|+2", ">+3") | ||
| func isBlockScalarHeader(s string) bool { | ||
| if len(s) == 0 || (s[0] != '|' && s[0] != '>') { | ||
| return false | ||
| } | ||
| rest := s[1:] | ||
| switch rest { | ||
| case "", "-", "+": | ||
| return true | ||
| } | ||
| if len(rest) == 1 { | ||
| return rest[0] >= '1' && rest[0] <= '9' | ||
| } | ||
| if len(rest) == 2 { | ||
| r0, r1 := rest[0], rest[1] | ||
| if r0 >= '1' && r0 <= '9' { | ||
| return r1 == '-' || r1 == '+' | ||
| } | ||
| if r0 == '-' || r0 == '+' { | ||
| return r1 >= '1' && r1 <= '9' | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // validateBlockScalarExpressionSizes scans the YAML lines for multi-line block scalars | ||
|
|
@@ -160,10 +252,10 @@ func validateBlockScalarExpressionSizes(lines []string, maxSize int) error { | |
| } | ||
|
|
||
| // Detect the start of a block scalar: a YAML key whose value is | or > | ||
| // e.g. " run: |" or " script: >" | ||
| // e.g. " run: |" or " script: >" or " run: |+" or " run: |2-" | ||
| if colonIdx := strings.Index(trimmed, ":"); colonIdx > 0 { | ||
| afterColon := strings.TrimSpace(trimmed[colonIdx+1:]) | ||
| if afterColon == "|" || afterColon == ">" || strings.HasPrefix(afterColon, "|-") || strings.HasPrefix(afterColon, ">-") { | ||
| if isBlockScalarHeader(afterColon) { | ||
| inBlock = true | ||
| blockKey = strings.TrimSpace(trimmed[:colonIdx]) | ||
| blockStartLine = i | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.