perf: reuse compiler process when using sass-embedded #1195

renspoesse · 2024-03-29T15:02:12Z

This PR contains a:

Motivation / Use-Case

This implements the Shared Resources proposal that was accepted and implemented in Dart Sass 1.70.0. By reusing the same compiler process when compiling multiple files, this significantly improves performance for tools like webpack.

Breaking Changes

None

Additional Info

Closes: #1163

This implements the Shared Resources proposal that was accepted and implemented in Dart Sass 1.70.0. By reusing the same compiler process when compiling multiple files, this significantly improves performance for tools like webpack. Closes: webpack-contrib#1163

linux-foundation-easycla · 2024-03-29T15:02:16Z

The committers listed above are authorized under a signed CLA.

✅ login: renspoesse / name: Rens Poesse (b111ffd, ea88129, ecc9206, 2e8a443, e75dbde, 2514e16, 5952f5a)

evenstensberg

Could you sign the CLA?

renspoesse · 2024-03-29T15:06:54Z

src/index.js

@@ -74,7 +74,7 @@ async function loader(content) {
  let result;

  try {
-    result = await compile(sassOptions, options);
+    result = await compile(sassOptions);


Removed unused argument.

renspoesse · 2024-03-29T15:07:44Z

src/utils.js

+        if (!sassEmbeddedCompiler) {
+          // Create a long-running compiler process that can be reused
+          // for compiling individual files.
+          sassEmbeddedCompiler = await implementation.initAsyncCompiler();


I'm not sure about the lifetime of a webpack loader. If webpack provides some hook for this, we could close the compiler process by calling sassEmbeddedCompiler.dispose().

Firstly let's implement the modern-compiler for the api option, because it is different mode and can have differents problems, in future when sass finished and stabilizated everythings we will use modern-compiler API by default.

Yes, we should close dispose compiler, we can use such code:

if (!sassEmbeddedCompiler && loader._compiler) { sassEmbeddedCompiler = await implementation.initAsyncCompiler(); loader._compiler.hooks.shutdown.tap("name", () => { sassEmbeddedCompiler.dispose() }) }

Some people can run loader in multi threading way and there is no compiler in the such case

Also will be great to make some benches, for example for bootstrap to undestand how it is better

Some people can run loader in multi threading way and there is no compiler in the such case

Could you clarify this? (Why) can we not use the long-running subprocess in that case?

It is slow for perf, initial start will be very long and we don't need subprocess

I've implemented your suggestions. Re:

Also will be great to make some benches, for example for bootstrap to undestand how it is better

is there a pre-existing repo or setup I can use to benchmark this?

No, let's just check it, I will put it in release notes

For what it's worth, I don't have the exact numbers but on a larger project I'm working on, modern-compiler is up to 10 seconds faster than modern. Also modern would just crash when compiling a changed file that's imported in various other files, whereas with modern-compiler it works fine.

renspoesse · 2024-03-29T22:26:35Z

src/utils.js

+        // there is no webpack compiler object in such case.
+        if (webpackCompiler) {
+          const key = isDartSass ? "dart-sass" : "sass-embedded";
+          if (!sassModernCompilers[key]) {


I'm not sure if getCompileFn() can be invoked with different (implementation) options for a single instance of the sass-loader module, but I guess it can? In that case we should maintain a compiler instance for each implementation.

I see, let's use WeakMap with options to prevent such behaviour and memory leaking

renspoesse · 2024-03-29T22:28:40Z

src/utils.js

+            // for compiling individual files.
+            const compiler = await implementation.initAsyncCompiler();
+            webpackCompiler.hooks.shutdown.tap("sass-loader", () => {
+              compiler.dispose();


In my debugging this hook was never called; not sure when it should be. In any case, should we delete sassModernCompilers[key] after disposing? If webpack may reuse the sass-loader module after the shutdown phase, we should, otherwise the compiler won't be recreated on the next run.

After shutdown should nothing happend in theory, weird, it should happend

alexander-akait

Looks good, let's look at tests and solve small nitpicks above

alexander-akait · 2024-04-02T16:23:34Z

src/utils.js

+              });
+            }
+          }
+          return sassModernCompilers.get(implementation).compileStringAsync(data, rest);


Let's add a test case where we have two sass loaders with different options and use modern-compiler, just make sure everything works fine

Does the test from 183765a look good to you? I'm not sure how to otherwise have two sass-loaders; we can't chain them within a single webpack config, right? Because they don't output Sass.

I am fine with test, maybe I will add more later, want to look at this today

alexander-akait · 2024-04-02T16:26:36Z

Thank you for your work, looks like a test passed, so let's add for two loader with different options and we can merge it, I will check the shutdown hook

codecov · 2024-04-03T14:29:28Z

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.11%. Comparing base (31789cc) to head (ea88129).
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/utils.js	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
- Coverage   94.44%   94.11%   -0.33%     
==========================================
  Files           3        3              
  Lines         360      374      +14     
  Branches      132      137       +5     
==========================================
+ Hits          340      352      +12     
- Misses         18       20       +2     
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alexander-akait · 2024-04-09T13:42:16Z

Thank you, I will make perf benchmarks and show them in the release, also I want to finish built-in webpack resolver support for modern and modern-compiler API

alexander-akait · 2024-04-10T15:48:09Z

It really faster (sass-embedded):

asset main.js 931 KiB [compared for emit] (name: main) 1 related asset
runtime modules 1.58 KiB 7 modules
cacheable modules 890 KiB
  asset modules 5.2 KiB
    data:image/svg+xml,%3csvg xmlns=%27.. 281 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 281 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 279 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 161 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 271 bytes [built] [code generated]
    + 14 modules
  javascript modules 885 KiB
    modules by path ../../node_modules/ 509 KiB 12 modules
    modules by path ./src/ 376 KiB
      ./src/index.js 108 bytes [built] [code generated]
      ./src/style.scss 1.31 KiB [built] [code generated]
      ../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 1284 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 841 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 515 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 524 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 507 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 467 ms

Just sass:

asset main.js 931 KiB [compared for emit] (name: main) 1 related asset
runtime modules 1.58 KiB 7 modules
cacheable modules 890 KiB
  asset modules 5.2 KiB
    data:image/svg+xml,%3csvg xmlns=%27.. 281 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 281 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 279 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 161 bytes [built] [code generated]
    data:image/svg+xml,%3csvg xmlns=%27.. 271 bytes [built] [code generated]
    + 14 modules
  javascript modules 885 KiB
    modules by path ../../node_modules/ 509 KiB 12 modules
    modules by path ./src/ 376 KiB
      ./src/index.js 108 bytes [built] [code generated]
      ./src/style.scss 1.31 KiB [built] [code generated]
      ../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 4688 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 3680 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 3118 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 3124 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 3263 ms
asset main.js 931 KiB [emitted] (name: main) 1 related asset
cached modules 515 KiB [cached] 33 modules
runtime modules 1.58 KiB 7 modules
../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!../../dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/style.scss 375 KiB [built] [code generated]
webpack 5.91.0 compiled successfully in 3055 ms

Around 10x times

evenstensberg reviewed Mar 29, 2024

View reviewed changes

renspoesse commented Mar 29, 2024

View reviewed changes

renspoesse added 2 commits March 29, 2024 11:49

refactor: add modern-compiler API option and call to dispose()

5952f5a

refactor: allow implementation to change

2e8a443

renspoesse commented Mar 29, 2024

View reviewed changes

test: perform same tests for modern-compiler as for modern

b111ffd

alexander-akait reviewed Apr 2, 2024

View reviewed changes

renspoesse added 3 commits April 8, 2024 13:31

refactor: use WeakMap

e75dbde

test: add test for switching implementation with modern-compiler API

2514e16

style: ran prettier

ea88129

renspoesse force-pushed the perf-reuse-compiler-process-when-using-sass-embedded branch from 2d39fc1 to ea88129 Compare April 8, 2024 18:32

alexander-akait approved these changes Apr 9, 2024

View reviewed changes

alexander-akait merged commit cef40a8 into webpack-contrib:master Apr 9, 2024
9 of 11 checks passed

alexander-akait mentioned this pull request Apr 10, 2024

fix: gracefully shutting down webpack/webpack-cli#4145

Merged

alexander-akait mentioned this pull request Apr 10, 2024

fix: use watchClose hook #1196

Merged

6 tasks

nex3 mentioned this pull request May 9, 2024

Errors when using with sass-loader sass/embedded-host-node#296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: reuse compiler process when using sass-embedded #1195

perf: reuse compiler process when using sass-embedded #1195

renspoesse commented Mar 29, 2024 •

edited

linux-foundation-easycla bot commented Mar 29, 2024 •

edited

evenstensberg left a comment

renspoesse Mar 29, 2024

renspoesse Mar 29, 2024

alexander-akait Mar 29, 2024

alexander-akait Mar 29, 2024

renspoesse Mar 29, 2024

alexander-akait Mar 29, 2024

renspoesse Mar 29, 2024

alexander-akait Apr 2, 2024

renspoesse Apr 2, 2024 •

edited

renspoesse Mar 29, 2024 •

edited

alexander-akait Apr 2, 2024 •

edited

renspoesse Mar 29, 2024

alexander-akait Apr 2, 2024

alexander-akait left a comment •

edited

alexander-akait Apr 2, 2024

renspoesse Apr 3, 2024 •

edited

alexander-akait Apr 3, 2024

alexander-akait commented Apr 2, 2024

codecov bot commented Apr 3, 2024 •

edited

alexander-akait commented Apr 9, 2024

alexander-akait commented Apr 10, 2024

perf: reuse compiler process when using sass-embedded #1195

perf: reuse compiler process when using sass-embedded #1195

Conversation

renspoesse commented Mar 29, 2024 • edited

Motivation / Use-Case

Breaking Changes

Additional Info

linux-foundation-easycla bot commented Mar 29, 2024 • edited

evenstensberg 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

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

Choose a reason for hiding this comment

renspoesse Apr 2, 2024 • edited

Choose a reason for hiding this comment

renspoesse Mar 29, 2024 • edited

Choose a reason for hiding this comment

alexander-akait Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renspoesse Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Apr 2, 2024

codecov bot commented Apr 3, 2024 • edited

Codecov Report

alexander-akait commented Apr 9, 2024

alexander-akait commented Apr 10, 2024

renspoesse commented Mar 29, 2024 •

edited

linux-foundation-easycla bot commented Mar 29, 2024 •

edited

renspoesse Apr 2, 2024 •

edited

renspoesse Mar 29, 2024 •

edited

alexander-akait Apr 2, 2024 •

edited

alexander-akait left a comment •

edited

renspoesse Apr 3, 2024 •

edited

codecov bot commented Apr 3, 2024 •

edited