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 1 commit
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
20 changes: 13 additions & 7 deletions lib/src/builder/builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@evanweible-wf evanweible-wf Apr 4, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

return;
}

final libraryUnit = _tryParseCompilationUnit(source, buildStep.inputId);
if (libraryUnit == null) {
return;
Expand Down Expand Up @@ -79,7 +75,18 @@ class OverReactBuilder extends Builder {
}

if (outputs.isNotEmpty) {
await _writePart(buildStep, outputs);
final outputId = buildStep.inputId.changeExtension(outputExtension);

// Verify that the library file has an `.over_react.g.dart` part.
final expectedPart = p.basename(outputId.path);
final partUris = libraryUnit.directives
.whereType<PartDirective>()
.map((p) => p.uri.stringValue);
if (!partUris.contains(expectedPart)) {
log.warning('Missing "part \'$expectedPart\';".');
}

await _writePart(buildStep, outputId, outputs);
}
}

Expand Down Expand Up @@ -111,8 +118,7 @@ class OverReactBuilder extends Builder {
}
}

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

final buffer = new StringBuffer()
Expand Down
41 changes: 25 additions & 16 deletions test/vm_tests/builder/over_react_builder_test.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@TestOn('vm')
import 'dart:async';
import 'dart:io';

import 'package:build/build.dart';
Expand All @@ -14,36 +15,37 @@ typedef void LogRecordFunction(LogRecord record);
main() {
group('overReactBuilder', () {
final builder = overReactBuilder(null);
final logger = Logger('overReactBuilderTestLogger');

AssetReader reader;
InMemoryAssetWriter writer;
AssetWriterSpy writerSpy;
List<String> logMsgs = <String>[];
List<Level> logLevels = <Level>[];
StreamSubscription logSub;
List<LogRecord> logs = <LogRecord>[];

setUp(() async {
reader = await PackageAssetReader.currentIsolate(
rootPackage: 'over_react',
);

writer = new InMemoryAssetWriter();
writerSpy = AssetWriterSpy(writer);

logSub = logger.onRecord.listen(logs.add);
});

tearDown(() {
logMsgs.clear();
tearDown(() async {
await logSub?.cancel();
logs.clear();
reader = null;
writer = null;
writerSpy = null;
});

void verifyNoErrorLogs() {
expect(logLevels, isNot(contains(Level.SEVERE)));
expect(logLevels, isNot(contains(Level.WARNING)));
}

void recordLogs(LogRecord record) {
logMsgs.add(record.message);
logLevels.add(record.level);
expect(logs.where((log) => log.level >= Level.WARNING), isEmpty,
reason: 'Expected no logs at WARNING or SEVERE level, but got:\n'
'\t${logs.join('\n\t')}');
}

void checkBuildForFile(String assetPath, String expectedOutputAssetPath,
Expand All @@ -55,32 +57,39 @@ main() {
expectedOutputAssetPath : expectedContent
};

final logger = Logger('overReactBuilderTestLogger');
final sub = logger.onRecord.listen(recordLogs);
await runBuilder(builder, [inputAsset], reader, writerSpy, AnalyzerResolvers(), logger: logger);
final actual = writerSpy.assetsWritten;
sub.cancel();

checkOutputs(expected, actual, writer);
verifyNoErrorLogs();
}

test('does not produce a build output for a file with no over_react annotations', () async {
var basicAsset = makeAssetId('over_react|test_fixtures/source_files/no_annotations.dart');
await runBuilder(builder, [basicAsset], reader, writerSpy, AnalyzerResolvers());
await runBuilder(builder, [basicAsset], reader, writerSpy, AnalyzerResolvers(), logger: logger);

expect(writerSpy.assetsWritten, isEmpty);
verifyNoErrorLogs();
});

test('does not produce a build output for just a part file', () async {
var basicAsset = makeAssetId('over_react|test_fixtures/source_files/part_of_basic_library.dart');
await runBuilder(builder, [basicAsset], reader, writerSpy, AnalyzerResolvers());
await runBuilder(builder, [basicAsset], reader, writerSpy, AnalyzerResolvers(), logger: logger);

expect(writerSpy.assetsWritten, isEmpty);
verifyNoErrorLogs();
});

test('warns if the .over_react.g.dart part is missing', () async {
var libraryAsset = makeAssetId('over_react|test_fixtures/source_files/missing_over_react_g_part/library.dart');
await runBuilder(builder, [libraryAsset], reader, writerSpy, AnalyzerResolvers(), logger: logger);
final expectedWarning = logs.firstWhere((log) {
return log.level == Level.WARNING && log.message == 'Missing "part \'library.over_react.g.dart\';".';
}, orElse: () => null);
expect(expectedWarning, isNotNull,
reason: 'Expected a WARNING log for the missing over_react part.');
});

group('for backwards compatible boilerplate:', () {
test('builds from basic component file', () async {
await checkBuildForFile(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import 'package:over_react/over_react.dart';

part 'part.dart';
17 changes: 17 additions & 0 deletions test_fixtures/source_files/missing_over_react_g_part/part.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
part of 'library.dart';

@Factory()
UiFactory<BasicPartOfLibProps> BasicPartOfLib = _$BasicPartOfLib;

@Props()
class _$BasicPartOfLibProps extends UiProps {
String basicProp;
}

@Component()
class BasicPartOfLibComponent extends UiComponent<BasicPartOfLibProps> {
@override
render() {
return Dom.div()('foo');
}
}