Dart Sass without fibers should use renderSync instead of render

[filipesilva]

• Operating System: Windows
• Node Version: 10.16.0
• NPM Version: 6.9.0
• webpack Version: 4.30.0
• sass-loader Version: 7.1.0

Expected Behavior

When using Dart Sass and not using fibers, the renderSync method should always be used instead of the render method (see https://github.com/sass/dart-sass#javascript-api, #435 (comment) and sass/dart-sass#744 (comment) for details). Always using render has severe performance implications.

Actual Behavior

The render method is always used with Dart sass.

Code

sass-loader/lib/loader.js

Lines 137 to 144 in e279f2a

	if (implementation === 'dart-sass') {
	if (!semver.satisfies(version, '^1.3.0')) {
	throw new Error(
	`Dart Sass version ${version} is incompatible with ^1.3.0.`
	);
	}
	return module.render.bind(module);

How Do We Reproduce?

No external reproduction needed. It's in the source code for this package.

[alexander-akait]

Thanks for issue, i will investigate this for next major release, i think i will start work on next release at this week, a lot of other issues

[filipesilva]

SGTM, thanks for looking at it!

[jhnns]

We could make it configurable via loader options. This pushes the responsibility/complexity back to the user. The problem is that the sass-loader has no clue if your project uses any custom importer where the async API is necessary.

I should also mention that compiling something synchronously is one of the worst things you can do in a webpack build. In a regular webpack build there are hundreds of async module builds simultaneously. If you do something synchronously, it will pause all other module builds. I'm pretty sure that most people will get worse performance results if they do something synchronously 😁

[filipesilva]

I think configurable makes sense if the plugin cannot determine it. I can compare performance numbers if you guide me a little bit on how I can change it in my node modules. I tried to change it myself in sass/dart-sass#744 (comment) but it looks like I was missing something.

[nex3]

We could make it configurable via loader options. This pushes the responsibility/complexity back to the user. The problem is that the sass-loader has no clue if your project uses any custom importer where the async API is necessary.

This would make sense, but the default should still probably be running synchronously (which means WebPack's custom importer should be synchronous as well).

I should also mention that compiling something synchronously is one of the worst things you can do in a webpack build. In a regular webpack build there are hundreds of async module builds simultaneously. If you do something synchronously, it will pause all other module builds. I'm pretty sure that most people will get worse performance results if they do something synchronously 😁

The logic of compiling Sass—which is by far the bottleneck of Sass compilation—is inherently synchronous. There's nothing about parsing strings or manipulating ASTs that can be done out-of-process, so making it asynchronous doesn't buy you anything in terms of speed (and in fact makes it much slower because it has to bounce back to the event loop all the time). The only reason it has an asynchronous mode at all is to support asynchronous plugins.

[alexander-akait]

This would make sense, but the default should still probably be running synchronously (which means WebPack's custom importer should be synchronous as well).

In theory it is possible https://github.com/webpack/enhanced-resolve#resolve (we need use sync), but here we will have other problem - it is decrease sass compilation time, but increase webpack compilation time, no golden way here

[nex3]

The overhead of running Sass asynchronously tends to range between 2x and 3x the total synchronous compilation time for large projects, not to mention the memory overhead that @filipesilva is running into. This dwarfs file IO time, which means it's also likely to dwarf the benefits you'd get from increased parallelism while waiting on file IO.

[Yuriy-Svetlov]

Hi @nex3 !

Thank you for your work!

1 - Are you considering caching? To increase the performance of file processing
2 - sass.renderSync - Is it recommended? I mean Dart-Sass for JS npm install sass --save-dev.

[nex3]

1 - Are you considering caching? To increase the performance of file processing

Filesystem operations aren't the bottleneck here. The issue, as I explained above, is the overhead of going back to the event loop for every method call in asynchronous mode.

2 - sass.renderSync - Is it recommended? I mean Dart-Sass for JS npm install sass --save-dev.

Yes, renderSync() is recommended for Dart Sass.

[Yuriy-Svetlov]

@nex3
Thank you!

Can I set your avatar in my plugin?

[nex3]

I'd prefer it if you didn't.

[Yuriy-Svetlov]

I'd prefer it if you didn't.

@nex3 Ok. I will not add a photo of your avatar. It was just needed for the quote you made. Then I'll just add your nickname.

Here are these quotes

The logic of compiling Sass—which is by far the bottleneck of Sass compilation—is inherently synchronous. There's nothing about parsing strings or manipulating ASTs that can be done out-of-process, so making it asynchronous doesn't buy you anything in terms of speed (and in fact makes it much slower because it has to bounce back to the event loop all the time). The only reason it has an asynchronous mode at all is to support asynchronous plugins.

The overhead of running Sass asynchronously tends to range between 2x and 3x the total synchronous compilation time for large projects, not to mention the memory overhead that @filipesilva is running into. This dwarfs file IO time, which means it's also likely to dwarf the benefits you'd get from increased parallelism while waiting on file IO.

[niksy]

What’s the status of this issue? I thinks what @jhnns proposed—pushing the responsibility/complexity back to the user—is the best way forward. Make sass-loader async by default, but strongly recommend (in documentation) switching to sync API when using Dart Sass.

[alexander-akait]

We can't use sync api because we need resolve your @import/@use and etc, it will bring a big losing of perf

[niksy]

We can't use sync api because we need resolve your @import/@use and etc, it will bring a big losing of perf

This is still developer/user issue and it’s something that could be mentioned in documentation. Currently there really isn’t any good way forward other than opt-in to sync API and deal with consequences.

As for resolving module at-rules, that’s included inside stats.includedFiles which is provided by Sass render result and it’s not related to any Webpack implementation other than custom Webpack importer.

[alexander-akait]

We can't enable sync api, resolver are always async

[niksy]

We can't enable sync api, resolver are always async

Do you mean Webpack resolver or Sass resolver? Because Webpack enhanced resolve has sync option IIRC.

[shantanu2307]

1 - Are you considering caching? To increase the performance of file processing

Are we not caching the results?

[alexander-akait]

I want to close this issue, because we can't use renderSync, but if you need improve perf just use sass-embedded and modern-compiler, async compilation will be faster and incremental builds will be faster (faster then with sync)

Anyway feel free to feedback