From d6a01f0907d900d7ee61590f5bdbb0ebf941c2ee Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 1 Aug 2026 20:09:57 +0200 Subject: [PATCH] perf(resolving-deps-resolver): order node ids without rendering a sort key (#13568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../faster-peer-dep-path-finalization.md | 5 ++++ .../resolving-deps-resolver/src/node_id.rs | 10 ++++++- .../src/node_id/tests.rs | 26 +++++++++++++++++++ .../src/resolve_peers.rs | 6 ++--- .../src/resolve_peers/context.rs | 7 ----- .../src/resolve_peers/finalize.rs | 11 +++----- 6 files changed, 46 insertions(+), 19 deletions(-) create mode 100644 .changeset/faster-peer-dep-path-finalization.md create mode 100644 pnpm/crates/resolving-deps-resolver/src/node_id/tests.rs diff --git a/.changeset/faster-peer-dep-path-finalization.md b/.changeset/faster-peer-dep-path-finalization.md new file mode 100644 index 0000000000..2e2c968bcd --- /dev/null +++ b/.changeset/faster-peer-dep-path-finalization.md @@ -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. diff --git a/pnpm/crates/resolving-deps-resolver/src/node_id.rs b/pnpm/crates/resolving-deps-resolver/src/node_id.rs index 0ef01c5cd0..a291c92c10 100644 --- a/pnpm/crates/resolving-deps-resolver/src/node_id.rs +++ b/pnpm/crates/resolving-deps-resolver/src/node_id.rs @@ -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; diff --git a/pnpm/crates/resolving-deps-resolver/src/node_id/tests.rs b/pnpm/crates/resolving-deps-resolver/src/node_id/tests.rs new file mode 100644 index 0000000000..1ea58441e3 --- /dev/null +++ b/pnpm/crates/resolving-deps-resolver/src/node_id/tests.rs @@ -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"), + ], + ); +} diff --git a/pnpm/crates/resolving-deps-resolver/src/resolve_peers.rs b/pnpm/crates/resolving-deps-resolver/src/resolve_peers.rs index 18280a353c..6e86e2f810 100644 --- a/pnpm/crates/resolving-deps-resolver/src/resolve_peers.rs +++ b/pnpm/crates/resolving-deps-resolver/src/resolve_peers.rs @@ -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) diff --git a/pnpm/crates/resolving-deps-resolver/src/resolve_peers/context.rs b/pnpm/crates/resolving-deps-resolver/src/resolve_peers/context.rs index affc30ae7c..d9dfc0043e 100644 --- a/pnpm/crates/resolving-deps-resolver/src/resolve_peers/context.rs +++ b/pnpm/crates/resolving-deps-resolver/src/resolve_peers/context.rs @@ -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. /// diff --git a/pnpm/crates/resolving-deps-resolver/src/resolve_peers/finalize.rs b/pnpm/crates/resolving-deps-resolver/src/resolve_peers/finalize.rs index 1f41ed4902..757b2e3691 100644 --- a/pnpm/crates/resolving-deps-resolver/src/resolve_peers/finalize.rs +++ b/pnpm/crates/resolving-deps-resolver/src/resolve_peers/finalize.rs @@ -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 = HashMap::default(); let mut visiting = HashSet::default(); let mut node_ids: Vec = 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 = participants.iter().cloned().collect(); let neighbors = |node_id: &NodeId| -> Vec { @@ -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 };