perf(resolve): improve file existence check

[bluwy]

Description

7x perf improvement for tryFileResolve file existence check (NOTE: through testing this does not translate to faster preceivable performance)

One of the drawbacks of fs.accessSync to check file readable is that it errors if the file doesn't exist. node takes time to generate the errors that we don't use. This PR calls statSync and check readability ourselves, skipping any error generation.

Additional context

Thanks @ArnaudBarre for providing the flamegraph of initial server run.

Here's a gist comparing different ways to check file existence. fs.existsSync is the fastest but it doesn't check for readability, using this PR the perf numbers are actually still quite close to fs.existsSync.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

Hmm this fix isn't actually robust as we're checking owner file permissions only. I can't seem to find how nodejs checks if the current process has permission for the file. This StackOverflow answer sums up the issue. (EDIT: looks like the checks are done natively)

[ArnaudBarre]

Should we really take care of checking read permissions?
Just making this up but if .mts exist but not readable for local user because of manip and .ts also exist.
Then locally the server will use .ts and the CI will use .mts
I would just return the first path that exist that the node/esm algorithm found and then if fails later is it can be loaded.

[patak-dev]

@ArnaudBarre that is a good point, and I wonder why we didn't get a report yet for that. Maybe it is because it is extremely hard to debug. @bluwy I think that if we don't have read permission, then failing with a clear error is a better way to go. Just to be safe, maybe we should merge this PR for 4.1.

[sapphi-red]

If not generating errors improves performance, maybe we can consider using this unsafeAccessSync function?

import pathModule from 'path'
const binding = process.binding('fs')

function unsafeAccessSync(path, mode) {
  const ctx = { path };
  binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx);
  return ctx.errno === undefined && ctx.error === undefined
}

This function uses the internal binding and won't be guaranteed to work in every version. So we should run a test on every major version. But if it improves the performance significantly, I guess it's worth to do that.

Also we can use process.binding('fs').internalModuleStat that returns whether the path is a file or directory or doesn't exist. Node.js uses this internally when resolving.
https://github.com/nodejs/node/blob/7738844fe357a4b918f8063b4ee97dc205eae4c8/src/node_file.cc#L1083-L1101
https://github.com/nodejs/node/blob/7738844fe357a4b918f8063b4ee97dc205eae4c8/lib/internal/modules/cjs/loader.js#L160

[bluwy]

If not generating errors improves performance, maybe we can consider using this unsafeAccessSync function?

I didn't know you can access internals like that 😲 I think it would be safer to avoid that for now, as Arnaud brought up a good point that we probably don't need to check read permissions when resolving. For other cases, we may still need it for correctness, but I checked that they're not hot paths so it may be fine.

I'll make another commit to simplify the change and keep access.

[davidwallacejackson]

Tried this out on a large app, and unfortunately I didn't see a significant difference -- the runs I've posted show a small improvement, but I retried a few times and in some cases this PR was actually slower by a couple seconds. I don't see any way this change could have actually made things slower (I assume it was just natural variation on my own machine), but I'm definitely not seeing a consistent improvement:

Using Vite 4.0.2

initial load

reload

With this PR

initial load

reload

I've attached the profiles for each load as well, in case they're helpful.

[bluwy]

Thanks for testing @davidwallacejackson! I checked the cpuprofiles and I can't quite tell what to improve as both seem to gave a different timeline. I can see lesser calls to fs.access though which is a plus. With the initial_load_without_pr.cpuprofile, I can see esbuild taking most of the time though

[ArnaudBarre]

Didn't check myself but I think having esbuild as bottleneck seems more expected than fs.access. And probably can make bigger difference on Windows where FS is often slower.

[bluwy]

Yeah I think we need to investigate further on the esbuild part, but besides that I think this PR is good to go for now for 4.1.

[MoustaphaDev]

I don't know vite a lot but this not being in a try catch could be an issue as it would throw an error for virtual modules
See this error an Astro user encountered, seems like caused by this change