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: preserve symlinks while scanning user code #12818

Closed

Conversation

patak-dev
Copy link
Member

Description

Testing an idea we were discussing with @bluwy. We can avoid costly realPathSync calls for user code while scanning. I haven't noticed a diff in perf though. @sapphi-red maybe when you are back to your Windows machine you could try this one?


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Apr 10, 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 Apr 10, 2023
@patak-dev
Copy link
Member Author

Interesting, one down, one up for the benchmarks https://github.com/vitejs/vite-benchmark/actions/runs/4660951837. Maybe there is something wrong with the PR, I wouldn't expect things to be slower after it.

@patak-dev
Copy link
Member Author

Another run with the same result https://github.com/vitejs/vite-benchmark/actions/runs/4664331499. @fi3ework said that there could be a 1% variance that is noise though.

@bluwy
Copy link
Member

bluwy commented Apr 11, 2023

I ran this locally with a project with many entires (git merge main first as the optimizer change seems to affect the timing), I'm seeing a small decrease of 290s to 283ms. Maybe it's more obvious on other OSes.

@fi3ework
Copy link
Member

Ran twice on Windows (add a os inputs for workflow)

It shows FCP did get a perf gain 🤔.

@patak-dev
Copy link
Member Author

I did 50 times run in Linux and it shows ~0.75% improvement of the 2.7-slow case, and almost no difference in the other case: https://github.com/vitejs/vite-benchmark/actions/runs/4682746568

If this is safe, it looks like a good optimization. We can also hold it until the next minor to keep stabilizing 4.3 and getting to users sooner.

@sapphi-red
Copy link
Member

I ran the "1000 React components" benchmark on my machine with this PR.

Babel (cold start) was 4949.9ms and SWC (cold start) was 3046.5ms.
It's a 150ms - 200ms perf gain.

@patak-dev patak-dev added this to the 5.0 milestone Jun 8, 2023
@sapphi-red
Copy link
Member

I noticed that the scanner uses the same Package Cache with the normal pipeline. So the scanner's preserveSymlinks: true result might pollute the Package Cache. Then, the normal pipeline will resolve the path with preserveSymlinks: true when we expect the path to be resolved with preserveSymlinks: false.

const container = await createPluginContainer(config)
if (scanContext?.cancelled) return
const plugin = esbuildScanPlugin(config, container, deps, missing, entries)

function esbuildScanPlugin(
config: ResolvedConfig,
container: PluginContainer,
depImports: Record<string, string>,
missing: Record<string, string>,
entries: string[],
): Plugin {
const seen = new Map<string, string | undefined>()
const resolve = async (
id: string,
importer?: string,
options?: ResolveIdOptions,
) => {
const key = id + (importer && path.dirname(importer))
if (seen.has(key)) {
return seen.get(key)
}
const resolved = await container.resolveId(
id,
importer && normalizePath(importer),
{
...options,
scan: true,
},
)

resolvePlugin({
...config.resolve,
root: config.root,
isProduction: config.isProduction,
isBuild,
packageCache: config.packageCache,

@patak-dev
Copy link
Member Author

Thanks for the review @sapphi-red. I'm no longer sure about adding the extra complexity of changing preserverSymlinks for user code only, even if we could use separate package caches. Let's close this PR for now.

@patak-dev patak-dev closed this Oct 16, 2023
@sapphi-red sapphi-red deleted the perf/preserve-symlinks-while-scanning-user-code branch October 29, 2023 16:23
@patak-dev patak-dev mentioned this pull request Dec 7, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants