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: back to temporal optimizer dirs #12622

Merged
merged 7 commits into from
Mar 28, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Mar 27, 2023

Closes #12592

Description

Reverts #12582
Reverts #12603 (partial, there was a fix for #12593 in it)

@sun0day found that there was a ~80ms perf regression in https://github.com/sun0day/vite-2.7-slow after #12582. Using write: false in esbuild forces esbuild to populate result.outputFiles. It is counter-intuitive, but this is slower than writing files to disk and reading them back.

@dominikg presented concerns about the new scheme after write: false (see #12592). We tried to avoid potential issues in #12603 with @bluwy. So the implementation ended up as complex as before, and slower.

The benefit of the new scheme was to fix #9986. This PR gets us back to the temporal dirs scheme using the new rename-rename-then-delete technique proposed by @dominikg. Hopefully, we will regain the 80ms and keep #9986 closed after this PR.

There are also some clean-ups.

A change compared to pre-#12582 is that when no dependencies are found, we were not committing the result. This means that for projects without dependencies, the scanner will run every time because we never saved a _metadata.json that indicates there aren't dependencies. After this PR, we are now committing the optimization result in this case too.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 27, 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 27, 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.

Some nits below, but this works great locally for me. RIP #12582 2023-2023

packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member Author

RIP #12582 2023-2023

We'll miss you #12582 🥲

bluwy
bluwy previously approved these changes Mar 28, 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.

Nice comments!

Co-authored-by: Dominik G. <dominik.goepel@gmx.de>
@patak-dev patak-dev enabled auto-merge (squash) March 28, 2023 19:59
@patak-dev patak-dev merged commit 8da0422 into main Mar 28, 2023
@patak-dev patak-dev deleted the perf/back-to-temporal-optimizer-dirs branch March 28, 2023 20:35
@alex-kinokon
Copy link
Contributor

this PR is causing #13506

@dominikg
Copy link
Contributor

dominikg commented Aug 5, 2023

@alex-kinokon care to elaborate? this PR mostly restores behavior that was present before.

@alex-kinokon
Copy link
Contributor

Copied from the issue: If a file does not exist, it will throw 504 Outdated Optimize Dep and ask the user to reload the page. Reloading the page obviously won’t make the file appear.

@dominikg
Copy link
Contributor

dominikg commented Aug 5, 2023

This looks to be a broken dependency, the error message in this case is indeed a bit misleading, but it is not the cause of the issue here. It wouldn't have worked either way.

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.

ENOENT: no such file or directory, rename ... deps_temp
4 participants