Skip to content

fix(arborist): skip bundle checks without an applicable override - #10128

Open
jeffwidman wants to merge 1 commit into
npm:latestfrom
jeffwidman:perf/skip-unneeded-bundle-checks
Open

jeffwidman wants to merge 1 commit into
npm:latestfrom
jeffwidman:perf/skip-unneeded-bundle-checks

Conversation

@jeffwidman

Copy link
Copy Markdown
Member

Move the existing no-applicable-override return in Edge.satisfiedBy() ahead of the bundle-membership check.

Why this removes work

Bundle membership determines whether dependency validation should use an overridden spec or the original spec. Without an applicable override, that distinction cannot affect the result, but the current ordering still asks node.inDepBundle, potentially traversing the dependency graph.

Checking the existing cheap condition first avoids that irrelevant traversal. This is a guard reorder, not a new cache, a skipped dependency-validation step, or a public API change. When an override applies, the existing bundle handling and validation logic are unchanged.

The regression test covers both absent override state and an inherited override set with no applicable rule. It checks both satisfying and unsatisfying versions and fails if bundle membership is queried.

Local measurements

I applied only this guard reorder to an isolated npm 11.17.0 installation using Node 24.22.0, then compared it with an unmodified installation. Neither arm includes the separate visited-set optimization.

The workload was a large workspace snapshot with 5,405 original lockfile entries, updating prettier to 3.9.10:

# Lockfile writer, with the manifest updated before invoking npm:
node <npm-installation>/bin/npm-cli.js install prettier@3.9.10 \
  --package-lock-only --ignore-scripts --audit=false --fund=false \
  --timing --force --offline

# Compatibility-check shape, with the original manifest:
node <npm-installation>/bin/npm-cli.js install prettier@3.9.10 \
  --package-lock-only --ignore-scripts --audit=false --fund=false \
  --timing --dry-run=true --offline

Each invocation started from fresh copies of the same inputs and a warm metadata cache, in a four-CPU/12-GB container with networking disabled. One warmup per arm was excluded, followed by three measured alternating pairs. Timed runs used patched source directly, without preload hooks or profiling.

Command Median CPU: original -> patched Median wall: original -> patched
Lockfile writer 9.277 -> 7.359 s (20.7% lower) 8.080 -> 6.326 s (21.7% lower)
Dry-run checker 7.565 -> 5.853 s (22.6% lower) 6.528 -> 4.824 s (26.1% lower)

All package manifests and lockfiles were byte-identical across variants. npm output matched after removing timing information; every invocation exited successfully.

A tiny-project control with one local dependency used seven alternating measured pairs after warmups. CPU was 0.346975 -> 0.348489 s and wall was 0.271516 -> 0.272622 s, both about 0.4% higher, with identical output. Those small differences do not establish a material regression.

These are local native-command measurements, not a claim about every project or complete dependency-update jobs. The large fixture is not included. The synthetic example below independently demonstrates the eliminated work.

The benchmark runtime uses the older bundle/shrinkwrap predicate; this PR uses current upstream's inDepBundle predicate. In both cases the change only moves the no-override guard ahead of the existing predicate.

Validation

On this standalone branch based on latest, using Node 24.22.0:

  • node bin/npm-cli.js test --workspace=@npmcli/arborist --ignore-scripts -- --jobs=4 — passed: 59 test files; 100% statements, branches, functions, and lines.
  • node node_modules/eslint/bin/eslint.js workspaces/arborist/lib/edge.js workspaces/arborist/test/edge.js — passed.
  • Public synthetic example below — passed: 128 valid edges in both variants; recursive bundle-traversal calls reduced from 49,408 to zero.
  • The npm CLI-wide suite was not run.
Public synthetic reproduction

Run from the repository root after installing development dependencies. This counts work, not elapsed time. The graph has no overrides, so bundle membership cannot affect these edge-satisfaction results.

node <<'JS'
const assert = require('node:assert/strict')
const {resolve} = require('node:path')
const Node = require(resolve('workspaces/arborist/lib/node.js'))

const names = Array.from({length: 128}, (_, i) => `provider-${i}`)
const root = new Node({
  path: '/synthetic',
  pkg: {
    name: 'root',
    version: '1.0.0',
    dependencies: Object.fromEntries(names.map(name => [name, '*'])),
  },
})
const shared = new Node({
  parent: root,
  pkg: {name: 'shared', version: '1.0.0'},
})
const providers = names.map(name => new Node({
  parent: root,
  pkg: {name, version: '1.0.0', dependencies: {shared: '*'}},
}))

let bundleTraversalCalls = 0
const getBundler = Node.prototype.getBundler
Node.prototype.getBundler = function (...args) {
  bundleTraversalCalls++
  return getBundler.apply(this, args)
}
try {
  for (const provider of providers) {
    assert.equal(provider.edgesOut.get('shared').satisfiedBy(shared), true)
  }
} finally {
  Node.prototype.getBundler = getBundler
}
console.log({validatedEdges: providers.length, bundleTraversalCalls})
JS

Prepared with assistance from GitHub Copilot. Validation results above were obtained locally.

References

  • Related to npm/cli#10127, but this branch is independent and does not include that change. That PR makes necessary traversals cheaper; this PR avoids traversals whose result is irrelevant. The benchmark savings should not be added together.
  • Retains the override/bundle semantics covered by npm/cli#9235.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

3 participants