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

Improve Sass compilation performance when importing all components #1752

Closed
wants to merge 3 commits into from

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Feb 27, 2020

We currently import the settings, tools and helpers layers within each component so that users can import just a single component (e.g. @import govuk/components/button/button) and it'll import its own dependencies.

However, Ruby Sass in particular is slow to resolve these imports, and so when importing govuk/all, the fact that every component imports the dependencies again has a big impact on the compilation time.

By moving everything except the imports into a separate file (_style.scss) and including that in the original component .scss file, we can then import just the styles when importing all and shortcut all of the imports.

We can use the same logic to remove imports of dependent components (e.g. character count imports the styles for textarea).

Finally, we can remove two redundant imports of the typography helper, as we already import all helpers.

Ruby Sass

Before: 30s
After: 7.8s

➜ sass --version
Ruby Sass 3.7.4

Baseline (master)

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  30.02s user 0.99s system 98% cpu 31.614 total

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  30.00s user 0.96s system 98% cpu 31.363 total

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  29.29s user 0.91s system 98% cpu 30.524 total

After changes (sass-perf)

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  7.80s user 0.35s system 98% cpu 8.304 total

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  7.89s user 0.35s system 98% cpu 8.321 total

➜ time sass -C src/govuk/all.scss > /dev/null
sass -C src/govuk/all.scss > /dev/null  7.68s user 0.32s system 99% cpu 8.049 total

LibSass

Before: 0.2s
After: 0.18s

➜ sassc -v
sassc: 3.6.1
libsass: 3.6.3
sass2scss: 1.1.1
sass: 3.5

Baseline (master)

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.20s user 0.02s system 98% cpu 0.224 total

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.20s user 0.02s system 98% cpu 0.224 total

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.20s user 0.02s system 99% cpu 0.224 total

After changes (sass-perf)

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.18s user 0.03s system 87% cpu 0.242 total

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.17s user 0.02s system 93% cpu 0.198 total

➜ time sassc src/govuk/all.scss > /dev/null
sassc src/govuk/all.scss > /dev/null  0.16s user 0.02s system 98% cpu 0.183 total

Dart Sass

Before: 0.5s
After: 0.25s

➜ /usr/local/Cellar/sass/1.23.7/bin/sass --version
1.23.7

Baseline (master)

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.53s user 0.22s system 100% cpu 0.743 total

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.50s user 0.21s system 99% cpu 0.714 total

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.53s user 0.23s system 100% cpu 0.754 total

After changes (sass-perf)

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.23s user 0.10s system 101% cpu 0.331 total

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.24s user 0.10s system 101% cpu 0.332 total

➜ time /usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null
/usr/local/Cellar/sass/1.23.7/bin/sass src/govuk/all.scss > /dev/null  0.24s user 0.10s system 101% cpu 0.340 total

We currently import the settings, tools and helpers layers within each component so that users can import just a single component (e.g. `@import govuk/components/button/button`) and it'll import its own dependencies.

However, Ruby Sass in particular is slow to resolve these imports, and so when importing `govuk/all`, the fact that every component imports the dependencies again has a big impact on the compilation time.

By moving everything except the imports into a separate file (`_style.scss`) and including that in the original component .scss file, we can then import just the styles when importing all and shortcut all of the imports.
We already import `helpers/all`, so we don't need to then import the typography helpers separately.
@NickColley
Copy link
Contributor

NickColley commented Feb 27, 2020

Apart from it being a bit weird it seems like a good way forward, nice thinking.

custom build

What should people do to get a performant custom build.
Should we deprecate the current button/button import pattern entirely?
Maybe not worth the trouble unless people run into it.

Explaining the two files

Should we have documentation explaining why we have two files?
I don't think we do this for Nunjucks perhaps so maybe this is a nice to have.

Does this solve the original issue?

There seems like big improvements on ruby sass, but we know the issues are more to do Rails wrappers running ruby code regardless of the compiler, since even the libsass sassc-rails implementation seemed to have issues.

Should we run this benchmark against something more real world?

Is this a breaking change?

This might be a breaking change for any folks (low risk) that import only @components/all? I think it's a low impact breaking change but may need to go into 4.0.0

@36degrees
Copy link
Contributor Author

Just on the last point whilst I think about the others…

  • seems like big improvements on ruby sass, but we know the issues are more to do Rails wrappers running ruby code regardless of the compiler, since even the libsass sassc-rails implementation seemed to have issues. Should we run this benchmark against something more real world?

I think the next step might be to publish this as a pre-release and ask GOV.UK to do some benchmarking to understand if it improves things for them?

@36degrees
Copy link
Contributor Author

This might be a breaking change for any folks (low risk) that import only @components/all? I think it's a low impact breaking change but may need to go into 4.0.0

I guess we could add a single import of the three layers to components/all to mitigate that, if we wanted to? It'd reduce performance slightly again, but should still have a significant positive impact overall.

@36degrees
Copy link
Contributor Author

What should people do to get a performant custom build.
Should we deprecate the current button/button import pattern entirely?
Maybe not worth the trouble unless people run into it.

Good question. I think I'd expect people to start with a copy of govuk/all.scss and customise it to be more specific / remove the bits they don't need?

@import "settings/all";
@import "tools/all";
@import "helpers/all";

@import "core/all";
@import "objects/all";

@import "components/button/style";
@import "components/input/style";
@import "components/header/style";
@import "components/footer/style";

@import "utilities/all";
// Removed overrides as not using them

The use of /style does seem a bit strange, but maybe no more so than button/button

Another option might be just to make this a proper breaking change, and remove the ability to import a single component without having to also import the settings, tools and helpers layers. I'm not sure what we'd do about dependent components though?

I guess we should also bear in mind that at some point in the next 12 - 18 months we'll probably want to look at moving to the @uses syntax anyway?

@NickColley
Copy link
Contributor

NickColley commented Feb 27, 2020

I think we should be taking more time to consider deprecation, some things we can't communicate deprecation that easily but Sass we often can do so pretty well with warnings. That said it might be that we just leave it as is until @uses yes...

@36degrees
Copy link
Contributor Author

Having reviewed this as a team, we think that although it should be a non-breaking change for most users, it:

  • increases the complexity of the codebase
  • adds yet another way that users could import GOV.UK Frontend
  • does nothing to improve compilation performance for users that are importing individual components, e.g. @import govuk/components/button/button
  • would be a breaking change for anyone importing govuk/components/all without also importing the underlying layers (which we think would be an unusual way to import it, but should currently work)

On that basis, we now think that the best approach would be to remove the settings, tools and helpers imports from the components entirely, and update our documentation to tell users to import those layers if they are importing individual components.

There's more information on next steps in #1671 (comment).

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

Successfully merging this pull request may close these issues.

3 participants