[sass] Improve compilation speed using a different pow implementation by markov00 · Pull Request #5674 · elastic/eui

markov00 · 2022-03-01T23:14:59Z

Summary

This PR reduces the SASS compilation times using a different math.pow implementation code.

I've noticed that compiling @elastic/charts SASS files (that imports some mixins and variables from EUI) takes a very long time.
I've tried to debug the status of the SASS compilation and I've found that the time spent on the pow function available in src/global_styling/functions/_math.scss is huge and slow down a lot the compilation.

I tried to replace the pow operation with the implementation coming from https://github.com/strarsis/sass-math-pow and the speed gain was significant to consider opening a draft/testing PR here.

In particular I've tested this assumption both with sass and with dart-sass (the native dart version, not the js port) to compile our sass styles with the following results:

from:

sass src/theme_light.scss dist/theme_light.css  5.66s user 0.10s system 98% cpu 5.871 total

to:

sass src/theme_light.scss dist/theme_light.css  0.54s user 0.06s system 100% cpu 0.591 total

a 10x speedup

Locally I've tested the same compiling EUI and these are the results:

from:

to:

~~I'd like to run this PR to check the difference in a CI run (I've checked one of the latest PR and the compilation time was around 110seconds).~~ UPDATE:
In this PR the SCSS compilation took 22 seconds vs the ~120seconds in the current master

from (I've picked the one of the latest PR on the repo):

04:08:16 $ node ./scripts/compile-scss.js $npm_package_name
04:08:17 … Compiling src/themes/legacy/legacy_dark.scss
04:08:17 … Compiling src/themes/legacy/legacy_light.scss
04:08:17 … Compiling src/themes/amsterdam/theme_dark.scss
04:08:17 … Compiling src/themes/amsterdam/theme_light.scss
04:09:07 Browserslist: caniuse-lite is outdated. Please run:
04:09:07 npx browserslist@latest --update-db
04:09:12 ✔ Finished compiling src/themes/legacy/legacy_dark.scss to dist/eui_legacy_dark.css, dist/eui_legacy_dark.min.css, dist/eui_legacy_dark.json, dist/eui_legacy_dark.json.d.ts, src-docs/src/views/theme/_json/eui_legacy_dark.json
04:09:27 ✔ Finished compiling src/themes/amsterdam/theme_dark.scss to dist/eui_theme_dark.css, dist/eui_theme_dark.min.css, dist/eui_theme_dark.json, dist/eui_theme_dark.json.d.ts, src-docs/src/views/theme/_json/eui_theme_dark.json
04:09:53 ✔ Finished compiling src/themes/legacy/legacy_light.scss to dist/eui_legacy_light.css, dist/eui_legacy_light.min.css, dist/eui_legacy_light.json, dist/eui_legacy_light.json.d.ts, src-docs/src/views/theme/_json/eui_legacy_light.json
04:10:18 ✔ Finished compiling src/themes/amsterdam/theme_light.scss to dist/eui_theme_light.css, dist/eui_theme_light.min.css, dist/eui_theme_light.json, dist/eui_theme_light.json.d.ts, src-docs/src/views/theme/_json/eui_theme_light.json
04:10:18 Done in 122.44s.

to:

01:01:36 $ node ./scripts/compile-scss.js $npm_package_name
01:01:38 … Compiling src/themes/legacy/legacy_dark.scss
01:01:38 … Compiling src/themes/legacy/legacy_light.scss
01:01:38 … Compiling src/themes/amsterdam/theme_dark.scss
01:01:38 … Compiling src/themes/amsterdam/theme_light.scss
01:01:48 Browserslist: caniuse-lite is outdated. Please run:
01:01:48 npx browserslist@latest --update-db
01:01:51 ✔ Finished compiling src/themes/legacy/legacy_dark.scss to dist/eui_legacy_dark.css, dist/eui_legacy_dark.min.css, dist/eui_legacy_dark.json, dist/eui_legacy_dark.json.d.ts, src-docs/src/views/theme/_json/eui_legacy_dark.json
01:01:53 ✔ Finished compiling src/themes/amsterdam/theme_dark.scss to dist/eui_theme_dark.css, dist/eui_theme_dark.min.css, dist/eui_theme_dark.json, dist/eui_theme_dark.json.d.ts, src-docs/src/views/theme/_json/eui_theme_dark.json
01:01:55 ✔ Finished compiling src/themes/legacy/legacy_light.scss to dist/eui_legacy_light.css, dist/eui_legacy_light.min.css, dist/eui_legacy_light.json, dist/eui_legacy_light.json.d.ts, src-docs/src/views/theme/_json/eui_legacy_light.json
01:01:58 ✔ Finished compiling src/themes/amsterdam/theme_light.scss to dist/eui_theme_light.css, dist/eui_theme_light.min.css, dist/eui_theme_light.json, dist/eui_theme_light.json.d.ts, src-docs/src/views/theme/_json/eui_theme_light.json
01:01:58 Done in 22.10s.

Checklist

kibanamachine · 2022-03-01T23:43:36Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5674/

snide · 2022-03-02T01:23:59Z

Interesting! I had to write this a long, long time ago. As I remember, I needed this for ways to calculate luminance and respective contrast ratios. This among many other reasons are why I'm excited to move to a CSS-in-JS world and leave Sass behind.

On a cursory check I'm seeing no changes in the contrast calculations so this looks OK at least from an output standpoint on the Sass.

Here's the blog I wrote about doing all this in Sass, which felt pretty novel at the time.
https://www.elastic.co/blog/keeping-up-with-kibana-2018-05-02

markov00 · 2022-03-02T08:29:36Z

Interesting! I had to write this a long, long time ago. As I remember, I needed this for ways to calculate luminance and respective contrast ratios. This among many other reasons are why I'm excited to move to a CSS-in-JS world and leave Sass behind.
On a cursory check I'm seeing no changes in the contrast calculations so this looks OK at least from an output standpoint on the Sass.

Here's the blog I wrote about doing all this in Sass, which felt pretty novel at the time. https://www.elastic.co/blog/keeping-up-with-kibana-2018-05-02

Thanks for sharing, I also saw that the pow calculation was changed probably since your initial implementation by a community member here #3013

I hope, SASS or not, we can soon switch to the new proposal for computing the contrast ratios called APCA that will be the default adopted in the WCAG 3 (currently in a draft state)

thompsongl · 2022-03-02T16:39:03Z

🚀
Seems to make sense to do:

Install sass-math-pow
Replace our custom pow function with @import "~sass-math-pow/sass/math-pow";
Update usages of pow with poly-pow

cchaos · 2022-03-02T16:41:50Z

Also an easy way to test that this still results in the same output values (or at least better if not similar since I've found sometimes the calculations are slightly off) is to run the Sass to JSON script and see the diffs.

markov00 · 2022-03-02T16:48:40Z

@cchaos I've just verified it right now checking the MD5 of the resulting files and they all coincide (except for the non minified version due to slight different indentation) but min and json are the same

MD5 (./eui_theme_dark.min.css) = 0f760e940251f1f7649462f5513a4ded
MD5 (./eui_theme_dark.json) = 49662e02a44e2ad2553d5890cb4e8b0d
MD5 (./eui_theme_dark.json.d.ts) = 14f759f96c4c361e62d68a111ccb48e3

MD5 (./eui_theme_light.min.css) = e30c61d0d5ea855b059a985ca965993c
MD5 (./eui_theme_light.json) = 4755f762d8405736c11b5448eb9e811b
MD5 (./eui_theme_light.json.d.ts) = dda335d8f5581e741824f7600d27d522

MD5 (./eui_legacy_dark.min.css) = 1777b7402597bd776f4030eaa9dc4d15
MD5 (./eui_legacy_dark.json) = 57d5a3d31acfef39eee7b456a908a46f
MD5 (./eui_legacy_dark.json.d.ts) = 967c2d947c2bd4c69ce7960111353b31

MD5 (./eui_legacy_light.min.css) = 0eec7bc83cbe6d764e0df7a126eff3c9
MD5 (./eui_legacy_light.json) = 5982861d35008880157fd4750723a15a
MD5 (./eui_legacy_light.json.d.ts) = 2722ca6eec780696c20e6917f5aeb020

markov00 · 2022-03-02T16:51:04Z

🚀 Seems to make sense to do:

Install sass-math-pow

Replace our custom pow function with @import "~sass-math-pow/sass/math-pow";

Update usages of pow with poly-pow

Thanks @thompsongl In fact I was going to ask what was is your preference in this case, adding the dependency or copying the files. Happy to change it.

~~Is ok if I skip the sass lint rule for camelCase function names in this case? because it can be triggered by poly-pow~~

cchaos · 2022-03-02T16:58:57Z

What is an MD5? 😬

markov00 · 2022-03-02T17:00:07Z

@thompsongl I've tried importing it via using ~ but it only works with a sass-loader that is available under webpack. the compile-scss script doesn't have that loader so I need to use a relative path like

@import "../../../node_modules/sass-math-pow/sass/math-pow";

markov00 · 2022-03-02T17:02:13Z

What is an MD5? 😬

Is a simple hash value computed from the file, it's used to compare if the content of two files are the same

thompsongl · 2022-03-02T17:30:35Z

I've tried importing it via using ~ but it only works with a sass-loader that is available under webpack. the compile-scss script doesn't have that loader so I need to use a relative path like
@import "../../../node_modules/sass-math-pow/sass/math-pow";

Just synced with Marco. The Sass file import will cause problems for consumers using targeted imports.
I think we pull in the pow function as a separate file along with the MIT license header (but remove the addition to notice.txt).
Sound ok @chandlerprall?

chandlerprall · 2022-03-02T19:08:53Z

Sounds great!

kibanamachine · 2022-03-03T10:34:08Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5674/

kibanamachine · 2022-03-03T12:20:55Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5674/

thompsongl

Pushed one organization commit, but the rest looks great; thanks @markov00!

Confirmed that the compiled CSS does not change with the new function.

kibanamachine · 2022-03-03T16:27:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5674/

spalger · 2022-03-21T16:11:55Z

I just want to say, this had an extraordinary impact on the Kibana bundling time... Is there a chance that we can make some sort of effort to maintain this improvement?

thompsongl · 2022-03-21T16:22:59Z

The code is in the EUI repo so we can make any necessary changes. But I'm not sure what that maintenance looks like after we convert everything to CSS-in-JS and no longer use Sass.

Test replacement of current SASS pow implementation

9e1adc5

markov00 added 2 commits March 2, 2022 18:33

Moved the pow function to its own file

1b4c763

Removed the license from the NOTICE file

94d66ee

nickofthyme mentioned this pull request Mar 2, 2022

feat(styles): eui Amsterdam theme elastic/elastic-charts#1463

Merged

6 tasks

Merge branch 'main' into 2022_03_01-improve_pow

7a89198

Updated CHANGELOG.md

19338cb

markov00 marked this pull request as ready for review March 3, 2022 13:43

markov00 requested a review from thompsongl March 3, 2022 13:44

keep math.scss

da8e861

thompsongl approved these changes Mar 3, 2022

View reviewed changes

markov00 merged commit 20592c1 into elastic:main Mar 3, 2022

markov00 deleted the 2022_03_01-improve_pow branch March 3, 2022 17:04

thompsongl mentioned this pull request Mar 4, 2022

Upgrade EUI to v51.1.0 elastic/kibana#126926

Merged

Conversation

markov00 commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Mar 1, 2022

Uh oh!

snide commented Mar 2, 2022

Uh oh!

markov00 commented Mar 2, 2022

Uh oh!

thompsongl commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Mar 2, 2022

Uh oh!

markov00 commented Mar 2, 2022

Uh oh!

markov00 commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Mar 2, 2022

Uh oh!

markov00 commented Mar 2, 2022

Uh oh!

markov00 commented Mar 2, 2022

Uh oh!

thompsongl commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chandlerprall commented Mar 2, 2022

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 3, 2022

Uh oh!

spalger commented Mar 21, 2022

Uh oh!

thompsongl commented Mar 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

markov00 commented Mar 1, 2022 •

edited

Loading

thompsongl commented Mar 2, 2022 •

edited

Loading

markov00 commented Mar 2, 2022 •

edited

Loading

thompsongl commented Mar 2, 2022 •

edited

Loading