Skip to content

Commit e764239

Browse files
jbedardjbedard
authored
perf: avoid temp arrays in sortExprLabels (#2395)
**What type of PR is this?** Perf - memory reduction. **What package or component does this PR mostly affect?** all **What does this PR do? Why is it needed?** Same as bazel-contrib/buildtools#1487 where this code was vendored from. **Which issues(s) does this PR fix?** Fixes # **Other notes for review**
1 parent b9fe795 commit e764239

3 files changed

Lines changed: 276 additions & 65 deletions

File tree

‎v2/rule/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ go_test(
2929
"directives_test.go",
3030
"merge_test.go",
3131
"rule_test.go",
32+
"sort_labels_test.go",
3233
"value_test.go",
3334
],
3435
embed = [":rule"],
@@ -55,6 +56,7 @@ filegroup(
5556
"rule.go",
5657
"rule_test.go",
5758
"sort_labels.go",
59+
"sort_labels_test.go",
5860
"types.go",
5961
"value.go",
6062
"value_test.go",

‎v2/rule/sort_labels.go‎

Lines changed: 65 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ limitations under the License.
1616
package rule
1717

1818
import (
19-
"sort"
19+
"slices"
2020
"strings"
2121

2222
bzl "github.com/bazelbuild/buildtools/build"
@@ -27,88 +27,88 @@ import (
2727
// expressions. This function is intended to be used with bzl.Walk.
2828
func sortExprLabels(e bzl.Expr, _ []bzl.Expr) {
2929
list, ok := e.(*bzl.ListExpr)
30-
if !ok || len(list.List) == 0 {
30+
if !ok || len(list.List) < 2 {
3131
return
3232
}
3333

34-
keys := make([]stringSortKey, len(list.List))
35-
for i, elem := range list.List {
36-
s, ok := elem.(*bzl.StringExpr)
37-
if !ok {
34+
// Check that all elements are strings
35+
for _, elem := range list.List {
36+
if _, ok := elem.(*bzl.StringExpr); !ok {
3837
return // don't sort lists unless all elements are strings
3938
}
40-
keys[i] = makeSortKey(i, s)
4139
}
4240

43-
before := keys[0].x.Comment().Before
44-
keys[0].x.Comment().Before = nil
45-
sort.Sort(byStringExpr(keys))
46-
keys[0].x.Comment().Before = append(before, keys[0].x.Comment().Before...)
47-
for i, k := range keys {
48-
list.List[i] = k.x
49-
}
50-
}
41+
// A comment block above the first element is pinned to the top of the
42+
// list, matching buildifier.
43+
before := list.List[0].Comment().Before
44+
list.List[0].Comment().Before = nil
5145

52-
// Code below this point is adapted from
53-
// github.com/bazelbuild/buildtools/build/rewrite.go
46+
slices.SortStableFunc(list.List, compareStringExpr)
5447

55-
// A stringSortKey records information about a single string literal to be
56-
// sorted. The strings are first grouped into four phases: most strings,
57-
// strings beginning with ":", strings beginning with "//", and strings
58-
// beginning with "@". The next significant part of the comparison is the list
59-
// of elements in the value, where elements are split at `.' and `:'. Finally
60-
// we compare by value and break ties by original index.
61-
type stringSortKey struct {
62-
phase int
63-
split []string
64-
value string
65-
original int
66-
x bzl.Expr
48+
list.List[0].Comment().Before = append(before, list.List[0].Comment().Before...)
6749
}
6850

69-
func makeSortKey(index int, x *bzl.StringExpr) stringSortKey {
70-
key := stringSortKey{
71-
value: x.Value,
72-
original: index,
73-
x: x,
74-
}
51+
// Code below this point matches the sort order of
52+
// github.com/bazelbuild/buildtools/build/rewrite.go
7553

76-
switch {
77-
case strings.HasPrefix(x.Value, ":"):
78-
key.phase = 1
79-
case strings.HasPrefix(x.Value, "//"):
80-
key.phase = 2
81-
case strings.HasPrefix(x.Value, "@"):
82-
key.phase = 3
54+
// compareStringExpr compares two string literals to be sorted. The strings
55+
// are first grouped into four phases: most strings, strings beginning with
56+
// ":", strings beginning with "//", and strings beginning with "@". The next
57+
// significant part of the comparison is the list of elements in the value,
58+
// where elements are split at `.' and `:'. Finally we compare by value,
59+
// leaving equal values in their original order.
60+
func compareStringExpr(a, b bzl.Expr) int {
61+
sa := a.(*bzl.StringExpr).Value
62+
sb := b.(*bzl.StringExpr).Value
63+
64+
if phaseA, phaseB := labelPhase(sa), labelPhase(sb); phaseA != phaseB {
65+
return phaseA - phaseB
8366
}
8467

85-
key.split = strings.Split(strings.Replace(x.Value, ":", ".", -1), ".")
86-
return key
68+
return compareStringExpValue(sa, sb)
8769
}
8870

89-
// byStringExpr implements sort.Interface for a list of stringSortKey.
90-
type byStringExpr []stringSortKey
91-
92-
func (x byStringExpr) Len() int { return len(x) }
93-
func (x byStringExpr) Swap(i, j int) { x[i], x[j] = x[j], x[i] }
94-
95-
func (x byStringExpr) Less(i, j int) bool {
96-
xi := x[i]
97-
xj := x[j]
98-
99-
if xi.phase != xj.phase {
100-
return xi.phase < xj.phase
71+
func labelPhase(s string) int {
72+
switch {
73+
case strings.HasPrefix(s, ":"):
74+
return 1
75+
case strings.HasPrefix(s, "//"):
76+
return 2
77+
case strings.HasPrefix(s, "@"):
78+
return 3
10179
}
102-
for k := 0; k < len(xi.split) && k < len(xj.split); k++ {
103-
if xi.split[k] != xj.split[k] {
104-
return xi.split[k] < xj.split[k]
80+
return 0
81+
}
82+
83+
// compareStringExpValue compares the `.'/`:' separated segments of two
84+
// values without splitting them: a separator ends a segment, so it sorts
85+
// before any other character, and `.' and `:' compare as equal. Values with
86+
// equal segments are ordered by raw value.
87+
func compareStringExpValue(a, b string) int {
88+
for i := 0; i < len(a) && i < len(b); i++ {
89+
if a[i] != b[i] {
90+
sepA := a[i] == '.' || a[i] == ':'
91+
sepB := b[i] == '.' || b[i] == ':'
92+
if sepA != sepB {
93+
if sepA {
94+
return -1
95+
}
96+
return 1
97+
}
98+
if !sepA {
99+
if a[i] < b[i] {
100+
return -1
101+
}
102+
return 1
103+
}
104+
// Both are separators, which compare as equal.
105105
}
106106
}
107-
if len(xi.split) != len(xj.split) {
108-
return len(xi.split) < len(xj.split)
109-
}
110-
if xi.value != xj.value {
111-
return xi.value < xj.value
107+
108+
if len(a) != len(b) {
109+
return len(a) - len(b)
112110
}
113-
return xi.original < xj.original
111+
112+
// The values differ only by separators.
113+
return strings.Compare(a, b)
114114
}

‎v2/rule/sort_labels_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
/* Copyright 2026 The Bazel Authors. All rights reserved.
2+
3+
Licensed under the Apache License, Version 2.0 (the "License");
4+
you may not use this file except in compliance with the License.
5+
You may obtain a copy of the License at
6+
7+
http://www.apache.org/licenses/LICENSE-2.0
8+
9+
Unless required by applicable law or agreed to in writing, software
10+
distributed under the License is distributed on an "AS IS" BASIS,
11+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
See the License for the specific language governing permissions and
13+
limitations under the License.
14+
*/
15+
16+
package rule
17+
18+
import (
19+
"testing"
20+
21+
bzl "github.com/bazelbuild/buildtools/build"
22+
)
23+
24+
func sortAndFormat(t *testing.T, src string) string {
25+
t.Helper()
26+
f, err := bzl.ParseBuild("BUILD.bazel", []byte(src))
27+
if err != nil {
28+
t.Fatal(err)
29+
}
30+
for _, stmt := range f.Stmt {
31+
bzl.Walk(stmt, sortExprLabels)
32+
}
33+
return string(bzl.Format(f))
34+
}
35+
36+
// Comments attached to list elements move with their element when the list is
37+
// sorted, including a comment block above the first element.
38+
func TestSortExprLabelsComments(t *testing.T) {
39+
for name, tc := range map[string]struct {
40+
src, want string
41+
}{
42+
"before comment on first element moves with it": {
43+
src: `deps = [
44+
# comment on b
45+
":b",
46+
":a",
47+
]
48+
`,
49+
want: `deps = [
50+
# comment on b
51+
":a",
52+
":b",
53+
]
54+
`,
55+
},
56+
"before comment on middle element moves with it": {
57+
src: `deps = [
58+
":c",
59+
# comment on b
60+
":b",
61+
":a",
62+
]
63+
`,
64+
want: `deps = [
65+
":a",
66+
# comment on b
67+
":b",
68+
":c",
69+
]
70+
`,
71+
},
72+
"suffix comments move with elements": {
73+
src: `deps = [
74+
":b", # comment on b
75+
":a", # comment on a
76+
]
77+
`,
78+
want: `deps = [
79+
":a", # comment on a
80+
":b", # comment on b
81+
]
82+
`,
83+
},
84+
"already sorted list with leading comment unchanged": {
85+
src: `deps = [
86+
# comment on a
87+
":a",
88+
":b",
89+
]
90+
`,
91+
want: `deps = [
92+
# comment on a
93+
":a",
94+
":b",
95+
]
96+
`,
97+
},
98+
} {
99+
t.Run(name, func(t *testing.T) {
100+
got := sortAndFormat(t, tc.src)
101+
if got != tc.want {
102+
t.Errorf("got:\n%s\nwant:\n%s", got, tc.want)
103+
}
104+
})
105+
}
106+
}
107+
108+
// The buildifier sort order splits values on "." and ":", comparing the
109+
// resulting segments, so a separator sorts before any other character and
110+
// "." and ":" compare equal, with ties broken by raw value then input order.
111+
func TestSortExprLabelsOrdering(t *testing.T) {
112+
for name, tc := range map[string]struct {
113+
src, want string
114+
}{
115+
"separator sorts before dash": {
116+
src: `deps = [
117+
":foo-bar",
118+
":foo.bar",
119+
]
120+
`,
121+
want: `deps = [
122+
":foo.bar",
123+
":foo-bar",
124+
]
125+
`,
126+
},
127+
"separator sorts before plus": {
128+
src: `deps = [
129+
":a+b",
130+
":a.b",
131+
]
132+
`,
133+
want: `deps = [
134+
":a.b",
135+
":a+b",
136+
]
137+
`,
138+
},
139+
"colon separator sorts before digits": {
140+
src: `deps = [
141+
":a5",
142+
":a:2",
143+
]
144+
`,
145+
want: `deps = [
146+
":a:2",
147+
":a5",
148+
]
149+
`,
150+
},
151+
"dot and colon separators tie broken by raw value": {
152+
src: `deps = [
153+
":a:b",
154+
":a.b",
155+
]
156+
`,
157+
want: `deps = [
158+
":a.b",
159+
":a:b",
160+
]
161+
`,
162+
},
163+
"duplicate values keep input order": {
164+
src: `deps = [
165+
":a", # first
166+
":a", # second
167+
]
168+
`,
169+
want: `deps = [
170+
":a", # first
171+
":a", # second
172+
]
173+
`,
174+
},
175+
"relative phase sorts before absolute": {
176+
src: `deps = [
177+
"//x",
178+
"/x",
179+
]
180+
`,
181+
want: `deps = [
182+
"/x",
183+
"//x",
184+
]
185+
`,
186+
},
187+
"empty string sorts first": {
188+
src: `deps = [
189+
"x",
190+
"",
191+
":a",
192+
]
193+
`,
194+
want: `deps = [
195+
"",
196+
"x",
197+
":a",
198+
]
199+
`,
200+
},
201+
} {
202+
t.Run(name, func(t *testing.T) {
203+
got := sortAndFormat(t, tc.src)
204+
if got != tc.want {
205+
t.Errorf("got:\n%s\nwant:\n%s", got, tc.want)
206+
}
207+
})
208+
}
209+
}

0 commit comments

Comments
 (0)