Skip to content

perf(pacquet): collapse leaf-package occurrences in the dependencies tree#11847

Merged
zkochan merged 1 commit into
mainfrom
pacquet-collapse-leaf-occurrences
May 22, 2026
Merged

perf(pacquet): collapse leaf-package occurrences in the dependencies tree#11847
zkochan merged 1 commit into
mainfrom
pacquet-collapse-leaf-occurrences

Conversation

@zkochan

@zkochan zkochan commented May 22, 2026

Copy link
Copy Markdown
Member

Summary

Mirrors pnpm's pkgIsLeaf reuse: a package with no `dependencies`, `optionalDependencies`, `peerDependencies`, or `peerDependenciesMeta` collapses every parent reference onto one tree node keyed by the package id, instead of allocating a fresh `NodeId` per occurrence. Peer resolution and every downstream `HashMap<NodeId, _>` see the reduced node set.

`NodeId` becomes an enum:

```rust
pub enum NodeId {
Counter(u64), // per-occurrence (existing path, non-leaves)
Leaf(Arc), // shared across every leaf occurrence
}
```

The tree insert switches to `entry().and_modify(min depth).or_insert_with(...)` so collapsed-leaf depth still folds to the shallowest occurrence, matching pnpm's `Math.min` arm.

Two conservative deviations from the issue:

  • `pkg_is_leaf` returns `false` when `result.manifest` is `None` — resolvers that defer manifest reading can't be wrongly collapsed.
  • Kept upstream's per-visit depth-fold for collapsed leaves; the peer pass reads `tree_node.depth` on every node, so we still need the min.

Closes #11844.

Test plan

  • `cargo check --workspace --all-targets` clean
  • `cargo clippy --locked --workspace --all-targets -- --deny warnings` clean
  • `cargo nextest run -p pacquet-resolving-deps-resolver -p pacquet-package-manager -p pacquet-cli` — 440/440 pass
  • `dedupes_when_the_same_package_appears_in_two_subtrees` extended to assert `a` and `b` point at the same shared NodeId and the tree carries exactly one `shared` entry

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of leaf dependencies (packages with no sub-dependencies) during tree resolution.
    • Enhanced deduplication accuracy for shared packages appearing across multiple dependency subtrees.
  • Tests

    • Strengthened test assertions for dependency deduplication validation.

Review Change Stack

…tree

Mirrors pnpm's `pkgIsLeaf` reuse: a package with no `dependencies`,
`optionalDependencies`, `peerDependencies`, or `peerDependenciesMeta`
collapses every parent reference onto one tree node keyed by the
package id, instead of allocating a fresh `NodeId` per occurrence.
Peer resolution and every downstream `HashMap<NodeId, _>` see the
reduced node set.

Closes #11844.
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: da269bdb-ba6a-4f7c-8a07-28cba9c6f38a

📥 Commits

Reviewing files that changed from the base of the PR and between 3422cec and 3c44d80.

📒 Files selected for processing (5)
  • pacquet/crates/resolving-deps-resolver/src/node_id.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/node_id.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/node_id.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/node_id.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/node_id.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
🔇 Additional comments (7)
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (2)

27-35: LGTM!


81-84: LGTM!

pacquet/crates/resolving-deps-resolver/src/tests.rs (2)

129-130: LGTM!


195-207: LGTM!

pacquet/crates/resolving-deps-resolver/src/node_id.rs (1)

18-25: LGTM!

Also applies to: 28-40, 45-48

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)

420-427: LGTM!

Also applies to: 460-479, 673-690

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (1)

215-220: LGTM!

Also applies to: 311-311, 337-337, 362-362, 369-369, 410-410, 427-427, 443-445, 462-462, 517-521, 572-574, 583-590, 612-612


📝 Walkthrough

Walkthrough

NodeId transitions from a Copy u64 struct to an enum with Counter and Leaf variants. The resolver detects leaf packages (those with no child dependencies) and assigns them reused NodeId::leaf(&id) instead of fresh counters. Non-leaf packages continue using per-occurrence Counter ids. The peer resolver adjusts ownership patterns to clone NodeId consistently. Documentation and tests reflect the new per-occurrence versus shared-leaf semantics.

Changes

Leaf-Node Collapse Optimization

Layer / File(s) Summary
NodeId enum redesign
pacquet/crates/resolving-deps-resolver/src/node_id.rs
NodeId changes from struct NodeId(u64) to enum NodeId { Counter(u64), Leaf(Arc<str>) }. New leaf(&str) constructor allocates shared ids for leaf packages. next() returns the Counter variant. Display formats counter values or leaf strings. Copy, PartialOrd, and Ord traits are removed.
Leaf detection and tree node upsert
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
resolve_node detects leaf packages via pkg_is_leaf helper and assigns NodeId::leaf(&id) for packages with no dependencies, optional dependencies, or peer dependencies. Tree insertion changes from unconditional insert to entry().or_insert_with() pattern, where repeated leaf visits update the node with the shallowest observed depth. Non-leaf nodes continue using fresh Counter ids.
Ownership adjustments in peer resolution
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
NodeId values are consistently cloned when stored in walker state or passed through recursion. build_peer_id signature changes to accept &NodeId. Internal HashSet comparisons shift to reference-based membership. All changes localize to state propagation and graph construction without altering exported APIs.
Documentation and test updates
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs, pacquet/crates/resolving-deps-resolver/src/tests.rs
resolved_tree.rs documentation clarifies per-occurrence ids for non-leaf packages and shared-node collapse for leaves. Tests use borrowed NodeId references and strengthen dedup assertions: verifying shared resolves to the same child NodeId under both parent subtrees and confirming exactly one dependencies_tree entry per shared package id.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • #11844 — Core optimization issue describing leaf-node collapse design and implementation approach (enum-based NodeId and upsert logic for shared leaves).

Possibly related PRs

  • pnpm/pnpm#11774 — Introduces the NodeId-based resolved/peer-resolution pipeline in resolving-deps-resolver; this PR's NodeId refactor and leaf-specific logic build directly on that foundation.

Poem

🐰 A counter becomes two kinds of beasts,
Fresh nodes for branches, shared IDs for leaves—
No more redundant trees by the dozens,
The resolver hops lighter, like fleet-footed cousins,
Dedup triumphs, the collapsing achieves!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'perf(pacquet): collapse leaf-package occurrences in the dependencies tree' accurately and concisely describes the main optimization: collapsing leaf-package nodes in the dependency tree to improve performance.
Linked Issues check ✅ Passed The PR implementation fully addresses all primary objectives from issue #11844: changing NodeId to an enum with Counter and Leaf variants, implementing leaf detection based on absence of dependencies/peerDependencies, using entry().or_insert_with() for first-writer semantics, and collapsing leaves to shared nodes while preserving per-occurrence depth folding.
Out of Scope Changes check ✅ Passed All changes are tightly scoped to the leaf-collapse optimization: NodeId type refactoring, leaf detection logic, tree insertion semantics, peer-resolution borrowing adjustments, and test updates directly support the stated objective with no extraneous modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pacquet-collapse-leaf-occurrences

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.03      8.1±0.24ms   537.6 KB/sec    1.00      7.8±0.08ms   552.6 KB/sec

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.55%. Comparing base (5353fcb) to head (3c44d80).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...quet/crates/resolving-deps-resolver/src/node_id.rs 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11847      +/-   ##
==========================================
+ Coverage   87.51%   87.55%   +0.03%     
==========================================
  Files         204      204              
  Lines       24371    24394      +23     
==========================================
+ Hits        21329    21357      +28     
+ Misses       3042     3037       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.487 ± 0.121 2.365 2.693 1.00
pacquet@main 2.499 ± 0.091 2.362 2.655 1.00 ± 0.06
pnpm 4.931 ± 0.033 4.888 4.989 1.98 ± 0.10
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.4866053679,
      "stddev": 0.12132253999547483,
      "median": 2.4296696102,
      "user": 2.7391414399999996,
      "system": 3.81943874,
      "min": 2.3647601987,
      "max": 2.6933798316999997,
      "times": [
        2.6933798316999997,
        2.6508669067,
        2.3928174137,
        2.5327857027,
        2.5891716476999997,
        2.3647601987,
        2.3994090767,
        2.3948123007,
        2.4599301437,
        2.3881204567
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.4988154719,
      "stddev": 0.0911977637056548,
      "median": 2.5119665567,
      "user": 2.79810594,
      "system": 3.8081689399999994,
      "min": 2.3622272986999997,
      "max": 2.6554170047,
      "times": [
        2.4472507267,
        2.5068809927,
        2.5365787607,
        2.3866953637,
        2.3622272986999997,
        2.6554170047,
        2.5170521207,
        2.4378851577,
        2.5463718847,
        2.5917954087
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.931267035399999,
      "stddev": 0.03334715751915896,
      "median": 4.9247512977,
      "user": 8.34141464,
      "system": 4.324075439999999,
      "min": 4.8875247087,
      "max": 4.9887522717,
      "times": [
        4.9207839177,
        4.8875247087,
        4.9454972217,
        4.9068063797,
        4.9527176427,
        4.9287186777,
        4.9887522717,
        4.8984629007,
        4.9091528236999995,
        4.9742538097
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 709.3 ± 49.4 680.1 848.4 1.00
pacquet@main 720.7 ± 60.2 673.4 877.4 1.02 ± 0.11
pnpm 2744.6 ± 70.0 2669.1 2906.1 3.87 ± 0.29
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.70928836378,
      "stddev": 0.04941757059226638,
      "median": 0.6947523743800001,
      "user": 0.41165251999999997,
      "system": 1.5700459799999997,
      "min": 0.68005587438,
      "max": 0.8484337353800001,
      "times": [
        0.8484337353800001,
        0.6996865473800001,
        0.6984549653800001,
        0.6941960773800001,
        0.70552094738,
        0.68005587438,
        0.6929393673800001,
        0.6848407393800001,
        0.6953086713800001,
        0.6934467123800001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.72072106028,
      "stddev": 0.06020493479272523,
      "median": 0.70219337288,
      "user": 0.39082602,
      "system": 1.5935422799999999,
      "min": 0.6734395123800001,
      "max": 0.8773635623800001,
      "times": [
        0.7527925153800001,
        0.7268143423800001,
        0.6734395123800001,
        0.68660686938,
        0.6788138733800001,
        0.8773635623800001,
        0.7089302753800001,
        0.6877977713800001,
        0.6954564703800001,
        0.7191954103800001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.7445834424799997,
      "stddev": 0.07000393850065967,
      "median": 2.7314663023800003,
      "user": 3.3717189199999993,
      "system": 2.3088752799999996,
      "min": 2.66914362738,
      "max": 2.90607168038,
      "times": [
        2.72886031238,
        2.78895669938,
        2.90607168038,
        2.66914362738,
        2.68763269438,
        2.68464838838,
        2.73407229238,
        2.78478080338,
        2.7065529383799998,
        2.75511498838
      ]
    }
  ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(pacquet): collapse leaf-package occurrences in the dependencies tree like pnpm does

2 participants