perf: async fs calls in resolve and package handling

[patak-dev]

Description

getRealPath in resolve is taking ~8% of the workload in https://github.com/yyx990803/vite-dev-server-perf, and it is calling fs.realPathSync.native. After this PR, it takes ~2% by using fsp.realPath (equivalent to the .native sync version).

I tested the speed improvement using the test in the repo instead of the cpuprofile, but it isn't stable on my machine. I think that even if we now have a ton of promises being generated, the non-blocking fs calls should still make this change worth it. We need to test in some other benchmarks.

The PR also changes a fs.readFileSync in package.ts to be async, and that triggered a whole bunch of async/await changes in the codebase too.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

I don't know why Windows isn't happy (at least in CI) when using fsp.realPath. Using fs.realPathSync.native works, so for now we are back to sync for windows.

This makes this PR a bit more controversial, because it will mean a small regression on Windows due to the extra promises being generated. But a lot of them are needed due to the fsp.readFile change, and I ended up reducing the number of created promises at refactor: avoid creating promises in tryCleanFsResolve. So I think this may still be a good step.

If someone with more Windows knowledge finds a way to make fsp.realPath work, appreciated! But now we can also do that in another PR.

[patak-dev]

@userquin tested in his Windows machine, and d3c8f90 is working for him with the same node version as CI. @sapphi-red, maybe you could try too when you have some time? Even if it works, I don't think we can just fallback to sync for CI 🤔

[userquin]

It works on first run after build (pnpm test-serve), second run fails with same CI errors (node 20.10.0 LTS):

[sapphi-red]

I found a bug in Node (nodejs/node#51031) and maybe that's the culprit. I pushed a commit to use fs.realpath.native instead of fsp.realpath.

[patak-dev]

I found a bug in Node (nodejs/node#51031) and maybe that's the culprit. I pushed a commit to use fs.realpath.native instead of fsp.realpath.

Woah! Amazing find @sapphi-red 👏🏼 ❤️

I'm curious to see if this PR improves the benchmarks on your machine. I don't know why there is so much variance when I measure on mine.

[sapphi-red]

Here's the results of https://github.com/yyx990803/vite-dev-server-perf on my Windows machine. It's seems on a par.

before

1000 TS modules (50x20) loaded in: 1049ms (runs: [1274,1040,1060,1021,1049])
1000 TS modules (20x50) loaded in: 965ms (runs: [1008,934,946,966,965])
2000 TS modules (100x20) loaded in: 2237ms (runs: [2355,2252,2226,2237,2213])
5000 TS modules (250x20) loaded in: 5960ms (runs: [6592,6022,5874,5915,5960])
10000 TS modules (400x25) loaded in: 12265ms (runs: [13409,12106,12134,12265,12337])

after

1000 TS modules (50x20) loaded in: 1069ms (runs: [1257,1031,1051,1069,1071])
1000 TS modules (20x50) loaded in: 956ms (runs: [1033,956,971,941,946])
2000 TS modules (100x20) loaded in: 2249ms (runs: [2358,2249,2228,2191,2268])
5000 TS modules (250x20) loaded in: 5972ms (runs: [6610,5983,5936,5972,5927])
10000 TS modules (400x25) loaded in: 12152ms (runs: [13560,11932,11866,12162,12152])

I'm not sure why the "before" run is faster than the one in #15195 (comment) though 🤔 .

[bluwy]

Is there a reason we're changing the pattern for when the real path is resolved? I kinda prefer the old way as the function would return the finalized resolved path directly, but maybe I'm missing an optimization made here.

[patak-dev]

See 0faf06c, doing it in this way avoids creating many promises in this function.

But I don't think we will be doing it this way if we merge #15279. I'm unsure whether generating all the promises is justified. readdirSync takes almost no time compared to other things, and we could expand the tree in parallel (but keeping get/set sync). There are some fsp.readFile in this PR though. I think we need to reevaluate and maybe break this one in pieces after #15279

[bluwy]

I think the idea of this PR is fine to merge too even if there's no surface improvement. But IIRC what dominik discovered for tsconfck was that async within graph is better for large fs, sync within graph is better for small fs.

See 0faf06c, doing it in this way avoids creating many promises in this function.

I don't think we should optimize the code to avoid creating promises, it should work fine like before as long as it's scheduled 🤔

[patak-dev]

Why won't we try to avoid creating 10+ promises for each tryCleanFsResolve 🤔
Doing an await for them in a for loop isn't free either. We have merged PRs to move from a .startsWith('/') to [0] === '/'. I would imagine this has a way bigger effect, more thinking on the garbage collector, etc. But I think we will revert this change if we want to merge this one.

About populating the fs tree cache async, ya, we need to review what works better.

[bluwy]

Hmm, I can't argue that no await is definitely faster that some awaits 😄 I don't mind as much now looking more at it, but initially I didn't think it was worth the perf. startsWith was different that it was an easy change, otherwise I would keep startsWith for readability.