[jeffwidman]

Use a traversal-local Set to track visited nodes in Arborist’s getBundler(), while retaining the existing behavior for explicitly supplied arrays.

Why this should reduce work

The existing path array is a visited collection, not a stack: nodes are appended and never popped. Each includes() searches the growing collection again. A dependency shared by many packages can therefore require substantial repeated scanning even though the traversal already avoids revisiting nodes.

A set expresses the same visited-node membership test without those linear searches. This does not change traversal order, bundle selection, or the dependency graph, and does not introduce a persistent cache or invalidation rules.

Compatibility

The optional traversal argument is exposed as an array in the public TypeScript declarations. Explicit arrays therefore continue through the original array operations, including mutation order, existing duplicates, and already-visited behavior. Only normal calls that omit the argument start with a set.

The added regression test covers those array behaviors and verifies that separate default calls have independent traversal state.

Local measurements

I applied this same method change directly to an isolated npm 11.17.0 installation, using Node 24.22.0. These final measurements do not use a preload hook or a CPU profiler.

For a large workspace snapshot with 5,405 original lockfile entries, I updated prettier to 3.9.10 with:

node <npm-installation>/bin/npm-cli.js install prettier@3.9.10 \
  --package-lock-only --ignore-scripts --audit=false --fund=false \
  --timing --force --offline

Each sample started from the same manifests and lockfile, in a fresh directory, with a warmed metadata cache. After one warmup per variant, three measured pairs alternated execution order.

Median	Original	Patched	Change
Child-process user + system CPU	9.468 s	8.310 s	12.2% lower
Wall time	8.280 s	7.128 s	13.9% lower

Every resulting lockfile was byte-identical across variants.

A tiny-project control with one local dependency used seven measured alternating pairs, excluding warmups:

Median	Original	Patched
CPU	0.360330 s	0.360057 s
Wall time	0.286228 s	0.286556 s

Those control differences are effectively noise. These are local native-command measurements, not a claim about every project or complete dependency-update jobs. The large workspace fixture is not included; the public synthetic example below reproduces the eliminated membership-search work without relying on that fixture.

Validation

On this PR’s upstream checkout, 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/node.js workspaces/arborist/test/node.js — passed.
• Separate generated-graph differential checks — passed: 300 graphs, 15,000 default-call comparisons, and 15,000 explicit-array comparisons, including node relocation and comparison of the populated arrays.
• The npm CLI-wide test suite was not run.

Public synthetic reproduction of the avoided work

Run this from the patched repository root after installing its development dependencies. It constructs 512 packages sharing one dependency and compares the retained array path with the set path.

The counts are logical membership-search work, not a timing benchmark or an estimate of overall speedup.

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

class VisitedArray extends Array {
  lookups = 0
  entriesSearched = 0

  includes (node) {
    const index = super.indexOf(node)
    this.lookups++
    this.entriesSearched += index < 0 ? this.length : index + 1
    return index >= 0
  }
}

class VisitedSet extends Set {
  lookups = 0

  has (node) {
    this.lookups++
    return super.has(node)
  }
}

const names = Array.from({length: 512}, (_, 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'},
})
for (const name of names) {
  new Node({
    parent: root,
    pkg: {name, version: '1.0.0', dependencies: {shared: '*'}},
  })
}

const array = new VisitedArray()
const set = new VisitedSet()
assert.equal(shared.getBundler(array), null)
assert.equal(shared.getBundler(set), null)
assert.equal(array.lookups, 1538)
assert.equal(array.entriesSearched, 133889)
assert.equal(set.lookups, 1538)
console.log({
  arrayLookups: array.lookups,
  arrayEntriesSearched: array.entriesSearched,
  setLookups: set.lookups,
})
JS

The same 1,538 membership checks search 133,889 array entries in total; the set performs 1,538 membership lookups. Both return the same result.

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

References

• Public Arborist TypeScript declarations, which expose getBundler(path?: Node[]).
• No linked issue; this is a standalone performance improvement.

[jeffwidman]

Compatibility question for maintainers: is the optional traversal-state argument to Node#getBundler() supported public API, or internal recursion bookkeeping?

This determines whether we can simplify the patch:

• Arborist exports Node, and DefinitelyTyped declares getBundler(path?: Node[]): Node | null. However, those are third-party declarations; the Arborist README does not document this method or argument. I don’t want to infer an upstream compatibility guarantee from the types alone.
• In the Arborist production callers I checked, only recursive calls pass the argument explicitly; other callers omit it. The collection tracks visited-node membership rather than acting as a stack, so a Set fits its purpose directly.
• This patch conservatively preserves explicit-array behavior, including mutation of the supplied array, while using a Set for default calls. The reported performance gains already include that compatibility path.

If the argument is supported API, dropping array support would break callers passing arrays and should follow the appropriate semver policy. If it is internal, I would prefer removing the compatibility branch and keeping the traversal Set-only. Simply converting incoming arrays to Sets would not preserve the existing array-mutation behavior.

Would you prefer retaining array compatibility for a patch release, or treating the argument as internal and simplifying this PR to Set-only?

I haven’t identified an external caller depending on the array argument; the compatibility handling is precautionary, not evidence of known downstream usage.