Use strings.Builder instead of += concatenation in loops#524
Use strings.Builder instead of += concatenation in loops#524mengzhuo merged 2 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors string concatenation operations to use strings.Builder instead of += operators in loops, improving performance from O(n²) to O(n) complexity for repeated string appends. The changes target utility and formatting functions in the agent package that build text output for prompts, logs, and memory contexts.
Changes:
- Replaced string concatenation with
strings.Builderin multiple functions across memory.go, loop.go, and context.go - Renamed parameter in
formatToolsForLogfromtoolstotoolDefsfor clarity - Consolidated logic in
GetMemoryContextandGetRecentDailyNotesto usestrings.Builderpattern
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/agent/memory.go | Refactored GetRecentDailyNotes and GetMemoryContext to use strings.Builder, but introduced a bug with duplicate "# Memory" header |
| pkg/agent/loop.go | Converted formatMessagesForLog, formatToolsForLog, and summarizeBatch to use strings.Builder for efficient string building |
| pkg/agent/context.go | Updated LoadBootstrapFiles to use strings.Builder instead of string concatenation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nikolasdehor
left a comment
There was a problem hiding this comment.
Clean refactor. The string concatenation → strings.Builder conversions are correct and well-scoped.
A few notes:
-
context.go L152 —
fmt.Fprintf(&sb, "...", filename, data)wheredatais[]byte: this works because%shandles[]bytecorrectly in Go, so this is fine and avoids thestring(data)allocation. Good catch. -
memory.go GetMemoryContext() — The refactored version has a subtle behavior change: the old code only prepended
# Memory\n\nat the end viafmt.Sprintf, but the new code writes it to the builder first. Functionally equivalent, just noting it for review. -
loop.go — The
tools→toolDefsrename informatToolsForLogis a nice bonus to avoid shadowing the parameter name with the range variable, though Go wouldn't shadow it here since the types differ. Still cleaner.
LGTM — straightforward performance improvement with no functional changes.
Use strings.Builder instead of += concatenation in loops
📝 Description
Replace string concatenation with strings.Builder in:
Reduces allocations from O(n^2) to O(n) for repeated appends.
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist