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

Sass compiles VERY slowly since v1.59 (8 times slower than v1.58) #1913

Closed
Rezyan opened this issue Mar 13, 2023 · 29 comments · Fixed by #1916 or #1929
Closed

Sass compiles VERY slowly since v1.59 (8 times slower than v1.58) #1913

Rezyan opened this issue Mar 13, 2023 · 29 comments · Fixed by #1916 or #1929
Assignees

Comments

@Rezyan
Copy link

Rezyan commented Mar 13, 2023

Introduction

Hi,

I've just migrated from v1.58.3 to v1.59.2, and since, Sass compiles VERY VERY VERY slowly. It worked fast under v1.58.3. My Node.js is v18.14.2.

Sass version Node.js version Compilation time
1.58.3 18.14.2 28 seconds
1.59.2 18.14.2 3 minutes and 55 seconds

Has anyone encountered a similar issue? Thanks. 👍

Scenario

I'm using the JavaScript API of Sass through https://github.com/dlmanning/gulp-sass.

Screenshots

v1.58.3:

v1 58 3

v1.59.2:

v1 59 2

@Rezyan Rezyan changed the title Sass doesn't compile since v1.59 Sass compile **VERY** slowly since v1.59 Mar 13, 2023
@Rezyan Rezyan changed the title Sass compile **VERY** slowly since v1.59 Sass compile VERY slowly since v1.59 Mar 13, 2023
@Rezyan Rezyan changed the title Sass compile VERY slowly since v1.59 Sass compile VERY slowly since v1.59 ((8 times slower than v1.58) Mar 13, 2023
@Rezyan Rezyan changed the title Sass compile VERY slowly since v1.59 ((8 times slower than v1.58) Sass compile VERY slowly since v1.59 (8 times slower than v1.58) Mar 13, 2023
@flannanl
Copy link

I'm seeing the same issue after moving up to 1.59. I am running the sass command directly and my node version is 18.15.0.

@Rezyan Rezyan changed the title Sass compile VERY slowly since v1.59 (8 times slower than v1.58) Sass compiles VERY slowly since v1.59 (8 times slower than v1.58) Mar 13, 2023
@Rezyan
Copy link
Author

Rezyan commented Mar 13, 2023

I'm seeing the same issue after moving up to 1.59. I am running the sass command directly and my node version is 18.15.0.

Ok, so since you are running the sass command directly, I guess the issue couldn't come from my Gulp task. The issue must surely come from Sass v1.59 release.

@s100
Copy link

s100 commented Mar 13, 2023

+1 to this issue. In our case the previous compile time was a few seconds, but now it's 15 minutes plus, long enough for our builds to start timing out. I am attempting to put together a minimal reproduction of the problem - as in, a stylesheet which compiles quickly under 1.58.3 but slowly under 1.59.0 - but our CSS is very complicated and pulls in a lot of third-party styles, so it's taking me some time. I am guessing that this was introduced under https://github.com/sass/dart-sass/pull/1903/files but that is only a guess. This doesn't seem to be anything to do with repeated SCSS module loading - so far my minimal stylesheet is just one (extremely long) file, with no @imports.

Something I have noticed is that we make extensive, repeated use of this pattern:

  .#{$prefix}--data-table--short thead tr,
  .#{$prefix}--data-table--short tbody tr,
  .#{$prefix}--data-table--short tbody tr th {
    height: rem(32px);
  }

  // and similarly, about 200 more times

, we have $prefix: 'bx' earlier in the CSS. If I inline that variable, making:

  .bx--data-table--short thead tr,
  .bx--data-table--short tbody tr,
  .bx--data-table--short tbody tr th {
    height: rem(32px);
  }

  // ... etc.

then compile time seems to drop back to normal.

@TopGrd
Copy link

TopGrd commented Mar 14, 2023

long enough for our builds to start timing out

same with me

@jathak
Copy link
Member

jathak commented Mar 14, 2023

Currently working on investigating this.

Just to check, is everyone here using the JS-compiled version of Sass (the NPM sass package), or is anyone using the Dart VM binary (either directly through the standalone releases, or via the sass-embedded NPM package)?

@bdbvb
Copy link

bdbvb commented Mar 14, 2023

NPM "sass" package (javascript).

@flannanl
Copy link

I'm using NPM sass too

@Rezyan
Copy link
Author

Rezyan commented Mar 14, 2023

Currently working on investigating this.

Just to check, is everyone here using the JS-compiled version of Sass (the NPM sass package), or is anyone using the Dart VM binary (either directly through the standalone releases, or via the sass-embedded NPM package)?

I'm also using the npm sass package, as explained in my first comment. Thank you for the investigation. 👍

@ntkme
Copy link
Contributor

ntkme commented Mar 14, 2023

Currently working on investigating this.

Just to check, is everyone here using the JS-compiled version of Sass (the NPM sass package), or is anyone using the Dart VM binary (either directly through the standalone releases, or via the sass-embedded NPM package)?

I can confirm the same regression with dart-sass-embedded with interpolation in selector.

@ntkme
Copy link
Contributor

ntkme commented Mar 14, 2023

CPU Flame Chart
Screenshot 2023-03-14 at 11 55 54 AM

@jathak
Copy link
Member

jathak commented Mar 14, 2023

Yeah, it looks like the root cause of this might be the call to getText here: https://github.com/sass/dart-sass/blob/main/lib/src/interpolation_map.dart#L156. Going to check if changing that to use a SpanScanner improves things Nevermind, SpanScanner just calls getText itself, so no improvement there

@jathak
Copy link
Member

jathak commented Mar 14, 2023

The latest version of Dart Sass (1.59.3) should fix this from the tests I've run on both Node and the Dart VM, so please let me know if you're still encountering performance issues on 1.59.3 that weren't present on 1.58.x.

@Rezyan
Copy link
Author

Rezyan commented Mar 14, 2023

The latest version of Dart Sass (1.59.3) should fix this from the tests I've run on both Node and the Dart VM, so please let me know if you're still encountering performance issues on 1.59.3 that weren't present on 1.58.x.

@jathak I'm still encountering performance issues on v1.59.3 that weren't present on v1.58.3, but a little less than on v1.59.2. See the comparison table below:

Sass version Compilation time Compilation time difference from v1.58.3
1.58.3 28 seconds -
1.59.2 3 minutes and 55 seconds +3 minutes and 27 seconds
(x8.4)
1.59.3 1 minute +32 seconds
(x2.1)

1 59 3

@ntkme
Copy link
Contributor

ntkme commented Mar 14, 2023

@jathak

  • Each call to InterpolationMap.mapSpan results in 2 calls to FileSpan.getText (one for left and one for right).
  • In 1.59.2 InterpolationMap.mapSpan is called 4 times for a single interpolation, meaning 8 calls to FileSpan.getText.
  • In 1.59.3 LazyFileSpan is introduced so that we no longer call FileSpan.getText unless an exception is thrown or sourceMap is requested.
    • If sourceMap is requested, we still have 2 calls to FileSpan.getText for each interpolation... I confirmed this via CPU profiling. I think this is where @Elysiome is seeing 4x improvement (8 calls becomes 2 calls) but it is still a regression.

@ntkme
Copy link
Contributor

ntkme commented Mar 14, 2023

If we want to fix the regression without reverting the feature, we probably have to improve the performance of FileSpan.getText itself, or find a way to scan/seek the underneath buffer directly and bypass the weird String/List operations that FileSpan.getText does.

Not sure if it is feasible, maybe we can get span information for interpolation during parse stage and store it in AST, so that we don't have to backtrace it during compile stage.

@jathak
Copy link
Member

jathak commented Mar 14, 2023

Right, I forgot about source maps. I can think of a few possibilities that could speed things up on the margins, but I think it makes sense to wait until Natalie gets back on Monday to discuss with her about any more complicated changes here.

In the meantime, @Elysiome, given your compilation was already taking 28 seconds before this change, you may want to consider using the sass-embedded package instead of sass. It should be a drop-in replacement, and the speedup you get from running on the Dart VM instead of via Node could cancel out the slowdown here (and once we do improve it further, you'll get an even faster build than you had previously).

@Rezyan
Copy link
Author

Rezyan commented Mar 14, 2023

Right, I forgot about source maps. I can think of a few possibilities that could speed things up on the margins, but I think it makes sense to wait until Natalie gets back on Monday to discuss with her about any more complicated changes here.

In the meantime, @Elysiome, given your compilation was already taking 28 seconds before this change, you may want to consider using the sass-embedded package instead of sass. It should be a drop-in replacement, and the speedup you get from running on the Dart VM instead of via Node could cancel out the slowdown here (and once we do improve it further, you'll get an even faster build than you had previously).

Well, the npm package looks very easy to migrate. But I have to install Dart in order for sass to work, right? I'll try.

@ntkme
Copy link
Contributor

ntkme commented Mar 14, 2023

But I have to install Dart in order for sass to work, right?

The Dart VM is embedded in the npm package.

@Rezyan
Copy link
Author

Rezyan commented Mar 15, 2023

I've just tried sass-embedded package on v1.58.3 (the version I was using before the performance regression), but there are 2 problems:

  1. This generates an import error for one of the libraries I use (select2-bootstrap-5-theme):
Error in plugin "sass"
Message:
    node_modules\select2-bootstrap-5-theme\src\select2-bootstrap-5-theme.scss
Error: Can't find stylesheet to import.
  ╷
5 │ @import "node_modules/bootstrap/scss/functions";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  node_modules\select2-bootstrap-5-theme\src\select2-bootstrap-5-theme.scss 5:9  @import
  app\asset\sass\vendor\select2.scss 4:9       

select2-bootstrap-5-theme\src\select2-bootstrap-5-theme.scss:

/*!
 * Select2 v4 Bootstrap 5 theme v1.3.0
*/

@import "node_modules/bootstrap/scss/functions";
@import "node_modules/bootstrap/scss/variables";
@import "node_modules/bootstrap/scss/mixins";

@import "include-all";
  1. Even removing the problematic part (see 1.), the sass-embedded package is even slower than the sass package:
Sass package and version Compilation time Compilation time difference from sass@1.58.3
sass@1.58.3 28 seconds -
sass@1.59.2 3 minutes and 55 seconds +3 minutes and 27 seconds
(x8.4)
sass@1.59.3 1 minute +32 seconds
(x2.1)
―――――――――― ―――――――――― ――――――――――
sass-embedded@1.58.3 42 seconds +14 seconds
(x1.5)
sass-embedded@1.59.2 1 minute and 35 seconds +1 minute and 7 seconds
(x3.4)
sass-embedded@1.59.3 50 seconds +22 seconds
(x1.8)

What would you recommend to me, please? Should I use sass or migrate to sass-embedded (even if currently slower)? Furthermore, I don't know how I could solve the problem detailed in 1..

@ntkme
Copy link
Contributor

ntkme commented Mar 15, 2023

While sass-embedded is designed as a drop-in replacement, performance wise there are a few caveats:

  1. Do you know if your code is using legacy JS API (e.g. render or renderSync) or new JS API (e.g. compile or compileSync)? In sass-embedded the legacy API is emulated and have a huge performance impact if input files (including all dependencies) are large. Switching to the new JS API will improve it by a lot (to a degree that sass-embedded should be almost always faster for large compilation requests).

  2. Also, in sass, sync methods are faster than async, but in sass-embedded, it is the other way around that async methods are significantly faster than sync methods.

  3. sass-embedded will suffer if you have lots of different entry points, this has been improved by Improve launch performance by skipping wrapper script embedded-host-node#207, but still the cost is not zero.

Related performance issues:

@Rezyan
Copy link
Author

Rezyan commented Mar 15, 2023

While sass-embedded is designed as a drop-in replacement, performance wise there are a few caveats:

  1. Do you know if your code is using legacy JS API (e.g. render or renderSync) or new JS API (e.g. compile or compileSync)? In sass-embedded the legacy API is emulated and have a huge performance impact if input files (including all dependencies) are large. Switching to the new JS API will improve it by a lot (to a degree that sass-embedded should be almost always faster for large compilation requests).

I'm using gulp-sass, and according to the code, this library use the legacy JS API (e.g. render or renderSync):

  1. Also, in sass, sync methods are faster than async, but in sass-embedded, it is the other way around that async methods are significantly faster than sync methods.

I'm currently using the sync() method, because it was always faster with sass package.

  1. sass-embedded will suffer if you have lots of different entry points, this has been improved by Improve launch performance by skipping wrapper script embedded-host-node#207, but still the cost is not zero.

I've about 20 entry points, but a lots of partial files (at least 100) are used to render these 20 files.


By the way, do you have any idea for the 1. point describing an error during an import? Does the import works differently between sass and sass-embedded?

@ntkme
Copy link
Contributor

ntkme commented Mar 15, 2023

By the way, do you have any idea for the 1. point describing an error during an import? Does the import works differently between sass and sass-embedded?

I suspect there are some inconsistency in sass-embedded's legacy API emulation with the sass' implementation.

@s100
Copy link

s100 commented Mar 16, 2023

As of sass@1.59.3, our build is now running to completion in a perfectly timely fashion. I see that a few other people are still reporting performance regressions, but from our perspective this is fixed now. Thank you very much for your prompt response to this issue!

@nex3
Copy link
Contributor

nex3 commented Mar 20, 2023

@Elysiome Can you open a separate issue with a stand-alone reproduction of the import failure you're seeing?

@nex3
Copy link
Contributor

nex3 commented Mar 20, 2023

I'm going to re-open this as I look into a way to handle this even if spans are eagerly materialized. dart-lang/source_span#93 should allow us to expand these spans without allocating a bunch of large strings.

@alexander-xyz
Copy link

I use native dart sass for compilation big sass file. If I use --embed-source-map it compiles VERY slowly since from version 1.59.0, up to 1.60.0. Version 1.58.3 comile it in 1.5 sec, version 1.60.0 - 13 sec. Without source maps speed almost the same (but little bit longer maybe just for 100ms)

@kerryj89
Copy link

kerryj89 commented Apr 4, 2023

Can confirm that there's a noticeable slowdown as I finally upgraded from 1.45.1 to 1.60.0 today. I am running sass-cli to compile my Sass (one entrypoint file and --style expanded --load-path option pointed to node_modules). Compile went from around 5s to 1min 20s.


Downgrading to 1.58.3 brought it back down to 5s. The next version that I could install (1.59.1) took 7min 30s to compile. So while the latest version is drastically better than 1.59.1 was, it is still drastically slower than 1.58.3.

I am using Node v16.17.0. Going to downgrade to 1.58.3 for now.

nex3 added a commit that referenced this issue Apr 6, 2023
Instead of calling `SourceFile.getText()`, which creates string copies
of a substantial subset of the text of the file every time, this
directly accesses the file's underlying code units without doing any
copies.

Closes #1913
nex3 added a commit that referenced this issue Apr 6, 2023
Instead of calling `SourceFile.getText()`, which creates string copies
of a substantial subset of the text of the file every time, this
directly accesses the file's underlying code units without doing any
copies.

Closes #1913
@nex3 nex3 closed this as completed in #1929 Apr 6, 2023
nex3 added a commit that referenced this issue Apr 6, 2023
Instead of calling `SourceFile.getText()`, which creates string copies
of a substantial subset of the text of the file every time, this
directly accesses the file's underlying code units without doing any
copies.

Closes #1913
@nex3
Copy link
Contributor

nex3 commented Apr 6, 2023

1.61.0 is releasing which should fix this issue. If you're still running into performance issues relative to 1.58, please provide a minimal reproduction that shows off the bad performance and I'll take another look at it.

@Rezyan
Copy link
Author

Rezyan commented Apr 7, 2023

1.61.0 is releasing which should fix this issue. If you're still running into performance issues relative to 1.58, please provide a minimal reproduction that shows off the bad performance and I'll take another look at it.

@nex3 Since my previous comment, I have reorganized my SASS files:

  • Before:
    • I had no common files except for vendors.
    • Each web page loaded only 2 CSS files generated by SASS:
      • 1 file for the vendors style.
      • 1 file for the page style which included the website common style.
  • Now:
    • I have common files.
    • Each web page loads between 2 and 5 CSS files generated by SASS:
      • 1 file for the vendors style.
      • 1 file for the page style.
      • Between 0 and 3 files for the website common style.

Thus, the performance has been greatly improved (even with the v1.59 which had a really huge performance issue).

Here's a performance comparison table with my new SASS structure, in case you're interested:

Sass package and version Compilation time Compilation time difference from sass@1.58.3
sass@1.58.3 15 seconds -
sass@1.59.2 36 seconds +21 seconds
sass@1.59.3 27 seconds +12 seconds
sass@1.61.0 17 seconds +2 seconds
―――――――――― ―――――――――― ――――――――――
sass-embedded@1.58.3 31 seconds +16 seconds
sass-embedded@1.59.2 34 seconds +19 seconds
sass-embedded@1.59.3 28 seconds +13 seconds
sass-embedded@1.61.0 24 seconds +9 seconds

We see a slight drop in performance between v1.58.3 and v1.61.0, but it's not very significant. Thank you very much for improving the performance with each release. The issue has been solved for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants