Upgrade original targets (thought-machine#2942)

* Add targetset

* start using it

* Add AllTargets

* Update a couple of tests

* Add tests

* Preserve original ordering

* Exclude gocritic for test files, my tests don't need any more criticism

* Version bump

* Fix IsOriginalTarget logic

* goddamn tabses, we hates them

.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:

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
 --------------

VERSION

@@ -1 +1 @@
-17.4.0-beta.3
+17.4.0-beta.4

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"],
-)
+)

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 {

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.

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),

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")

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[:]
+}

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())
+}