Improve pnp loader speed and memory: jszip implementation

[goloveychuk]

What's the problem this PR addresses?

rework of #6671
TODO:

• check security?
• tests
• elaborate mixed compression logic
• benchmarks?
• hide exports and interfaces for external consumers?

...

How did you fix it?

...

Checklist

• I have read the Contributing Guide.

• 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.

[goloveychuk]

@arcanis review pls.
Todo:

1. have question about one test libzip/incons-file-count-overflow.zip
2. need benchmark?
3. mixed compression logic should be checked

[goloveychuk]

@merceyz @RDIL maybe you want to review?

[goloveychuk]

@arcanis if you want I can move jszip implementation into my repo, and add as external dependency from berry.

[arcanis]

Since the main benefit of this PR is to decrease the runtime footprint (by comparison the install doesn't have a lot to gain, since only unplugged packages are extracted on the disk and there's very little of them), I'd rather keep the existing codepath for unplug (to simplify the changes as much as possible).

[arcanis]

Suggested change

	experimentalZipImplementation: {
	pnpZipImplementation: {

[arcanis]

We use 456789000 (see SAFE_TIME)

[arcanis]

nit: Can you move it to a ZipImplementation.ts file?

[arcanis]

Hey @goloveychuk! No it's fine, I prefer to have it inside this repository at least as long as it's experimental; I still have to review this PR, it's pretty large (even discounting the test artifacts) and I've been extremely busy at work since the beginning of the year. I'll try to finish the review by next week.

[goloveychuk]

Thank you.
Tests could be rewritten from snapshot file to inline snapshots or code which defines expected files list.

[goloveychuk]

hey, @arcanis gentle reminder. Thanks!

[arcanis]

I think the implementation looks good. I didn't see any perf difference when running the basic tests, but they are very very simple so it's not surprising. A better way to benchmark would be to use our yarn bench command (see scripts/bench-folder.ts), but it doesn't support using a different benchmark command between tests at the moment.

[goloveychuk]

@arcanis I also suggest to rethink caching
https://github.com/goloveychuk/berry/blob/fb552204a46f364d75d08117e88f044f063c3703/packages/yarnpkg-libzip/sources/ZipFS.ts#L860
In many cases it leads to high memory consumption without benefits.
Usually js files are cached by infra:

• commonjs is caching scripts in require.cache
• esm loaders are caching compiled and linked module
• webpack jest are caching transformed modules

It's a rear case when some infra will read same file in one pnp runtime.
And benefits are not clear.
Especially with js zip impl, which reads file from filesystem without additional operations like loading full archive into memory. Which should be same speed as node_modules read from fs.
Exception for above is compressed archives. But again, I'm not sure any infra will read same file twice.

Safest solution is to hide caching under specific Impl class, to let them decide what is best strategy for caching.
Wdyt?

[goloveychuk]

I can do it in a new PR

[arcanis]

It's a rear case when some infra will read same file in one pnp runtime.
And benefits are not clear.

The libzip doesn't allow (or didn't allow, perhaps they fixed it?) to read files that just got written (ie writeFileSync(p); readFileSync(p); was crashing). The file caching was to workaround this issue. It's fine to get rid of it in the js backend 👍

[goloveychuk]

@arcanis made changes in last commit

[goloveychuk]

Anything to fix to merge pr? Thanks.

[goloveychuk]

Thanks for merge!
Could you please create release?

[adrian-gierakowski]

is this the default now, or do I need to somehow opt-in? (release notes say "Experimental JS-based zip parser")

[goloveychuk]

'pnpZipBackend: js'
In yarnrc. Use latest yarn version, it has fds leak fix.

[neoncube2]

Since this is experimental, I figured I'd let you know that I'm currently using pnpZipBackend: js on my project, and it seems to work fine :)

[goloveychuk]

Speed, memory improvements?

[arcanis]

I personally didn't see any change in our large project at work when running TypeScript - neither in perfs nor in memory 😔

[adrian-gierakowski]

No noticeable memory usage reduction here either

[neoncube2]

I profiled my project on Windows, using procmon, when running the command yarn run webpack serve --mode=development.

• Packages: 797
• Size of .yarn/cache/: 35MB
• Max memory usage with default zip backend, after discarding outliers: ~580KB
• Max memory usage with JS zip backend, after discarding outliers: ~560KB

It does look like the JS zip backend might be saving a bit of memory, especially since I'm guessing that most of the 560KB/580KB is taken up by webpack, etc.

Interestingly, the JS zip backend also seems to be more consistent, always staying around 560KB, while the default zip backend occasionally spikes all the way up to ~750KB (I observed this several times while profiling, but didn't include these outliers in the results).