perf(hash-persister): probe dirty packages before rdeps propagation

hash-persister/hash-persister.go

@@ -14,6 +14,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/bazel-contrib/target-determinator/cli"
@@ -272,6 +273,9 @@ func runSeeded(cfg *config) (seededOutcome, error) {
 			seedData.TargetEdges,
 			seedData.SeedCompatibilityFingerprint,
 		)
+		if cfg.SeedableOutput {
+			persistedData.DependencyHashes = seedData.DependencyHashes
+		}
 		if err := pkg.WritePersistedData(cfg.OutputFile, persistedData); err != nil {
 			return seededOutcome{}, fmt.Errorf("failed to persist hashes: %w", err)
 		}
@@ -281,6 +285,37 @@ func runSeeded(cfg *config) (seededOutcome, error) {
 		return outcome, nil
 	}
 
+	commitRev, err := pkg.NewLabelledGitRev(cfg.Context.WorkspacePath, cfg.CommitSha, "commit")
+	if err != nil {
+		return seededOutcome{}, fmt.Errorf("failed to resolve commit %s: %w", cfg.CommitSha, err)
+	}
+
+	// Phase: probe dirty packages to prune false-positive rdeps.
+	//
+	// When a BUILD.bazel changes, all targets in the package are marked
+	// dirty and their rdeps cascade through the graph. But most existing
+	// targets are often unchanged (e.g. a new sibling was added). A small
+	// probe query on just the dirty packages lets us compare hashes against
+	// the seed and only propagate rdeps from targets that actually changed.
+	unprunedDirtyStarCount := len(dirtyResult.DirtyStarLabels)
+	if unprunedDirtyStarCount > len(dirtyResult.DirtyLabels) {
+		// Kept in a temporary: probePruneDirtySet returns a nil result
+		// alongside its error, so assigning straight to dirtyResult would
+		// discard the set the fallback is supposed to carry on with.
+		pruned, probeErr := probePruneDirtySet(cfg, commitRev, dirtyResult, seedData, changedFiles)
+		if probeErr != nil {
+			log.Printf("Probe pruning failed, continuing with unpruned dirty set: %v", probeErr)
+		} else {
+			dirtyResult = pruned
+			// The report drives CI metrics, so it has to describe the set
+			// actually used rather than the one before pruning.
+			outcome.DirtyTargetCount = len(dirtyResult.DirtyStarLabels)
+			log.Printf("Probe pruning: %d dirty* -> %d dirty* (%d eliminated)",
+				unprunedDirtyStarCount, len(dirtyResult.DirtyStarLabels),
+				unprunedDirtyStarCount-len(dirtyResult.DirtyStarLabels))
+		}
+	}
+
 	estimatedRecomputedTargets := countDirtySeedTargets(seedData.TargetHashes, dirtyResult.DirtyStarLabels)
 	seedTargetCount := len(seedData.TargetHashes)
 	if shouldFallbackForRecomputation(
@@ -320,11 +355,6 @@ func runSeeded(cfg *config) (seededOutcome, error) {
 		return fallback("unscopable_target_pattern", fmt.Sprintf("cannot scope targets pattern: %v", err))
 	}
 
-	commitRev, err := pkg.NewLabelledGitRev(cfg.Context.WorkspacePath, cfg.CommitSha, "commit")
-	if err != nil {
-		return seededOutcome{}, fmt.Errorf("failed to resolve commit %s: %w", cfg.CommitSha, err)
-	}
-
 	phaseStart = time.Now()
 	scopedTargets, err := pkg.ParseTargetsList(scopedPattern)
 	if err != nil {
@@ -384,6 +414,124 @@ func runSeeded(cfg *config) (seededOutcome, error) {
 	return outcome, nil
 }
 
+// probePruneDirtySet runs a small probe query on just the dirty packages,
+// hashes those targets with seed hashes for their dependencies, and returns
+// a pruned DirtySetResult where rdeps are only propagated from targets
+// whose hash actually changed.
+func probePruneDirtySet(
+	cfg *config,
+	commitRev pkg.LabelledGitRev,
+	dirtyResult *pkg.DirtySetResult,
+	seedData *pkg.PersistedHashData,
+	changedFiles map[string]string,
+) (*pkg.DirtySetResult, error) {
+	phaseStart := time.Now()
+
+	probe, err := probePackages(cfg, commitRev, dirtyResult.DirtyPackages, seedData)
+	if err != nil {
+		return nil, err
+	}
+	log.Printf("Phase probe completed in %v (%d hashed, %d source files, in %d packages)",
+		time.Since(phaseStart), len(probe.Hashes), len(probe.SourceFiles),
+		len(dirtyResult.DirtyPackages))
+
+	return pkg.PruneDirtySet(dirtyResult, seedData, probe, changedFiles), nil
+}
+
+// probePackages queries and hashes targets in the given packages, seeding
+// dependency hashes from the seed file so that only the dirty packages need
+// a Bazel query.
+func probePackages(
+	cfg *config,
+	commitRev pkg.LabelledGitRev,
+	dirtyPackages []string,
+	seedData *pkg.PersistedHashData,
+) (pkg.ProbeResult, error) {
+	// Query the dirty packages raw, deliberately bypassing the targets
+	// pattern. That pattern excludes manual-tagged targets, which covers
+	// most labels in a dirty package (npm links, platform() rules, JS build
+	// internals). Those labels still appear in the seed's edge map, so the
+	// probe must hash them to prove they are unchanged. ":*" rather than
+	// ":all" so source files are included too.
+	probePattern := buildProbePattern(dirtyPackages)
+	probeTargets, err := pkg.ParseTargetsList(probePattern)
+	if err != nil {
+		return pkg.ProbeResult{}, fmt.Errorf("failed to parse probe targets: %w", err)
+	}
+
+	probeResults, probeCleanup, err := pkg.LoadIncompleteMetadata(cfg.Context, commitRev, probeTargets)
+	if err != nil {
+		probeCleanup()
+		return pkg.ProbeResult{}, fmt.Errorf("probe query failed: %w", err)
+	}
+	defer probeCleanup()
+
+	probeSeedHashes, err := buildExternalSeedHashes(seedData, dirtyPackages)
+	if err != nil {
+		return pkg.ProbeResult{}, err
+	}
+	if err := probeResults.TargetHashCache.SeedHashes(probeSeedHashes); err != nil {
+		return pkg.ProbeResult{}, fmt.Errorf("cannot seed probe hashes: %w", err)
+	}
+
+	if err := probeResults.PrefillCache(); err != nil {
+		return pkg.ProbeResult{}, fmt.Errorf("probe hashing failed: %w", err)
+	}
+
+	// Read hashes straight from the cache rather than from MatchingTargets:
+	// it additionally holds the transitively-computed hashes of dependencies
+	// outside the probed packages, which cost nothing extra to include.
+	return pkg.ProbeResult{
+		Hashes:      probeResults.TargetHashCache.ExtractHexHashes(),
+		SourceFiles: probeResults.TargetHashCache.SourceFileLabels(),
+	}, nil
+}
+
+// buildProbePattern returns a bazel query expression covering every target
+// in the given packages, including manual-tagged rules and source files.
+func buildProbePattern(dirtyPackages []string) string {
+	if len(dirtyPackages) == 0 {
+		return "set()"
+	}
+	terms := make([]string, 0, len(dirtyPackages))
+	for _, p := range dirtyPackages {
+		if p == "//" {
+			terms = append(terms, "//:*")
+		} else {
+			terms = append(terms, p+":*")
+		}
+	}
+	return "(" + strings.Join(terms, " + ") + ")"
+}
+
+// buildExternalSeedHashes collects seed hashes for all targets NOT in the
+// given packages, so that dependency hashes resolve without a full query.
+func buildExternalSeedHashes(
+	seedData *pkg.PersistedHashData,
+	dirtyPackages []string,
+) (map[string][]byte, error) {
+	dirtyPkgSet := make(map[string]bool, len(dirtyPackages))
+	for _, p := range dirtyPackages {
+		dirtyPkgSet[p] = true
+	}
+	hashes := make(map[string][]byte)
+	for _, source := range []map[string]map[string]string{seedData.TargetHashes, seedData.DependencyHashes} {
+		for label, configMap := range source {
+			if dirtyPkgSet[pkg.LabelPackage(label)] {
+				continue
+			}
+			for configStr, hashHex := range configMap {
+				hashBytes, err := hex.DecodeString(hashHex)
+				if err != nil {
+					return nil, fmt.Errorf("invalid seed hash for %s: %w", label, err)
+				}
+				hashes[label+"\x00"+configStr] = hashBytes
+			}
+		}
+	}
+	return hashes, nil
+}
+
 func countDirtySeedTargets(targetHashes map[string]map[string]string, dirtyLabels map[string]bool) int {
 	count := 0
 	for label := range targetHashes {
@@ -425,19 +573,24 @@ func validateSeed(seedData *pkg.PersistedHashData, expectedSha, expectedFingerpr
 
 func reusableSeedHashes(seedData *pkg.PersistedHashData, dirtyLabels map[string]bool) (map[string][]byte, error) {
 	hashes := make(map[string][]byte)
-	for label, configMap := range seedData.TargetHashes {
-		if dirtyLabels[label] {
-			continue
-		}
-		for configStr, hashHex := range configMap {
-			hashBytes, err := hex.DecodeString(hashHex)
-			if err != nil {
-				return nil, fmt.Errorf("invalid hash hex for %s: %w", label, err)
+	// Dependency hashes are as reusable as target hashes for seeding the
+	// cache; they are only kept in a separate bucket so that diffing sees
+	// the target set alone.
+	for _, source := range []map[string]map[string]string{seedData.TargetHashes, seedData.DependencyHashes} {
+		for label, configMap := range source {
+			if dirtyLabels[label] {
+				continue
 			}
-			if len(hashBytes) != sha256.Size {
-				return nil, fmt.Errorf("invalid hash length for %s: got %d bytes, want %d", label, len(hashBytes), sha256.Size)
+			for configStr, hashHex := range configMap {
+				hashBytes, err := hex.DecodeString(hashHex)
+				if err != nil {
+					return nil, fmt.Errorf("invalid hash hex for %s: %w", label, err)
+				}
+				if len(hashBytes) != sha256.Size {
+					return nil, fmt.Errorf("invalid hash length for %s: got %d bytes, want %d", label, len(hashBytes), sha256.Size)
+				}
+				hashes[label+"\x00"+configStr] = hashBytes
 			}
-			hashes[label+"\x00"+configStr] = hashBytes
 		}
 	}
 	return hashes, nil
@@ -482,6 +635,7 @@ func mergePersistedData(
 		}
 	}
 	mergedEdges := mergePersistedEntries(seedData.TargetEdges, dirtyLabels, freshEdges)
+	persistedData.DependencyHashes = pkg.ExtractDependencyHashes(persistedData.TargetHashes, mergedEdges, queryResults.TargetHashCache)
 	persistedData.FormatVersion = pkg.CurrentPersistedHashFormatVersion
 	persistedData.SeedCompatibilityFingerprint = compatibilityFingerprint
 	persistedData.TargetEdges = mergedEdges

hash-persister/hash-persister_test.go

@@ -280,3 +280,23 @@ func TestMergePersistedEntriesReplacesDirtyState(t *testing.T) {
 		t.Fatalf("merged edges = %#v, want %#v", got, wantEdges)
 	}
 }
+
+func TestBuildProbePattern(t *testing.T) {
+	// ":*" rather than ":all" so source files are covered, and no targets
+	// pattern wrapper so manual-tagged targets are not filtered out.
+	for _, tc := range []struct {
+		name     string
+		packages []string
+		want     string
+	}{
+		{"empty", nil, "set()"},
+		{"root", []string{"//"}, "(//:*)"},
+		{"several", []string{"//", "//ci", "//tools/binaries"}, "(//:* + //ci:* + //tools/binaries:*)"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := buildProbePattern(tc.packages); got != tc.want {
+				t.Errorf("buildProbePattern(%v) = %q, want %q", tc.packages, got, tc.want)
+			}
+		})
+	}
+}

pkg/dirty_set.go

@@ -170,6 +170,137 @@ func ComputeDirtySet(
 	return result
 }
 
+// ProbeResult is what a probe of the dirty packages learned about them.
+type ProbeResult struct {
+	// Hashes maps "label\x00configuration" to the hex hash computed at the
+	// destination revision.
+	Hashes map[string]string
+	// SourceFiles is the set of probed labels that are source files. Their
+	// dirtiness comes from the git diff rather than from Hashes.
+	SourceFiles map[string]bool
+}
+
+// PruneDirtySet narrows a DirtySetResult by propagating reverse
+// dependencies only from targets that actually changed, rather than from
+// every target in a dirty package. A BUILD.bazel edit marks its whole
+// package dirty, so in a high-fanout package the unpruned rdeps closure can
+// cover most of the repository even when only one target really changed.
+//
+// A source file is unchanged exactly when the git diff does not mention it.
+// Every other label is unchanged when its probe hash matches the seed;
+// labels the probe or the seed omits are conservatively treated as changed.
+//
+// DirtyLabels and DirtyPackages are preserved — those packages still need
+// re-listing via wildcards — and only DirtyStarLabels is recomputed.
+func PruneDirtySet(
+	original *DirtySetResult,
+	seed *PersistedHashData,
+	probe ProbeResult,
+	changedFiles map[string]string,
+) *DirtySetResult {
+	actuallyChanged := findChangedTargets(original.DirtyLabels, seed, probe, changedFiles)
+	newDirtyStar := propagateFrom(original.DirtyLabels, actuallyChanged, seed.TargetEdges)
+
+	return &DirtySetResult{
+		DirtyLabels:     original.DirtyLabels,
+		DirtyStarLabels: newDirtyStar,
+		DirtyPackages:   original.DirtyPackages,
+	}
+}
+
+// findChangedTargets returns the subset of dirtyLabels that changed between
+// the seed and the destination revision.
+func findChangedTargets(
+	dirtyLabels map[string]bool,
+	seed *PersistedHashData,
+	probe ProbeResult,
+	changedFiles map[string]string,
+) map[string]bool {
+	changed := make(map[string]bool)
+	for label := range dirtyLabels {
+		if probe.SourceFiles[label] {
+			if _, ok := changedFiles[labelToPath(label)]; ok {
+				changed[label] = true
+			}
+			continue
+		}
+		if targetHashChanged(label, seed.SeedHashes(label), probe.Hashes) {
+			changed[label] = true
+		}
+	}
+	return changed
+}
+
+// labelToPath returns the workspace-relative path a main-repo label refers
+// to, e.g. "//pkg:sub/f.java" becomes "pkg/sub/f.java".
+func labelToPath(label string) string {
+	if idx := strings.Index(label, "//"); idx >= 0 {
+		label = label[idx+len("//"):]
+	}
+	pkg, name, found := strings.Cut(label, ":")
+	switch {
+	case !found:
+		return pkg
+	case pkg == "":
+		return name
+	default:
+		return pkg + "/" + name
+	}
+}
+
+// targetHashChanged reports whether a single target's probe hash differs
+// from its seed hash. Returns true for new targets (nil seedConfigs) and
+// targets missing from probe results.
+func targetHashChanged(label string, seedConfigs map[string]string, probeHashes map[string]string) bool {
+	if seedConfigs == nil {
+		return true
+	}
+	for config, seedHex := range seedConfigs {
+		probeHex, ok := probeHashes[label+"\x00"+config]
+		if !ok || probeHex != seedHex {
+			return true
+		}
+	}
+	return false
+}
+
+// propagateFrom builds DirtyStarLabels by including all directly dirty
+// labels and BFS-propagating rdeps only from the actuallyChanged subset.
+func propagateFrom(dirtyLabels, actuallyChanged map[string]bool, edges map[string][]string) map[string]bool {
+	rdeps := BuildRdeps(edges)
+
+	// Every directly dirty label is dirty* regardless of whether it
+	// changed, but membership of the result must not be mistaken for
+	// having been traversed: an unchanged dirty label still has to be
+	// walked through to reach what lies behind it. Hence a separate
+	// visited set from the result set.
+	result := make(map[string]bool, len(dirtyLabels))
+	for label := range dirtyLabels {
+		result[label] = true
+	}
+
+	visited := make(map[string]bool, len(actuallyChanged))
+	queue := make([]string, 0, len(actuallyChanged))
+	for label := range actuallyChanged {
+		if !visited[label] {
+			visited[label] = true
+			queue = append(queue, label)
+		}
+	}
+	for len(queue) > 0 {
+		current := queue[0]
+		queue = queue[1:]
+		for _, rdep := range rdeps[current] {
+			result[rdep] = true
+			if !visited[rdep] {
+				visited[rdep] = true
+				queue = append(queue, rdep)
+			}
+		}
+	}
+	return result
+}
+
 func isFallbackTrigger(basename string) bool {
 	if strings.HasSuffix(basename, ".bzl") {
 		return true