Open aaronlademann-wf opened 5 years ago
I suspect the real issue here isn't so much that something in particular is wrong with maps in asynchronous mode, but that working asynchronously is slow in general (to what degree this is an issue with the Dart VM's implementation of async
/await
versus a fundamental issue with the asynchronous paradigm, I don't know). Map operations likely just involve a higher number of function calls for whatever reason, which are substantially more expensive asynchronously.
I think the best solution here is to just not call compileAsync()
. The Dart VM provides the waitFor()
function, and I strongly recommend that performance-sensitive users that need asynchronous behavior in importers or functions use it to call compile()
instead.
@nex3 the problem we're facing is that we're utilizing the sass_builder
package so that we can use package:<some_dependency>/<some_scss_file>
imports, and sass_builder
uses compileAsync
.
I am in the process of attempting to write a standalone dart script that calls compile
, but I am currently stuck on figuring out why relative imports within the .scss
file imported by another package via a package:
import don't work.
I wonder if the sass_builder
could utilize waitFor
instead of compileAsync
?
@nex3 I have an implementation of sass_builder
locally that is now calling sass.compile()
after only a single await
within its Builder.build()
method:
final buildStepAsString = await buildStep.readAsString(inputId);
final cssOutput = sass.compileString(
buildStepAsString,
syntax: sass.Syntax.forPath(inputId.path),
importers: [new BuildImporter(buildStep, buildStepAsString)],
style: _getValidOutputStyle());
Unfortunately, the performance implications are the same.
So I guess it begs the question... if async
is really this slow at IO operations, why would the new Dart build system be asynchronous? It seems - unless I'm missing something - that there are some fairly major performance issues within the build system pertaining to parsing / locating Uri
s. This also makes me think that its not a matter of any particular Sass construct that is slow (as you correctly alluded to) - but rather the sheer number of / the granularity of our @import
s that could be what is causing such an extreme performance degradation.
cc/ @kevmoo @robbecker-wf @evanweible-wf
the problem we're facing is that we're utilizing the
sass_builder
package so that we can usepackage:<some_dependency>/<some_scss_file>
imports, andsass_builder
usescompileAsync
.
There may be other reasons to use sass_builder
, but for what it's worth, Dart Sass does natively support package:
imports (when running from Dart source). All you have to do is pass the packageResolver
option to compile()
or compileAsync()
.
I have an implementation of
sass_builder
locally that is now callingsass.compile()
after only a singleawait
within itsBuilder.build()
method:final buildStepAsString = await buildStep.readAsString(inputId); final cssOutput = sass.compileString( buildStepAsString, syntax: sass.Syntax.forPath(inputId.path), importers: [new BuildImporter(buildStep, buildStepAsString)], style: _getValidOutputStyle());
Unfortunately, the performance implications are the same.
Something doesn't add up here. If I understand you right, you're saying that:
compileStringAsync()
takes about 15s.compileString()
takes about 3s.sass_builder
with compileStringAsync()
takes about 15s.sass_builder
with compileString()
still takes about 15s.Something must be strange about your sass_builder
test setup, because even if BuildImporter
causes compileString()
to be very slow, compileStringAsync()
should be yet slower. Asynchrony adds substantial overhead even in the relatively quick case of the Bootstrap tests, and that should be even more visible if you're doing a bunch of map operations. I wonder if somehow your Builder.build()
edit didn't get picked up by the build system?
As an aside: I know you said that the Workiva Sass files aren't open source, but if you'd ever be willing to open source them (or a portion of them that uses maps heavily), they'd be very valuable as a benchmark resource that we can use to test the effect of changes on real-world Sass stylesheets.
@nex3
Dart Sass does natively support package: imports (when running from Dart source)
I've tried creating a standalone compiler that utilizes packageResolver
- but it seems that relative paths are broken once the compiler enters a file that was imported via a package:
import.
// package_two/sass/app.scss
@import 'package:package_one/sass/api';
// package_one/sass/_api.scss
.foo {
color: rebeccapurple;
}
// Throws "Can't find stylesheet to import." when _api.scss is imported by package_two
// > https://github.com/sass/dart-sass/blob/a0e63ac704c6c98c8a9123872e3a8c1f312bb2ed/lib/src/visitor/evaluate.dart#L859
@import '../some/relative/path/_some_partial.scss';
Can you file a separate issue for that? Relative imports are definitely supposed to work there.
@nex3 I filed #631 for the relative path issue along with a reduced test case you can pull down locally.
@aaronlademann-wf Do you have any more questions I can answer, or should I close this issue?
Hi Natalie!
I have been tasked with troubleshooting some substandard
pub build_runner build
times within Workiva's (massive) Dart application ecosystem as we try to finish our transition to the Dart 2 SDK.One of the things that takes a very long time is compiling CSS in packages that depend on
sass_builder
. I have ruled outsass_builder
itself as the culprit by confirming that similarly sluggish.scss
compile times can be reproduced in our ecosystem by callingsass.compileAsync()
directly instead of relying onsass_builder
to make that call.Unfortunately, none of the Workiva
.scss
source code is open sourced - but I will do my best to explain the situation in hopes that you might be able to give me some clues about which features we might be using in excess that would be most likely to result in such an enormous compile time delta compared tosass.compile()
.Raw Numbers
As a control for my troubleshooting, I have set up a little project locally that has all of the Twitter Bootstrap
.scss
files, and then I compilebootstrap.scss
using bothsass.compile()
andsass.compileAsync()
.Compile Time For Control:
bootstrap.scss
compile()
compileAsync()
As you can see, while slightly slower (~1.25x) - its certainly not earth-shaking.
Compile Time For an Internal Workiva Sass File Similar to Bootstrap
compile()
compileAsync()
As you can see, async compilation for Workiva's "bootstrap" that we use is 5x slower.
Differences Between Workiva's "Bootstrap" and the original Twitter Bootstrap
The biggest difference I can speak to with certainty when comparing Workiva's "bootstrap" to Twitter bootstrap is that we make extensive use of Sass Maps for variables / configuration. Many of them have nested Maps - and we utilize
map-get
/map-get-deep
extensively to recursively access nested key/value pairs.The other difference is that there is significantly more granularity when it comes to the number of
.scss
partials in Workiva's "bootstrap" source.Since the extent of our Sass Map usage is such that even just spiking out the performance implications of removing them would take many days (possibly weeks) - not hours, I'm trying to take a very measured approach to any further efforts to reduce compilation time.
And so, what I'm hoping to get from you is your expert opinion on the likelihood of those two things having a clear causal relationship with the significant increase in compile time when calling
compileAsync
vs.compile
.Your opinion(s) will help me "sell" with a reasonable amount of confidence any potential refactors / spikes to internal stakeholders.
Thank you so much for your time @nex3 !!!
cc/ @evanweible-wf @robbecker-wf @greglittlefield-wf @joelleibow-wf