From 09bb5dce22621ecdb5a6db29dadf6441b6477f47 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Sun, 16 Aug 2026 17:24:16 -0700 Subject: [PATCH] perf: avoid temp sort key arrays in sortStringExprs --- build/BUILD.bazel | 1 + build/rewrite.go | 141 ++++++++--------- build/sort_test.go | 260 ++++++++++++++++++++++++++++++++ build/testdata/079.build.golden | 42 ++++++ build/testdata/079.bzl.golden | 43 ++++++ build/testdata/079.in | 43 ++++++ 6 files changed, 462 insertions(+), 68 deletions(-) create mode 100644 build/sort_test.go create mode 100644 build/testdata/079.build.golden create mode 100644 build/testdata/079.bzl.golden create mode 100644 build/testdata/079.in diff --git a/build/BUILD.bazel b/build/BUILD.bazel index e1d3f6f12..fc3610929 100644 --- a/build/BUILD.bazel +++ b/build/BUILD.bazel @@ -39,6 +39,7 @@ go_test( "quote_test.go", "rewrite_test.go", "rule_test.go", + "sort_test.go", "utils_test.go", "walk_test.go", ], diff --git a/build/rewrite.go b/build/rewrite.go index 953013014..9573e81a3 100644 --- a/build/rewrite.go +++ b/build/rewrite.go @@ -672,22 +672,16 @@ func sortStringExprs(list []Expr) []Expr { } } - chunk := make([]stringSortKey, 0, j-i) - for index, x := range list[i:j] { - chunk = append(chunk, makeSortKey(index, x.(*StringExpr))) - } - if !sort.IsSorted(byStringExpr(chunk)) || !isUniq(chunk) { - before := chunk[0].x.Comment().Before - chunk[0].x.Comment().Before = nil + chunk := byStringExpr(list[i:j]) + if !sort.IsSorted(chunk) || !isUniq(chunk) { + before := chunk[0].Comment().Before + chunk[0].Comment().Before = nil - sort.Sort(byStringExpr(chunk)) + sort.Stable(chunk) chunk = uniq(chunk) - chunk[0].x.Comment().Before = before - for offset, key := range chunk { - list[i+offset] = key.x - } - list = append(list[:(i+len(chunk))], list[j:]...) + chunk[0].Comment().Before = before + list = append(list[:i+len(chunk)], list[j:]...) } i = j @@ -698,85 +692,96 @@ func sortStringExprs(list []Expr) []Expr { // uniq removes duplicates from a list, which must already be sorted. // It edits the list in place. -func uniq(sortedList []stringSortKey) []stringSortKey { +func uniq(sortedList byStringExpr) byStringExpr { out := sortedList[:0] - for _, sk := range sortedList { - if len(out) == 0 || sk.value != out[len(out)-1].value { - out = append(out, sk) + for _, x := range sortedList { + if len(out) == 0 || x.(*StringExpr).Value != out[len(out)-1].(*StringExpr).Value { + out = append(out, x) } } return out } // isUniq reports whether the sorted list only contains unique elements. -func isUniq(list []stringSortKey) bool { - for i := range list { - if i+1 < len(list) && list[i].value == list[i+1].value { +func isUniq(list byStringExpr) bool { + for i := 1; i < len(list); i++ { + if list[i-1].(*StringExpr).Value == list[i].(*StringExpr).Value { return false } } return true } -// A stringSortKey records information about a single string literal to be -// sorted. The strings are first grouped into four phases: most strings, -// strings beginning with ":", strings beginning with "//", and strings -// beginning with "@". The next significant part of the comparison is the list -// of elements in the value, where elements are split at `.' and `:'. Finally -// we compare by value and break ties by original index. -type stringSortKey struct { - phase int - split []string - value string - original int - x Expr +// byStringExpr implements sort.Interface for a list of string expressions. +// TODO: once the go directive reaches 1.21, drop this type and use +// slices.SortStableFunc/slices.IsSortedFunc with compareStringExpr instead, +// which also avoids boxing the slice into a sort.Interface. +type byStringExpr []Expr + +func (x byStringExpr) Len() int { return len(x) } +func (x byStringExpr) Swap(i, j int) { x[i], x[j] = x[j], x[i] } + +func (x byStringExpr) Less(i, j int) bool { + return compareStringExpr(x[i].(*StringExpr), x[j].(*StringExpr)) < 0 } -func makeSortKey(index int, x *StringExpr) stringSortKey { - key := stringSortKey{ - value: x.Value, - original: index, - x: x, +// compareStringExpr compares two string literals to be sorted. The strings +// are first grouped into four phases: most strings, strings beginning with +// ":", strings beginning with "//", and strings beginning with "@". The next +// significant part of the comparison is the list of elements in the value, +// where elements are split at `.' and `:'. Finally we compare by value, +// leaving equal values in their original order. +func compareStringExpr(a, b *StringExpr) int { + if phaseA, phaseB := labelPhase(a.Value), labelPhase(b.Value); phaseA != phaseB { + return phaseA - phaseB } + return compareStringExprValue(a.Value, b.Value) +} + +func labelPhase(s string) int { switch { - case strings.HasPrefix(x.Value, ":"): - key.phase = 1 - case strings.HasPrefix(x.Value, "//") || (tables.StripLabelLeadingSlashes && !strings.HasPrefix(x.Value, "@")): - key.phase = 2 - case strings.HasPrefix(x.Value, "@"): - key.phase = 3 + case strings.HasPrefix(s, ":"): + return 1 + case strings.HasPrefix(s, "//") || (tables.StripLabelLeadingSlashes && !strings.HasPrefix(s, "@")): + return 2 + case strings.HasPrefix(s, "@"): + return 3 } - - key.split = strings.Split(strings.Replace(x.Value, ":", ".", -1), ".") - return key + return 0 } -// byStringExpr implements sort.Interface for a list of stringSortKey. -type byStringExpr []stringSortKey - -func (x byStringExpr) Len() int { return len(x) } -func (x byStringExpr) Swap(i, j int) { x[i], x[j] = x[j], x[i] } - -func (x byStringExpr) Less(i, j int) bool { - xi := x[i] - xj := x[j] - - if xi.phase != xj.phase { - return xi.phase < xj.phase - } - for k := 0; k < len(xi.split) && k < len(xj.split); k++ { - if xi.split[k] != xj.split[k] { - return xi.split[k] < xj.split[k] +// compareStringExprValue compares the `.'/`:' separated segments of two +// values without splitting them: a separator ends a segment, so it sorts +// before any other character, and `.' and `:' compare as equal. Values with +// equal segments are ordered by raw value. +func compareStringExprValue(a, b string) int { + for i := 0; i < len(a) && i < len(b); i++ { + if a[i] != b[i] { + sepA := a[i] == '.' || a[i] == ':' + sepB := b[i] == '.' || b[i] == ':' + if sepA != sepB { + if sepA { + return -1 + } + return 1 + } + if !sepA { + if a[i] < b[i] { + return -1 + } + return 1 + } + // Both are separators, which compare as equal. } } - if len(xi.split) != len(xj.split) { - return len(xi.split) < len(xj.split) - } - if xi.value != xj.value { - return xi.value < xj.value + + if len(a) != len(b) { + return len(a) - len(b) } - return xi.original < xj.original + + // The values differ only by separators. + return strings.Compare(a, b) } // fixMultilinePlus turns diff --git a/build/sort_test.go b/build/sort_test.go new file mode 100644 index 000000000..033e50bef --- /dev/null +++ b/build/sort_test.go @@ -0,0 +1,260 @@ +/* +Copyright 2026 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package build + +import ( + "testing" + + "github.com/bazelbuild/buildtools/tables" +) + +// sortAndFormat parses src, applies sortStringExprs to the list in each +// top-level assignment, and returns the formatted result. +func sortAndFormat(t *testing.T, src string) string { + t.Helper() + f, err := ParseBuild("BUILD", []byte(src)) + if err != nil { + t.Fatal(err) + } + for _, stmt := range f.Stmt { + as, ok := stmt.(*AssignExpr) + if !ok { + t.Fatalf("statement is not an assignment: %v", stmt) + } + list, ok := as.RHS.(*ListExpr) + if !ok { + t.Fatalf("assignment RHS is not a list: %v", as.RHS) + } + list.List = sortStringExprs(list.List) + } + return string(Format(f)) +} + +func runSortTests(t *testing.T, cases map[string]struct{ src, want string }) { + t.Helper() + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := sortAndFormat(t, tc.src) + if got != tc.want { + t.Errorf("got:\n%s\nwant:\n%s", got, tc.want) + } + }) + } +} + +func TestSortStringExprsOrdering(t *testing.T) { + runSortTests(t, map[string]struct{ src, want string }{ + "separator sorts before dash": { + src: `deps = [ + ":foo-bar", + ":foo.bar", +] +`, + want: `deps = [ + ":foo.bar", + ":foo-bar", +] +`, + }, + "separator sorts before plus": { + src: `deps = [ + ":a+b", + ":a.b", +] +`, + want: `deps = [ + ":a.b", + ":a+b", +] +`, + }, + "colon separator sorts before digits": { + src: `deps = [ + ":a5", + ":a:2", +] +`, + want: `deps = [ + ":a:2", + ":a5", +] +`, + }, + "dot and colon separators tie broken by raw value": { + src: `deps = [ + ":a:b", + ":a.b", +] +`, + want: `deps = [ + ":a.b", + ":a:b", +] +`, + }, + "relative phase sorts before absolute": { + src: `deps = [ + "//x", + "/x", +] +`, + want: `deps = [ + "/x", + "//x", +] +`, + }, + "empty string sorts first": { + src: `deps = [ + "x", + "", + ":a", +] +`, + want: `deps = [ + "", + "x", + ":a", +] +`, + }, + "phases order relative then absolute then repo": { + src: `deps = [ + "@r//x", + "//x", + ":x", + "x", +] +`, + want: `deps = [ + "x", + ":x", + "//x", + "@r//x", +] +`, + }, + }) +} + +func TestSortStringExprsComments(t *testing.T) { + runSortTests(t, map[string]struct{ src, want string }{ + "before comment on first element is pinned to the top": { + src: `deps = [ + # comment on b + ":b", + ":a", +] +`, + want: `deps = [ + # comment on b + ":a", + ":b", +] +`, + }, + "before comment on later element starts a new chunk": { + src: `deps = [ + ":c", + # comment on b + ":b", + ":a", +] +`, + want: `deps = [ + ":c", + # comment on b + ":a", + ":b", +] +`, + }, + "suffix comments move with elements": { + src: `deps = [ + ":b", # comment on b + ":a", # comment on a +] +`, + want: `deps = [ + ":a", # comment on a + ":b", # comment on b +] +`, + }, + }) +} + +func TestSortStringExprsChunks(t *testing.T) { + runSortTests(t, map[string]struct{ src, want string }{ + "non-string element separates chunks": { + src: `deps = [ + ":c", + ":b", + X, + ":a", +] +`, + want: `deps = [ + ":b", + ":c", + X, + ":a", +] +`, + }, + }) +} + +func TestSortStringExprsDeduplication(t *testing.T) { + runSortTests(t, map[string]struct{ src, want string }{ + "duplicates are removed keeping the first occurrence": { + src: `deps = [ + ":a", # comment one + ":b", + ":a", # comment two +] +`, + want: `deps = [ + ":a", # comment one + ":b", +] +`, + }, + }) +} + +func TestSortStringExprsStripLabelLeadingSlashes(t *testing.T) { + tables.StripLabelLeadingSlashes = true + defer func() { tables.StripLabelLeadingSlashes = false }() + + runSortTests(t, map[string]struct{ src, want string }{ + "plain values sort into the absolute phase": { + src: `deps = [ + "@r//x", + "x", + ":a", +] +`, + want: `deps = [ + ":a", + "x", + "@r//x", +] +`, + }, + }) +} diff --git a/build/testdata/079.build.golden b/build/testdata/079.build.golden new file mode 100644 index 000000000..0cf51b843 --- /dev/null +++ b/build/testdata/079.build.golden @@ -0,0 +1,42 @@ +go_library( + name = "ordering", + deps = [ + "/x", + "x", + ":a:2", + ":a.b", + ":a+b", + ":a5", + ":a_b", + ":foo.bar", + ":foo-bar", + "//x", + "@r//x:y", + ], +) + +go_library( + name = "dedup", + deps = [ + ":a", + ":b", + ], +) + +go_library( + name = "keepsorted", + deps = [ + # keep sorted + # comment on b + ":a", + ":b", + ], +) + +go_library( + name = "suffix", + deps = [ + ":a", # comment on a + ":b", # comment on b + ], +) diff --git a/build/testdata/079.bzl.golden b/build/testdata/079.bzl.golden new file mode 100644 index 000000000..a5a6977aa --- /dev/null +++ b/build/testdata/079.bzl.golden @@ -0,0 +1,43 @@ +go_library( + name = "ordering", + deps = [ + ":foo-bar", + ":foo.bar", + ":a+b", + ":a.b", + ":a5", + ":a:2", + ":a_b", + "//x", + "/x", + "x", + "@r//x:y", + ], +) + +go_library( + name = "dedup", + deps = [ + ":b", + ":a", + ":b", + ], +) + +go_library( + name = "keepsorted", + deps = [ + # keep sorted + # comment on b + ":a", + ":b", + ], +) + +go_library( + name = "suffix", + deps = [ + ":b", # comment on b + ":a", # comment on a + ], +) diff --git a/build/testdata/079.in b/build/testdata/079.in new file mode 100644 index 000000000..e7b769019 --- /dev/null +++ b/build/testdata/079.in @@ -0,0 +1,43 @@ +go_library( + name = "ordering", + deps = [ + ":foo-bar", + ":foo.bar", + ":a+b", + ":a.b", + ":a5", + ":a:2", + ":a_b", + "//x", + "/x", + "x", + "@r//x:y", + ], +) + +go_library( + name = "dedup", + deps = [ + ":b", + ":a", + ":b", + ], +) + +go_library( + name = "keepsorted", + deps = [ + # keep sorted + # comment on b + ":b", + ":a", + ], +) + +go_library( + name = "suffix", + deps = [ + ":b", # comment on b + ":a", # comment on a + ], +)