Skip to content
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

11% npm install speed increase: add caching to fromUrl #24

Merged
merged 1 commit into from
Jun 26, 2017

Conversation

mikesherov
Copy link
Contributor

@mikesherov mikesherov commented Jun 20, 2017

TL:DR

~11% speed up in the average npm install run

Details

You would not believe how often fromUrl gets hit in the average npm i run. On a fairly large private project that installs 1764 packages, if I track number of cache hits and misses, I get:

Hit rate:

miss 5713
hit  240941

This translates to build times (w package-lock.json but without a node_modules dir, which is common in CI runs) of:

Before:

added 1764 packages in 51.943s
added 1764 packages in 51.135s
added 1764 packages in 50.688s
added 1764 packages in 52.048s
added 1764 packages in 52.156s
added 1764 packages in 54.817s

After

added 1764 packages in 46.1s
added 1764 packages in 47.784s
added 1764 packages in 49.382s

@mikesherov mikesherov changed the title add caching to fromUrl 11% npm install speed increase: add caching to fromUrl Jun 20, 2017
@iarna
Copy link
Contributor

iarna commented Jun 26, 2017

The unbounded nature of this cache makes me a bit nervous, but I'm ok taking this until and if memory profiling points to change.

@iarna iarna merged commit 3e376eb into npm:master Jun 26, 2017
@mikesherov
Copy link
Contributor Author

Thanks, I had a similar concern, but believe it or not, it was only slightly more memory than without the cache, presumably because of lots of memory thrashing due to unbounded object creation in npm proper.

If it ends up being a bigger deal, we can jam lru-cache on it, as you're suggesting.

Thanks for merging!

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