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

fix: avoid recreating existing, valid symlinks #53

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stevenpetryk
Copy link

@stevenpetryk stevenpetryk commented Aug 19, 2024

This is a somewhat lazy approach to fixing an issue where pnpm regenerates symlinks, even if the existing symlink is perfectly fine. This behavior (of clobbering valid symlinks) can have adverse downstream effects, like triggering watchers and invalidating caches (the mtime changes).

This is not the ideal solution, because it incurs the cost of a failed readlink if the symlink does not currently exist (which is always the case in a fresh pnpm install, and sometimes the case in a pnpm install that needs to move things around).

But in the happy case—when the node_modules are already mostly correct, at Discord, we saw a 0.2-0.5s speedup in pnpm install. So I think it's worth doing this, and then considering this function more holistically if we find time.

This is a somewhat lazy approach to fixing an issue where pnpm
regenerates symlinks, even if the existing symlink is perfectly fine.
This behavior (of clobbering valid symlinks) can have adverse downstream
effects, like triggering watchers and invalidating caches (the mtime
changes).

This is not the ideal solution, because it incurs the cost of a failed
`readlink` if the symlink does _not_ currently exist (which is always
the case in a fresh pnpm install, and _sometimes_ the case in a pnpm
install that needs to move things around).

But in the happy case—when the node_modules are already mostly correct,
at Discord, we saw a 0.2-0.5s speedup in `pnpm install`. So I think it's
worth doing this, and then considering this function more holistically
if we find time.
@stevenpetryk stevenpetryk marked this pull request as ready for review August 19, 2024 20:01
@zkochan
Copy link
Member

zkochan commented Aug 19, 2024

I am afraid to merge this as it can make install slower in other scenarios. Like with clean install.

@stevenpetryk
Copy link
Author

stevenpetryk commented Aug 20, 2024

I am afraid to merge this as it can make install slower in other scenarios. Like with clean install.

I'll do some benchmarks on Discord's repo, with and without our (equivalent) patch. I'll leave the CAS and virtual store intact in between runs so it just tests symlink creation.

@stevenpetryk
Copy link
Author

stevenpetryk commented Aug 20, 2024

Alright, I did the following:

  • Moved virtual store to /tmp (to make rm -rf'ing node_modules easier)
  • Used the same exact pnpm version with and without the patch
  • Gave each version 5 warmup pnpm installs

Mysteriously, no matter how many times I try, with this patch, a fresh install is marginally faster. It makes me wonder if something else is going on (maybe pnpm tries to double-write some links?)

Without patch:

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.94s user 2.26s system 190% cpu 3.251 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.03s user 2.42s system 198% cpu 3.254 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.03s user 2.50s system 202% cpu 3.216 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.89s user 2.39s system 201% cpu 3.109 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.93s user 2.34s system 199% cpu 3.134 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.03s user 2.57s system 201% cpu 3.269 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.01s user 2.65s system 200% cpu 3.317 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.02s user 2.32s system 192% cpu 3.294 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.98s user 2.39s system 193% cpu 3.292 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  4.01s user 2.39s system 192% cpu 3.331 total

With patch:

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.82s user 1.65s system 181% cpu 3.006 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.88s user 1.65s system 183% cpu 3.012 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.76s user 1.58s system 184% cpu 2.894 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.79s user 1.65s system 178% cpu 3.045 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.87s user 1.65s system 185% cpu 2.969 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.77s user 1.57s system 186% cpu 2.862 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.86s user 1.66s system 184% cpu 2.996 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.92s user 1.77s system 182% cpu 3.114 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.96s user 1.82s system 185% cpu 3.113 total

$ rm -rf **/node_modules && time ./result/bin/pnpm install --silent
./result/bin/pnpm install --silent  3.96s user 1.66s system 183% cpu 3.053 total

@zkochan
Copy link
Member

zkochan commented Aug 21, 2024

We need to measure it on this package.json: https://github.com/pnpm/pnpm.io/blob/main/benchmarks/fixtures/alotta-files/package.json

@zkochan
Copy link
Member

zkochan commented Aug 24, 2024

For me it shows worth results, when testing the change on https://github.com/pnpm/pnpm.io/blob/main/benchmarks/fixtures/alotta-files/package.json

@zkochan
Copy link
Member

zkochan commented Aug 24, 2024

Will this work? #54

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