perf: reduce allocations in the printer and string-list sorting#1476
perf: reduce allocations in the printer and string-list sorting#1476jbedard wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request optimizes the seq method in build/print.go by only copying the argument slice when a nil element is present. The reviewer suggested a further optimization to avoid a full second pass over the slice by copying the non-nil prefix and iterating only over the remaining elements.
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.
| args := list | ||
| for _, x := range *list { | ||
| // nil arguments may be added by some linter checks, filter them out because | ||
| // they may cause NPE. | ||
| if x != nil { | ||
| *args = append(*args, x) | ||
| if x == nil { | ||
| filtered := make([]Expr, 0, len(*list)) | ||
| for _, y := range *list { | ||
| if y != nil { | ||
| filtered = append(filtered, y) | ||
| } | ||
| } | ||
| args = &filtered | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
When a nil element is found, we can optimize the filtering process by avoiding a full second pass over the entire slice. Instead, we can create the filtered slice with a length equal to the index i of the first nil element, use the highly optimized built-in copy function to copy the non-nil prefix, and then only loop over the remaining elements starting from i+1. Ensure that this optimization is consistent with the previous behavior and does not introduce any unintended side effects.
| args := list | |
| for _, x := range *list { | |
| // nil arguments may be added by some linter checks, filter them out because | |
| // they may cause NPE. | |
| if x != nil { | |
| *args = append(*args, x) | |
| if x == nil { | |
| filtered := make([]Expr, 0, len(*list)) | |
| for _, y := range *list { | |
| if y != nil { | |
| filtered = append(filtered, y) | |
| } | |
| } | |
| args = &filtered | |
| break | |
| } | |
| } | |
| args := list | |
| for i, x := range *list { | |
| if x == nil { | |
| filtered := make([]Expr, i, len(*list)) | |
| copy(filtered, (*list)[:i]) | |
| for _, y := range (*list)[i+1:] { | |
| if y != nil { | |
| filtered = append(filtered, y) | |
| } | |
| } | |
| args = &filtered | |
| break | |
| } | |
| } |
References
- When modifying code, ensure that changes are consistent with previous behavior, especially if the previous behavior was intentional.
1216415 to
b0c00d5
Compare
Buildtools PR checklist
Description
Reduce array copying to only when necessary, which seems like the exception and not the norm.