From 95d0eea829558fe797eeee1f7522f44166b2ef4b Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 10 Oct 2026 17:04:08 +0100 Subject: [PATCH 1/7] Add benchmark --- src/query/BUILD | 17 ++++- src/query/changes_benchmark_test.go | 101 ++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 src/query/changes_benchmark_test.go 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_benchmark_test.go b/src/query/changes_benchmark_test.go new file mode 100644 index 000000000..8d469c562 --- /dev/null +++ b/src/query/changes_benchmark_test.go @@ -0,0 +1,101 @@ +package query + +import ( + "fmt" + "math/rand" + "testing" + + "gopkg.in/op/go-logging.v1" + + "github.com/thought-machine/please/src/core" +) + +// Use this to prevent compiler optimisations from removing anything. +var diffResult map[*core.BuildTarget]struct{} + +func BenchmarkDiffGraphs(b *testing.B) { + logging.SetLevel(logging.WARNING, "") + for _, numPackages := range []int{1000, 10000} { + before := createSyntheticGraph(numPackages) + after := createSyntheticGraph(numPackages) + numTargets := len(after.Graph.AllTargets()) + + b.Run(fmt.Sprintf("Identical/%d", numTargets), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + diffResult = diffGraphs(before, 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 i := 0; i < b.N; i++ { + diffResult = diffGraphs(before, changed) + } + }) + } +} + +// 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 +} From 1b201b08bfbbb04efd8c2670969421f7ad268e73 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 10 Oct 2026 17:48:58 +0100 Subject: [PATCH 2/7] Parallelise --- src/core/graph.go | 12 ++++++++++++ src/query/BUILD | 1 + src/query/changes.go | 24 ++++++++++++++++++++---- 3 files changed, 33 insertions(+), 4 deletions(-) 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/query/BUILD b/src/query/BUILD index 557f80f22..fa49be623 100644 --- a/src/query/BUILD +++ b/src/query/BUILD @@ -11,6 +11,7 @@ go_library( deps = [ "///third_party/go/github.com_please-build_gcfg//:gcfg", "///third_party/go/golang.org_x_exp//maps", + "///third_party/go/golang.org_x_sync//errgroup", "//src/build", "//src/cli", "//src/cli/logging", diff --git a/src/query/changes.go b/src/query/changes.go index 584e17a17..fd031746a 100644 --- a/src/query/changes.go +++ b/src/query/changes.go @@ -4,8 +4,11 @@ import ( "bytes" "crypto/sha1" "path/filepath" + "runtime" "sort" + "golang.org/x/sync/errgroup" + "github.com/thought-machine/please/src/build" "github.com/thought-machine/please/src/core" ) @@ -33,11 +36,24 @@ 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) + ch := make(chan *core.BuildTarget, 1000) + var g errgroup.Group + g.SetLimit(runtime.NumCPU()) + for afterTarget := range after.Graph.IterTargets() { + g.Go(func() error { + if beforeTarget := before.Graph.Target(afterTarget.Label); beforeTarget == nil || targetChanged(before, after, beforeTarget, afterTarget) || configChanged { + ch <- afterTarget + } + return nil + }) + } + go func() { + g.Wait() + close(ch) + }() 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{}{} - } + for t := range ch { + changed[t] = struct{}{} } return changed } From 76bca56100f27dcad2771d55e43f653bf959515e Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 11 Oct 2026 10:00:36 +0100 Subject: [PATCH 3/7] Add summarisation --- src/query/changes.go | 72 ++++++++++++++++------------- src/query/changes_benchmark_test.go | 5 +- src/query/changes_test.go | 22 ++++----- 3 files changed, 53 insertions(+), 46 deletions(-) diff --git a/src/query/changes.go b/src/query/changes.go index fd031746a..e18619251 100644 --- a/src/query/changes.go +++ b/src/query/changes.go @@ -16,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)) @@ -32,16 +32,23 @@ 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{}{} + // 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 + } ch := make(chan *core.BuildTarget, 1000) var g errgroup.Group g.SetLimit(runtime.NumCPU()) for afterTarget := range after.Graph.IterTargets() { g.Go(func() error { - if beforeTarget := before.Graph.Target(afterTarget.Label); beforeTarget == nil || targetChanged(before, after, beforeTarget, afterTarget) || configChanged { + if beforeHash, present := before.targets[afterTarget.Label]; !present || beforeHash != targetHash(after, afterTarget) { ch <- afterTarget } return nil @@ -51,7 +58,6 @@ func diffGraphs(before, after *core.BuildState) map[*core.BuildTarget]struct{} { g.Wait() close(ch) }() - changed := map[*core.BuildTarget]struct{}{} for t := range ch { changed[t] = struct{}{} } @@ -103,37 +109,37 @@ 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 - } - h1, err1 := sourceHash(s1, t1) - h2, err2 := sourceHash(s2, t2) - return !bytes.Equal(h1, h2) || err1 != nil || err2 != nil -} - -// 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 +// 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 { - continue // Skip in-repo tools, that will be handled via revdeps. + if _, ok := tool.Label(); !ok { + h.Write([]byte(tool.String())) } - // Tools outside the repo shouldn't change, so hashing the resolved tool path is enough. - hash = append(hash, toolPathHash(state, tool)...) } - return hash, nil + h.Sum(ret[:0]) + return ret } -func toolPathHash(state *core.BuildState, tool core.BuildInput) (hash []byte) { - h := sha1.New() - for _, path := range tool.LocalPaths(state.Graph) { - h.Write([]byte(path)) +// 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 { + m := map[core.BuildLabel][sha1.Size]byte{} + // TODO(peterebden): Would we benefit from a graph member to tell us how big this is? + for t := range state.Graph.IterTargets() { + m[t.Label] = targetHash(state, t) } - return h.Sum(nil) + return &GraphSummary{ + configHash: state.Hashes.Config, + targets: m, + } +} + +// A GraphSummary holds a summary of a build graph that is sufficient for change calculation +type GraphSummary struct { + configHash []byte + targets map[core.BuildLabel][sha1.Size]byte } diff --git a/src/query/changes_benchmark_test.go b/src/query/changes_benchmark_test.go index 8d469c562..3261205eb 100644 --- a/src/query/changes_benchmark_test.go +++ b/src/query/changes_benchmark_test.go @@ -17,13 +17,14 @@ 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 i := 0; i < b.N; i++ { - diffResult = diffGraphs(before, after) + diffResult = diffGraphs(summary, after) } }) @@ -37,7 +38,7 @@ func BenchmarkDiffGraphs(b *testing.B) { b.Run(fmt.Sprintf("SomeChanged/%d", numTargets), func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - diffResult = diffGraphs(before, changed) + diffResult = diffGraphs(summary, changed) } }) } 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) { From 32093b0bfa5633222319ae2474c8f7d723ed29f6 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 11 Oct 2026 10:07:20 +0100 Subject: [PATCH 4/7] slightly complex implementation --- src/please.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/please.go b/src/please.go index 2551c9deb..546dee3f3 100644 --- a/src/please.go +++ b/src/please.go @@ -998,6 +998,13 @@ var buildFunctions = map[string]func() int{ } readConfig() _, before := runBuild(core.WholeGraph, buildOpts{}) + var summary *query.GraphSummary + var wg sync.WaitGroup + wg.Go(func() { + summary = query.Summarise(before) + before = nil + go runtime.GC() + }) // 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 +1015,8 @@ var buildFunctions = map[string]func() int{ if !success { return 1 } - for _, target := range query.DiffGraphs(before, after, files, level, includeSubrepos) { + wg.Wait() + for _, target := range query.DiffGraphs(summary, after, files, level, includeSubrepos) { fmt.Println(target.String()) } return 0 From 49b4b4bf0f63b5555c913d97df62661aa8be0200 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 11 Oct 2026 10:09:28 +0100 Subject: [PATCH 5/7] Simpler --- src/please.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/please.go b/src/please.go index 546dee3f3..5d3a01178 100644 --- a/src/please.go +++ b/src/please.go @@ -998,13 +998,11 @@ var buildFunctions = map[string]func() int{ } readConfig() _, before := runBuild(core.WholeGraph, buildOpts{}) - var summary *query.GraphSummary - var wg sync.WaitGroup - wg.Go(func() { - summary = query.Summarise(before) - before = nil - go runtime.GC() - }) + 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 { @@ -1015,7 +1013,6 @@ var buildFunctions = map[string]func() int{ if !success { return 1 } - wg.Wait() for _, target := range query.DiffGraphs(summary, after, files, level, includeSubrepos) { fmt.Println(target.String()) } From f55fc5cfdda40d25443afca32cb2dd5076687c6c Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 11 Oct 2026 10:39:20 +0100 Subject: [PATCH 6/7] Parallelise summarisation --- src/query/changes.go | 29 +++++++++++++++++++++++------ src/query/changes_benchmark_test.go | 25 ++++++++++++++++++------- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/query/changes.go b/src/query/changes.go index e18619251..3e470a963 100644 --- a/src/query/changes.go +++ b/src/query/changes.go @@ -5,7 +5,9 @@ import ( "crypto/sha1" "path/filepath" "runtime" + "slices" "sort" + "sync" "golang.org/x/sync/errgroup" @@ -48,7 +50,7 @@ func diffGraphs(before *GraphSummary, after *core.BuildState) map[*core.BuildTar g.SetLimit(runtime.NumCPU()) for afterTarget := range after.Graph.IterTargets() { g.Go(func() error { - if beforeHash, present := before.targets[afterTarget.Label]; !present || beforeHash != targetHash(after, afterTarget) { + if _, present := before.targets[targetHash(after, afterTarget)]; !present { ch <- afterTarget } return nil @@ -127,10 +129,25 @@ func targetHash(state *core.BuildState, target *core.BuildTarget) (ret [sha1.Siz // 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 { - m := map[core.BuildLabel][sha1.Size]byte{} - // TODO(peterebden): Would we benefit from a graph member to tell us how big this is? - for t := range state.Graph.IterTargets() { - m[t.Label] = targetHash(state, t) + targets := slices.Collect(state.Graph.IterTargets()) + chunks := slices.Collect(slices.Chunk(targets, len(targets)/runtime.NumCPU())) + 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{}{} + } } return &GraphSummary{ configHash: state.Hashes.Config, @@ -141,5 +158,5 @@ func Summarise(state *core.BuildState) *GraphSummary { // A GraphSummary holds a summary of a build graph that is sufficient for change calculation type GraphSummary struct { configHash []byte - targets map[core.BuildLabel][sha1.Size]byte + targets map[[sha1.Size]byte]struct{} } diff --git a/src/query/changes_benchmark_test.go b/src/query/changes_benchmark_test.go index 3261205eb..296104bf7 100644 --- a/src/query/changes_benchmark_test.go +++ b/src/query/changes_benchmark_test.go @@ -10,9 +10,6 @@ import ( "github.com/thought-machine/please/src/core" ) -// Use this to prevent compiler optimisations from removing anything. -var diffResult map[*core.BuildTarget]struct{} - func BenchmarkDiffGraphs(b *testing.B) { logging.SetLevel(logging.WARNING, "") for _, numPackages := range []int{1000, 10000} { @@ -23,8 +20,8 @@ func BenchmarkDiffGraphs(b *testing.B) { b.Run(fmt.Sprintf("Identical/%d", numTargets), func(b *testing.B) { b.ReportAllocs() - for i := 0; i < b.N; i++ { - diffResult = diffGraphs(summary, after) + for b.Loop() { + diffGraphs(summary, after) } }) @@ -37,8 +34,22 @@ func BenchmarkDiffGraphs(b *testing.B) { } b.Run(fmt.Sprintf("SomeChanged/%d", numTargets), func(b *testing.B) { b.ReportAllocs() - for i := 0; i < b.N; i++ { - diffResult = diffGraphs(summary, changed) + 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) } }) } From 6d3d32fe16cbffee9f446aad3efb7f229a6e9219 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sun, 11 Oct 2026 10:46:31 +0100 Subject: [PATCH 7/7] Chunk this bit too, quite a lot faster --- src/query/BUILD | 1 - src/query/changes.go | 35 ++++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/query/BUILD b/src/query/BUILD index fa49be623..557f80f22 100644 --- a/src/query/BUILD +++ b/src/query/BUILD @@ -11,7 +11,6 @@ go_library( deps = [ "///third_party/go/github.com_please-build_gcfg//:gcfg", "///third_party/go/golang.org_x_exp//maps", - "///third_party/go/golang.org_x_sync//errgroup", "//src/build", "//src/cli", "//src/cli/logging", diff --git a/src/query/changes.go b/src/query/changes.go index 3e470a963..56afd042b 100644 --- a/src/query/changes.go +++ b/src/query/changes.go @@ -9,8 +9,6 @@ import ( "sort" "sync" - "golang.org/x/sync/errgroup" - "github.com/thought-machine/please/src/build" "github.com/thought-machine/please/src/core" ) @@ -45,23 +43,26 @@ func diffGraphs(before *GraphSummary, after *core.BuildState) map[*core.BuildTar return changed } - ch := make(chan *core.BuildTarget, 1000) - var g errgroup.Group - g.SetLimit(runtime.NumCPU()) - for afterTarget := range after.Graph.IterTargets() { - g.Go(func() error { - if _, present := before.targets[targetHash(after, afterTarget)]; !present { - ch <- afterTarget + 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) + } } - return nil + changes[i] = chunkChanges }) } - go func() { - g.Wait() - close(ch) - }() - for t := range ch { - changed[t] = struct{}{} + wg.Wait() + for _, chunk := range changes { + for _, t := range chunk { + changed[t] = struct{}{} + } } return changed } @@ -130,7 +131,7 @@ func targetHash(state *core.BuildState, target *core.BuildTarget) (ret [sha1.Siz // 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, len(targets)/runtime.NumCPU())) + 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 {