Skip to content

[Perf] nvm_resolve_local_alias: skip nvm_version for an exact, installed version - #3945

Closed
hridhinvarkeyjoseph wants to merge 1 commit into
nvm-sh:masterfrom
hridhinvarkeyjoseph:perf/resolve-local-alias-exact
Closed

hridhinvarkeyjoseph wants to merge 1 commit into
nvm-sh:masterfrom
hridhinvarkeyjoseph:perf/resolve-local-alias-exact

Conversation

@hridhinvarkeyjoseph

@hridhinvarkeyjoseph hridhinvarkeyjoseph commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Part of #3416 (the second cost described in #3416 (comment)).

When an alias resolves to an exact vX.Y.Z that is already installed, nvm_resolve_local_alias still passes it through nvm_version, which can't change the result. This returns it directly after an nvm_is_version_installed check. Anything else (partial versions, node, lts/*, iojs, not-installed versions) still goes through nvm_version as before.

In the fast test fixtures this took nvm_resolve_local_alias on an exact alias from ~63ms to ~22ms per call.

This does not address the bigger cost in the original report, where default -> node has to resolve the node/stable implicit aliases. Happy to look at that separately, or to fold this into #3937 if you'd prefer.

Adds a test that mocks nvm_version to confirm it's skipped for an exact installed version and still used for one that isn't installed.

@hridhinvarkeyjoseph

Copy link
Copy Markdown
Contributor Author

@ljharb I've also cherry-picked this commit onto #3937, since both are the same exact-version fast path on the startup route. Would you prefer to review them together there (and I'll close this one), or keep this as a separate PR?

The 3 failing checks look unrelated to the change: the two WSL nvm install jobs timed out after 45m, and xenial (bash) failed after 1m. Happy to look into any of them if they're not known flakes.

@hridhinvarkeyjoseph

Copy link
Copy Markdown
Contributor Author

Closing in favour of #3937, which now includes this commit, so there's one PR for this startup fast path instead of two overlapping ones.

@ljharb

ljharb commented Oct 5, 2026

Copy link
Copy Markdown
Member

In the future, please don't ever close a PR you've opened; PR refs are permanent and can never be removed.

@hridhinvarkeyjoseph

Copy link
Copy Markdown
Contributor Author

@ljharb Understood, sorry about that. I've reopened this.

Since #3937 also has this commit cherry-picked, which would you prefer?

@hridhinvarkeyjoseph

Copy link
Copy Markdown
Contributor Author

Folded into #3937.

@hridhinvarkeyjoseph

Copy link
Copy Markdown
Contributor Author

Sorry, I closed this by mistake despite your earlier note — reopened. Its commit is already in #3937 (now approved, with your tweak), so please close this one whenever it suits you.

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.

2 participants