From c37e8ebe9421cd732078b537d7be5c8e5aad1552 Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Fri, 12 Feb 2016 14:53:17 -0800 Subject: [PATCH 1/2] incremental rebuilds with watch --- lib/src/generate/build_impl.dart | 48 ++++++-- lib/src/generate/watch_impl.dart | 1 + test/common/copy_builder.dart | 21 +++- test/generate/watch_test.dart | 185 ++++++++++++++++++++++++++++++- 4 files changed, 238 insertions(+), 17 deletions(-) diff --git a/lib/src/generate/build_impl.dart b/lib/src/generate/build_impl.dart index 8b77acfce..24a177068 100644 --- a/lib/src/generate/build_impl.dart +++ b/lib/src/generate/build_impl.dart @@ -9,7 +9,6 @@ import 'package:logging/logging.dart'; import 'package:path/path.dart' as path; import '../asset/asset.dart'; -import '../asset/cache.dart'; import '../asset/exceptions.dart'; import '../asset/id.dart'; import '../asset/reader.dart'; @@ -85,15 +84,21 @@ class BuildImpl { /// Deletes all previous output files. Future _deletePreviousOutputs() async { if (await _reader.hasInput(_buildOutputsId)) { - // Cache file exists, just delete all outputs contained in it. + /// Cache file exists, delete all outputs which don't appear in the + /// [_assetGraph], or are marked as needing an update. + /// + /// Removes all files from [_inputsByPackage] regardless of state. var previousOutputs = JSON.decode(await _reader.readAsString(_buildOutputsId)); await _writer.delete(_buildOutputsId); _inputsByPackage[_buildOutputsId.package]?.remove(_buildOutputsId); - await Future.wait(previousOutputs.map((output) { + await Future.wait(previousOutputs.map((output) async { var outputId = new AssetId.deserialize(output); _inputsByPackage[outputId.package]?.remove(outputId); - return _writer.delete(outputId); + var node = _assetGraph.get(outputId); + if (node == null || (node as GeneratedAssetNode).needsUpdate) { + await _writer.delete(outputId); + } })); return; } @@ -176,15 +181,18 @@ class BuildImpl { final outputs = []; int phaseGroupNum = 0; for (var group in _phaseGroups) { - final groupOutputs = []; + /// Collects all the ids for files which are output by this stage. This + /// also includes files which didn't get regenerated because they weren't, + /// dirty unlike [outputs] which only gets files which were explicitly + /// generated in this build. + final groupOutputIds = new Set(); for (var phase in group) { var inputs = _matchingInputs(phase.inputSets); for (var builder in phase.builders) { // TODO(jakemac): Optimize, we can run all the builders in a phase // at the same time instead of sequentially. await for (var output - in _runBuilder(builder, inputs, phaseGroupNum)) { - groupOutputs.add(output); + in _runBuilder(builder, inputs, phaseGroupNum, groupOutputIds)) { outputs.add(output); } } @@ -192,10 +200,10 @@ class BuildImpl { /// Once the group is done, add all outputs so they can be used in the next /// phase. - for (var output in groupOutputs) { + for (var outputId in groupOutputIds) { _inputsByPackage.putIfAbsent( - output.id.package, () => new Set()); - _inputsByPackage[output.id.package].add(output.id); + outputId.package, () => new Set()); + _inputsByPackage[outputId.package].add(outputId); } phaseGroupNum++; } @@ -250,7 +258,7 @@ class BuildImpl { /// Runs [builder] with [inputs] as inputs. Stream _runBuilder(Builder builder, Iterable primaryInputs, - int phaseGroupNum) async* { + int phaseGroupNum, Set groupOutputs) async* { for (var input in primaryInputs) { var expectedOutputs = builder.declareOutputs(input); @@ -278,7 +286,17 @@ class BuildImpl { /// Skip the build step if none of the outputs need updating. var skipBuild = !expectedOutputs.any((output) => (_assetGraph.get(output) as GeneratedAssetNode).needsUpdate); - if (skipBuild) continue; + if (skipBuild) { + /// If we skip the build, we still need to add the ids as outputs for + /// any files which were output last time, so they can be used by + /// subsequent phases. + for (var output in expectedOutputs) { + if (await _reader.hasInput(output)) { + groupOutputs.add(output); + } + } + continue; + } var inputAsset = new Asset(input, await _reader.readAsString(input)); var buildStep = new BuildStepImpl(inputAsset, expectedOutputs, _reader, @@ -286,6 +304,11 @@ class BuildImpl { await builder.build(buildStep); await buildStep.complete(); + /// Mark all outputs as no longer needing an update. + for (var output in expectedOutputs) { + (_assetGraph.get(output) as GeneratedAssetNode).needsUpdate = false; + } + /// Update the asset graph based on the dependencies discovered. for (var dependency in buildStep.dependencies) { var dependencyNode = _assetGraph.addIfAbsent( @@ -297,6 +320,7 @@ class BuildImpl { /// Yield the outputs. for (var output in buildStep.outputs) { + groupOutputs.add(output.id); yield output; } } diff --git a/lib/src/generate/watch_impl.dart b/lib/src/generate/watch_impl.dart index ad34da95b..316f6c8ed 100644 --- a/lib/src/generate/watch_impl.dart +++ b/lib/src/generate/watch_impl.dart @@ -138,6 +138,7 @@ class WatchImpl { void clearNodeAndDeps(AssetId id) { var node = _assetGraph.get(id); if (node == null) return; + if (node is GeneratedAssetNode) node.needsUpdate = true; _assetCache.remove(id); for (var output in node.outputs) { clearNodeAndDeps(output); diff --git a/test/common/copy_builder.dart b/test/common/copy_builder.dart index 4b8d852f9..63b8be660 100644 --- a/test/common/copy_builder.dart +++ b/test/common/copy_builder.dart @@ -6,16 +6,33 @@ import 'dart:async'; import 'package:build/build.dart'; class CopyBuilder implements Builder { + /// If > 0, then multiple copies will be output, using the copy number as an + /// additional extension. final int numCopies; + + /// The file extension to add to files. final String extension; + + /// The package in which to output files. final String outputPackage; - CopyBuilder({this.numCopies: 1, this.extension: 'copy', this.outputPackage}); + /// Copies content from this asset into all files, instead of the primary + /// asset. + final AssetId copyFromAsset; + + CopyBuilder( + {this.numCopies: 1, + this.extension: 'copy', + this.outputPackage, + this.copyFromAsset}); Future build(BuildStep buildStep) async { var ids = declareOutputs(buildStep.input.id); for (var id in ids) { - buildStep.writeAsString(new Asset(id, buildStep.input.stringContents)); + var content = copyFromAsset == null + ? buildStep.input.stringContents + : await buildStep.readAsString(copyFromAsset); + buildStep.writeAsString(new Asset(id, content)); } } diff --git a/test/generate/watch_test.dart b/test/generate/watch_test.dart index 6dccfec91..7432a57c3 100644 --- a/test/generate/watch_test.dart +++ b/test/generate/watch_test.dart @@ -64,8 +64,9 @@ main() { .notifyWatchers(new WatchEvent(ChangeType.ADD, 'a/web/b.txt')); result = await nextResult(results); - checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/b.txt.copy': 'b',}, - result, writer.assets); + checkOutputs({'a|web/b.txt.copy': 'b',}, result, writer.assets); + // Previous outputs should still exist. + expect(writer.assets[makeAssetId('a|web/a.txt.copy')], 'a'); }); test('rebuilds on deleted files', () async { @@ -88,7 +89,185 @@ main() { .notifyWatchers(new WatchEvent(ChangeType.REMOVE, 'a/web/a.txt')); result = await nextResult(results); - checkOutputs({'a|web/b.txt.copy': 'b',}, result, writer.assets); + + // Shouldn't rebuild anything, no outputs. + checkOutputs({}, result, writer.assets); + + // The old output file should no longer exist either. + expect(writer.assets[makeAssetId('a|web/a.txt.copy')], isNull); + // Previous outputs should still exist. + expect(writer.assets[makeAssetId('a|web/b.txt.copy')], 'b'); + }); + }); + + group('multiple phases', () { + test('edits propagate through all phases', () async { + var phases = [ + [ + new Phase([new CopyBuilder()], [new InputSet('a')]), + ], + [ + new Phase([ + new CopyBuilder() + ], [ + new InputSet('a', filePatterns: ['**/*.copy']) + ]), + ] + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a'}, writer).listen(results.add); + + var result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/a.txt.copy.copy': 'a'}, + result, writer.assets); + + await writer.writeAsString(makeAsset('a|web/a.txt', 'b')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.MODIFY, 'a/web/a.txt')); + + result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'b', 'a|web/a.txt.copy.copy': 'b'}, + result, writer.assets); + }); + + test('adds propagate through all phases', () async { + var phases = [ + [ + new Phase([new CopyBuilder()], [new InputSet('a')]), + ], + [ + new Phase([ + new CopyBuilder() + ], [ + new InputSet('a', filePatterns: ['**/*.copy']) + ]), + ] + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a'}, writer).listen(results.add); + + var result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/a.txt.copy.copy': 'a'}, + result, writer.assets); + + await writer.writeAsString(makeAsset('a|web/b.txt', 'b')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.ADD, 'a/web/b.txt')); + + result = await nextResult(results); + checkOutputs({'a|web/b.txt.copy': 'b', 'a|web/b.txt.copy.copy': 'b'}, + result, writer.assets); + // Previous outputs should still exist. + expect(writer.assets[makeAssetId('a|web/a.txt.copy')], 'a'); + expect(writer.assets[makeAssetId('a|web/a.txt.copy.copy')], 'a'); + }); + + test('deletes propagate through all phases', () async { + var phases = [ + [ + new Phase([new CopyBuilder()], [new InputSet('a')]), + ], + [ + new Phase([ + new CopyBuilder() + ], [ + new InputSet('a', filePatterns: ['**/*.copy']) + ]), + ] + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a', 'a|web/b.txt': 'b'}, writer) + .listen(results.add); + + var result = await nextResult(results); + checkOutputs({ + 'a|web/a.txt.copy': 'a', + 'a|web/a.txt.copy.copy': 'a', + 'a|web/b.txt.copy': 'b', + 'a|web/b.txt.copy.copy': 'b' + }, result, writer.assets); + + await writer.delete(makeAssetId('a|web/a.txt')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.REMOVE, 'a/web/a.txt')); + + result = await nextResult(results); + // Shouldn't rebuild anything, no outputs. + checkOutputs({}, result, writer.assets); + + // Derived outputs should no longer exist. + expect(writer.assets[makeAssetId('a|web/a.txt.copy')], isNull); + expect(writer.assets[makeAssetId('a|web/a.txt.copy.copy')], isNull); + // Other outputs should still exist. + expect(writer.assets[makeAssetId('a|web/b.txt.copy')], 'b'); + expect(writer.assets[makeAssetId('a|web/b.txt.copy.copy')], 'b'); + }); + }); + + /// Tests for updates + group('secondary dependency', () { + test('of an output file is edited', () async { + var phases = [ + [ + new Phase([ + new CopyBuilder(copyFromAsset: makeAssetId('a|web/b.txt')) + ], [ + new InputSet('a', filePatterns: ['web/a.txt']) + ]), + ], + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a', 'a|web/b.txt': 'b'}, writer) + .listen(results.add); + + var result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'b'}, result, writer.assets); + + await writer.writeAsString(makeAsset('a|web/b.txt', 'c')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.MODIFY, 'a/web/b.txt')); + + result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'c'}, result, writer.assets); + }); + + test( + 'of an output which is derived from another generated file is edited', + () async { + var phases = [ + [ + new Phase([ + new CopyBuilder() + ], [ + new InputSet('a', filePatterns: ['web/a.txt']) + ]), + ], + [ + new Phase([ + new CopyBuilder(copyFromAsset: makeAssetId('a|web/b.txt')) + ], [ + new InputSet('a', filePatterns: ['web/a.txt.copy']) + ]), + ], + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a', 'a|web/b.txt': 'b'}, writer) + .listen(results.add); + + var result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/a.txt.copy.copy': 'b'}, result, writer.assets); + + await writer.writeAsString(makeAsset('a|web/b.txt', 'c')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.MODIFY, 'a/web/b.txt')); + + result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy.copy': 'c'}, result, writer.assets); }); }); }); From 41806f1c5514778a22ba40ddc8aeee1e51d01bbe Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Tue, 16 Feb 2016 09:11:31 -0800 Subject: [PATCH 2/2] Fix build_outputs.json for rebuilds, and speed up rebuilds --- lib/src/asset_graph/graph.dart | 3 +++ lib/src/asset_graph/node.dart | 5 ++++- lib/src/generate/build_impl.dart | 25 ++++++++++++++++--------- lib/src/generate/watch_impl.dart | 24 ++++++++++++++++-------- test/asset_graph/graph_test.dart | 8 +++++--- test/generate/watch_test.dart | 32 +++++++++++++++++++++++++++++++- 6 files changed, 75 insertions(+), 22 deletions(-) diff --git a/lib/src/asset_graph/graph.dart b/lib/src/asset_graph/graph.dart index 7843c2182..286bab4fd 100644 --- a/lib/src/asset_graph/graph.dart +++ b/lib/src/asset_graph/graph.dart @@ -36,6 +36,9 @@ class AssetGraph { /// Removes [node] from the graph. AssetNode remove(AssetId id) => _nodesById.remove(id); + /// Gets all nodes in the graph. + Iterable get allNodes => _nodesById.values; + @override toString() => _nodesById.values.toList().toString(); } diff --git a/lib/src/asset_graph/node.dart b/lib/src/asset_graph/node.dart index 2c0c8e294..d11145b10 100644 --- a/lib/src/asset_graph/node.dart +++ b/lib/src/asset_graph/node.dart @@ -34,8 +34,11 @@ class GeneratedAssetNode extends AssetNode { /// Whether or not this asset needs to be updated. bool needsUpdate; + /// Whether the asset was actually output. + bool wasOutput; + GeneratedAssetNode(this.builder, this.primaryInput, this.generatingPhaseGroup, - this.needsUpdate, AssetId id) + this.needsUpdate, this.wasOutput, AssetId id) : super(id); @override diff --git a/lib/src/generate/build_impl.dart b/lib/src/generate/build_impl.dart index 24a177068..2f1b5627e 100644 --- a/lib/src/generate/build_impl.dart +++ b/lib/src/generate/build_impl.dart @@ -34,8 +34,8 @@ class BuildImpl { bool _buildRunning = false; final _logger = new Logger('Build'); - BuildImpl(this._assetGraph, this._reader, this._writer, - this._packageGraph, this._phaseGroups); + BuildImpl(this._assetGraph, this._reader, this._writer, this._packageGraph, + this._phaseGroups); /// Runs a build /// @@ -61,11 +61,13 @@ class BuildImpl { _logger.info('Running build phases'); var result = await _runPhases(); - // Write out the new build_outputs file. + /// Write out the new build_outputs file. + var allOuputs = _assetGraph.allNodes + .where((node) => node is GeneratedAssetNode && node.wasOutput); var buildOutputsAsset = new Asset( _buildOutputsId, JSON.encode( - result.outputs.map((output) => output.id.serialize()).toList())); + allOuputs.map((output) => output.id.serialize()).toList())); await _writer.writeAsString(buildOutputsAsset); return result; @@ -280,7 +282,7 @@ class BuildImpl { _assetGraph.addIfAbsent( output, () => new GeneratedAssetNode( - builder, input, phaseGroupNum, true, output)); + builder, input, phaseGroupNum, true, false, output)); } /// Skip the build step if none of the outputs need updating. @@ -291,7 +293,7 @@ class BuildImpl { /// any files which were output last time, so they can be used by /// subsequent phases. for (var output in expectedOutputs) { - if (await _reader.hasInput(output)) { + if ((_assetGraph.get(output) as GeneratedAssetNode).wasOutput) { groupOutputs.add(output); } } @@ -304,9 +306,12 @@ class BuildImpl { await builder.build(buildStep); await buildStep.complete(); - /// Mark all outputs as no longer needing an update. + /// Mark all outputs as no longer needing an update, and mark `wasOutput` + /// as `false` for now (this will get reset to true later one). for (var output in expectedOutputs) { - (_assetGraph.get(output) as GeneratedAssetNode).needsUpdate = false; + (_assetGraph.get(output) as GeneratedAssetNode) + ..needsUpdate = false + ..wasOutput = false; } /// Update the asset graph based on the dependencies discovered. @@ -314,12 +319,14 @@ class BuildImpl { var dependencyNode = _assetGraph.addIfAbsent( dependency, () => new AssetNode(dependency)); - /// We care about all [expectedOutputs], not just real outputs. + /// We care about all [expectedOutputs], not just real outputs. Updates + /// to dependencies may cause a file to be output which wasn't before. dependencyNode.outputs.addAll(expectedOutputs); } /// Yield the outputs. for (var output in buildStep.outputs) { + (_assetGraph.get(output.id) as GeneratedAssetNode).wasOutput = true; groupOutputs.add(output.id); yield output; } diff --git a/lib/src/generate/watch_impl.dart b/lib/src/generate/watch_impl.dart index 316f6c8ed..3a9fc51fe 100644 --- a/lib/src/generate/watch_impl.dart +++ b/lib/src/generate/watch_impl.dart @@ -111,7 +111,7 @@ class WatchImpl { _runningWatch = true; _resultStreamController = new StreamController(); _nextBuildScheduled = false; - var updatedInputs = new Set(); + var updatedInputs = new Map(); doBuild([bool force = false]) { // Don't schedule more builds if we are turning down. @@ -128,24 +128,32 @@ class WatchImpl { /// Remove any updates that were generated outputs or otherwise not /// interesting. - updatedInputs.removeWhere(_shouldSkipInput); + var updatesToRemove = updatedInputs.keys.where(_shouldSkipInput).toList(); + updatesToRemove.forEach(updatedInputs.remove); if (updatedInputs.isEmpty && !force) { return; } _logger.info('Preparing for next build'); _logger.info('Clearing cache for invalidated assets'); - void clearNodeAndDeps(AssetId id) { + void clearNodeAndDeps(AssetId id, ChangeType rootChangeType) { var node = _assetGraph.get(id); if (node == null) return; - if (node is GeneratedAssetNode) node.needsUpdate = true; + if (node is GeneratedAssetNode) { + node.needsUpdate = true; + } _assetCache.remove(id); for (var output in node.outputs) { - clearNodeAndDeps(output); + clearNodeAndDeps(output, rootChangeType); + } + + /// For deletes, prune the graph. + if (rootChangeType == ChangeType.REMOVE) { + _assetGraph.remove(id); } } - for (var input in updatedInputs) { - clearNodeAndDeps(input); + for (var input in updatedInputs.keys) { + clearNodeAndDeps(input, updatedInputs[input]); } updatedInputs.clear(); @@ -181,7 +189,7 @@ class WatchImpl { _allListeners.add(watcher.events.listen((WatchEvent e) { _logger.fine('Got WatchEvent for path ${e.path}'); var id = new AssetId(package.name, path.normalize(e.path)); - updatedInputs.add(id); + updatedInputs[id] = e.type; scheduleBuild(); })); } diff --git a/test/asset_graph/graph_test.dart b/test/asset_graph/graph_test.dart index f12f73bc7..f4f039415 100644 --- a/test/asset_graph/graph_test.dart +++ b/test/asset_graph/graph_test.dart @@ -34,17 +34,19 @@ main() { return node; } - test('add, contains, get', () { + test('add, contains, get, allNodes', () { + var expectedNodes = []; for (int i = 0; i < 5; i++) { - testAddNode(); + expectedNodes.add(testAddNode()); } + expect(graph.allNodes, unorderedEquals(expectedNodes)); }); test('addIfAbsent', () { var node = makeAssetNode(); expect(graph.addIfAbsent(node.id, () => node), same(node)); expect(graph.contains(node.id), isTrue); - + var otherNode = new AssetNode(node.id); expect(graph.addIfAbsent(otherNode.id, () => otherNode), same(node)); expect(graph.contains(otherNode.id), isTrue); diff --git a/test/generate/watch_test.dart b/test/generate/watch_test.dart index 7432a57c3..f865e2400 100644 --- a/test/generate/watch_test.dart +++ b/test/generate/watch_test.dart @@ -98,6 +98,35 @@ main() { // Previous outputs should still exist. expect(writer.assets[makeAssetId('a|web/b.txt.copy')], 'b'); }); + + test('rebuilds properly update build_outputs.json', () async { + var phases = [ + [ + new Phase([new CopyBuilder()], [new InputSet('a')]), + ] + ]; + var writer = new InMemoryAssetWriter(); + var results = []; + startWatch(phases, {'a|web/a.txt': 'a', 'a|web/b.txt': 'b'}, writer) + .listen(results.add); + + var result = await nextResult(results); + checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/b.txt.copy': 'b',}, + result, writer.assets); + + await writer.writeAsString(makeAsset('a|web/c.txt', 'c')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.ADD, 'a/web/c.txt')); + await writer.delete(makeAssetId('a|web/a.txt')); + FakeWatcher + .notifyWatchers(new WatchEvent(ChangeType.REMOVE, 'a/web/a.txt')); + + result = await nextResult(results); + checkOutputs({'a|web/c.txt.copy': 'c'}, result, writer.assets); + + expect(writer.assets[makeAssetId('a|.build/build_outputs.json')], + '[["a","web/b.txt.copy"],["a","web/c.txt.copy"]]'); + }); }); group('multiple phases', () { @@ -260,7 +289,8 @@ main() { .listen(results.add); var result = await nextResult(results); - checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/a.txt.copy.copy': 'b'}, result, writer.assets); + checkOutputs({'a|web/a.txt.copy': 'a', 'a|web/a.txt.copy.copy': 'b'}, + result, writer.assets); await writer.writeAsString(makeAsset('a|web/b.txt', 'c')); FakeWatcher