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

perf(zip)!: don't access the filesystem from wasm #5178

Merged
merged 6 commits into from
Jan 18, 2023
Merged

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Jan 6, 2023

Depends on #5179.

What's the problem this PR addresses?

This PR makes ZipFS read the archive directly inside the constructor from Node and passing a buffer to libzip instead of making libzip read it with a shimmed FS.

Consequently, ZipFS now only accesses the filesystem twice for each archive (first time in the constructor and second time in saveAndClose) and therefore doesn't have to keep an fd handle to the archive while it is open.
This means that now:

  • all "out of file descriptors" errors should finally be gone
  • the only limit to how many archives can be open at the same time is memory.

This could have the consequence of allowing us to increase the fetcher and linker concurrency limits, but I haven't tested it yet.

Fixes #5172.
Fixes #4488.

How did you fix it?

This has the advantage of greatly reducing the bundle size since libzip doesn't have to bundle the FS shims anymore:

Before After Change
Bundle size (bytes) 2800175 2693300 -3.82%
PnP Hook size (bytes) 162041 136565 -15.72%

It also seems to bring performance improvements on yarn install (though earlier I saw some mixed results when benchmarking the time of accessing many archives so if anybody wants to run more benchmarks feel free, but I'd say that it's good enough since I could consistently reproduce the Yarn speedup):

Reflink aren't supported! Installs may be quite slower than necessary
Testing install-full-cold
Benchmark 1: node ./before.cjs install --mode skip-build
  Time (mean ± σ):     20.536 s ±  0.848 s    [User: 41.778 s, System: 2.380 s]
  Range (min … max):   19.821 s … 22.663 s    10 runs
 
Benchmark 2: node ./after.cjs install --mode skip-build
  Time (mean ± σ):     20.170 s ±  0.705 s    [User: 41.661 s, System: 2.380 s]
  Range (min … max):   19.583 s … 22.006 s    10 runs
 
Summary
  'node ./after.cjs install --mode skip-build' ran
    1.02 ± 0.06 times faster than 'node ./before.cjs install --mode skip-build'
Testing install-cache-only
Benchmark 1: node ./before.cjs install --mode skip-build
  Time (mean ± σ):     10.256 s ±  1.375 s    [User: 12.546 s, System: 1.329 s]
  Range (min … max):    9.248 s … 12.956 s    10 runs
 
Benchmark 2: node ./after.cjs install --mode skip-build
  Time (mean ± σ):      9.310 s ±  0.175 s    [User: 12.402 s, System: 1.316 s]
  Range (min … max):    9.078 s …  9.595 s    10 runs
 
Summary
  'node ./after.cjs install --mode skip-build' ran
    1.10 ± 0.15 times faster than 'node ./before.cjs install --mode skip-build'
Testing install-cache-and-lock
Benchmark 1: node ./before.cjs install --mode skip-build
  Time (mean ± σ):      2.239 s ±  0.015 s    [User: 3.452 s, System: 0.514 s]
  Range (min … max):    2.206 s …  2.257 s    10 runs
 
Benchmark 2: node ./after.cjs install --mode skip-build
  Time (mean ± σ):      2.112 s ±  0.043 s    [User: 3.325 s, System: 0.485 s]
  Range (min … max):    2.027 s …  2.169 s    10 runs
 
Summary
  'node ./after.cjs install --mode skip-build' ran
    1.06 ± 0.02 times faster than 'node ./before.cjs install --mode skip-build'
Testing install-ready
Benchmark 1: node ./before.cjs add dummy-pkg@link:./dummy-pkg --mode skip-build
  Time (mean ± σ):     942.9 ms ±   7.2 ms    [User: 1253.3 ms, System: 100.4 ms]
  Range (min … max):   931.6 ms … 953.5 ms    10 runs
 
Benchmark 2: node ./after.cjs add dummy-pkg@link:./dummy-pkg --mode skip-build
  Time (mean ± σ):     823.4 ms ±  16.4 ms    [User: 1145.1 ms, System: 83.3 ms]
  Range (min … max):   802.9 ms … 861.7 ms    10 runs
 
Summary
  'node ./after.cjs add dummy-pkg@link:./dummy-pkg --mode skip-build' ran
    1.15 ± 0.02 times faster than 'node ./before.cjs add dummy-pkg@link:./dummy-pkg --mode skip-build'

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz self-requested a review January 6, 2023 07:43
@arcanis
Copy link
Member

arcanis commented Jan 6, 2023

That seems reasonable. It's also what #5145 does.

.yarn/versions/0cd7b07a.yml Outdated Show resolved Hide resolved
@arcanis arcanis merged commit 1431217 into master Jan 18, 2023
@arcanis arcanis deleted the paul/perf/zip-buffer branch January 18, 2023 15:07
@belgattitude
Copy link

belgattitude commented Jan 21, 2023

@paul-soporan I sometimes bench pnpm and yarn... Lately I saw a perf boost somewhere from yarn 4.0.0-rc.35 and 36 with a monorepo (nextjs, vite, esbuild, vitest...)

Not sure if it's linked but wanted to let you know.

The benchs:

Note that it's the first time the install was globally equal than pnpm (YARN_COMPRESSION_LEVEL=0, node_modules, supportedArch current and dedupe)... pnpm does download more (peer-deps are handled differently and no support for supportedArch afaik - so the comparison in not totally right)

Speedup appreciated ♻️🌳

@OmarJaroudi
Copy link

Is this change available in yarn 3.x?

@merceyz
Copy link
Member

merceyz commented Feb 3, 2023

No, this PR is a breaking change so it can't be backported.

@MarcCelani-at
Copy link

@merceyz I only read the summary here, didn't dig into the details, but can you help me understand why we can't backport this? We really need this in our monorepo :) IDK if there is a timeline for yarn 4 or not...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants