-
Notifications
You must be signed in to change notification settings - Fork 355
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
16x performance difference between sass and node-sass #1534
Comments
Thanks for the repro! I can't run your tests directly (ironically due to
It looks like native Dart Sass is about 1.35x as slow as LibSass, which is a bit high—we normally expect it to be pretty close to equivalent. JS Dart Sass is about 2.75x as slow as Dart Sass, which is within the expected cost range of the JS compilation, but stacks with the 35% slowdown to be 3.75x as slow as LibSass which is pretty bad. Doing a quick profile of the Dart executable here, it looks like a bunch of time is being spent resolving It's worth noting as ever that no pure-JS Sass implementation is ever going to be particularly fast. Keep an eye on the JS Embedded Host for a more performant way to invoke Sass from JS. In your case, since you're not actually passing in any custom functions or importers, you could also simply invoke the native Dart Sass CLI executable to do your compilation for a major speed boost. |
There's also inherently going to be some extra |
Okay, digging in more, it seems like the import resolution is absolutely the major source of slowdown here. I added a simple cache to the built-in filesystem importer, and I'm seeing these numbers now:
That puts native Dart Sass essentially equivalent to LibSass. JS Dart Sass is still about 3.2x slower, but the absolute time is certainly lower. I'll have to figure out how to productionize this cache (since the naive version would interfere with files that change over time, as in Some other takeaways:
|
Some interesting observations here. Yes, this set of scss does have a huuuuuuge amount of repeated @imports, as it's a library intended to allow single components to be used standalone but which also produces big roll-up CSS builds, and it's broken down its scss into a myriad separate small files that are all imported everywhere. I'm not at all surprised that the sass compiles are pretty long whatever tool is used. The point about the new module system is a great one, and we're working on that -- we have the usual library issue that not all our users have been ready to move from node-sass/libsass to dart sass, but we're preparing to make dart sass the pre-req for the next major version and convert everything to use the new module system and @use, which should be a big improvement. But we're not quite there yet and will probably need to support this version for a while yet! I'm interested with your command-line timings. Although they do show quite a slow-down, it's at least within the kind of range I was originally expecting. I get the same: I ran node-sass directly at the command line to compile this file and it took about 16s, and I did the same with (dart) sass (JS) and it took about 40s. Roughly the same order as your timings. But when calling into the API from my jest test it is taking nearly six minutes!! 40s I could deal with, though even faster would be even nicer :-) Yeah, I could just run the command line compile for my test, but I'm still curious as to why calling through the API is making such a huge difference. |
btw, I'm on a Mac. Just out of interest I've loaded an updated test that uses sass instead of node-sass as a PR and run the tests as a github action, here: https://github.com/carbon-design-system/ibm-cloud-cognitive/pull/1340/checks?check_run_id=3911374913. You can see the original (node-sass) "styles.test.js" and the updated (sass) "styles2.test.js" runs here: |
That's a great question. The Dart Sass JS API has some behavior differences from the JS CLI, particularly around the way I've packaged up the various performance improvements this issue has inspired into #1535. I'll be re-running our entire benchmark suite to get more solid numbers, but here are the improved numbers for the carbon example:
|
@dcwarwick I'd like to add this to our standard benchmark suite, but it looks like it's not yet compatible with Node Sass 6.0.0, and thus not compatible with Node 16. Is there an issue tracking that? |
Yay! That looks really cool. And in the mean time I will switch to calling out to sass through the CLI rather than in through the JS API to get me some of that 40s action, which is a little slower than node-sass but has the advantage that we'll actually be testing what we're building! |
Oops. What's not compatible? We weren't actually declaring node-sass as a dev dependency, so it was just picking up some random version of node-sass from another dependency, but I just ran the compile with node-sass 6.0.1 and it seemed to work ok. |
It's just a declaration issue—wherever you're picking up |
Yeah, sorry, the package won't currently work on Node 16.x -- we have a dependency @carbon/bundler which requires node-sass 4.14.1. This dependency is obsolete, and we have an issue to remove it carbon-design-system/ibm-products#1144 so we might be able to get rid of that in a week or two. I'm running on Node lts/fermium=14 where all's fine. |
ok, so by changing expect(
renderSync({
file: scssReleasedOnly,
includePaths,
style: 'expanded',
}).css.toString()
).toMatchSnapshot(); to expect(
execSync(
`sass --style=expanded --load-path ${includePath1} --load-path ${includePath2} ${scssReleasedOnly}`
).toString()
).toMatchSnapshot(); the test takes 40s to complete on github (it's "styles3.test.js" below), vs somewhere around 300s ("styles2.test.js"). The node-sass original ("styles.test.js") takes about 15s. That makes a 2.5x slowdown, and we can live with that, so I'm going to go ahead and make that change in our repo now. |
@nex3 FWIW we have now removed the obsolete dependency, so there shouldn't be any problem using node 16 with our package now. Relevant tests are now using execFileSync rather than the API and the timings are manageable :-) |
I've seen issues referring to performance before, and the responses generally come down to it depending how
sass
is being used and what stylesheet is being compiled, which people then seem strangely loth to explain. I'm happy to show exactly how I'm using `sass and what stylesheet I am compiling, so I hope this is a useful issue to raise.We have a jest test for a component library that compiles a stylesheet and compares the result to a snapshot. This test is still using
node-sass
, although we've moved to usingsass
for our build, so I tried to update the test to usesass
too.The test looks something like this:
Now this SCSS file loads, indirectly, a big and complex tree of dependent SCSS files, and on my machine it takes about 20 secs to complete the test:
I changed 'node-sass' to 'sass' on the require, as
sass
is already a dependency. I also added* @jest-environment node
to the header comment. Then I ran the test again, and usingsass
instead ofnode-sass
it takes about 345 secs to complete the test:This is a very substantial increase in time, and indeed would cause the total run time of the entire test suite to grow by a factor of more than 3 times. I am aware that JS
sass
will not be quite as fast asnode-sass
, but this represents a much larger factor than I was expecting. I'm interested in whether this is what would be expected, and whether there is something about the way I am calling or usingsass
that could be changed to give me better/faster results.The full test file: https://github.com/carbon-design-system/ibm-cloud-cognitive/blob/main/packages/cloud-cognitive/src/__tests__/styles.test.js
Steps to reproduce the issue:
yarn
yarn jest styles
. This will run the test usingnode-sass
.* @jest-environment node
into the copyright comment at the top of the file and to change 'node-sass' to 'sass' in the firstrequire
.yarn jest styles
again. This will run the test usingsass
.The stylesheet being compiled is /packages/cloud-cognitive/src/index-without-carbon-released-only.scss, which is just the entry point for quite a complex set of SCSS files. But this issue is not meant to be a "please debug my repo" request! Rather, I'm hoping that showing a real-life example with a substantial performance hit will be useful, both to the sass team and to anyone else who might experience something similar.
The text was updated successfully, but these errors were encountered: