Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: improve isFileReadable performance #12397

Merged
merged 3 commits into from
Mar 30, 2023

Conversation

AriPerkkio
Copy link
Contributor

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:

image

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)

image

Additional context

There are existing tests which nicely cover all requirements:

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
Copy link

stackblitz bot commented Mar 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added performance Performance related enhancement p4-important Violate documented behavior or significantly improves performance (priority) labels Mar 13, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AriPerkkio
Copy link
Contributor Author

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
Copy link
Member

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

I still think we should merge this PR.

@patak-dev patak-dev 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
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly 👍 with this.

@patak-dev patak-dev merged commit acf3a14 into vitejs:main Mar 30, 2023
@AriPerkkio AriPerkkio deleted the perf/is-file-readable branch March 30, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority) performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants