perf(hash-persister): probe dirty packages before rdeps propagation - #20
Merged
Merged
Conversation
mattnworb
approved these changes
Sep 22, 2026
honnix
force-pushed
the
honnix/probe-before-propagate
branch
from
September 23, 2026 07:24
4b23835 to
f8dfa97
Compare
When a BUILD.bazel changes, all targets in the package are marked dirty and their reverse dependencies cascade through the graph. For high-fanout packages like tools/binaries (which contains widely-used aliases), adding a single new target inflates the dirty set to hundreds of thousands of targets even though existing targets are unchanged. Add a probe phase to runSeeded that queries only the dirty packages, hashes those targets against seed dependency hashes, and compares with the seed. Only targets whose hash actually changed propagate to reverse dependencies. This dramatically reduces the scoped query size when BUILD.bazel changes don't affect most existing targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
honnix
force-pushed
the
honnix/probe-before-propagate
branch
from
September 23, 2026 07:58
f8dfa97 to
69a0ab4
Compare
Temporary diagnostic to understand why 191K targets remain dirty after probe pruning. Logs each changed target with its reason (new_target, hash_differs, missing_from_probe). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Include hashes for edge-map-only targets (manual-tagged deps, platform() rules, generated file outputs) in the seedable target_hashes. These targets are already computed in the hash cache via recursive Hash() calls but were previously not persisted, causing the probe to treat them as "new" and over-propagate rdeps. Also removes the temporary diagnostic logging from the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…atching targets The probe query uses the same targets pattern that excludes manual-tagged targets. ProbeHashesFromQueryResults only extracted hashes for matching targets, missing dependency-only labels (platform, npm, generated files) even though their hashes were computed transitively during PrefillCache. Use ProbeHashesFromCache to extract all cached hashes, so the probe can compare every label in the seed against its current hash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two gaps left the probe unable to compare most dirty-package labels, so they were conservatively marked changed and their reverse dependencies propagated anyway. Seed side: AddDependencyHashes iterated only edge keys, missing leaf labels that appear solely as dependency values (source files, npm /ref targets). Measured on a real seed, 4417 of 13081 dirty labels were missed this way, including //:service-info.yaml whose rdeps closure is 95k targets. Probe side: the probe query applied the targets pattern, which excludes manual-tagged targets. platform() rules, npm link targets and JS build internals are all manual-tagged, so the probe hashed only 115 of 13081 dirty labels. Query the dirty packages raw with ":*" instead, which covers manual-tagged rules and source files alike. With both gaps closed every dirty label has a seed hash and a probe hash to compare. On the measured case only 7 targets genuinely change, and their combined rdeps closure is 7, so dirty* should fall from ~280k to roughly the 13081 directly dirty labels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consolidation pass over the probe work, no behaviour change: - Drop ProbeHashesFromQueryResults, dead since the probe switched to reading the cache, and replace the package-level ProbeHashesFromCache with a TargetHashCache.ExtractHexHashes method next to ExtractHashes. It only ever touched the cache and could not fail, so the QueryResults parameter and error return were both noise. - Index only the labels AddDependencyHashes actually needs instead of every cached hash, avoiding a transient map-of-maps over ~530k entries. - Fix its doc comment to name the exported identifier, size the propagateFrom result map by the set it is actually filled from, and trim doc comments that restated their signatures. - Cover the two behaviours that iteration left untested: dependency values reached only as edge values, and the probe pattern shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TargetHashCache returns a zero-length slice rather than a hash for a source-file label whose file does not exist, or which is a directory spuriously listed in srcs (bazelbuild/bazel#14611, #14678). ExtractHashes only filters nil, so the sentinel passed through. Persisting dependency values started surfacing those labels in the seed, and seed validation requires every hash to be sha256-sized, so a single one rejected the entire seed and fell the run back to full hashing. Omit them instead. A label without a seed hash is treated as changed, which is the safe direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ersal Source files were being persisted into the seed so the probe could compare their hashes. That was the wrong instrument: runSeeded already computes a git diff, and a source-file label maps one to one onto a path, so git answers the question outright. Hashing to rediscover it added 485k of the 505k labels the previous commit put in the seed, more than twenty times the 20k rule and generated labels that genuinely need one, and dangling source-file labels are also the only source of the empty-hash sentinel that rejected whole seeds. The probe already queries every label in the dirty packages, so it reports which of them are source files; those are judged by the git diff and the rest by hash, and AddDependencyHashes skips them entirely. Fixes a separate bug this exposed: propagateFrom pre-seeded the result set with every dirty label and then used that same set as the BFS visited set, so propagation stopped at the first dirty label instead of passing through it and dropped everything behind. Usually masked, because a changed dependency normally changes its consumer's hash and makes it a root in its own right, but the failure mode is a missing impacted target, so the sets are now kept apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency hashes were being written into TargetHashes. That map is the target set diffing compares, so the seeded run merged them into its output and the shadow comparison reported 81509 added targets, every one of them a dependency label rather than a real target. Once the incremental output feeds target selection directly the same labels would become phantom impacted targets. Persist them under a separate dependency_hashes key instead. Seeding reads both, since either is equally reusable as a pre-computed hash, and diffing keeps seeing TargetHashes alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
honnix
marked this pull request as ready for review
September 23, 2026 15:22
…ng it encoding/json marshals a whole value into one in-memory buffer before writing any of it, growing that buffer by doubling. A seedable artifact is over a gigabyte, so persisting one meant gigabytes of allocation and copying and a peak resident size around twice the output. That phase measured 41.5s against 3s for the compact artifact, tracking output size rather than the work involved. Emit the large maps entry by entry into a buffered writer so memory stays flat. Labels and hex hashes never need escaping, so strings take a straight copy and anything else defers to the stdlib rather than reimplementing its escaping rules. Keys are sorted as encoding/json sorts them, keeping the artifact reproducible. The compact artifact keeps using the stdlib: it is two orders of magnitude smaller, so the buffering costs little and hand-indenting would not earn its keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… buffering it" This reverts commit 8af1231.
The persist phase is 39s for a seedable artifact against 3s for a compact one, and an attempt to cut it by streaming the encode changed nothing measurable, so the cost is not where it was assumed to be. Time each sub-step separately, and split marshalling from the file write, so the phase can be attributed to hash collection, edge extraction, dependency hashes, formatting or disk rather than guessed at. The split mirrors what json.Encoder already does internally, so the output is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattnworb
requested changes
Sep 23, 2026
mattnworb
left a comment
mattnworb
left a comment
Member
There was a problem hiding this comment.
Two things I think we should fix before merging. The probe error path currently panics instead of falling back, and the execution report keeps the pre-pruning dirty count. Details inline.
Instrumenting the persist phase showed it is dominated by ExtractEdges at 27.7s of 42.6s, not by formatting (5.2s) or the disk write (0.7s). Rule inputs arrive as strings and leave as strings, but each of the 13.6 million occurrences was parsed into a Label and formatted back, then inserted into a set keyed on the full ~100-byte string. Those occurrences are only 1.21 million distinct labels, so canonicalise each distinct string once and reuse it, and deduplicate by sorting and compacting rather than by hashing every dependency into a set. Shortcutting on the label's shape instead would be wrong: "//pkg:pkg" canonicalises to "//pkg", so skipping the parse would record labels inconsistent with how matching targets are written and make seed lookups silently miss. Memoising still canonicalises, just once per string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit 80b504a.
probePruneDirtySet returns a nil result alongside its error, so assigning it straight to dirtyResult set it to nil on the error path. The code then logged that it was continuing with the unpruned set and dereferenced nil on the next line, turning a recoverable probe failure into a panic. Hold the result in a temporary and only adopt it on success. Also update the execution report's dirty target count after a successful prune. It drives CI metrics, and reporting the pre-probe figure would have shown 280k dirty targets for a run that actually used 13k. Both found in review by mattbrown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattnworb
approved these changes
Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A BUILD.bazel edit marks its whole package dirty, and every label in that package then propagates reverse dependencies. In a high-fanout package the resulting closure covers most of the repository even when one target really changed. This adds a probe phase that decides which dirty labels actually changed before propagating from them.
How it works
After
ComputeDirtySet, probe the dirty packages and prune://pkg:*wildcards. The targets pattern excludesmanual-tagged targets, and npm link targets,platform()rules and JS build internals are all manual-tagged, so applying it would leave most dirty labels unhashed.For the probe to compare a label the seed has to carry its hash, so the seedable output additionally records hashes for edge-map labels that are not matching targets. These live under a separate
dependency_hasheskey:target_hashesis the target set diffing compares, and mixing them in makes them surface as added targets.Source files are deliberately excluded from the seed.
runSeededalready computes a git diff and a source-file label maps one to one onto a path, so git answers the question outright — hashing to rediscover it added twenty times more labels than the rule and generated labels that genuinely need one.Also fixed
propagateFrompre-seeded the result set with every dirty label and reused it as the BFS visited set, so propagation stopped at the first dirty label instead of passing through it and dropped everything behind. Usually masked, because a changed dependency normally changes its consumer's hash and makes it a root in its own right, but the failure mode is a missing impacted target.Measured
On a 13-file change touching high-fanout tooling packages:
13,082 is the number of directly dirty labels, so propagation adds essentially nothing on top. The probe costs 23s of that. Output verified identical to the authoritative full hash.
Gains depend on the change: a PR touching one service component has a far smaller dirty set, while one that trips a fallback trigger (
.bzl,MODULE.bazel, a lockfile) still runs full mode.Test plan
target_hashesbazel test //pkg:pkg_test //hash-persister:all