CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds #280

evanweible-wf · 2019-04-01T17:21:39Z

CPLAT-5152

Motivation

We've seen unexpectedly large rebuild times in codebases that consume this package and its builder. After some digging, it has been determined that the main reason for this is due to how the over_react builder is currently implemented:

The PartBuilder util from the source_gen package always retrieves the LibraryElement for each file that is a library (i.e. not a part file). This requires that the analysis session resolves the entire element tree for that file, which is expensive but more importantly results in reads on all transitively imported files. These reads are then stored in the asset graph as inputs for that build step, which consequently means that the over_react builder will be re-run on many files unnecessarily because the inputs have changed.

Changes

The over_react builder only needs the AST for each file, so the solution here is to replace our usage of PartBuilder with a custom Builder that manually reads the library source for each BuildStep and calls the analyzer API parseCompilationUnit() to get the info needed to generate the .over_react.g.dart part files.

Perf Improvement

Some numbers to demonstrate the performance improvement delivered by this change:

Average clean build time:

Averaged over 5 runs.
pub run build_runner clean && pub run build_runner build

Project	Before	After
web_skin_dart	122s	107s
wdesk_sdk	112s	45s

Average re-build time:

Averaged over 5 runs. Rebuilt after changing one entry point file in lib/
that has a large number of imports/exports.

Change file -> pub run build_runner build

Project	File	Before	After
web_skin_dart	`lib/ui_core.dart`	42.9s	21.22s
wdesk_sdk	`lib/app_infrastructure.dart`	63s	2.2s

Release Notes

Optimize the over_react builder to avoid unnecessary asset reads. Informal testing in a large codebase that consumes over_react has shown in the worst case a speed up on hot-rebuilds from ~1 minute to ~2 seconds.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

The PartBuilder util from the source_gen package always retrieves the LibraryElement for each file that is a library (i.e. not a part file). This requires that the analysis session resolves the entire element tree for that file, which is expensive but more importantly results in reads on all transitively imported files. These reads are then stored in the asset graph as inputs for that build step, which consequently means that the over_react builder will be re-run on many files unnecessarily because the inputs have changed. The over_react builder only needs the AST for each file, so the solution here is to replace our usage of PartBuilder with a custom Builder that manually reads the library source for each BuildStep and calls the analyzer API `parseCompilationUnit()` to get the info needed to generate the `.over_react.g.dart` part files. In preliminary testing, we've seen this approach reduce the rebuild time from ~1 minute to ~7 seconds when changing a single file in a large library that has many transitive imports.

The build_web_compilers|entrypoint builder does not need to look at any files in lib/. For the test/ directory it only needs to consider the .browser_test.dart extension as entry points - doing so prevents DDC/dart2js calls on the source _test.dart files as well as the non-browser test bootstrap files (.node_test.dart and .vm_test.dart).

aviary3-wk · 2019-04-01T17:22:00Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

corwinsheahan-wf

This is very cool. I had a couple of comments/questions.

corwinsheahan-wf · 2019-04-02T02:23:48Z

lib/src/builder/builder.dart

+    final parts = libraryUnit.directives
+      .whereType<PartDirective>()
+      // Ignore `.over_react.g.dart` parts - that's what we're generating.
+      .where((part) => !part.uri.stringValue.endsWith(outputExtension));


We may also need to ignore parts from other generated files. That was an issue with the old approach, did you verify that it's not an issue here?

I didn't see any issues, but this reminds me that I meant to add an another check for each part directive so that we only parse the compilation units if the part file contains over react declarations. Currently this is only doing that check on the parent library file

I think it would be safer to ignore anything ending in .g.dart and explicitly not support over_react annotations in generated files. We could run into a situation where we're parsing a file that doesn't exist yet.

corwinsheahan-wf · 2019-04-02T02:25:51Z

lib/src/builder/builder.dart

+        partSource,
+        url: idToPackageUri(partId));
+      CompilationUnit partUnit;
+      try {


This code is very similar to the parsing and generating for the main library code above. What's the reason not to DRY this up?

Yeah, the main difference is that I have to parse the compilation unit for the library earlier so that I can check the directives for a PartOfDirective (i.e. to see if the file is actually a part file) so that we can bail early. I can probably dry it up though

corwinsheahan-wf · 2019-04-02T02:30:15Z

lib/src/builder/builder.dart

+      ..writeln('part of $partOf;')
+      ..writeln()
+      ..writeln(_headerLine)
+      ..writeln('// OverReactGenerator')


#nit Personally, I'd advocate for not using the same header as the generator. It could lead to confusion if a consumer is examining our outputs.

Fair enough, I was going for minimal changes to generated code but I guess that doesn't really matter since we're building to cache.

greglittlefield-wf

+1

Have we let the Dart team know about this performance issue?

evanweible-wf · 2019-04-02T15:46:32Z

@greglittlefield-wf @corwinsheahan-wf Just pushed a commit to tidy up the builder implementation a bit.

Have we let the Dart team know about this performance issue?

Not yet, I want to do a few more tests with a simpler builder and simpler codebase that still uses buildStep.inputLibrary and then I plan on asking in their gitter chat about that cost in general. Will probably end up opening an issue in the build project around documenting those potential pitfalls as well as an issue in source_gen since there is currently no way to opt out of that behavior with their utils.

lib/src/builder/builder.dart

- DRY up logic around parsing the compilation unit, the over_react declarations, and generating the over_react implementation code. - Ignore all .g.dart part files. - Only parse/generate for files that have over_react annotations.

lib/src/builder/builder.dart

evanweible-wf · 2019-04-04T17:01:43Z

Some numbers to demonstrate the performance improvement delivered by this change:

Average clean build time:

Averaged over 5 runs.
pub run build_runner clean && pub run build_runner build

Project	Before	After
web_skin_dart	122s	107s
wdesk_sdk	112s	45s

Average re-build time:

Averaged over 5 runs. Rebuilt after changing one entry point file in lib/
that has a large number of imports/exports.

Change file -> pub run build_runner build

Project	File	Before	After
web_skin_dart	`lib/ui_core.dart`	42.9s	21.22s
wdesk_sdk	`lib/app_infrastructure.dart`	63s	2.2s

greglittlefield-wf · 2019-04-04T17:10:08Z

lib/src/builder/builder.dart

@@ -21,10 +21,6 @@ class OverReactBuilder extends Builder {
  @override
  FutureOr<void> build(BuildStep buildStep) async {
    final source = await buildStep.readAsString(buildStep.inputId);
-    if (!_mightContainDeclarations(source)) {


Why was this removed? I would imagine performing this regex match is faster than parsing the compilation unit. Or, is that not the case?

This diff is outdated, but still applies to the new version of the file

Without this change, we are no longer providing a warning in the scenario where a consumer has over_react-annotated code but did not include the .over_react.g.dart part file (which is something that the PartBuilder from source_gen did for us).

I didn't post these numbers but I did run a comparison between this commit and the previous commit and there was no noticeable difference in clean build or rebuild times on WSD or wdesk_sdk. I was a bit surprised by that, but my best guess is that the file reads and the AST parsing are cached.. or maybe they're just both fast? I'm not totally sure.

That's a bit surprising to me, too. I know file reads are pretty fast, but I expected AST parsing to not be super cheap. 🤷‍♂️

I'm fine with it as-is, then!

evanweible-wf · 2019-04-04T20:43:39Z

QA +1

CI passes
Consumer tested with WSD and wdesk_sdk

evanweible-wf · 2019-04-04T20:44:52Z

@Workiva/release-management-p

+ Without the use of fully-resolved AST since we no longer have access to it for build perf reasons (see: #280)

evanweible-wf added 3 commits April 1, 2019 11:04

Update generated assets.

54a17c0

rmconsole-wf added the Merge Requirements Unmet label Apr 1, 2019

rmconsole5-wk changed the title ~~Optimize builder's use of asset reads to avoid unnecessary rebuilds~~ CPLAT-5152 Optimize builder's use of asset reads to avoid unnecessary rebuilds Apr 1, 2019

charliekump-wf changed the title ~~CPLAT-5152 Optimize builder's use of asset reads to avoid unnecessary rebuilds~~ CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds Apr 1, 2019

evanweible-wf requested review from corwinsheahan-wf and greglittlefield-wf April 1, 2019 17:39

evanweible-wf added 2 commits April 1, 2019 12:28

Remove now-unused source_gen Generator.

37977f4

Remove now-unused source_gen depenency.

b55ce60

evanweible-wf mentioned this pull request Apr 1, 2019

CPLAT-4003 Upgrade to analyzer ^0.35.0 #281

Merged

3 tasks

corwinsheahan-wf reviewed Apr 2, 2019

View reviewed changes

greglittlefield-wf approved these changes Apr 2, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met Merge Requirements Unmet and removed Merge Requirements Unmet Merge Requirements Met labels Apr 2, 2019

evanweible-wf commented Apr 2, 2019

View reviewed changes

lib/src/builder/builder.dart Outdated Show resolved Hide resolved

Improvements to builder implementation:

f24a057

- DRY up logic around parsing the compilation unit, the over_react declarations, and generating the over_react implementation code. - Ignore all .g.dart part files. - Only parse/generate for files that have over_react annotations.

evanweible-wf force-pushed the optimize_builder branch from 7ae6a6c to f24a057 Compare April 2, 2019 15:55

corwinsheahan-wf reviewed Apr 2, 2019

View reviewed changes

lib/src/builder/builder.dart Outdated Show resolved Hide resolved

Warn if .over_react.g.dart part is missing.

93d4593

corwinsheahan-wf approved these changes Apr 4, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 4, 2019

greglittlefield-wf reviewed Apr 4, 2019

View reviewed changes

greglittlefield-wf approved these changes Apr 4, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Apr 4, 2019

rmconsole6-wk merged commit 02ea76b into master Apr 4, 2019

rmconsole6-wk deleted the optimize_builder branch April 4, 2019 20:45

rmconsole-readonly-wk mentioned this pull request Apr 5, 2019

CPLAT-4956 CPLAT-4955 Release over_react 2.4.0+dart2 #283

Merged

aaronlademann-wf added a commit that referenced this pull request May 1, 2019

Update builder to differentiate Component2 annotations

8bc90b1

+ Without the use of fully-resolved AST since we no longer have access to it for build perf reasons (see: #280)

aaronlademann-wf mentioned this pull request May 1, 2019

CPLAT-5211 Add @Component2 annotation #289

Merged

9 tasks

aaronlademann-wf added a commit that referenced this pull request May 2, 2019

Update builder to differentiate Component2 annotations

1cd24b1

+ Without the use of fully-resolved AST since we no longer have access to it for build perf reasons (see: #280)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds #280

CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds #280

evanweible-wf commented Apr 1, 2019 •

edited

Loading

aviary3-wk commented Apr 1, 2019

corwinsheahan-wf left a comment

corwinsheahan-wf Apr 2, 2019

evanweible-wf Apr 2, 2019

corwinsheahan-wf Apr 2, 2019

corwinsheahan-wf Apr 2, 2019

evanweible-wf Apr 2, 2019

corwinsheahan-wf Apr 2, 2019

evanweible-wf Apr 2, 2019

greglittlefield-wf left a comment

evanweible-wf commented Apr 2, 2019 •

edited

Loading

evanweible-wf commented Apr 4, 2019 •

edited

Loading

greglittlefield-wf Apr 4, 2019

greglittlefield-wf Apr 4, 2019

evanweible-wf Apr 4, 2019 •

edited

Loading

greglittlefield-wf Apr 4, 2019

evanweible-wf commented Apr 4, 2019

evanweible-wf commented Apr 4, 2019

CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds #280

CPLAT-5152: Optimize builder's use of asset reads to avoid unnecessary rebuilds #280

Conversation

evanweible-wf commented Apr 1, 2019 • edited Loading

Motivation

Changes

Perf Improvement

Release Notes

Review

QA Checklist

Merge Checklist

aviary3-wk commented Apr 1, 2019

Security Insights

Action Items

corwinsheahan-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

evanweible-wf commented Apr 2, 2019 • edited Loading

evanweible-wf commented Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanweible-wf Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanweible-wf commented Apr 4, 2019

evanweible-wf commented Apr 4, 2019

evanweible-wf commented Apr 1, 2019 •

edited

Loading

evanweible-wf commented Apr 2, 2019 •

edited

Loading

evanweible-wf commented Apr 4, 2019 •

edited

Loading

evanweible-wf Apr 4, 2019 •

edited

Loading