diff --git a/src/core/graph.go b/src/core/graph.go index 3ae136b16..6eb63db40 100644 --- a/src/core/graph.go +++ b/src/core/graph.go @@ -153,6 +153,18 @@ func (graph *BuildGraph) AllTargets() BuildTargets { return targets } +// IterTargets returns a sequence of all targets in the graph. +// TODO(peterebden): Make this AllTargets() (for consistency) and bin the other version (being careful about shell_output.go and xml_results.go which might care about order) +func (graph *BuildGraph) IterTargets() iter.Seq[*BuildTarget] { + return func(yield func(*BuildTarget) bool) { + for _, v := range graph.targets.Items() { + if !yield(v) { + break + } + } + } +} + // AllPackages returns an iterator over all packages in the graph. They are returned in no particular order. func (graph *BuildGraph) AllPackages() iter.Seq[*Package] { return func(yield func(*Package) bool) { diff --git a/src/please.go b/src/please.go index 2551c9deb..5d3a01178 100644 --- a/src/please.go +++ b/src/please.go @@ -998,6 +998,11 @@ var buildFunctions = map[string]func() int{ } readConfig() _, before := runBuild(core.WholeGraph, buildOpts{}) + log.Debug("Summarising before state...") + summary := query.Summarise(before) + before = nil + go runtime.GC() + log.Debug("Parsing after state...") // N.B. Ignore failure here; if we can't parse the graph before then it will suffice to // assume that anything we don't know about has changed. if err := scm.Checkout(original); err != nil { @@ -1008,7 +1013,7 @@ var buildFunctions = map[string]func() int{ if !success { return 1 } - for _, target := range query.DiffGraphs(before, after, files, level, includeSubrepos) { + for _, target := range query.DiffGraphs(summary, after, files, level, includeSubrepos) { fmt.Println(target.String()) } return 0 diff --git a/src/query/BUILD b/src/query/BUILD index 26a780b6a..557f80f22 100644 --- a/src/query/BUILD +++ b/src/query/BUILD @@ -1,3 +1,5 @@ +subinclude("//build_defs:benchmark") + go_library( name = "query", srcs = glob( @@ -20,7 +22,10 @@ go_library( go_test( name = "query_test", - srcs = glob(["*_test.go"]), + srcs = glob( + ["*_test.go"], + exclude = ["*_benchmark_test.go"], + ), data = ["completions_test_repo"], deps = [ ":query", @@ -31,3 +36,13 @@ go_test( "//src/parse", ], ) + +benchmark( + name = "changes_benchmark", + srcs = ["changes_benchmark_test.go"], + deps = [ + ":query", + "///third_party/go/gopkg.in_op_go-logging.v1//:go-logging.v1", + "//src/core", + ], +) diff --git a/src/query/changes.go b/src/query/changes.go index 584e17a17..56afd042b 100644 --- a/src/query/changes.go +++ b/src/query/changes.go @@ -4,7 +4,10 @@ import ( "bytes" "crypto/sha1" "path/filepath" + "runtime" + "slices" "sort" + "sync" "github.com/thought-machine/please/src/build" "github.com/thought-machine/please/src/core" @@ -13,7 +16,7 @@ import ( // DiffGraphs calculates the difference between two build graphs. // Note that this is not symmetric; targets that have been removed from 'before' do not appear // (because this is designed to be fed into 'plz test' and we can't test targets that no longer exist). -func DiffGraphs(before, after *core.BuildState, files []string, level int, includeSubrepos bool) core.BuildLabels { +func DiffGraphs(before *GraphSummary, after *core.BuildState, files []string, level int, includeSubrepos bool) core.BuildLabels { log.Notice("Calculating difference...") changed := diffGraphs(before, after) log.Debugf("Number of changed targets on a non-recursive diff between before and after build graphs: %d", len(changed)) @@ -29,14 +32,36 @@ func Changes(state *core.BuildState, files []string, level int, includeSubrepos } // diffGraphs performs a non-recursive diff of two build graphs. -func diffGraphs(before, after *core.BuildState) map[*core.BuildTarget]struct{} { - configChanged := !bytes.Equal(before.Hashes.Config, after.Hashes.Config) - log.Debugf("Has config changed between before and after build states: %v", configChanged) - +func diffGraphs(before *GraphSummary, after *core.BuildState) map[*core.BuildTarget]struct{} { changed := map[*core.BuildTarget]struct{}{} - for _, afterTarget := range after.Graph.AllTargets() { - if beforeTarget := before.Graph.Target(afterTarget.Label); beforeTarget == nil || targetChanged(before, after, beforeTarget, afterTarget) || configChanged { - changed[afterTarget] = struct{}{} + // Shortcut everything if these aren't the same + if !bytes.Equal(before.configHash, after.Hashes.Config) { + log.Debug("Config has changed, emitting every target") + for t := range after.Graph.IterTargets() { + changed[t] = struct{}{} + } + return changed + } + + targets := slices.Collect(after.Graph.IterTargets()) + chunks := slices.Collect(slices.Chunk(targets, max(len(targets)/runtime.NumCPU(), 100))) + changes := make([][]*core.BuildTarget, len(chunks)) + var wg sync.WaitGroup + for i, chunk := range chunks { + wg.Go(func() { + chunkChanges := make([]*core.BuildTarget, 0, len(chunk)) + for _, t := range chunk { + if _, present := before.targets[targetHash(after, t)]; !present { + chunkChanges = append(chunkChanges, t) + } + } + changes[i] = chunkChanges + }) + } + wg.Wait() + for _, chunk := range changes { + for _, t := range chunk { + changed[t] = struct{}{} } } return changed @@ -87,37 +112,52 @@ func changedTargets(state *core.BuildState, files []string, changed map[*core.Bu return ls } -// targetChanged returns true if the given two targets are not equivalent. -func targetChanged(s1, s2 *core.BuildState, t1, t2 *core.BuildTarget) bool { - h1 := build.RuleHash(s1, t1, true, false) - h2 := build.RuleHash(s2, t2, true, false) - if !bytes.Equal(h1, h2) { - return true +// targetHash returns a combined hash of the target which we use to check if they have changed. +func targetHash(state *core.BuildState, target *core.BuildTarget) (ret [sha1.Size]byte) { + h := sha1.New() + // TODO(peterebden): We could slightly improve this by having an interface to pass our own hasher in instead of hashing this twice. + h.Write(build.RuleHash(state, target, true, false)) + // The only thing we need to check source-wise is how tool paths resolve outside the repo. + // Changes to any tools within the repo are caught here anyway. + for _, tool := range target.AllTools() { + if _, ok := tool.Label(); !ok { + h.Write([]byte(tool.String())) + } } - h1, err1 := sourceHash(s1, t1) - h2, err2 := sourceHash(s2, t2) - return !bytes.Equal(h1, h2) || err1 != nil || err2 != nil + h.Sum(ret[:0]) + return ret } -// sourceHash performs a partial source hash on a target to determine if it's changed. -// This is a bit different to the one in the build package since we can't assume everything is -// necessarily present (and for performance reasons don't want to hash *everything*). -func sourceHash(state *core.BuildState, target *core.BuildTarget) ([]byte, error) { - var hash []byte - for _, tool := range target.AllTools() { - if _, ok := tool.Label(); ok { - continue // Skip in-repo tools, that will be handled via revdeps. +// Summarise calculates a summary of a build state that is sufficient for change calculation, allowing the caller to free the 'before' state. +func Summarise(state *core.BuildState) *GraphSummary { + targets := slices.Collect(state.Graph.IterTargets()) + chunks := slices.Collect(slices.Chunk(targets, max(len(targets)/runtime.NumCPU(), 100))) + hashes := make([][][sha1.Size]byte, len(chunks)) + var wg sync.WaitGroup + for i, chunk := range chunks { + wg.Go(func() { + chunkHashes := make([][sha1.Size]byte, len(chunk)) + for j, t := range chunk { + chunkHashes[j] = targetHash(state, t) + } + hashes[i] = chunkHashes + }) + } + wg.Wait() + m := map[[sha1.Size]byte]struct{}{} + for _, chunk := range hashes { + for _, h := range chunk { + m[h] = struct{}{} } - // Tools outside the repo shouldn't change, so hashing the resolved tool path is enough. - hash = append(hash, toolPathHash(state, tool)...) } - return hash, nil + return &GraphSummary{ + configHash: state.Hashes.Config, + targets: m, + } } -func toolPathHash(state *core.BuildState, tool core.BuildInput) (hash []byte) { - h := sha1.New() - for _, path := range tool.LocalPaths(state.Graph) { - h.Write([]byte(path)) - } - return h.Sum(nil) +// A GraphSummary holds a summary of a build graph that is sufficient for change calculation +type GraphSummary struct { + configHash []byte + targets map[[sha1.Size]byte]struct{} } diff --git a/src/query/changes_benchmark_test.go b/src/query/changes_benchmark_test.go new file mode 100644 index 000000000..296104bf7 --- /dev/null +++ b/src/query/changes_benchmark_test.go @@ -0,0 +1,113 @@ +package query + +import ( + "fmt" + "math/rand" + "testing" + + "gopkg.in/op/go-logging.v1" + + "github.com/thought-machine/please/src/core" +) + +func BenchmarkDiffGraphs(b *testing.B) { + logging.SetLevel(logging.WARNING, "") + for _, numPackages := range []int{1000, 10000} { + before := createSyntheticGraph(numPackages) + summary := Summarise(before) + after := createSyntheticGraph(numPackages) + numTargets := len(after.Graph.AllTargets()) + + b.Run(fmt.Sprintf("Identical/%d", numTargets), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + diffGraphs(summary, after) + } + }) + + // Modify roughly 1% of the targets in the 'after' graph. + changed := createSyntheticGraph(numPackages) + for i, t := range changed.Graph.AllTargets() { + if i%100 == 0 { + t.Command += " && echo changed" + } + } + b.Run(fmt.Sprintf("SomeChanged/%d", numTargets), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + diffGraphs(summary, changed) + } + }) + } +} + +func BenchmarkSummarise(b *testing.B) { + logging.SetLevel(logging.WARNING, "") + for _, numPackages := range []int{1000, 10000, 100000} { + b.Run(fmt.Sprintf("Size/%d", numPackages), func(b *testing.B) { + graph := createSyntheticGraph(numPackages) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + Summarise(graph) + } + }) + } +} + +// createSyntheticGraph creates a build state with a graph that looks vaguely like a real repo. +// It is deterministic, so calling it twice with the same arguments produces equivalent graphs. +// Each package contains a library (with a couple of hidden intermediate rules), a test, and a binary; +// libraries depend on a handful of libraries in earlier packages. +func createSyntheticGraph(numPackages int) *core.BuildState { + state := core.NewDefaultBuildState() + r := rand.New(rand.NewSource(42)) + components := []string{ + "src", "main", "cmd", "tools", "utils", "common", "query", "process", "update", + "run", "build", "assets", "frontend", "backend", "worker", + } + libs := make([]core.BuildLabel, 0, numPackages) + addTarget := func(pkg *core.Package, name string) *core.BuildTarget { + t := core.NewBuildTarget(core.BuildLabel{PackageName: pkg.Name, Name: name}) + t.Command = fmt.Sprintf("$TOOLS_GO tool compile -o $OUT -p %s $SRCS", pkg.Name) + t.AddOutput(name + ".a") + t.AddLabel("go") + t.AddTool(core.SystemPathLabel{Name: "go", Path: state.Config.Path()}) + t.Visibility = []core.BuildLabel{core.WholeGraph[0]} + state.Graph.AddTarget(t) + pkg.AddTarget(t) + return t + } + for i := 0; i < numPackages; i++ { + name := fmt.Sprintf("%s/%s/pkg%d", components[r.Intn(len(components))], components[r.Intn(len(components))], i) + pkg := core.NewPackage(name) + state.Graph.AddPackage(pkg) + base := components[r.Intn(len(components))] + + srcs := addTarget(pkg, "_"+base+"#srcs") + for j := 0; j < 1+r.Intn(10); j++ { + srcs.AddSource(core.FileLabel{File: fmt.Sprintf("%s_%d.go", base, j), Package: name}) + } + asm := addTarget(pkg, "_"+base+"#asm") + asm.AddDependency(srcs.Label) + lib := addTarget(pkg, base) + lib.AddSource(srcs.Label) + lib.AddDependency(srcs.Label) + lib.AddDependency(asm.Label) + for j := 0; j < len(libs) && j < 1+r.Intn(8); j++ { + lib.AddDependency(libs[r.Intn(len(libs))]) + } + libs = append(libs, lib.Label) + + test := addTarget(pkg, base+"_test") + test.AddSource(core.FileLabel{File: base + "_test.go", Package: name}) + test.AddDependency(lib.Label) + test.Test = &core.TestFields{Command: "$TEST"} + + bin := addTarget(pkg, base+"_bin") + bin.AddSource(core.FileLabel{File: "main.go", Package: name}) + bin.AddDependency(lib.Label) + bin.IsBinary = true + } + return state +} diff --git a/src/query/changes_test.go b/src/query/changes_test.go index e60be48a1..7bcafd320 100644 --- a/src/query/changes_test.go +++ b/src/query/changes_test.go @@ -17,15 +17,15 @@ func TestDiffGraphs(t *testing.T) { t2 := addTarget(s2, "//src/query:changes", nil, "src/query/changes.go") addTarget(s1, "//src/query:changes_test", t1, "src/query/changes_test.go") t4 := addTarget(s2, "//src/query:changes_test", t2, "src/query/changes_test.go") - assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(s1, s2, nil, -1, false)) + assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(Summarise(s1), s2, nil, -1, false)) t2.Command = "nope nope nope" - assert.EqualValues(t, []core.BuildLabel{t2.Label, t4.Label}, DiffGraphs(s1, s2, nil, -1, false)) + assert.EqualValues(t, []core.BuildLabel{t2.Label, t4.Label}, DiffGraphs(Summarise(s1), s2, nil, -1, false)) t2.AddLabel("nope") t4.AddLabel("test") s2.SetIncludeAndExclude(nil, []string{"nope", "test"}) - assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(s1, s2, nil, -1, false)) + assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(Summarise(s1), s2, nil, -1, false)) } func TestDiffGraphsIncludeNothing(t *testing.T) { @@ -37,7 +37,7 @@ func TestDiffGraphsIncludeNothing(t *testing.T) { t1 = addTarget(s2, "//src/core:core", nil, "src/core/core_changed.go") t2 = addTarget(s2, "//src/query:changes", t1, "src/query/changes.go") addTarget(s2, "//src/query:changes_test", t2, "src/query/changes_test.go") - assert.EqualValues(t, []core.BuildLabel{t1.Label}, DiffGraphs(s1, s2, nil, 0, false)) + assert.EqualValues(t, []core.BuildLabel{t1.Label}, DiffGraphs(Summarise(s1), s2, nil, 0, false)) } func TestDiffGraphsIncludeDirect(t *testing.T) { @@ -49,7 +49,7 @@ func TestDiffGraphsIncludeDirect(t *testing.T) { t1 = addTarget(s2, "//src/core:core", nil, "src/core/core_changed.go") t2 = addTarget(s2, "//src/query:changes", t1, "src/query/changes.go") addTarget(s2, "//src/query:changes_test", t2, "src/query/changes_test.go") - assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label}, DiffGraphs(s1, s2, nil, 1, false)) + assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label}, DiffGraphs(Summarise(s1), s2, nil, 1, false)) } func TestDiffGraphsLevel(t *testing.T) { @@ -63,7 +63,7 @@ func TestDiffGraphsLevel(t *testing.T) { t2 = addTarget(s2, "//src/query:changes", t1, "src/query/changes.go") t3 = addTarget(s2, "//src/query:changes_test", t2, "src/query/changes_test.go") addTarget(s2, "//src/query:changes_test2", t3, "src/query/changes_test2.go") - assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label, t3.Label}, DiffGraphs(s1, s2, nil, 2, false)) + assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label, t3.Label}, DiffGraphs(Summarise(s1), s2, nil, 2, false)) } func TestDiffGraphsIncludeTransitive(t *testing.T) { @@ -75,7 +75,7 @@ func TestDiffGraphsIncludeTransitive(t *testing.T) { t1 = addTarget(s2, "//src/core:core", nil, "src/core/core_changed.go") t2 = addTarget(s2, "//src/query:changes", t1, "src/query/changes.go") t3 := addTarget(s2, "//src/query:changes_test", t2, "src/query/changes_test.go") - assert.EqualValues(t, core.BuildLabels{t1.Label, t2.Label, t3.Label}, DiffGraphs(s1, s2, nil, -1, false)) + assert.EqualValues(t, core.BuildLabels{t1.Label, t2.Label, t3.Label}, DiffGraphs(Summarise(s1), s2, nil, -1, false)) } func TestDiffGraphsStopsAtSubrepos(t *testing.T) { @@ -94,7 +94,7 @@ func TestDiffGraphsStopsAtSubrepos(t *testing.T) { addTarget(s2, "//src/core:core", t3) // t3 should not be changed here - its subrepo has but we should see that the targets generated in it are still identical - assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label}, DiffGraphs(s1, s2, []string{"go.mod"}, -1, false)) + assert.EqualValues(t, []core.BuildLabel{t1.Label, t2.Label}, DiffGraphs(Summarise(s1), s2, []string{"go.mod"}, -1, false)) } func TestDiffGraphsStillChecksTargetsInSubrepos(t *testing.T) { @@ -113,9 +113,9 @@ func TestDiffGraphsStillChecksTargetsInSubrepos(t *testing.T) { t4 := addTarget(s2, "//src/core:core", t3) // t3 should now count as changed - it has a different source file - and that should propagate to t4 - assert.EqualValues(t, []core.BuildLabel{t1.Label, t4.Label, t2.Label, t3.Label}, DiffGraphs(s1, s2, []string{"go.mod"}, -1, true)) + assert.EqualValues(t, []core.BuildLabel{t1.Label, t4.Label, t2.Label, t3.Label}, DiffGraphs(Summarise(s1), s2, []string{"go.mod"}, -1, true)) // If includeSubrepos=false, t4 should still count as changed, although we won't see t3. - assert.EqualValues(t, []core.BuildLabel{t1.Label, t4.Label, t2.Label}, DiffGraphs(s1, s2, []string{"go.mod"}, -1, false)) + assert.EqualValues(t, []core.BuildLabel{t1.Label, t4.Label, t2.Label}, DiffGraphs(Summarise(s1), s2, []string{"go.mod"}, -1, false)) } func TestChangesIncludesDataDirs(t *testing.T) { @@ -134,7 +134,7 @@ func TestSameToolHashNoChange(t *testing.T) { target.AddTool(core.SystemPathLabel{Name: "non-existent", Path: s1.Config.Path()}) target = addTarget(s2, "//src/core:core", nil, "src/core/core.go") target.AddTool(core.SystemPathLabel{Name: "non-existent", Path: s2.Config.Path()}) - assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(s1, s2, nil, -1, false)) + assert.EqualValues(t, []core.BuildLabel{}, DiffGraphs(Summarise(s1), s2, nil, -1, false)) } func TestChangesIncludesRootTarget(t *testing.T) {