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: use thread for preprocessors #13584

Merged
merged 20 commits into from
Jan 22, 2024

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jun 21, 2023

Description

This is just a POC yet. I need to polish the code and handle some edge cases (For example, when options.importer is used).

Running Vite on my project1 with this PR reduces the build time from 11.04s to 9.43s.

I made a PR with CSS preprocessors, but in theory this technique would be effective in the following case:

  • the code is computation heavy (= not blocked by file I/O or network)
  • doesn't require calling a function from the main thread frequently (= the total overhead of a function call from the main thread is negligible)
  • the code runs on Node.js (e.g. written in JS or Wasm) (= doesn't use NAPI or offloads the computation to other executables)

I guess this can be applied to plugin-vue and plugin-react too.

Additional context


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.

Footnotes

  1. https://github.com/traPtitech/traQ_S-UI, removed PWA plugin and brotli plugin (as they take some time)

@sapphi-red sapphi-red added feat: css performance Performance related enhancement labels Jun 21, 2023
@stackblitz
Copy link

stackblitz bot commented Jun 21, 2023

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

@sapphi-red sapphi-red force-pushed the perf/use-thread-for-preprocessors branch from 66a0c9e to 6d45801 Compare August 13, 2023 14:44
@sapphi-red
Copy link
Member Author

sapphi-red commented Aug 15, 2023

The implementation should be fine now. The edge cases are now handled.

I think there's three points that needs to be discussed.

  1. Whether it's worth having the complexity
    • This PR will only improve performance if the project uses CSS preprocessors and doesn't use advanced features such as custom importer (sass) or custom resolver (less).
  2. Mitigate the change to preprocessCSS (related: Stabilizing `preprocessCSS()` #13815)
    • This PR changes preprocessCSS to start a worker each time and I guess it could severely hurt the performance. I exposed a new function called createCSSPreprocessor and by migrating to that the projects can benefit from this PR. To mitigate it further, we can pass a flag to compileCSS so that it will never use the workers (then it will work mostly the same as before).
  3. Moving the Worker + FakeWorker implementation to okie
    • okie advocates as a "Dead simple worker threads pool" but my extension might be no longer simple 😅. I'm not sure if I should create a PR to okie or create a separate package.

I tested the PR with some project/reproductions.

Project Name Description Notes v4.4.9
(build time1 / dev start time2)
This PR
(build time / dev start time)
Diff
(build time / dev start time)
traQ_S-UI Vue + Sass. The same project with the same config I used in the OP. Many components has <style lang="scss">. 12.265s / 7.37s 10.991s / 6.01s -10.4% / -18.5%
Element UI project Vue + Sass. The reproduction in #6736 A single import './index.scss'. 5.082s / 3.67s 5.050s / 3.58s -0.00% / -0.02%
Vuetify 2 project (1) Vue + Sass. The reproduction in #7719 Vuetify 2's every component imports a Sass file. 8.358s / 7.97s 5.817s / 4.55s -30.4% / -42.9%
Vuetify 2 project (2) Vue + Sass. The reproduction in #12393 Same with above. 8.756s / 7.86s 6.092s / 4.46s -30.4% / -43.2%
directus Vue + Sass. A project I found by searching GitHub. A single import './styles/main.scss' + <style lang="scss"> with some components. 30.256s / --- 29.459s / --- -2.6% / ---
Excalidraw React + Sass. A project I found by searching GitHub. I removed plugin-pwa and plugin-checker. Some .tsx files have import './foo.scss'. 8.186s / --- 7.672s / --- -6.2% / ---
PocketBase Svelte + Sass. A project I found by searching GitHub. A single import "./scss/main.scss" + Some .svelte files have <style lang="scss">. (Using the createCSSPreprocessor in plugin-svelte might improve this result.) 10.353s / --- 9.694s / --- -6.3% / ---

I didn't test with less/stylus because I didn't find a project that uses less or stylus and not small.

Footnotes

  1. dev start time: Finish time shown in the network tab of Chrome dev tools

  2. build time: output of Measure-Command { npm run build } (powershell)

@sapphi-red sapphi-red added the p3-significant High priority enhancement (priority) label Sep 6, 2023
@bluwy bluwy added this to the 5.1 milestone Nov 29, 2023
@sapphi-red sapphi-red marked this pull request as ready for review December 1, 2023 10:42
@sapphi-red
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 3d7c314: 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

@sapphi-red
Copy link
Member Author

sapphi-red commented Dec 1, 2023

Finally finished this PR 😅

Added an experimental css.preprocessorOptions[extension].maxWorkers option that defaults to 0.
But I wonder if it's better to name it css.preprocessorMaxWorkers.
Because .sass/.scss (also .styl/.stylus) uses the same worker pool, setting css.preprocessorOptions.sass.maxWorkers: true also enables for .scss.

@patak-dev
Copy link
Member

patak-dev commented Dec 1, 2023

This is awesome! css.preprocessorMaxWorkers sounds better to me 👍🏼
Or maybe css.prepocessorOptions.shared.maxWorkers?

@sapphi-red
Copy link
Member Author

Changed to css.preprocessorMaxWorkers 👍

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
bluwy
bluwy previously approved these changes Jan 22, 2024
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.

Looks great! 🚀

@patak-dev patak-dev enabled auto-merge (squash) January 22, 2024 10:22
@patak-dev patak-dev merged commit acd795f into vitejs:main Jan 22, 2024
10 checks passed
@sapphi-red sapphi-red deleted the perf/use-thread-for-preprocessors branch February 21, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-significant High priority enhancement (priority) performance Performance related enhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants