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

Use worker threads to implement render() #868

Open
nex3 opened this issue Oct 30, 2019 · 7 comments
Open

Use worker threads to implement render() #868

nex3 opened this issue Oct 30, 2019 · 7 comments
Labels
enhancement help wanted JavaScript Issues particular to the Node.js distribution

Comments

@nex3
Copy link
Contributor

nex3 commented Oct 30, 2019

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 nex3 added enhancement help wanted JavaScript Issues particular to the Node.js distribution labels Oct 30, 2019
@hegelocampus
Copy link

hegelocampus commented Jan 16, 2020

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.

@nex3
Copy link
Contributor Author

nex3 commented Jan 16, 2020

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.

@lehni
Copy link

lehni commented Apr 17, 2021

It looks like this feature has now become more urgent than before, since node-fibers has officially reached the end: laverdet/node-fibers@8f28098

@jamesrwaugh
Copy link

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.

@nex3
Copy link
Contributor Author

nex3 commented May 26, 2021

@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.

@ArcanoxDragon
Copy link

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?

@nex3
Copy link
Contributor Author

nex3 commented Apr 18, 2023

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.

wfleming added a commit to wfleming/esbuild-sass-plugin that referenced this issue May 18, 2023
Hunting down some more reading on sass perf, I found this gh issue:
sass/dart-sass#868. That led to me looking at
sass-embedded as a drop-in replacement for sass. I tried that, and yep,
it does what it says on the tin. Works the same, but async is way faster
and comparable to sync perf (with the benefit of much higher throughput
if you're doing a lot of stuff concurrently). My entire build wall-clock
time is now down to 8s, and the slowest css bundles are 4.5s.

Biggest downside seems to be that dart isn't compatible with musl libc,
so I had to swap the base docker image I was using for my esbuild
pipeline. Unclear to me if there are other tradeoffs.
wfleming added a commit to wfleming/esbuild-sass-plugin that referenced this issue Jun 13, 2023
Hunting down some more reading on sass perf, I found this gh issue:
sass/dart-sass#868. That led to me looking at
sass-embedded as a drop-in replacement for sass. I tried that, and yep,
it does what it says on the tin. Works the same, but async is way faster
and comparable to sync perf (with the benefit of much higher throughput
if you're doing a lot of stuff concurrently). My entire build wall-clock
time is now down to 8s, and the slowest css bundles are 4.5s.

Biggest downside seems to be that dart isn't compatible with musl libc,
so I had to swap the base docker image I was using for my esbuild
pipeline. Unclear to me if there are other tradeoffs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted JavaScript Issues particular to the Node.js distribution
Projects
None yet
Development

No branches or pull requests

5 participants