From 834c94c9c0c103ff7f8d6ad3ca268e16027727da Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 10:50:08 +0000 Subject: [PATCH] perf: cut get-impacted-targets --depEdgesFile CPU by about half - Sort the distance result on a rank computed once per label instead of a key closure that looked the label up in both maps and cloned it on every comparison. - Walk distances over borrowed labels: the memo, cycle guard and dependency lists no longer allocate a String per visit. - Build the hash and dep-edges maps in one bulk pass from the (sorted) entries instead of one tree search per insert. - Skip the second hash diff and the full copy of the starting hashes when the module graph is unchanged: no module-only targets exist then. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KJs3XyDHGXocPX8AFo8rZm --- src/main.rs | 33 +++++++---- src/model.rs | 164 ++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 150 insertions(+), 47 deletions(-) diff --git a/src/main.rs b/src/main.rs index cf93c0c7..42bdcf49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,12 +5,14 @@ use bazel_diff::fingerprint; use bazel_diff::git::{self, DerivationPolicy, Git}; use bazel_diff::hash::{generate_hashes, HashOptions}; use bazel_diff::model::{ - filter_and_sort_labels, impacted_targets, impacted_targets_with_distances, HashFileData, + dep_edges_from_slice, filter_and_sort_labels, impacted_targets, + impacted_targets_with_distances, HashFileData, }; use bazel_diff::module_graph::{bzlmod_enabled_for, impacted_with_module_changes}; use bazel_diff::partial::{self, Classification, Glob}; use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum}; use serde::Serialize; +use std::borrow::Cow; use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::fs::{self, File}; use std::io::{self, BufWriter, Write}; @@ -992,16 +994,23 @@ fn run_get_impacted(args: &GetImpactedTargetsArgs, verbose: bool) -> Result<()> let target_types = (!args.target_type.is_empty()) .then(|| args.target_type.iter().cloned().collect::>()); let output = if let Some(dep_edges_file) = &args.dep_edges_file { - let dep_edges: BTreeMap> = - serde_json::from_slice(&fs::read(dep_edges_file)?)?; - let module_impacted = impacted_with_module_changes(&from, &to, Some(&bazel))?; - let hash_impacted = impacted_targets(&from.hashes, &to.hashes, None, false)? - .into_iter() - .collect::>(); - let mut distance_from = from.hashes.clone(); - for label in module_impacted.difference(&hash_impacted) { - distance_from.remove(label); - } + let dep_edges = dep_edges_from_slice(&fs::read(dep_edges_file)?)?; + // Targets impacted only through a module change are made to look new, so they report + // distance 0. With an unchanged module graph there are none (the module-aware set is the + // hash diff itself), so the starting hashes are used as they are: no second diff, no copy. + let distance_from = if from.module_graph_json == to.module_graph_json { + Cow::Borrowed(&from.hashes) + } else { + let module_impacted = impacted_with_module_changes(&from, &to, Some(&bazel))?; + let hash_impacted = impacted_targets(&from.hashes, &to.hashes, None, false)? + .into_iter() + .collect::>(); + let mut distance_from = from.hashes.clone(); + for label in module_impacted.difference(&hash_impacted) { + distance_from.remove(label); + } + Cow::Owned(distance_from) + }; serde_json::to_string(&impacted_targets_with_distances( &distance_from, &to.hashes, @@ -1035,7 +1044,7 @@ fn run_explain(args: &ExplainArgs, verbose: bool) -> Result<()> { let to = HashFileData::read(&args.final_hashes) .context("Incorrect final hashes: file doesn't exist or can't be read.")?; ensure_same_mode(&from, &to)?; - let dep_edges: BTreeMap> = serde_json::from_slice( + let dep_edges = dep_edges_from_slice( &fs::read(&args.dep_edges_file) .context("Incorrect dep edges file: file doesn't exist or can't be read.")?, ) diff --git a/src/model.rs b/src/model.rs index de0d9529..84ee5c05 100644 --- a/src/model.rs +++ b/src/model.rs @@ -5,7 +5,7 @@ use serde::ser::{SerializeMap, SerializeStruct}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_json::{Map, Value}; use std::borrow::Cow; -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; use std::fs; use std::path::Path; @@ -320,6 +320,40 @@ impl<'de> Deserialize<'de> for JsonStr<'de> { } } +/// Parses a `--depEdgesFile` (`{label: [dependency labels]}`). `generate-hashes` writes it from a +/// sorted map, so the entries are collected and the map built in one bulk pass, as for [`Hashes`], +/// instead of searching the tree for every label; a repeated label keeps its last value, as +/// deserializing straight into a `BTreeMap` does. +pub fn dep_edges_from_slice(bytes: &[u8]) -> serde_json::Result>> { + struct DepEdges(BTreeMap>); + + impl<'de> Deserialize<'de> for DepEdges { + fn deserialize>(deserializer: D) -> Result { + struct DepEdgesVisitor; + + impl<'de> Visitor<'de> for DepEdgesVisitor { + type Value = DepEdges; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("an object mapping labels to label arrays") + } + + fn visit_map>(self, mut map: A) -> Result { + let mut entries = Vec::with_capacity(map.size_hint().unwrap_or(0)); + while let Some(entry) = map.next_entry::>()? { + entries.push(entry); + } + Ok(DepEdges(entries.into_iter().collect())) + } + } + + deserializer.deserialize_map(DepEdgesVisitor) + } + } + + serde_json::from_slice::(bytes).map(|edges| edges.0) +} + /// The `{label: "kind#hash~direct"}` object, parsed entry by entry. struct Hashes(BTreeMap); @@ -335,12 +369,16 @@ impl<'de> Deserialize<'de> for Hashes { } fn visit_map>(self, mut map: A) -> Result { - let mut hashes = BTreeMap::new(); + // `generate-hashes` writes labels in sorted order, so collecting the entries and + // building the map in one bulk pass replaces a root-to-leaf search (a string + // comparison per level) for every insert. `BTreeMap::from_iter` sorts stably and + // keeps the last value for a repeated label, exactly as repeated inserts would. + let mut entries = Vec::with_capacity(map.size_hint().unwrap_or(0)); while let Some((label, value)) = map.next_entry::>()? { let hash = TargetHash::parse(&value.0).map_err(de::Error::custom)?; - hashes.insert(label, hash); + entries.push((label, hash)); } - Ok(Hashes(hashes)) + Ok(Hashes(entries.into_iter().collect())) } } @@ -374,11 +412,18 @@ impl<'de> Deserialize<'de> for HashFileData { fn visit_map>(self, mut map: A) -> Result { let mut data = HashFileData::default(); + // Entries of the bare shape, buffered for one bulk build like `Hashes` does. They + // all follow the last `"hashes"` key (which replaces everything before it), so + // they are applied last, each overriding an earlier value for its label. + let mut bare = Vec::new(); // Labels always start with `//`, `@` or a repository name, so the two wrapper // keys cannot collide with one. while let Some(key) = map.next_key::>()? { match &*key.0 { - "hashes" => data.hashes = map.next_value::()?.0, + "hashes" => { + bare.clear(); + data.hashes = map.next_value::()?.0; + } "metadata" => { let metadata = map.next_value::()?; data.module_graph_json = metadata.module_graph_json; @@ -390,10 +435,15 @@ impl<'de> Deserialize<'de> for HashFileData { label => { let value = map.next_value::>()?; let hash = TargetHash::parse(&value.0).map_err(de::Error::custom)?; - data.hashes.insert(label.to_owned(), hash); + bare.push((label.to_owned(), hash)); } } } + if data.hashes.is_empty() { + data.hashes = bare.into_iter().collect(); + } else { + data.hashes.extend(bare); + } Ok(data) } } @@ -605,36 +655,38 @@ pub fn impacted_types( .collect() } -fn calculate_distance( - label: &str, - dep_edges: &BTreeMap>, +fn calculate_distance<'a>( + label: &'a str, + dep_edges: &'a BTreeMap>, impact_types: &HashMap, - memo: &mut HashMap, - visiting: &mut BTreeSet, + memo: &mut HashMap<&'a str, (usize, usize)>, + visiting: &mut HashSet<&'a str>, ) -> (usize, usize) { if let Some(distance) = memo.get(label) { return *distance; } if impact_types.get(label) == Some(&ImpactType::Direct) { - memo.insert(label.to_owned(), (0, 0)); + memo.insert(label, (0, 0)); return (0, 0); } - if !visiting.insert(label.to_owned()) { + if !visiting.insert(label) { return (0, 0); } + // Labels are borrowed from the impact map and the dep-edges file for the whole walk, so the + // memo, the cycle guard and the dependency list allocate no strings. let impacted_deps = dep_edges .get(label) .into_iter() .flatten() + .map(String::as_str) .filter(|dep| impact_types.contains_key(*dep)) - .cloned() .collect::>(); if impacted_deps.is_empty() { eprintln!( "[Warn] {label} was indirectly impacted, but has no impacted dependencies in the dep-edges file. Falling back to distance 0 -- see https://github.com/Tinder/bazel-diff/issues/268" ); visiting.remove(label); - memo.insert(label.to_owned(), (0, 0)); + memo.insert(label, (0, 0)); return (0, 0); } let label_package = label.split(':').next().unwrap_or(label); @@ -642,14 +694,14 @@ fn calculate_distance( let mut package_distance = usize::MAX; for dep in impacted_deps { let (dep_target, dep_package) = - calculate_distance(&dep, dep_edges, impact_types, memo, visiting); + calculate_distance(dep, dep_edges, impact_types, memo, visiting); target_distance = target_distance.min(dep_target.saturating_add(1)); package_distance = package_distance - .min(dep_package + usize::from(dep.split(':').next().unwrap_or(&dep) != label_package)); + .min(dep_package + usize::from(dep.split(':').next().unwrap_or(dep) != label_package)); } visiting.remove(label); let result = (target_distance, package_distance); - memo.insert(label.to_owned(), result); + memo.insert(label, result); result } @@ -661,23 +713,19 @@ pub fn impacted_targets_with_distances( exclude_external: bool, ) -> Result> { let impact_types = impacted_types(from, to); - let mut memo = HashMap::new(); + let mut memo = HashMap::with_capacity(impact_types.len()); + // Every walk removes what it adds, so one guard serves all of them. + let mut visiting = HashSet::new(); for label in impact_types.keys() { - calculate_distance( - label, - dep_edges, - &impact_types, - &mut memo, - &mut BTreeSet::new(), - ); + calculate_distance(label, dep_edges, &impact_types, &mut memo, &mut visiting); } - let mut impacted = memo + let impacted = memo .into_iter() .filter(|(label, _)| !exclude_external || !label.starts_with("//external:")) .filter_map(|(label, (target_distance, package_distance))| { - match accepts(&label, to, target_types) { + match accepts(label, to, target_types) { Ok(true) => Some(Ok(ImpactedTargetWithDistance { - label, + label: label.to_owned(), target_distance, package_distance, })), @@ -686,13 +734,23 @@ pub fn impacted_targets_with_distances( } }) .collect::>>()?; - impacted.sort_by_key(|target| { - ( - kind_rank(to.get(&target.label).or_else(|| from.get(&target.label))), - target.label.clone(), - ) + // Rank each label once and sort the pairs. A key closure would look the label up in both maps + // and clone it on every comparison: O(n log n) lookups and allocations for an n-label result. + // The labels are distinct (they are the memo's keys), so an unstable sort orders them exactly + // as a stable one would. + let mut ranked = impacted + .into_iter() + .map(|target| { + let rank = kind_rank(to.get(&target.label).or_else(|| from.get(&target.label))); + (rank, target) + }) + .collect::>(); + ranked.sort_unstable_by(|(left_rank, left), (right_rank, right)| { + left_rank + .cmp(right_rank) + .then_with(|| left.label.cmp(&right.label)) }); - Ok(impacted) + Ok(ranked.into_iter().map(|(_, target)| target).collect()) } #[cfg(test)] @@ -908,6 +966,42 @@ mod tests { assert!(reordered.dep_edges.is_empty()); } + #[test] + fn bulk_built_maps_match_insertion_for_unsorted_and_repeated_labels() { + // The readers build their maps in one pass; out-of-order and repeated labels must still + // land exactly as entry-by-entry inserts would place them: sorted, last value wins. + for bytes in [ + br#"{"//b:b":"b~b","//a:a":"a~a","//b:b":"c~c"}"#.as_slice(), + br#"{"hashes":{"//b:b":"b~b","//a:a":"a~a","//b:b":"c~c"}}"#.as_slice(), + ] { + let parsed = HashFileData::from_slice(bytes).unwrap(); + assert_eq!( + parsed.hashes, + BTreeMap::from([ + ("//a:a".into(), target("", "a", "a")), + ("//b:b".into(), target("", "c", "c")), + ]) + ); + } + // Bare labels after a wrapper override it; a wrapper after bare labels replaces them. + let after = HashFileData::from_slice(br#"{"hashes":{"//a:a":"a~a"},"//a:a":"b~b"}"#); + assert_eq!(after.unwrap().hashes["//a:a"], target("", "b", "b")); + let before = + HashFileData::from_slice(br#"{"//z:z":"z~z","hashes":{"//a:a":"a~a"}}"#).unwrap(); + assert_eq!(before.hashes.keys().collect::>(), ["//a:a"]); + + let edges = + dep_edges_from_slice(br#"{"//b:b":["//a:a"],"//a:a":[],"//b:b":["//c:c"]}"#).unwrap(); + assert_eq!( + edges, + BTreeMap::from([ + ("//a:a".to_owned(), vec![]), + ("//b:b".to_owned(), vec!["//c:c".to_owned()]), + ]) + ); + assert!(dep_edges_from_slice(br#"{"//a:a":"//b:b"}"#).is_err()); + } + #[test] fn rejects_malformed_hash_files() { for invalid in [