Skip to content

perf: replace O(n²) string concatenation with strings.Builder in expression_parser#22095

Merged
pelikhan merged 3 commits intomainfrom
copilot/fix-string-building-performance
Mar 21, 2026
Merged

perf: replace O(n²) string concatenation with strings.Builder in expression_parser#22095
pelikhan merged 3 commits intomainfrom
copilot/fix-string-building-performance

Conversation

Copy link
Contributor

Copilot AI commented Mar 21, 2026

BreakLongExpression and BreakAtParentheses accumulated characters via current += string(char) inside byte-by-byte loops, causing O(n²) allocations — hitting hardest on the long expressions these functions are specifically gated to handle.

Changes

  • BreakLongExpression: Replace current string accumulator with var current strings.Builder; eliminate the inner sb strings.Builder used for quoted strings by writing directly into current
  • BreakAtParentheses: Same strings.Builder replacement; use current.Len() instead of len(current) for the length check
  • Both functions: use trimmed := strings.TrimSpace(current.String()) local variable to avoid redundant .String() + TrimSpace double-calls at flush points

Before:

current := ""
// ...
current += string(char)

After:

var current strings.Builder
// ...
current.WriteByte(char)
// flush with: strings.TrimSpace(current.String())

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI linked an issue Mar 21, 2026 that may be closed by this pull request
4 tasks
Copilot AI and others added 2 commits March 21, 2026 05:16
…parser.go

Replace `current += string(char)` and `current += ...` accumulations with
`strings.Builder` in both `BreakLongExpression` and `BreakAtParentheses`.

- BreakLongExpression: single `var current strings.Builder`; also
  eliminates the inner `sb strings.Builder` that was used for quoted strings
  (now writes directly into `current`)
- BreakAtParentheses: same pattern; use `current.Len()` instead of
  `len(current)` for the length check

Fixes the O(n²) allocation pattern identified in the audit."

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b3b6cf6b-6f89-413d-9e51-ebabb6483911
Copilot AI changed the title [WIP] Fix O(n²) string building in expression_parser.go perf: replace O(n²) string concatenation with strings.Builder in expression_parser Mar 21, 2026
Copilot AI requested a review from pelikhan March 21, 2026 05:18
@pelikhan pelikhan marked this pull request as ready for review March 21, 2026 05:19
Copilot AI review requested due to automatic review settings March 21, 2026 05:19
@github-actions
Copy link
Contributor

Hey @Copilot 👋 — thanks for picking up this performance fix! Replacing current += string(char) with strings.Builder in BreakLongExpression and BreakAtParentheses is exactly the right call — Go string concatenation inside a loop is O(n²) and strings.Builder is the idiomatic fix.

However, the PR is currently empty (zero file changes — only an "Initial plan" commit), and the implementation + test tasks in the checklist are still unchecked. Per the contribution guidelines, an agent-created PR needs to:

  • Implement the code changes in pkg/workflow/expression_parser.go
  • Add/update tests in expression_parser_comprehensive_test.go (the project has a comprehensive test file there already)
  • Run make agent-finish to pass build, test, recompile, fmt, lint, and lint-errors gates

When you're ready to continue, here's a prompt to get it done:

Complete the WIP performance fix in `pkg/workflow/expression_parser.go`.

Context:
- PR branch: copilot/fix-string-building-performance
- Target file: pkg/workflow/expression_parser.go
- Functions to fix: `BreakLongExpression` (line ~323) and `BreakAtParentheses` (line ~410)

Task:
1. In `BreakLongExpression`, replace the `current` string variable with a `strings.Builder`.
   - Replace `current := ""` with `var currentBuilder strings.Builder`
   - Replace all `current += ...` with `currentBuilder.WriteString(...)` or `currentBuilder.WriteByte(...)`
   - Replace reads of `current` (e.g. `len(current)`, `strings.TrimSpace(current)`) with `currentBuilder.String()`
   - Reset the builder between lines using `currentBuilder.Reset()`

2. Apply the same `strings.Builder` refactor to `BreakAtParentheses`.

3. Ensure `strings` is still imported (it already is).

4. Add or update tests in `pkg/workflow/expression_parser_comprehensive_test.go` covering:
   - A very long expression (>MaxExpressionLineLength chars) exercising the new Builder path in `BreakLongExpression`
   - A long expression with nested parentheses exercising `BreakAtParentheses`
   - Verify output is identical to pre-refactor behavior (pure performance refactor — no behavioral change)

5. Run `make agent-finish` and ensure all checks pass (build, test, lint, recompile, fmt, lint-errors).

6. Update the PR body checklist to mark all steps complete.

Note

🔒 Integrity filtering filtered 1 item

Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

  • issue:#unknown (search_issues: has secrecy requirements that agent doesn't meet. The agent is not authorized to access private-scoped data.)

Generated by Contribution Check ·

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 performance of long-expression line breaking by eliminating quadratic string concatenation in favor of strings.Builder, targeting cases where expressions exceed configured length thresholds.

Changes:

  • Replaced per-character current += ... accumulation with strings.Builder in BreakLongExpression.
  • Applied the same strings.Builder approach to BreakAtParentheses, switching length checks to current.Len().
  • Reduced repeated TrimSpace(current.String()) calls at flush points via a local trimmed variable.

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

@pelikhan pelikhan merged commit deea5b3 into main Mar 21, 2026
115 checks passed
@pelikhan pelikhan deleted the copilot/fix-string-building-performance branch March 21, 2026 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[plan] Fix O(n²) string building in expression_parser.go

3 participants