-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Conversation
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.
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. |
Alright, I did the following:
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 |
We need to measure it on this package.json: https://github.com/pnpm/pnpm.io/blob/main/benchmarks/fixtures/alotta-files/package.json |
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 |
Will this work? #54 |
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.