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: reuse compiler process when using sass-embedded #1195

Conversation

renspoesse
Copy link
Contributor

@renspoesse renspoesse commented Mar 29, 2024

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

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
Copy link

linux-foundation-easycla bot commented Mar 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you sign the CLA?

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

try {
result = await compile(sassOptions, options);
result = await compile(sassOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed unused argument.

src/utils.js Outdated
if (!sassEmbeddedCompiler) {
// Create a long-running compiler process that can be reused
// for compiling individual files.
sassEmbeddedCompiler = await implementation.initAsyncCompiler();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@renspoesse renspoesse Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/utils.js Outdated
// there is no webpack compiler object in such case.
if (webpackCompiler) {
const key = isDartSass ? "dart-sass" : "sass-embedded";
if (!sassModernCompilers[key]) {
Copy link
Contributor Author

@renspoesse renspoesse Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@alexander-akait alexander-akait Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/utils.js Outdated
// for compiling individual files.
const compiler = await implementation.initAsyncCompiler();
webpackCompiler.hooks.shutdown.tap("sass-loader", () => {
compiler.dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/utils.js Outdated
});
}
}
return sassModernCompilers.get(implementation).compileStringAsync(data, rest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@renspoesse renspoesse Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexander-akait
Copy link
Member

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

Copy link

codecov bot commented Apr 3, 2024

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.

@renspoesse 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
Copy link
Member

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 alexander-akait merged commit cef40a8 into webpack-contrib:master Apr 9, 2024
9 of 11 checks passed
@alexander-akait
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the Sass Compiler API
3 participants