perf: improve isFileReadable performance by AriPerkkio · Pull Request #12397 · vitejs/vite

AriPerkkio · 2023-03-13T12:25:27Z

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.

bolt-new-by-stackblitz · 2023-03-13T12:25:30Z

Run & review this pull request in StackBlitz Codeflow.

packages/vite/src/node/utils.ts

patak-cat

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

AriPerkkio · 2023-03-13T15:11:45Z

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-cat · 2023-03-13T16:33:06Z

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-cat · 2023-03-13T20:50:24Z

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-cat · 2023-03-29T19:49:47Z

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

perf: improve isFileReadable performance

92cbbc3

patak-cat reviewed Mar 13, 2023

View reviewed changes

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved

chore: comment typo

38b90bd

patak-cat reviewed Mar 13, 2023

View reviewed changes

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved

patak-cat added performance Performance related enhancement p4-important Violate documented behavior or significantly improves performance (priority) labels Mar 13, 2023

AriPerkkio mentioned this pull request Mar 13, 2023

perf(reporters): overall improvements vitest-dev/vitest#3006

Merged

2 tasks

chore: simplify condition

c392e25

patak-cat approved these changes Mar 13, 2023

View reviewed changes

patak-cat added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p4-important Violate documented behavior or significantly improves performance (priority) labels Mar 13, 2023

bluwy reviewed Mar 13, 2023

View reviewed changes

patak-cat requested a review from sapphi-red March 29, 2023 19:49

sapphi-red approved these changes Mar 30, 2023

View reviewed changes

patak-cat merged commit acf3a14 into vitejs:main Mar 30, 2023

AriPerkkio deleted the perf/is-file-readable branch March 30, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: improve isFileReadable performance#12397

perf: improve isFileReadable performance#12397
patak-cat merged 3 commits intovitejs:mainfrom
AriPerkkio:perf/is-file-readable

AriPerkkio commented Mar 13, 2023

Uh oh!

bolt-new-by-stackblitz bot commented Mar 13, 2023

Uh oh!

Uh oh!

Uh oh!

patak-cat left a comment

Uh oh!

AriPerkkio commented Mar 13, 2023

Uh oh!

patak-cat commented Mar 13, 2023

Uh oh!

bluwy left a comment

Uh oh!

patak-cat commented Mar 13, 2023

Uh oh!

patak-cat commented Mar 29, 2023

Uh oh!

sapphi-red left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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')
	})
	}
	})

Uh oh!

Conversation

AriPerkkio commented Mar 13, 2023

Description

Before:

After:

Additional context

What is the purpose of this pull request?

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

Uh oh!

bolt-new-by-stackblitz bot commented Mar 13, 2023

Uh oh!

Uh oh!

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

AriPerkkio commented Mar 13, 2023

Uh oh!

patak-cat commented Mar 13, 2023

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Mar 13, 2023

Uh oh!

patak-cat commented Mar 29, 2023

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants