diff --git a/.golangci.yml b/.golangci.yml index d168aaff55..d054da176b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -23,6 +23,7 @@ issues: linters: - errcheck - dupl + - gocritic - path: src/core/config.go # The config struct is big & complex and there's usually only one. text: fieldalignment linters: diff --git a/ChangeLog b/ChangeLog index 39f6557f12..9ba31731e8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,7 @@ Version 17.4.0 * Add all commit date formats to supported git_show() format verbs (#2930) * Fix reduce builtin (#2925) * Fix issue with completing idents in the language server (#2917) + * Improve performance when a large number of input targets are supplied (#2942) Version 17.3.1 -------------- diff --git a/VERSION b/VERSION index 4880bd2cdf..c2e4e0d9a9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -17.4.0-beta.3 +17.4.0-beta.4 diff --git a/src/core/BUILD b/src/core/BUILD index c9e9b6169c..c8645451c3 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -2,30 +2,10 @@ subinclude("//build_defs:benchmark") go_library( name = "core", - srcs = [ - "atomic_bool.go", - "atomic_float32.go", - "build_env.go", - "build_input.go", - "build_label.go", - "build_target.go", - "cache.go", - "command_replacements.go", - "config.go", - "config_flags.go", - "cycle_detector.go", - "graph.go", - "lock.go", - "package.go", - "previous_op.go", - "resources.go", - "stamp.go", - "state.go", - "subrepo.go", - "test_results.go", - "utils.go", - "version.go", - ], + srcs = glob( + ["*.go"], + exclude = ["*_test.go"], + ), pgo_file = "//:pgo", visibility = ["PUBLIC"], deps = [ @@ -52,29 +32,7 @@ go_library( go_test( name = "core_test", - srcs = [ - "build_env_test.go", - "build_input_test.go", - "build_label_fuzz_test.go", - "build_label_test.go", - "build_target_benchmark_test.go", - "build_target_test.go", - "command_replacements_test.go", - "config_test.go", - "cycle_detector_test.go", - "graph_benchmark_test.go", - "graph_test.go", - "label_parse_test.go", - "lock_test.go", - "package_test.go", - "previous_op_test.go", - "stamp_test.go", - "state_test.go", - "subrepo_test.go", - "test_results_test.go", - "utils_benchmark_test.go", - "utils_test.go", - ], + srcs = glob(["*_test.go"]), data = ["test_data"], filter_srcs = False, # As above deps = [ @@ -102,4 +60,4 @@ benchmark( name = "graph_benchmark", srcs = ["graph_benchmark_test.go"], deps = [":core"], -) +) \ No newline at end of file diff --git a/src/core/build_label.go b/src/core/build_label.go index c59c0eac9a..4826098c7f 100644 --- a/src/core/build_label.go +++ b/src/core/build_label.go @@ -284,12 +284,35 @@ func (label BuildLabel) Includes(that BuildLabel) bool { return false } +// Compare compares this build label to another one (suitable for using with slices.SortFunc) +func (label BuildLabel) Compare(other BuildLabel) int { + if label.Subrepo != other.Subrepo { + if label.Subrepo < other.Subrepo { + return -1 + } + return 1 + } else if label.PackageName != other.PackageName { + if label.PackageName < other.PackageName { + return -1 + } + return 1 + } else if label.Name != other.Name { + if label.Name < other.Name { + return -1 + } + return 1 + } + return 0 +} + // Less returns true if this build label would sort less than another one. func (label BuildLabel) Less(other BuildLabel) bool { - if label.PackageName == other.PackageName { - return label.Name < other.Name + if label.Subrepo != other.Subrepo { + return label.Subrepo < other.Subrepo + } else if label.PackageName != other.PackageName { + return label.PackageName < other.PackageName } - return label.PackageName < other.PackageName + return label.Name < other.Name } // Paths is an implementation of BuildInput interface; we use build labels directly as inputs. @@ -535,6 +558,15 @@ func (key packageKey) String() string { return key.Name } +// BuildLabel returns a build label representing this package key. +func (key packageKey) BuildLabel() BuildLabel { + return BuildLabel{ + Subrepo: key.Subrepo, + PackageName: key.Name, + Name: "all", + } +} + // LooksLikeABuildLabel returns true if the string appears to be a build label, false if not. // Useful for cases like rule sources where sources can be a filename or a label. func LooksLikeABuildLabel(str string) bool { diff --git a/src/core/build_label_test.go b/src/core/build_label_test.go index 8ee242f226..bd48a4f788 100644 --- a/src/core/build_label_test.go +++ b/src/core/build_label_test.go @@ -179,6 +179,38 @@ func TestSingleSlash(t *testing.T) { assert.Equal(t, BuildLabel{PackageName: "common/go/pool", Name: "pool"}, label) } +func TestLess(t *testing.T) { + foo := ParseBuildLabel("//third_party:foo", "") + bar := ParseBuildLabel("//third_party:bar", "") + assert.True(t, bar.Less(foo)) + assert.False(t, foo.Less(bar)) + assert.False(t, foo.Less(foo)) +} + +func TestLessSubrepo(t *testing.T) { + foo := ParseBuildLabel("//third_party:foo", "") + bar := ParseBuildLabel("///baz//third_party:bar", "") + assert.False(t, bar.Less(foo)) + assert.True(t, foo.Less(bar)) + assert.False(t, foo.Less(foo)) +} + +func TestCompare(t *testing.T) { + foo := ParseBuildLabel("//third_party:foo", "") + bar := ParseBuildLabel("//third_party:bar", "") + assert.Equal(t, -1, bar.Compare(foo)) + assert.Equal(t, 1, foo.Compare(bar)) + assert.Equal(t, 0, foo.Compare(foo)) +} + +func TestCompareSubrepo(t *testing.T) { + foo := ParseBuildLabel("//third_party:foo", "") + bar := ParseBuildLabel("///baz//third_party:bar", "") + assert.Equal(t, 1, bar.Compare(foo)) + assert.Equal(t, -1, foo.Compare(bar)) + assert.Equal(t, 0, foo.Compare(foo)) +} + func TestMain(m *testing.M) { // Used to support TestComplete, the function it's testing re-execs // itself thinking that it's actually plz. diff --git a/src/core/state.go b/src/core/state.go index 65410ead66..88de3cfd10 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -286,8 +286,7 @@ type stateProgress struct { // The set of known states allStates []*BuildState // Targets that we were originally requested to build - originalTargets []BuildLabel - originalTargetMutex sync.Mutex + originalTargets *TargetSet // True if something about the build has failed. failed atomicBool // True if >= 1 target has failed to build @@ -423,14 +422,11 @@ func (state *BuildState) IsOriginalTarget(target *BuildTarget) bool { } func (state *BuildState) isOriginalTarget(target *BuildTarget, exact bool) bool { - state.progress.originalTargetMutex.Lock() - defer state.progress.originalTargetMutex.Unlock() - for _, original := range state.progress.originalTargets { - if original == target.Label || (!exact && original.IsAllTargets() && original.PackageName == target.Label.PackageName && state.ShouldInclude(target)) { - return true - } + if exact { + return state.progress.originalTargets.MatchExact(target.Label) } - return false + matched, wasExact := state.progress.originalTargets.Match(target.Label) + return matched && (wasExact || state.ShouldInclude(target)) } // IsOriginalTargetOrParent is like IsOriginalTarget but checks the target's parent too (if it has one) @@ -480,9 +476,7 @@ func (state *BuildState) AddOriginalTarget(label BuildLabel, addToList bool) { } } if addToList { - state.progress.originalTargetMutex.Lock() - state.progress.originalTargets = append(state.progress.originalTargets, label) - state.progress.originalTargetMutex.Unlock() + state.progress.originalTargets.Add(label) } state.addPendingParse(label, OriginalTarget, ParseModeNormal) } @@ -702,18 +696,12 @@ func (state *BuildState) NumDone() int { // ExpandOriginalLabels expands any pseudo-labels (ie. :all, ... has already been resolved to a bunch :all targets) // from the set of original labels. This will exclude non-test targets when we're building for test. func (state *BuildState) ExpandOriginalLabels() BuildLabels { - state.progress.originalTargetMutex.Lock() - targets := state.progress.originalTargets[:] - state.progress.originalTargetMutex.Unlock() - return state.ExpandLabels(targets) + return state.ExpandLabels(state.progress.originalTargets.AllTargets()) } // ExpandAllOriginalLabels is the same as ExpandOriginalLabels except it always includes non-test targets func (state *BuildState) ExpandAllOriginalLabels() BuildLabels { - state.progress.originalTargetMutex.Lock() - targets := state.progress.originalTargets[:] - state.progress.originalTargetMutex.Unlock() - return state.expandLabels(targets, false) + return state.expandLabels(state.progress.originalTargets.AllTargets(), false) } func AnnotateLabels(labels []BuildLabel) []AnnotatedOutputLabel { @@ -1394,6 +1382,7 @@ func NewBuildState(config *Configuration) *BuildState { packageWaits: cmap.New[packageKey, chan struct{}](cmap.DefaultShardCount, hashPackageKey), internalResults: make(chan *BuildResult, 1000), cycleDetector: cycleDetector{graph: graph}, + originalTargets: NewTargetSet(), }, initOnce: new(sync.Once), preloadDownloadOnce: new(sync.Once), diff --git a/src/core/state_test.go b/src/core/state_test.go index 5ac86a07ae..010988a24e 100644 --- a/src/core/state_test.go +++ b/src/core/state_test.go @@ -59,7 +59,8 @@ func TestExpandVisibleOriginalTargets(t *testing.T) { func TestExpandOriginalSubLabels(t *testing.T) { state := NewDefaultBuildState() - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "..."}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}, true) state.Include = []string{"go"} state.Exclude = []string{"py"} addTarget(state, "//src/core:target1", "go") @@ -76,7 +77,8 @@ func TestExpandOriginalSubLabels(t *testing.T) { func TestExpandOriginalLabelsOrdering(t *testing.T) { state := NewDefaultBuildState() state.AddOriginalTarget(BuildLabel{PackageName: "src/parse", Name: "parse"}, true) - state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "..."}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "all"}, true) + state.AddOriginalTarget(BuildLabel{PackageName: "src/core/tests", Name: "all"}, true) state.AddOriginalTarget(BuildLabel{PackageName: "src/build", Name: "build"}, true) addTarget(state, "//src/core:target1", "go") addTarget(state, "//src/core:target2", "py") diff --git a/src/core/target_set.go b/src/core/target_set.go new file mode 100644 index 0000000000..7e4ae97abc --- /dev/null +++ b/src/core/target_set.go @@ -0,0 +1,63 @@ +package core + +import ( + "sync" +) + +// A TargetSet contains a series of targets and supports efficiently checking for membership +// The zero value is not safe for use. +type TargetSet struct { + targets map[BuildLabel]struct{} + packages map[packageKey]struct{} + everything []BuildLabel + mutex sync.RWMutex +} + +// NewTargetSet returns a new TargetSet. +func NewTargetSet() *TargetSet { + return &TargetSet{ + targets: map[BuildLabel]struct{}{}, + packages: map[packageKey]struct{}{}, + } +} + +// Add adds a new target to this set. +func (ts *TargetSet) Add(label BuildLabel) { + ts.mutex.Lock() + defer ts.mutex.Unlock() + if label.IsAllSubpackages() { + panic("TargetSet doesn't support ... labels") + } else if label.IsAllTargets() { + ts.packages[label.packageKey()] = struct{}{} + } else { + ts.targets[label] = struct{}{} + } + ts.everything = append(ts.everything, label) +} + +// Match checks if this label is covered by the set (either explicitly or via :all) +// The first return checks if it's matched, the second if the match was exact or not. +func (ts *TargetSet) Match(label BuildLabel) (bool, bool) { + ts.mutex.RLock() + defer ts.mutex.RUnlock() + if _, present := ts.targets[label]; present { + return true, true + } + _, present := ts.packages[label.packageKey()] + return present, false +} + +// MatchExact checks if this label was explicitly added to the set (i.e. :all doesn't count) +func (ts *TargetSet) MatchExact(label BuildLabel) bool { + ts.mutex.RLock() + defer ts.mutex.RUnlock() + _, present := ts.targets[label] + return present +} + +// AllTargets returns a copy of the set of targets +func (ts *TargetSet) AllTargets() []BuildLabel { + ts.mutex.RLock() + defer ts.mutex.RUnlock() + return ts.everything[:] +} diff --git a/src/core/target_set_test.go b/src/core/target_set_test.go new file mode 100644 index 0000000000..02e1af5be1 --- /dev/null +++ b/src/core/target_set_test.go @@ -0,0 +1,53 @@ +package core + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestTargetSetMatch(t *testing.T) { + ts := NewTargetSet() + ts.Add(ParseBuildLabel("//src/core:core", "")) + ts.Add(ParseBuildLabel("//src/parse:all", "")) + matched, exact := ts.Match(ParseBuildLabel("//src/core:core", "")) + assert.True(t, matched) + assert.True(t, exact) + matched, exact = ts.Match(ParseBuildLabel("//src/core:core_test", "")) + assert.False(t, matched) + assert.False(t, exact) + matched, exact = ts.Match(ParseBuildLabel("//src/parse:parse", "")) + assert.True(t, matched) + assert.False(t, exact) + matched, exact = ts.Match(ParseBuildLabel("//src/parse:parse_test", "")) + assert.True(t, matched) + assert.False(t, exact) + matched, exact = ts.Match(ParseBuildLabel("//src/build", "")) + assert.False(t, matched) + assert.False(t, exact) +} + +func TestTargetSetMatchExact(t *testing.T) { + ts := NewTargetSet() + ts.Add(ParseBuildLabel("//src/core:core", "")) + ts.Add(ParseBuildLabel("//src/parse:all", "")) + assert.True(t, ts.MatchExact(ParseBuildLabel("//src/core:core", ""))) + assert.False(t, ts.MatchExact(ParseBuildLabel("//src/core:core_test", ""))) + assert.False(t, ts.MatchExact(ParseBuildLabel("//src/parse:parse", ""))) + assert.False(t, ts.MatchExact(ParseBuildLabel("//src/parse:parse_test", ""))) + assert.False(t, ts.MatchExact(ParseBuildLabel("//src/build", ""))) +} + +func TestAllTargets(t *testing.T) { + ts := NewTargetSet() + labels := []BuildLabel{ + ParseBuildLabel("//src/core:core", ""), + ParseBuildLabel("//src/core:core_test", ""), + ParseBuildLabel("//src/parse:all", ""), + ParseBuildLabel("//src/parse:parse_test", ""), + } + for _, label := range labels { + ts.Add(label) + } + assert.Equal(t, labels, ts.AllTargets()) +}