perf: cached fs utils #15279

patak-dev · 2023-12-07T11:22:13Z

Description

Note

To enable this optimization in vite@5.0.8+ set in your vite config:

  server: {
    fs: {
      cachedChecks: true
    }
  }

Or toggle the VITE_SERVER_FS_CACHED_CHECKS env variable.

Tip

To see the difference in resolveId, you can run the dev server using --profile, then press > P and Enter when the loading is done. A cpuprofile will be created. You can upload it and share it here if you think it would provide valuable feedback to decide if we should enable this feature by default

Implements a cached strategy for fs checks when the experimental option server.fs.cachedChecks: true is set.

I tested it with https://github.com/yyx990803/vite-dev-server-perf and resolveId times are cut in half. Also tested with the turbopack triangle benchmark using https://github.com/sapphi-red/performance-compare with the same results on my M1 pro.

An example profile diff for tryResolveRealFileWithExtensions (65ms -> 19ms)

Base: 65ms

Cached: 19ms

Context

We received a CPU profile for server start on a large app that takes a lot of time on Windows old machines, and 40% of the server time was spent resolving ids. In particular in fs.realPathSync.native and tryResolveRealFileWithExtensions.

We heavily optimized resolving during Vite 4.3 (see blog post with explanation). We have tried removing realpath calls at least for some cases before, for example, in the closed #12818.

We tried to avoid caching fs checks so far, but we changed that for the public dir in:

perf: in-memory public files check #15195

Details

This PR implements a cache for watched files in root (not symlinked). We construct the tree lazily using readdir and withFileTypes: true so for each dirent we know if it is a directory, file, or symlink (could be either). We expand all the dirents in the path on each fs check except for the symlinks. So apart from avoiding fs.existsSync, fs.statSync, etc, we can also avoid calls to realpath for each path we cached in the tree because we know there aren't any symlinks in their path.

The cached checks only run during dev because we need a watcher to add/unlink files and directories. For now, the optimization is disabled if watch is null or if there is a custom watch.ignore. We can later relax this restriction.

We discussed with @sheremet-va, and for Vitest to be able to use this optimization, we need to compose all the cached fs trees into a single one so several servers can share the cache of overlapping roots (important for monorepos). We can work on this in a future PR.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

stackblitz · 2023-12-07T11:22:15Z

Run & review this pull request in StackBlitz Codeflow.

patak-dev · 2023-12-07T15:48:33Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-12-07T16:05:27Z

📝 Ran ecosystem CI on 524a52a: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	✅ success	✅ success
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	✅ success	✅ success
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	❌ failure	✅ success
vitest	✅ success	✅ success

patak-dev · 2023-12-07T17:22:34Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-12-07T17:39:51Z

📝 Ran ecosystem CI on c929f96: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	✅ success	✅ success
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	✅ success	✅ success
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

patak-dev · 2023-12-07T21:52:38Z

Watching the addition of directories and files is more performant now after a343c66, we don't drop the parent dir dirents and instead add the entry as directory_maybe_symlink or file_maybe_symlink and we resolve it with a lstat lazily if it is ever accessed. We also need this if we want to share the dirents cache for different ResolvedConfigs.

patak-dev · 2023-12-09T08:08:05Z

Some details about the updates:

In feat: optimize tryResolveRealFileWithExtensions we avoid many normalizePath and search in the same cached fs tree calls. We could go further and optimize like this the whole tryFsResolve. We can try that in another PR, I think we need better abstractions around handling the DirentCache tree first.
In feat: avoid double normalizePath calls we avoid a few extra normalizePath calls. I saw in profiles that this is starting to take most of the time. If we abstract tryFsResolve we could do a single normalizePath for the whole tree. One issue I see is that I don't know if the fallback fs calls are slower when you pass them normalized paths so we should have both the fs native and the normalized path around? Something else to explore later.
There were fails at one point that I thought were related to Vitest having a custom watch.ignore. I pushed chore: remove watch null and custom ignored guard, and I no longer see these issues. I think we should merge the PR without this guard so more projects can test it out. We still need to review how watch.ignore and watch: null should affect the fs cache
In chore: remove async wip code, I remove some code that was initially intended to let us do async readdir calls. For now, the fs cache is sync and I don't know if we should change that. The readdirSync time is very low compared to everything else. I moved everything to async calls here perf: async fs calls in resolve and package handling #15211 and we didn't see a definite gain either. Maybe a better approach is to keep the tree sync only but auto-expand it in the background doing async readdir calls and assigning the result in sync.

There is also now a version sharing the fs trees, to improve Vitest in monorepo setups. I think we could review and merge that one later:

perf: cached fs utils with shared trees #15294

patak-dev · 2023-12-09T08:09:03Z

@sapphi-red tested in his Windows machine:

Results with performance-compare on my Windows laptop. It's a 5% improvement

Before: 4202.2ms (babel), 2666.0ms (swc)
After: 4046.8ms (babel), 2491.2ms (swc)

The results with https://github.com/yyx990803/vite-dev-server-perf.
Before
1000 TS modules (50x20) loaded in: 1183ms (runs: [1254,1094,1183,1185,1152])
1000 TS modules (20x50) loaded in: 1054ms (runs: [1126,1075,1052,1035,1054])
2000 TS modules (100x20) loaded in: 2556ms (runs: [2206,2556,2529,2559,2612])
5000 TS modules (250x20) loaded in: 6398ms (runs: [6491,6396,6473,6398,6190])
10000 TS modules (400x25) loaded in: 12524ms (runs: [12954,13004,12437,12437,12524])

After
1000 TS modules (50x20) loaded in: 1067ms (runs: [1130,1020,1012,1076,1067])
1000 TS modules (20x50) loaded in: 1025ms (runs: [1017,1054,1019,1025,1081])
2000 TS modules (100x20) loaded in: 2280ms (runs: [2220,2308,2280,2198,2313])
5000 TS modules (250x20) loaded in: 6253ms (runs: [6282,6253,6023,6131,6267])
10000 TS modules (400x25) loaded in: 12132ms (runs: [12365,12132,12003,11987,12477])

bluwy

I think my main concern so far is the graph fs eagerness, and that we're still doing a bit of cache-check work for existsSync (and others), that maybe it isn't as bad is we swap it with a isPathWatched check instead?

I don't think they're blockers to get this experiment out for testing, but hoping we don't have to change to much if we decide to go with a different approach.

About #15294, I'm not really comfortable with doing that and I think we should avoid it 😅 Feels a bit going too far.

packages/vite/src/node/server/index.ts

packages/vite/src/node/fsUtils.ts

bluwy · 2023-12-09T14:19:28Z

packages/vite/src/node/fsUtils.ts

+      // cached fsUtils is only used in the dev server for now, and only when the watcher isn't configured
+      // we can support custom ignored patterns later
+      fsUtils = commonFsUtils
+    } /* TODO: Enabling for testing, we need to review if this guard is needed


I'd be fine uncommenting this to add a guard, but if we do stablize the feature, I think we should figure out getting ignored to work.

If we do support ignored, the flow could maybe be more direct, in that we can cache the fs function calls directly?

I think we may not need this guard at the end. If the user is ignoring a certain folder or using watch: null, I think we could say that he needs to restart the server when adding or removing files.

Hmm thinking about it, I don't think we invalidate the module graph for files that aren't watched, so a reload wouldn't be useful either. So maybe your idea is fine 🤔

patak-dev · 2023-12-09T14:58:12Z

I think my main concern so far is the graph fs eagerness, and that we're still doing a bit of cache-check work for existsSync (and others), that maybe it isn't as bad is we swap it with a isPathWatched check instead?

Could you expand on the isPathWatched idea? About the graph fs eagerness, see #15279 (comment)

About #15294, I'm not really comfortable with doing that and I think we should avoid it 😅 Feels a bit going too far.

I think it will depend on the results that @sheremet-va sees with Vitest. If it speeds up things a lot, I don't think the extra complexity is that bad.

patak-dev · 2023-12-09T15:02:11Z

There were fails at one point that I thought were related to Vitest having a custom watch.ignore. I pushed chore: remove watch null and custom ignored guard, and I no longer see these issues. I think we should merge the PR without this guard so more projects can test it out. We still need to review how watch.ignore and watch: null should affect the fs cache

Oh... I accidentally enabled the flag by default in a98c9b4#diff-abb3345b6e3b2ec6d297c2ebc54cca85ae4487a31bac3cc9e78457f5114adb26L955, and I see this fail again. The problem is that a file is generated in outDir and I think that isn't being watched. I think we can still release without the guard so we get more feedback.

patak-dev · 2023-12-10T11:21:45Z

packages/vite/src/node/fsUtils.ts

+}
+
+export function createCachedFsUtils(config: ResolvedConfig): FsUtils {
+  const root = withoutTrailingSlash(config.root)


Note: I added withoutTrailingSlash because the rest of the cache expects this. I think config.root is always without a trailing slash because we do a path.resolve(config.root) : path.cwd(). At least in UNIX, these two always return without a trailing slash. @sapphi-red could you confirm this is also the case in Windows? We could add withoutTrailingSlash in another PR if not when resolving the root, then there are some places where we could directly use root + pathInRoot instead of a path.join() call.

I confirmed that path.resolve(variable) and process.cwd() both always return without a trailing slash on Windows 👍

Thanks for testing! I think I will remove the new withoutTrailingSlash function then so we can take any testing to also check this more in general.

ArnaudBarre

This is risky but way less invasive that I would have think!

bluwy

Some small nits but I think it's worth a try. Thanks for encapsulating the new fs utils all in one-file, that makes it easier to revisit the optimizations in the future.

bluwy · 2023-12-11T07:15:25Z

packages/vite/src/node/server/index.ts

+    cachedChecks:
+      server.fs?.cachedChecks ?? !!process.env.VITE_SERVER_FS_CACHED_CHECKS,


Are we planning to keep this env var, or is it only for testing?

I added it only for help with testing, for example if @sheremet-va would want to easily check the effect in Vitest. I think we could remove it later.

packages/vite/src/node/fsUtils.ts

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

perf: cached fs utils

4d666ab

patak-dev added the performance Performance related enhancement label Dec 7, 2023

patak-dev added 2 commits December 7, 2023 14:48

chore: merge main

aac3690

fix: temporal guard for custom watchers

524a52a

feat: add experimental server.fs.cacheChecks

c929f96

patak-dev marked this pull request as ready for review December 7, 2023 17:22

patak-dev added 3 commits December 7, 2023 20:42

feat: also enable fsUtils in createResolver

2fbf00a

chore: update types

2f25f9e

feat: optimize on add file or directory

a343c66

patak-dev mentioned this pull request Dec 7, 2023

perf: in-memory public files check #15195

Merged

4 tasks

patak-dev added 6 commits December 8, 2023 11:59

feat: optimize tryResolveRealFileWithExtensions

89df6d5

feat: avoid double normalizePath calls

d3d45fc

chore: update

8ea355e

chore: remove watch null and custom ignored guard

df42d68

chore: lint

93572c8

chore: merge main

c3d504b

patak-dev mentioned this pull request Dec 8, 2023

perf: cached fs utils with shared trees #15294

Draft

4 tasks

chore: remove async wip code

b35aefa

patak-dev mentioned this pull request Dec 9, 2023

perf: async fs calls in resolve and package handling #15211

Draft

4 tasks

perf: optimize joins

a98c9b4

bluwy reviewed Dec 9, 2023

View reviewed changes

patak-dev added 6 commits December 9, 2023 16:02

chore: set default to false

6fd956c

refactor: fsUtils.initWatcher

01742a5

feat: add VITE_SERVE_FS_CACHED_CHECKS

ab9e7d8

chore: fix typo

d25375e

fix: add guard for symlinked root

7c562d2

chore: ensure root without trailing slash

6c1b741

patak-dev commented Dec 10, 2023

View reviewed changes

patak-dev mentioned this pull request Dec 10, 2023

perf: temporary hack to avoid fs checks for /@react-refresh #15299

Merged

4 tasks

nits

5ede827

ArnaudBarre previously approved these changes Dec 10, 2023

View reviewed changes

bluwy reviewed Dec 11, 2023

View reviewed changes

chore: typo

476dc3b

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

patak-dev dismissed ArnaudBarre’s stale review via 476dc3b December 11, 2023 08:53

chore: remove unneeded withoutTrailingSlash util

d659920

patak-dev merged commit c9b61c4 into main Dec 12, 2023
15 checks passed

patak-dev deleted the perf/in-memory-fs-checks branch December 12, 2023 09:32

patak-dev mentioned this pull request Jan 24, 2024

feat: enable fs.cachedChecks by default #15704

Merged

4 tasks

hi-ogawa mentioned this pull request Feb 10, 2024

No fallback to index.js when importing a directory using an alias #15858

Closed

7 tasks

sapphi-red mentioned this pull request Feb 20, 2024

fix: resolve directory correctly when fs.cachedChecks: true #15983

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: cached fs utils #15279

perf: cached fs utils #15279

patak-dev commented Dec 7, 2023 •

edited

stackblitz bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

vite-ecosystem-ci bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

vite-ecosystem-ci bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

patak-dev commented Dec 9, 2023

patak-dev commented Dec 9, 2023

bluwy left a comment

bluwy Dec 9, 2023

patak-dev Dec 9, 2023

bluwy Dec 9, 2023

patak-dev commented Dec 9, 2023

patak-dev commented Dec 9, 2023

patak-dev Dec 10, 2023

sapphi-red Dec 12, 2023

patak-dev Dec 12, 2023

ArnaudBarre left a comment

bluwy left a comment

bluwy Dec 11, 2023

patak-dev Dec 11, 2023

		cachedChecks:
		server.fs?.cachedChecks ?? !!process.env.VITE_SERVER_FS_CACHED_CHECKS,

perf: cached fs utils #15279

perf: cached fs utils #15279

Conversation

patak-dev commented Dec 7, 2023 • edited

Description

Context

Details

What is the purpose of this pull request?

stackblitz bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

vite-ecosystem-ci bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

vite-ecosystem-ci bot commented Dec 7, 2023

patak-dev commented Dec 7, 2023

patak-dev commented Dec 9, 2023

patak-dev commented Dec 9, 2023

bluwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patak-dev commented Dec 9, 2023

patak-dev commented Dec 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArnaudBarre left a comment

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patak-dev commented Dec 7, 2023 •

edited