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: in-memory public files check #15195

Merged
merged 12 commits into from Dec 5, 2023
Merged

Conversation

patak-dev
Copy link
Member

Description

Around 4% of the time when running https://github.com/yyx990803/vite-dev-server-perf is spent on the serve public middleware.

This middleware will check every request before getting into the transform pipeline. Internally, it uses sirv, which does a fs.existsSync check for both filePath and filePath + '/index'. It seems that we can't configure it to avoid the /index suffix check, which I don't think we should be serving from the public dir. Even if this is fixed, there will still be a toll for every request because of the public dir feature.

This PR reads the public dir recursively on server start and keeps an in-memory set of all public files so we can avoid going through sirv.

The drawback is that a server restart is needed if new files are added to the public directory. If we want to move forward with this PR, we could decide to watch the public dir and add new files.

We are currently patching sirv to add a shouldServe option, so we can do a case check in case-insensitive platforms. If we merge this PR, we can also remove this patch and the check because the in-memory map will already filter these out.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Nov 30, 2023

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

@patak-dev patak-dev added the performance Performance related enhancement label Nov 30, 2023
@patak-dev
Copy link
Member Author

@ArnaudBarre is telling me he implemented the same (+the watcher) in rds: https://github.com/ArnaudBarre/rds/blob/main/src/server/public.ts, we should check rds more often :)

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Nov 30, 2023

For ideas only, most of my implementations where never tested on Windows 👀
But that's from there I also bring the idea of not using one big file watcher, having used multiple dedicated watcher (public, config, src) worked well and allowed to keep things independent (even if the entry point is a complicated merge of all, it really misses the plugin architecture of Vite)

@patak-dev patak-dev marked this pull request as draft November 30, 2023 13:36
@patak-dev
Copy link
Member Author

The watcher is now used to delete/add publicFiles to the in-memory cache, and the cache is also used in checkPublicFile when available (in dev, but later on we could add it to build time too if we'd like).

The publicFiles don't have the publicDir prefixed in the set, so we avoid path.join calls to do the checks.

@patak-dev patak-dev marked this pull request as ready for review November 30, 2023 20:59
@patak-dev
Copy link
Member Author

We can't remove the shouldServe patch because it is used during preview too (at least in this PR, but it is removed now from the serve public middleware)

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Nov 30, 2023
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 4bb5f14: Open

suite result latest scheduled
analogjs success success
astro success success
histoire failure failure
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas success success
sveltekit failure failure
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

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.

Cool!

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
@sapphi-red
Copy link
Member

Here's the results of https://github.com/yyx990803/vite-dev-server-perf on my Windows machine. It's around 3%.

before

1000 TS modules (50x20) loaded in: 1167ms (runs: [1387,1181,1167,1166,1160])
1000 TS modules (20x50) loaded in: 1086ms (runs: [1161,1086,1078,1082,1099])
2000 TS modules (100x20) loaded in: 2530ms (runs: [2632,2530,2523,2504,2543])
5000 TS modules (250x20) loaded in: 6675ms (runs: [7346,6790,6630,6598,6675])
10000 TS modules (400x25) loaded in: 13616ms (runs: [15038,13693,13616,13447,13249])

after

1000 TS modules (50x20) loaded in: 1150ms (runs: [1521,1162,1131,1132,1150])
1000 TS modules (20x50) loaded in: 1025ms (runs: [1069,1025,1058,997,1013])
2000 TS modules (100x20) loaded in: 2424ms (runs: [2562,2426,2424,2403,2400])
5000 TS modules (250x20) loaded in: 6524ms (runs: [6957,6547,6524,6462,6517])
10000 TS modules (400x25) loaded in: 13286ms (runs: [14343,13192,13206,13290,13286])

patak-dev and others added 2 commits December 2, 2023 20:55
Co-authored-by: 翠 / green <green@sapphi.red>
@patak-dev
Copy link
Member Author

Here's the results of yyx990803/vite-dev-server-perf on my Windows machine. It's around 3%.

Thanks for benchmarking! That isn't bad 🙌🏼

sapphi-red
sapphi-red previously approved these changes Dec 3, 2023
@patak-dev
Copy link
Member Author

Should we wait for 5.1 to merge this one? I think it is fine to release it in a patch.

@sapphi-red
Copy link
Member

It depends on when we start 5.1 beta for me. If we are starting in a week, then I'd say let's wait for that.

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.

I think we can cut this as a patch since it shouldn't work too differently that before to be noticable. Nice perf fix!

@patak-dev
Copy link
Member Author

It depends on when we start 5.1 beta for me. If we are starting in a week, then I'd say let's wait for that.

I don't think we have enough minor-only PRs yet to justify starting work on 5.1, so let's go with this one in a patch then.

@patak-dev patak-dev enabled auto-merge (squash) December 5, 2023 13:30
@patak-dev patak-dev merged commit 0f9e1bf into main Dec 5, 2023
15 checks passed
@patak-dev patak-dev deleted the perf/in-memory-public-files-check branch December 5, 2023 13:34
@cpojer
Copy link
Contributor

cpojer commented Dec 7, 2023

The drawback is that a server restart is needed if new files are added to the public directory. If we want to move forward with this PR, we could decide to watch the public dir and add new files.

This seems like a user-hostile change that feels like a regression given that it comes as a patch release. It does not match expectations that people have about Vite, considering that it doesn't work like that for source code. If you are sticking with this approach, how about integrating the behavior of vite-plugin-restart for the public/ folder?

@patak-dev
Copy link
Member Author

@cpojer I implemented watching of public files before this PR was merged, see #15195 (comment)

Are you seeing a regression? The public dir should work in the same way as before.

We're probably going to remove this approach once we merge #15279, as we can use that same cache for the checks (that is more performant when there are symlinks)

@cpojer
Copy link
Contributor

cpojer commented Dec 7, 2023

Oh fantastic, sorry I missed that change was part of this PR already 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants