Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 7 commits into from
Apr 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecc9206
perf: reuse compiler process when using sass-embedded
renspoesse Mar 28, 2024
5952f5a
refactor: add `modern-compiler` API option and call to `dispose()`
renspoesse Mar 29, 2024
2e8a443
refactor: allow `implementation` to change
renspoesse Mar 29, 2024
b111ffd
test: perform same tests for `modern-compiler` as for `modern`
renspoesse Mar 29, 2024
e75dbde
refactor: use WeakMap
renspoesse Apr 2, 2024
2514e16
test: add test for switching implementation with modern-compiler API
renspoesse Apr 2, 2024
ea88129
style: ran prettier
renspoesse Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

} catch (error) {
// There are situations when the `file`/`span.url` property do not exist
// Modern API
Expand Down
11 changes: 9 additions & 2 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ function getWebpackImporter(loaderContext, implementation, includePaths) {
}

let nodeSassJobQueue = null;
let sassEmbeddedCompiler = null;

/**
* Verifies that the implementation and version of Sass is supported by this loader.
Expand All @@ -665,10 +666,16 @@ function getCompileFn(implementation, options) {

if (isNewSass) {
if (options.api === "modern") {
return (sassOptions) => {
return async (sassOptions) => {
const { data, ...rest } = sassOptions;

return implementation.compileStringAsync(data, rest);
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 •
edited
Loading

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.

}

return sassEmbeddedCompiler.compileStringAsync(data, rest);
};
}

Expand Down