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

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

Merged
merged 7 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ targets:
exclude:
# This tests un-built behavior, and therefore should not be built
- "test/over_react/component_declaration/builder_helpers_test.dart"

over_react|overReactBuilder:
enabled: false

Expand All @@ -37,13 +38,9 @@ targets:
- "web/**"
exclude:
- "lib/src/builder/**"

build_web_compilers|entrypoint:
generate_for:
include:
- "example/**"
- "lib/*.dart"
- "test/*.dart"
- "web/*.dart"
exclude:
- "lib/src/builder/**"

- "example/**.dart"
- "test/*.browser_test.dart"
- "web/*.dart"
2 changes: 1 addition & 1 deletion example/builder/basic_library.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

144 changes: 140 additions & 4 deletions lib/src/builder/builder.dart
Original file line number Diff line number Diff line change
@@ -1,8 +1,144 @@
import 'dart:async';

import 'package:analyzer/analyzer.dart';
import 'package:build/build.dart';
import 'package:source_gen/source_gen.dart';
import 'package:dart_style/dart_style.dart';
import 'package:over_react/src/builder/generation/declaration_parsing.dart';
import 'package:over_react/src/builder/generation/impl_generation.dart';
import 'package:path/path.dart' as p;
import 'package:source_span/source_span.dart';

import './util.dart';
import './generator.dart';

Builder overReactBuilder(BuilderOptions options) =>
new PartBuilder([new OverReactGenerator()], outputExtension);
Builder overReactBuilder(BuilderOptions options) => new OverReactBuilder();

class OverReactBuilder extends Builder {
static final _headerLine = '// '.padRight(77, '*');

static final _formatter = new DartFormatter();

static final RegExp _overReactPartDirective = new RegExp(
r'''['"].*\.over_react\.g\.dart['"]''',
);

static bool _mightContainDeclarations(String source) {
return ParsedDeclarations.key_any_annotation.hasMatch(source) ||
_overReactPartDirective.hasMatch(source);
}

@override
FutureOr<void> build(BuildStep buildStep) async {
final source = await buildStep.readAsString(buildStep.inputId);
if (!_mightContainDeclarations(source)) {
return;
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved
}

CompilationUnit libraryUnit;
try {
libraryUnit = parseCompilationUnit(
source,
name: buildStep.inputId.path,
suppressErrors: false,
parseFunctionBodies: true);
} catch (error, stackTrace) {
log.severe('There was an error parsing the compilation unit for file: ${buildStep.inputId}');
log.fine(error);
log.fine(stackTrace);
return;
}

if (isPart(libraryUnit)) {
return;
}

final librarySourceFile = new SourceFile.fromString(
source,
url: idToPackageUri(buildStep.inputId));
final libraryDeclarations = new ParsedDeclarations(
libraryUnit,
librarySourceFile,
log);
if (libraryDeclarations.hasErrors) {
log.severe('There was an error parsing the file declarations for file: ${buildStep.inputId}');
return;
}

final outputs = <String>[];
final libraryGenerator = new ImplGenerator(log, librarySourceFile)
..generate(libraryDeclarations);
outputs.add(libraryGenerator.outputContentsBuffer.toString());

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (final part in parts) {
final partId = new AssetId.resolve(
part.uri.stringValue,
from: buildStep.inputId);
final partSource = await buildStep.readAsString(partId);
final partSourceFile = new SourceFile.fromString(
partSource,
url: idToPackageUri(partId));
CompilationUnit partUnit;
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

partUnit = parseCompilationUnit(
partSource,
name: partId.path,
suppressErrors: false,
parseFunctionBodies: true);
} catch (error, stackTrace) {
log.severe('There was an error parsing the compilation unit for file: $partId');
log.fine(error);
log.fine(stackTrace);
return;
}
final partDeclarations = new ParsedDeclarations(partUnit, partSourceFile, log);
if (partDeclarations.hasErrors) {
log.severe('There was an error parsing the file declarations for file: ${buildStep.inputId}');
return;
}
final partGenerator = new ImplGenerator(log, partSourceFile)
..generate(partDeclarations);
outputs.add(partGenerator.outputContentsBuffer.toString());
}

final nonEmptyOutputs = outputs
.map((s) => s.trim())
.where((s) => s.isNotEmpty);
if (nonEmptyOutputs.isEmpty) {
return;
}

await _writePart(buildStep, nonEmptyOutputs);
}

FutureOr<void> _writePart(BuildStep buildStep, Iterable<String> outputs) async {
final outputId = buildStep.inputId.changeExtension(outputExtension);
final partOf = "'${p.basename(buildStep.inputId.uri.toString())}'";

final buffer = new StringBuffer()
..writeln('// GENERATED CODE - DO NOT MODIFY BY HAND')
..writeln()
..writeln('part of $partOf;')
..writeln()
..writeln(_headerLine)
..writeln('// OverReactGenerator')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

..writeln(_headerLine);

for (final item in outputs) {
buffer
..writeln()
..writeln(item);
}

final output = _formatter.format(buffer.toString());
await buildStep.writeAsString(outputId, output);
}

@override
Map<String, List<String>> get buildExtensions => {
'.dart': [outputExtension],
};
}
73 changes: 0 additions & 73 deletions lib/src/builder/generator.dart

This file was deleted.

6 changes: 5 additions & 1 deletion lib/src/builder/util.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import 'dart:mirrors';

import 'package:analyzer/analyzer.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:path/path.dart' as p;
import 'package:build/build.dart' show AssetId;
import 'package:source_span/source_span.dart';
Expand Down Expand Up @@ -28,6 +28,10 @@ Uri idToPackageUri(AssetId id) {
path: p.url.join(id.package, id.path.replaceFirst('lib/', '')));
}

/// Returns true if the given compilation unit is a part file.
bool isPart(CompilationUnit unit) =>
unit.directives.any((directive) => directive is PartOfDirective);

/// Returns a string representing a [TypeParameterList], but type bounds removed.
///
/// Example:
Expand Down
2 changes: 1 addition & 1 deletion lib/src/component/abstract_transition.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/component/aria_mixin.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/component/prop_mixins.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/component/resize_sensor.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/util/class_names.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ dependencies:
build: ^1.0.0
built_redux: ^7.4.2
built_value: ^5.4.4
dart_style: '>=0.1.7 <2.0.0'
js: ^0.6.1+1
logging: ">=0.11.3+2 <1.0.0"
meta: ^1.1.6
path: ^1.5.1
react: ^4.6.0
source_gen: ^0.9.0
source_span: ^1.4.1
transformer_utils: ^0.2.0
w_common: ^1.13.0
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading