refactor: use strings.Builder to replace string concatenation - #5495
Conversation
volcano-sh-bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the String() methods across several files (cluster_info.go, node_info.go, cache.go, and session.go) to use strings.Builder and fmt.Fprintf instead of string concatenation, which improves memory efficiency. However, a critical syntax error was introduced in pkg/scheduler/api/cluster_info.go due to an extra closing parenthesis in a fmt.Fprintf call, which will cause a compilation failure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR refactors several String() implementations in the scheduler code to reduce allocations by replacing repeated string concatenation/fmt.Sprintf chains with strings.Builder.
Changes:
- Update
Session.String()to write into astrings.Builderviafmt.Fprintf. - Update
SchedulerCache.String()to build output viastrings.Builder(WriteString+fmt.Fprintf). - Update
NodeInfo.String()andClusterInfo.String()to accumulate task/node/job lines viastrings.Builder.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/scheduler/framework/session.go | Switches session string rendering to strings.Builder. |
| pkg/scheduler/cache/cache.go | Switches cache string rendering to strings.Builder. |
| pkg/scheduler/api/node_info.go | Switches node task list rendering in String() to strings.Builder. |
| pkg/scheduler/api/cluster_info.go | Switches cluster snapshot string rendering to strings.Builder (currently introduces a compile error). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a8f9e45 to
71e9aca
Compare
| return fmt.Sprintf("Node (%s): allocatable<%v> idle <%v>, used <%v>, releasing <%v>, oversubscribution <%v>, "+ | ||
| "state <phase %s, reaseon %s>, oversubscributionNode <%v>, offlineJobEvicting <%v>,taints <%v>%s, imageStates %v", | ||
| ni.Name, ni.Allocatable, ni.Idle, ni.Used, ni.Releasing, ni.OversubscriptionResource, ni.State.Phase, ni.State.Reason, ni.OversubscriptionNode, ni.OfflineJobEvicting, ni.Node.Spec.Taints, tasks, ni.ImageStates) | ||
| ni.Name, ni.Allocatable, ni.Idle, ni.Used, ni.Releasing, ni.OversubscriptionResource, ni.State.Phase, ni.State.Reason, ni.OversubscriptionNode, ni.OfflineJobEvicting, ni.Node.Spec.Taints, b.String(), ni.ImageStates) |
Signed-off-by: james <open4pd@4paradigm.com>
71e9aca to
b1df5d8
Compare
volcano-sh-bot
added
size/L
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JesseStutler The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
approved
What type of PR is this?
What this PR does / why we need it:
To improve performance, replace the inefficient string concatenation (+=) with
strings.BuilderWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?