perf: improve isFileReadable performance

[AriPerkkio]

Description

In #6868 the isFileReadable was optimized to not use try + catch for figuring out if file does not exist. In #10793 this improvement was reverted in favor of filesystem access checks. This PR combines both of these together.

This change has huge impact in performance when large amount of filesystem reads are done. In vitest-dev/vitest#3006 there is a reproduction case linked which runs 2.7s faster with these changes. If you need assistance for setting up the reproduction case let me know.

Before:

 Test Files  5000 passed (5000)
      Tests  5000 passed (5000)
   Start at  13:54:37
   Duration  8.61s (transform 21.58s, setup 9ms, collect 26.79s, tests 4.05s)

The captureLargerStackTrace and handleErrorFromBinding are from fs.accessSync:

After:

 Test Files  5000 passed (5000)
      Tests  5000 passed (5000)
   Start at  14:00:29
   Duration  5.73s (transform 10.47s, setup 9ms, collect 13.36s, tests 2.18s)

Additional context

There are existing tests which nicely cover all requirements:

vite/packages/vite/src/node/__tests__/utils.spec.ts

Lines 244 to 267 in a595b11

	describe('isFileReadable', () => {
	test("file doesn't exist", async () => {
	expect(isFileReadable('/does_not_exist')).toBe(false)
	})
	const testFile = require.resolve(
	'./utils/isFileReadable/permission-test-file',
	)
	test('file with normal permission', async () => {
	expect(isFileReadable(testFile)).toBe(true)
	})
	if (process.platform !== 'win32') {
	test('file with read-only permission', async () => {
	fs.chmodSync(testFile, '400')
	expect(isFileReadable(testFile)).toBe(true)
	})
	test('file without read permission', async () => {
	fs.chmodSync(testFile, '044')
	expect(isFileReadable(testFile)).toBe(false)
	fs.chmodSync(testFile, '644')
	})
	}
	})

The cost of using try+catch for checking file system is well explained here: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-2/

---

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.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

Thanks for digging into Vite core @AriPerkkio! This sounds good to me.

[AriPerkkio]

We just noticed that the isFileReadable seems to be not called that much on latest Vite. The performance metrics were recorded using vite@4.0.0 which was included in the reproduction setup. The isFileReadable is identical in 4.0.0 and latest Vite, but is not used that much anymore.

Anyway, I think it's still worth to merge this PR for possible future usage of isFileReadable.

[patak-dev]

For reference, @bluwy removed isFileReadable from the critical path in

• perf(resolve): improve file existence check #11436

I still think we should merge this PR.

[bluwy]

This seems to optimize towards failed checks more, which checking isFileReadable's usage, doesn't happen a lot, so I'm not sure 🤔

I'm not fully against not merging this though if others prefer to move forward, but I don't think there's a big reason to merge this yet, maybe it could be useful in the future. Thanks for investigating and fixing this though!

[patak-dev]

isFileReadable is used in the static and raw fs middleware, so even if it isn't in the critical path, serveStaticMiddleware comes before user post hooks (middlewares pushed in configureServer). The failing case is way slower than the positive case, so I think this optimization still makes sense. But I agree that we aren't in a hurry to merge it. Let's leave it open for a bit to see what others think.

[patak-dev]

@sapphi-red would you like to break the tie here? I still think we should merge this one, as the cost of a failure is so much higger than the extra check introduced in the happy path.

[sapphi-red]

I'm slightly 👍 with this.