dart-sass icon indicating copy to clipboard operation
dart-sass copied to clipboard

Use worker threads to implement render()

Open nex3 opened this issue 6 years ago • 7 comments

Currently, we implement the asynchronous render() method in Node.js in one of two ways:

  • By default, it's run through the AsyncEvaluator which is synchronized via code gen with the synchronous Evaluator. This is much slower than running synchronously, and imposes developer overhead by requiring synchronization.

  • If the user passes in a reference to the fibers package (which uses a C++ extension to add a coroutine-style asynchrony model to Node), we run the synchronous code path and use coroutines to call out to async helpers. This is as fast as running synchronously but requires the end user to opt in to a C++ compilation which may be difficult to compile.

Sass's business logic itself is fully synchronous, so we get no benefit from this asynchrony other than the ability to invoke asynchronous function and importer callbacks. I think we can avoid the downsides of both of these methods by running the whole Sass compiler in a worker thread and using Atomics.wait() to block that thread until the callback completes.

nex3 avatar Oct 30 '19 21:10 nex3

I am thinking about taking on the implementation of this and I had a few quick clarification questions:

  • First of all, is this still a desired enhancement?
  • To be explicitly clear, this would involve the porting of the needed components of the node.js worker_threads library, right?
  • Am I correct that the check for fibers along with the call to renderAsync would all be removed in favor of renderSync?
  • Then in src/node.dart a worker thread would be setup to deal with the rendering within _renderSync?

Sorry I've got all of these clarification questions, I don't want to write a bunch of code only to realize I completely misinterpreted what you were asking for here.

hegelocampus avatar Jan 16 '20 04:01 hegelocampus

First of all, is this still a desired enhancement?

Yes, definitely!

To be explicitly clear, this would involve the porting of the needed components of the node.js worker_threads library, right?

You wouldn't need to port anything. Dart compiled to JS can invoke JS libraries, so you'd just need to call this library.

Am I correct that the check for fibers along with the call to renderAsync would all be removed in favor of renderSync?

We'd definitely be getting rid of fibers. I think we want to retain a distinction between renderAsync() and renderSync() both for backwards compatibility, and so we can avoid the overhead of spinning up a worker thread when only synchronous callbacks are necessary.

Then in src/node.dart a worker thread would be setup to deal with the rendering within _renderSync?

You'd be doing this in _renderAsync rather than _renderSync.

nex3 avatar Jan 16 '20 22:01 nex3

It looks like this feature has now become more urgent than before, since node-fibers has officially reached the end: https://github.com/laverdet/node-fibers/commit/8f2809869cc92c28c92880c4a38317ae3dbe654d

lehni avatar Apr 17 '21 09:04 lehni

Because of the breakage of fibers in Node.JS v16 and this dependency, It currently looks like using sass and dart-sass will prevent a project from upgrading to v16.

jamesrwaugh avatar May 26 '21 15:05 jamesrwaugh

@lehni I'm working on a blog post describing the potential ways of working around this issue. We're going to be focusing on the Node embedded host as the main workaround.

@jamesrwaugh Sass doesn't actually declare a dependency on fibers—it just takes a parameter that's expected to follow the fibers API. So it won't actually prevent apps from upgrading.

nex3 avatar May 26 '21 22:05 nex3

What is the status of this issue? Our build time increased by about 500% when switching from node-sass to sass in our Webpack project. We have to revert back to node-sass and accept incompatibility with certain style libraries as a result because a 300 second build time is not acceptable.

Is there anything in particular blocking progress on this issue being implemented?

ArcanoxDragon avatar Apr 10 '23 23:04 ArcanoxDragon

If build times are a major limitation for you, I strongly suggest either using the native Sass CLI (available from GitHub releases as well as other places) or using the Node.js embedded host which also calls out to the native compiler.

To address your specific question: this is still tagged "help wanted", which means that the Sass team doesn't have time to focus on it specifically but we'd welcome external contributions. There's still a draft pull request that someone could pick up as a starting point.

nex3 avatar Apr 18 '23 21:04 nex3