perf: extract packages in a worker pool by zkochan · Pull Request #6850 · pnpm/pnpm

zkochan · 2023-07-23T12:02:55Z

previous PR: #6819

zkochan · 2023-08-05T00:30:40Z

@dmichon-msft do you see obvious issues with these changes? These make pnpm slower at least 2 times.

dmichon-msft

I'd need to see a CPU profile to know for sure, but this sounds like thread contention issues.

zkochan · 2023-08-08T00:24:06Z

Thanks for the suggestions. I did some of the suggested changes and now the performance seems to be the same as without the changes.

I still need to do the gunzip changes.

zkochan · 2023-08-08T01:19:25Z

OK, I did sync extraction too and some more benchmarks. It looks like, when the lockfile is up to date (so no resolution needs to happen) these optimizations actually make install about 33% faster. This is a great improvement.

When there is no lockfile, the performance is almost the same because resolution of the lockfile is a bottleneck.

zkochan · 2023-08-10T00:04:16Z

+    workerScriptPath: path.join(__dirname, 'worker/tarballWorker.js'),
+  })
+  // @ts-expect-error
+  global.finishWorkers = () => workerPool.finishAsync()


This is a hack. For now it should be fine as I want to minimize the changes in this PR but in the future we'll have to find a better solution to call this function.

nachoaldamav · 2023-08-13T14:51:04Z

Are this changes expected? (The workspace:* to versions) I tried to pull this PR to test it and every command fails with this error

✘ [ERROR] No matching export in "../../../Users/nacho/Documents/code/pnpm/fetching/tarball-fetcher/src/index.ts" for import "waitForFilesIndex" ../../../Users/nacho/Documents/code/pnpm/env/node.fetcher/src/index.ts:11:31: 11 │ import { createTarballFetcher, waitForFilesIndex } from '@pnpm/tarball-fetcher' ╵ ~~~~~~~~~~~~~~~~~ /Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1649 let error = new Error(`${text}${summary}`); ^ Error: Build failed with 1 error: ../../../Users/nacho/Documents/code/pnpm/env/node.fetcher/src/index.ts:11:31: ERROR: No matching export in "../../../Users/nacho/Documents/code/pnpm/fetching/tarball-fetcher/src/index.ts" for import "waitForFilesIndex" at failureErrorWithLog (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1649:15) at /Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1058:25 at runOnEndCallbacks (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1484:45) at buildResponseToResult (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1056:7) at /Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:1085:16 at responseCallbacks.<computed> (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:703:9) at handleIncomingPacket (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:762:9) at Socket.readFromStdout (/Users/nacho/Documents/code/pnpm/node_modules/.pnpm/esbuild@0.18.17/node_modules/esbuild/lib/main.js:679:7) at Socket.emit (node:events:514:28) at addChunk (node:internal/streams/readable:343:12) { errors: [ { detail: undefined, id: '', location: { column: 31, file: '../../../Users/nacho/Documents/code/pnpm/env/node.fetcher/src/index.ts', length: 17, line: 11, lineText: "import { createTarballFetcher, waitForFilesIndex } from '@pnpm/tarball-fetcher'", namespace: '', suggestion: '' }, notes: [], pluginName: '', text: 'No matching export in "../../../Users/nacho/Documents/code/pnpm/fetching/tarball-fetcher/src/index.ts" for import "waitForFilesIndex"' } ], warnings: [] }

Did you run pnpm install?

Yes, the weird thing is that I can go to the waitForFilesIndex import in Vscode

I don't know why. Try

git clean -fdx pnpm install

Didn't work, I removed my local repo to start from scratch

This didn't work either, I also tried in a codespace and it fails too, same error

But on github actions it works.

This should be fixed by #6938

zkochan · 2023-08-13T22:19:27Z

I will merge these soon but I will not release the changes as stable yet. For now, it will be released under a prerelease version.

ref #6850

gluxon · 2023-08-17T01:57:04Z

Excited for the perf improvements here!

To go along with vercel/next.js#54873 Includes pnpm/pnpm#6850, which improves installation performance.

To go along with vercel/next.js#54873 Includes pnpm/pnpm#6850, which improves installation performance. Closes WEB-1493

zkochan force-pushed the fetch-perf branch from 27e1818 to 1575bd2 Compare July 24, 2023 00:22

zkochan force-pushed the worker-pool branch from f68738b to 866cbb1 Compare July 24, 2023 00:23

zkochan force-pushed the fetch-perf branch from 1575bd2 to db11110 Compare August 4, 2023 22:23

zkochan force-pushed the worker-pool branch from e000e32 to 46a5fad Compare August 4, 2023 22:41

dmichon-msft reviewed Aug 7, 2023

View reviewed changes

Comment thread fetching/tarball-fetcher/src/remoteTarballFetcher.ts Outdated

Comment thread fetching/tarball-fetcher/src/worker/tarballWorker.ts Outdated

Comment thread fetching/tarball-fetcher/src/index.ts Outdated

dmichon-msft reviewed Aug 8, 2023

View reviewed changes

Comment thread store/cafs/src/writeBufferToCafs.ts Outdated

This was referenced Aug 8, 2023

perf: readdir the entire existing CAFS tree before the installer boots up #6921

Open

Write a custom fetcher in Rust #6663

Closed

zkochan force-pushed the fetch-perf branch from db11110 to 07e1f7e Compare August 8, 2023 21:07

zkochan force-pushed the worker-pool branch from 0af89f0 to 362531b Compare August 8, 2023 21:09

Base automatically changed from fetch-perf to main August 8, 2023 22:41

zkochan force-pushed the worker-pool branch from 808e1ab to 1eac178 Compare August 8, 2023 22:56

zkochan changed the title ~~perf: extract package in worker pool~~ perf: extract packages in a worker pool Aug 8, 2023

zkochan commented Aug 10, 2023

View reviewed changes

perf: use a worker pool to extract packages

8345ffb

zkochan force-pushed the worker-pool branch from 4cb34c9 to 8345ffb Compare August 13, 2023 09:37

zkochan marked this pull request as ready for review August 13, 2023 12:14

zkochan requested a review from a team August 13, 2023 13:40

nachoaldamav reviewed Aug 13, 2023

View reviewed changes

zkochan added 2 commits August 14, 2023 01:20

refactor: create a separate package for the worker

5dcc5b8

test: fix

f6b6a3b

zkochan merged commit 083bbf5 into main Aug 13, 2023

zkochan deleted the worker-pool branch August 13, 2023 23:07

zkochan added a commit that referenced this pull request Aug 14, 2023

fix: use the new tarball fetcher for fetching node.js

9ad1d45

ref #6850

zkochan mentioned this pull request Aug 14, 2023

fix: use the new tarball fetcher for fetching node.js #6938

Merged

zkochan added a commit that referenced this pull request Aug 14, 2023

fix: use the new tarball fetcher for fetching node.js (#6938)

58cc58f

ref #6850

zkochan mentioned this pull request Aug 14, 2023

Integrating pacquet to pnpm's existing codebase pnpm/pacquet#78

Open

zkochan mentioned this pull request Aug 29, 2023

v8.6.0 -> v8.6.3 breaks bzip2 tarballs #6761

Open

wbinnssmith added a commit to vercel/turborepo that referenced this pull request Sep 1, 2023

Update pnpm to 8.7.0

c0b599a

To go along with vercel/next.js#54873 Includes pnpm/pnpm#6850, which improves installation performance.

wbinnssmith mentioned this pull request Sep 1, 2023

Update pnpm to 8.7.1 vercel/turborepo#5861

Merged

gluxon mentioned this pull request Sep 4, 2023

fix(server): prevent intermittent hanging when requesting a tarball worker #7041

Merged

wbinnssmith added a commit to vercel/turborepo that referenced this pull request Sep 6, 2023

Update pnpm to 8.7.1 (#5861)

f9116c8

To go along with vercel/next.js#54873 Includes pnpm/pnpm#6850, which improves installation performance. Closes WEB-1493

gzm0 mentioned this pull request Oct 11, 2023

Fix #1301: Include the entire pnpm package in @pnpm//:pnpm aspect-build/rules_js#1306

Merged

Uh oh!

Conversation

zkochan commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan commented Aug 5, 2023

Uh oh!

dmichon-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zkochan commented Aug 8, 2023

Uh oh!

zkochan commented Aug 8, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkochan commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gluxon commented Aug 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zkochan commented Jul 23, 2023 •

edited

Loading

zkochan commented Aug 13, 2023 •

edited

Loading