-
Notifications
You must be signed in to change notification settings - Fork 5.6k
perf(npm): load npm resolution snapshot directly from lockfile #28647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1a58769
to
8cd172b
Compare
8cd172b
to
2d42c85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 71 out of 71 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
cli/npm/installer/common/bin_entries.rs:18
- [nitpick] Changing from a borrowed &str key to a String key may incur additional allocation overhead; please verify that this impact is acceptable from a performance standpoint.
seen_names: HashMap<String, &'a NpmPackageId>,
cli/args/lockfile.rs:36
- [nitpick] Consider adding inline documentation for the 'use_lockfile_v5' field to clearly describe when and why the lockfile v5 behavior is enabled.
pub use_lockfile_v5: bool,
cli/main.rs:187
- [nitpick] Verify that converting the npm registry info provider using as_npm_registry_api() produces the correct API expected by the LSP start function.
lsp::start(Arc::new(factory.npm_registry_info_provider()?.as_npm_registry_api())).await
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM to me, but I'll let @dsherret do the final review as he's way more familiar with these areas in Deno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self review
@@ -1159,9 +1199,10 @@ impl CliOptions { | |||
"fmt-component", | |||
"fmt-sql", | |||
"lazy-dynamic-imports", | |||
"lazy-npm-caching", | |||
"npm-lazy-caching", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by fix
Ok(self.workspace_factory()?.npmrc()?) | ||
} | ||
|
||
pub fn resolver_factory(&self) -> Result<&Arc<CliResolverFactory>, AnyError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why i moved these
struct DefaultRegistry; | ||
|
||
#[async_trait::async_trait(?Send)] | ||
impl deno_npm::registry::NpmRegistryApi for DefaultRegistry { | ||
async fn package_info( | ||
&self, | ||
_name: &str, | ||
) -> Result<Arc<NpmPackageInfo>, NpmRegistryPackageInfoLoadError> { | ||
Ok(Arc::new(NpmPackageInfo::default())) | ||
} | ||
} | ||
|
||
fn default_registry() -> Arc<dyn NpmRegistryApi + Send + Sync> { | ||
Arc::new(DefaultRegistry) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is duplicated across like 4 files in lsp tests, should find somewhere shared to put it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This is going to be great.
Fixes #27264. Fixes #28161. Currently the new lockfile version is gated behind an unstable flag (`--unstable-lockfile-v5`) until the next minor release, where it will become the default. The main motivation here is that it improves startup performance when using the global cache or `--node-modules-dir=auto`. In a create-next-app project, running an empty file: ``` ❯ hyperfine --warmup 25 -N --setup "rm -f deno.lock" "deno run --node-modules-dir=auto -A empty.js" "deno-this-pr run --node-modules-dir=auto -A empty.js" "deno-this-pr run --node-modules-dir=auto --unstable-lockfile-v5 empty.js" "deno run --node-modules-dir=manual -A empty.js" "deno-this-pr run --node-modules-dir=manual -A empty.js" Benchmark 1: deno run --node-modules-dir=auto -A empty.js Time (mean ± σ): 247.6 ms ± 1.7 ms [User: 228.7 ms, System: 19.0 ms] Range (min … max): 245.5 ms … 251.5 ms 12 runs Benchmark 2: deno-this-pr run --node-modules-dir=auto -A empty.js Time (mean ± σ): 169.8 ms ± 1.0 ms [User: 152.9 ms, System: 17.9 ms] Range (min … max): 168.9 ms … 172.5 ms 17 runs Benchmark 3: deno-this-pr run --node-modules-dir=auto --unstable-lockfile-v5 empty.js Time (mean ± σ): 16.2 ms ± 0.7 ms [User: 12.3 ms, System: 5.7 ms] Range (min … max): 15.2 ms … 19.2 ms 185 runs Benchmark 4: deno run --node-modules-dir=manual -A empty.js Time (mean ± σ): 16.2 ms ± 0.8 ms [User: 11.6 ms, System: 5.5 ms] Range (min … max): 14.9 ms … 19.7 ms 187 runs Benchmark 5: deno-this-pr run --node-modules-dir=manual -A empty.js Time (mean ± σ): 16.0 ms ± 0.9 ms [User: 12.0 ms, System: 5.5 ms] Range (min … max): 14.8 ms … 22.3 ms 190 runs Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options. Summary deno-this-pr run --node-modules-dir=manual -A empty.js ran 1.01 ± 0.08 times faster than deno run --node-modules-dir=manual -A empty.js 1.01 ± 0.07 times faster than deno-this-pr run --node-modules-dir=auto --unstable-lockfile-v5 empty.js 10.64 ± 0.60 times faster than deno-this-pr run --node-modules-dir=auto -A empty.js 15.51 ± 0.88 times faster than deno run --node-modules-dir=auto -A empty.js ``` When using the new lockfile version, this leads to a 15.5x faster startup time compared to the current deno version. Install times benefit as well, though to a lesser degree. `deno install` on a create-next-app project, with everything cached (just setting up node_modules from scratch): ``` ❯ hyperfine --warmup 5 -N --prepare "rm -rf node_modules" --setup "rm -rf deno.lock" "deno i" "deno-this-pr i" "deno-this-pr i --unstable-lockfile-v5" Benchmark 1: deno i Time (mean ± σ): 464.4 ms ± 8.8 ms [User: 227.7 ms, System: 217.3 ms] Range (min … max): 452.6 ms … 478.3 ms 10 runs Benchmark 2: deno-this-pr i Time (mean ± σ): 368.8 ms ± 22.0 ms [User: 150.8 ms, System: 198.1 ms] Range (min … max): 344.8 ms … 397.6 ms 10 runs Benchmark 3: deno-this-pr i --unstable-lockfile-v5 Time (mean ± σ): 211.9 ms ± 17.1 ms [User: 7.1 ms, System: 177.2 ms] Range (min … max): 191.3 ms … 233.4 ms 10 runs Summary deno-this-pr i --unstable-lockfile-v5 ran 1.74 ± 0.17 times faster than deno-this-pr i 2.19 ± 0.18 times faster than deno i ``` With lockfile v5, a 2.19x faster install time compared to the current deno.
Fixes #27264. Fixes #28161.
Currently the new lockfile version is gated behind an unstable flag (
--unstable-lockfile-v5
) until the next minor release, where it will become the default.The main motivation here is that it improves startup performance when using the global cache or
--node-modules-dir=auto
.In a create-next-app project, running an empty file:
When using the new lockfile version, this leads to a 15.5x faster startup time compared to the current deno version.
Install times benefit as well, though to a lesser degree.
deno install
on a create-next-app project, with everything cached (just setting up node_modules from scratch):With lockfile v5, a 2.19x faster install time compared to the current deno.