perf(resolving-deps-resolver): order node ids without rendering a sort key (#13568)
The peer-resolution passes sort `NodeId`s so their output does not depend
on hash iteration order. The comparison key was a freshly formatted
`String` per element — `"0:{value:020}"` for a counter, `"1:{id}"` for a
leaf — and `sort_by_key` re-invokes the key function on every comparison,
so each sort allocated `O(n log n)` strings. `build_final_dep_paths` and
the Tarjan pass under it run those sorts over every node that resolved a
peer, plus one sorted successor list per node.
Derive `Ord` on `NodeId` instead and sort the values directly. The
derived order is the same total order the rendered keys encoded: the
variant order puts counters before leaves, a fixed-width zero-padded
u64 compares like the number it renders, and leaf ids compare as their
`str` either way. The resolved graph is unchanged.
On the 114-project Bit workspace from
https://github.com/pnpm/pnpm/issues/13505, resolution drops from ~16.0s
to ~13.4s, with the final depPath pass falling from 2.50s to 0.76s. The
peer-heavy `workspace_full_resolution` benchmark goes from ~1.01s to
~0.85s.
This commit is contained in:
1 parent
d83eb4fc6d
commit
d6a01f0907
6 files changed
+46
-19
No files matched your search
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"pacquet": patch
|
||||
---
|
||||
|
||||
Resolution on large peer-heavy workspaces got faster: a Bit workspace with 114 projects and ~21,000 lockfile entries resolves in ~13.4s instead of ~16.0s. The resolved dependency graph is unchanged.
|
||||
@@ -13,8 +13,13 @@ use std::sync::{
|
||||
/// Leaves share a single tree node across every parent that references
|
||||
/// them.
|
||||
///
|
||||
/// The derived ordering — counters before leaves, counters by their
|
||||
/// numeric value, leaves by package id — is what the peer-resolution
|
||||
/// passes sort by to keep their output independent of hash iteration
|
||||
/// order.
|
||||
///
|
||||
/// [`DependenciesTree`]: super::resolved_tree::DependenciesTree
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
||||
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
|
||||
pub enum NodeId {
|
||||
/// Fresh per-occurrence counter value. Allocated by [`NodeId::next`].
|
||||
Counter(u64),
|
||||
@@ -47,3 +52,6 @@ impl std::fmt::Display for NodeId {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests;
|
||||
@@ -0,0 +1,26 @@
|
||||
use super::NodeId;
|
||||
|
||||
/// The peer-resolution passes sort `NodeId`s to keep their output
|
||||
/// independent of hash iteration order, so the order itself is part of
|
||||
/// the resolved lockfile: counters first, in numeric (not textual)
|
||||
/// order, then leaves by package id.
|
||||
#[test]
|
||||
fn sorts_counters_numerically_before_leaves() {
|
||||
let mut node_ids = vec![
|
||||
NodeId::leaf("zod@3.25.76"),
|
||||
NodeId::Counter(10),
|
||||
NodeId::leaf("react@19.2.8"),
|
||||
NodeId::Counter(2),
|
||||
];
|
||||
node_ids.sort();
|
||||
dbg!(&node_ids);
|
||||
assert_eq!(
|
||||
node_ids,
|
||||
vec![
|
||||
NodeId::Counter(2),
|
||||
NodeId::Counter(10),
|
||||
NodeId::leaf("react@19.2.8"),
|
||||
NodeId::leaf("zod@3.25.76"),
|
||||
],
|
||||
);
|
||||
}
|
||||
@@ -43,9 +43,7 @@ use crate::{
|
||||
node_id::NodeId,
|
||||
resolved_tree::{DirectDep, ResolvedTree},
|
||||
};
|
||||
use context::{
|
||||
CurrentProviderSource, SharedChain, importer_relative_link_dep_path, node_id_sort_key,
|
||||
};
|
||||
use context::{CurrentProviderSource, SharedChain, importer_relative_link_dep_path};
|
||||
use discovery::PeerDiscoveryCaches;
|
||||
pub(crate) use discovery::{PeerDiscoveryResult, PeerHoistDiscovery, apply_hoist_missing_scope};
|
||||
use pacquet_deps_path::DepPath;
|
||||
@@ -510,7 +508,7 @@ fn build_node_ids_by_previous_dep_path(
|
||||
return map;
|
||||
}
|
||||
let mut node_ids: Vec<&NodeId> = tree.dependencies_tree.keys().collect();
|
||||
node_ids.sort_by_key(|node_id| node_id_sort_key(node_id));
|
||||
node_ids.sort();
|
||||
for node_id in node_ids {
|
||||
if let Some(previous) = tree.dependencies_tree[node_id].previous_dep_path.as_ref()
|
||||
&& !map.contains_key(previous)
|
||||
|
||||
@@ -427,13 +427,6 @@ pub(super) fn remap_link_node_id(
|
||||
Some(NodeId::leaf(&format!("link:{rel}")))
|
||||
}
|
||||
|
||||
pub(super) fn node_id_sort_key(node_id: &NodeId) -> String {
|
||||
match node_id {
|
||||
NodeId::Counter(value) => format!("0:{value:020}"),
|
||||
NodeId::Leaf(value) => format!("1:{value}"),
|
||||
}
|
||||
}
|
||||
|
||||
/// Pull `(name, version)` out of a `ResolveResult` the peer-resolution
|
||||
/// stage can hash and compare on.
|
||||
///
|
||||
|
||||
@@ -8,10 +8,7 @@ use crate::{
|
||||
dependencies_graph::{DependenciesGraph, DependenciesGraphNode},
|
||||
node_id::NodeId,
|
||||
resolve_peers::{
|
||||
context::{
|
||||
SharedChain, link_node_id_as_dep_path, node_id_sort_key, peer_segment_names,
|
||||
pkg_name_version,
|
||||
},
|
||||
context::{SharedChain, link_node_id_as_dep_path, peer_segment_names, pkg_name_version},
|
||||
walker::{MissingPeerInfo, Walker},
|
||||
},
|
||||
resolved_tree::ResolvedPackage,
|
||||
@@ -245,7 +242,7 @@ impl Walker<'_> {
|
||||
let mut final_dep_paths: HashMap<NodeId, DepPath> = HashMap::default();
|
||||
let mut visiting = HashSet::default();
|
||||
let mut node_ids: Vec<NodeId> = self.node_external_peers.keys().cloned().collect();
|
||||
node_ids.sort_by_key(node_id_sort_key);
|
||||
node_ids.sort();
|
||||
for node_id in node_ids {
|
||||
self.final_dep_path_for_node(
|
||||
&node_id,
|
||||
@@ -482,7 +479,7 @@ impl Walker<'_> {
|
||||
.filter(|(_, peers)| !peers.is_empty())
|
||||
.map(|(node_id, _)| node_id.clone())
|
||||
.collect();
|
||||
participants.sort_by_key(node_id_sort_key);
|
||||
participants.sort();
|
||||
participants.dedup();
|
||||
let participant_set: HashSet<NodeId> = participants.iter().cloned().collect();
|
||||
let neighbors = |node_id: &NodeId| -> Vec<NodeId> {
|
||||
@@ -494,7 +491,7 @@ impl Walker<'_> {
|
||||
.filter(|peer| participant_set.contains(*peer))
|
||||
.cloned()
|
||||
.collect();
|
||||
out.sort_by_key(node_id_sort_key);
|
||||
out.sort();
|
||||
out
|
||||
};
|
||||
|
||||
|
||||
Reference in new issue
Block a user