Batch writes (#3771)

simolus3 · web-flow · commit f89332a205c2 · 2024-11-21T12:13:04.000-08:00
* Use batches for builds * Add changelog entry to build runner * Review feedback aAnother attempt for #3321, my [previous PR](#3418) for this got stale, this one is a bit simpler. This attempts to solve the issue of `build_runner` and external tools watching for file system events (like the analysis server) not playing too well together. At the moment, a common issue is that, as a user runs `build_runner build` and the generated assets are written as they are generated, the analysis server will see many file changes and keep re-analyzing sources in an incomplete state. By making `build_runner` cache pending writes in memory and then flushing them after a completed build, we'll likely get all changes to disk before they're picked up by external tools, reducing friction. I've implemented this at a fairly low level (the raw file readers and writers making up the default `IOEnvironment`). They have to opt-in to batches by checking for the presence of a zone key when reading or writing assets. We support batches by default when not in low-resources mode. A batch runs in a zone wrapping a whole build run. A todo is that I need to write integration tests for this, but it would be good to get a review for the overall approach before completing this. Closes #3321. * Bump version of build_runner_core * Make batch private, fix dependency resolution * Start fixing tests * Add lints dependency for out-of-workspace pkg
diff --git a/_test_common/lib/in_memory_writer.dart b/_test_common/lib/in_memory_writer.dart
@@ -38,4 +38,7 @@ class InMemoryRunnerAssetWriter extends InMemoryAssetWriter
     FakeWatcher.notifyWatchers(WatchEvent(
         ChangeType.REMOVE, p.absolute(id.package, p.fromUri(id.path))));
   }
+
+  @override
+  Future<void> completeBuild() async {}
 }
diff --git a/_test_common/lib/runner_asset_writer_spy.dart b/_test_common/lib/runner_asset_writer_spy.dart
@@ -20,4 +20,7 @@ class RunnerAssetWriterSpy extends AssetWriterSpy implements RunnerAssetWriter {
     _assetsDeleted.add(id);
     return _delegate.delete(id);
   }
+
+  @override
+  Future<void> completeBuild() async {}
 }
diff --git a/_test_common/lib/sdk.dart b/_test_common/lib/sdk.dart
@@ -12,8 +12,6 @@ import 'package:pub_semver/pub_semver.dart';
 import 'package:test/test.dart';
 import 'package:test_descriptor/test_descriptor.dart' as d;
 
-String _dartBinary = p.join(sdkBin, 'dart');
-
 final bool supportsUnsoundNullSafety =
     Version.parse(Platform.version.split(' ').first).major == 2;
 
@@ -54,13 +52,13 @@ Future<Process> startPub(String package, String command,
 /// The [script] should be a relative path under [package].
 Future<ProcessResult> runDart(String package, String script,
         {Iterable<String>? args}) =>
-    Process.run(_dartBinary, [script, ...?args],
+    Process.run(dartBinary, [script, ...?args],
         workingDirectory: p.join(d.sandbox, package));
 
 /// Starts the `dart` script [script] in [package] with [args].
 ///
 /// The [script] should be a relative path under [package].
 Future<Process> startDart(String package, String script,
         {Iterable<String>? args}) =>
-    Process.start(_dartBinary, [script, ...?args],
+    Process.start(dartBinary, [script, ...?args],
         workingDirectory: p.join(d.sandbox, package));
diff --git a/_test_common/pubspec.yaml b/_test_common/pubspec.yaml
@@ -1,7 +1,7 @@
 name: _test_common
 publish_to: none
 description: Test infra for writing build tests. Is not published.
-resolution: workspace
+#resolution: workspace
 
 environment:
   sdk: ^3.5.0
@@ -19,3 +19,16 @@ dependencies:
   test: ^1.16.0
   test_descriptor: ^2.0.0
   watcher: ^1.0.0
+
+dev_dependencies:
+  dart_flutter_team_lints: ^3.1.0
+
+dependency_overrides:
+  build:
+    path: ../build
+  build_config:
+    path: ../build_config
+  build_runner_core:
+    path: ../build_runner_core
+  build_test:
+    path: ../build_test
diff --git a/build_runner/CHANGELOG.md b/build_runner/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 2.4.14-wip
+
+- Write generated assets at the end of a build to avoid invalidating other
+  tools with a file watcher multiple times.
+
 ## 2.4.13
 
 - Bump the min sdk to 3.5.0.
diff --git a/build_runner/lib/src/watcher/delete_writer.dart b/build_runner/lib/src/watcher/delete_writer.dart
@@ -29,4 +29,9 @@ class OnDeleteWriter implements RunnerAssetWriter {
   Future writeAsString(AssetId id, String contents,
           {Encoding encoding = utf8}) =>
       _writer.writeAsString(id, contents, encoding: encoding);
+
+  @override
+  Future<void> completeBuild() async {
+    await _writer.completeBuild();
+  }
 }
diff --git a/build_runner/pubspec.yaml b/build_runner/pubspec.yaml
@@ -1,8 +1,8 @@
 name: build_runner
-version: 2.4.13
+version: 2.4.14-wip
 description: A build system for Dart code generation and modular compilation.
 repository: https://github.com/dart-lang/build/tree/master/build_runner
-resolution: workspace
+#resolution: workspace
 
 environment:
   sdk: ^3.5.0
@@ -20,7 +20,7 @@ dependencies:
   build_config: ">=1.1.0 <1.2.0"
   build_daemon: ^4.0.0
   build_resolvers: ^2.0.0
-  build_runner_core: ^7.2.0
+  build_runner_core: ^8.0.0-wip
   code_builder: ^4.2.0
   collection: ^1.15.0
   crypto: ^3.0.0
@@ -53,10 +53,15 @@ dev_dependencies:
     path: ../_test_common
   build_test: ^2.0.0
   build_web_compilers: ^4.0.0
+  dart_flutter_team_lints: ^3.1.0
   stream_channel: ^2.0.0
   test: ^1.25.5
   test_descriptor: ^2.0.0
   test_process: ^2.0.0
 
+dependency_overrides:
+  build_runner_core:
+    path: ../build_runner_core
+
 topics:
  - build-runner
diff --git a/build_runner/test/integration_tests/utils/build_descriptor.dart b/build_runner/test/integration_tests/utils/build_descriptor.dart
@@ -10,6 +10,7 @@ import 'dart:isolate';
 import 'package:_test_common/sdk.dart';
 import 'package:async/async.dart';
 import 'package:build/build.dart';
+import 'package:build_runner_core/build_runner_core.dart';
 import 'package:package_config/package_config.dart';
 import 'package:path/path.dart' as p;
 import 'package:stack_trace/stack_trace.dart';
@@ -106,7 +107,7 @@ Future<BuildTool> package(Iterable<d.Descriptor> otherPackages,
   ]).create();
   await Future.wait(otherPackages.map((d) => d.create()));
   await pubGet('a');
-  return BuildTool._('dart', ['run', 'build_runner']);
+  return BuildTool._(dartBinary, ['run', 'build_runner']);
 }
 
 /// Create a package in [d.sandbox] with a `tool/build.dart` script using
@@ -145,7 +146,7 @@ Future<BuildTool> packageWithBuildScript(
     ...contents
   ]).create();
   await pubGet('a');
-  return BuildTool._('dart', [p.join('tool', 'build.dart')]);
+  return BuildTool._(dartBinary, [p.join('tool', 'build.dart')]);
 }
 
 String _buildersFile(
diff --git a/build_runner_core/CHANGELOG.md b/build_runner_core/CHANGELOG.md
@@ -1,5 +1,9 @@
-## 7.3.3-wip
+## 8.0.0-wip
 
+- __Breaking__: Add `completeBuild` to `RunnerAssetWriter`, a method expected
+  to be called by the build system at the end of a completed build.
+- Add `wrapInBatch` to obtain a reader/writer pair that will batch writes
+  before flushing them at the end of a build.
 - Bump the min sdk to 3.6.0-dev.228.
 - Require analyzer ^6.9.0.
 - Fix analyzer deprecations.
diff --git a/build_runner_core/lib/build_runner_core.dart b/build_runner_core/lib/build_runner_core.dart
@@ -4,6 +4,7 @@
 
 export 'package:build/build.dart' show PostProcessBuildStep, PostProcessBuilder;
 
+export 'src/asset/batch.dart' show wrapInBatch;
 export 'src/asset/file_based.dart';
 export 'src/asset/finalized_reader.dart';
 export 'src/asset/reader.dart' show RunnerAssetReader;
diff --git a/build_runner_core/lib/src/asset/batch.dart b/build_runner_core/lib/src/asset/batch.dart
@@ -0,0 +1,163 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:async';
+import 'dart:convert';
+
+import 'package:build/build.dart';
+import 'package:glob/glob.dart';
+import 'package:meta/meta.dart';
+
+import '../environment/io_environment.dart';
+import 'reader.dart';
+import 'writer.dart';
+
+/// A batch of file system writes that should be committed at once instead of
+/// when [AssetWriter.writeAsBytes] or [AssetWriter.writeAsString] is called.
+///
+/// During a typical build run emitting generated files one-by-one, it's
+/// possible that other running tools such as an analysis server will have to
+/// re-analyze incomplete states multiple times.
+/// By storing pending outputs in memory first and then committing them at the
+/// end of the build, we have a better view over that needs to happen.
+///
+/// The default [IOEnvironment] uses readers and writes that are batch-aware
+/// outside of low-memory mode.
+final class _FileSystemWriteBatch {
+  final Map<AssetId, _PendingFileState> _pendingWrites = {};
+
+  _FileSystemWriteBatch._();
+
+  Future<void> completeWrites(RunnerAssetWriter writer) async {
+    await Future.wait(_pendingWrites.keys.map((id) async {
+      final pending = _pendingWrites[id]!;
+
+      if (pending.content case final content?) {
+        await writer.writeAsBytes(id, content);
+      } else {
+        await writer.delete(id);
+      }
+    }));
+
+    _pendingWrites.clear();
+  }
+}
+
+/// Wraps a pair of a [RunnerAssetReader] with path-prividing capabilities and
+/// a [RunnerAssetWriter] into a pair of readers and writers that will
+/// internally buffer writes and only flush them in
+/// [RunnerAssetWriter.completeBuild].
+///
+/// The returned reader will see pending writes by the returned writer before
+/// they are flushed to the file system.
+(RunnerAssetReader, RunnerAssetWriter) wrapInBatch({
+  required RunnerAssetReader reader,
+  required PathProvidingAssetReader pathProvidingReader,
+  required RunnerAssetWriter writer,
+}) {
+  final batch = _FileSystemWriteBatch._();
+
+  return (
+    BatchReader(reader, pathProvidingReader, batch),
+    BatchWriter(writer, batch),
+  );
+}
+
+final class _PendingFileState {
+  final List<int>? content;
+
+  const _PendingFileState(this.content);
+
+  bool get isDeleted => content == null;
+}
+
+@internal
+final class BatchReader extends AssetReader
+    implements RunnerAssetReader, PathProvidingAssetReader {
+  final RunnerAssetReader _inner;
+  final PathProvidingAssetReader _innerPathProviding;
+  final _FileSystemWriteBatch _batch;
+
+  BatchReader(this._inner, this._innerPathProviding, this._batch);
+
+  _PendingFileState? _stateFor(AssetId id) {
+    return _batch._pendingWrites[id];
+  }
+
+  @override
+  Future<bool> canRead(AssetId id) async {
+    if (_stateFor(id) case final state?) {
+      return !state.isDeleted;
+    } else {
+      return await _inner.canRead(id);
+    }
+  }
+
+  @override
+  Stream<AssetId> findAssets(Glob glob, {String? package}) {
+    return _inner
+        .findAssets(glob, package: package)
+        .where((asset) => _stateFor(asset)?.isDeleted != true);
+  }
+
+  @override
+  String pathTo(AssetId id) {
+    return _innerPathProviding.pathTo(id);
+  }
+
+  @override
+  Future<List<int>> readAsBytes(AssetId id) async {
+    if (_stateFor(id) case final state?) {
+      if (state.isDeleted) {
+        throw AssetNotFoundException(id);
+      } else {
+        return state.content!;
+      }
+    } else {
+      return await _inner.readAsBytes(id);
+    }
+  }
+
+  @override
+  Future<String> readAsString(AssetId id, {Encoding encoding = utf8}) async {
+    if (_stateFor(id) case final state?) {
+      if (state.isDeleted) {
+        throw AssetNotFoundException(id);
+      } else {
+        return encoding.decode(state.content!);
+      }
+    } else {
+      return await _inner.readAsString(id, encoding: encoding);
+    }
+  }
+}
+
+@internal
+final class BatchWriter extends RunnerAssetWriter {
+  final RunnerAssetWriter _inner;
+  final _FileSystemWriteBatch _batch;
+
+  BatchWriter(this._inner, this._batch);
+
+  @override
+  Future delete(AssetId id) async {
+    _batch._pendingWrites[id] = const _PendingFileState(null);
+  }
+
+  @override
+  Future<void> writeAsBytes(AssetId id, List<int> bytes) async {
+    _batch._pendingWrites[id] = _PendingFileState(bytes);
+  }
+
+  @override
+  Future<void> writeAsString(AssetId id, String contents,
+      {Encoding encoding = utf8}) async {
+    _batch._pendingWrites[id] = _PendingFileState(encoding.encode(contents));
+  }
+
+  @override
+  Future<void> completeBuild() async {
+    await _batch.completeWrites(_inner);
+  }
+}
diff --git a/build_runner_core/lib/src/asset/build_cache.dart b/build_runner_core/lib/src/asset/build_cache.dart
@@ -84,6 +84,9 @@ class BuildCacheWriter implements RunnerAssetWriter {
   @override
   Future delete(AssetId id) =>
       _delegate.delete(_cacheLocation(id, _assetGraph, _rootPackage));
+
+  @override
+  Future<void> completeBuild() async {}
 }
 
 AssetId _cacheLocation(AssetId id, AssetGraph assetGraph, String rootPackage) {
diff --git a/build_runner_core/lib/src/asset/file_based.dart b/build_runner_core/lib/src/asset/file_based.dart
@@ -107,6 +107,9 @@ class FileBasedAssetWriter implements RunnerAssetWriter {
       }
     });
   }
+
+  @override
+  Future<void> completeBuild() async {}
 }
 
 /// Returns the path to [id] for a given [packageGraph].
diff --git a/build_runner_core/lib/src/asset/writer.dart b/build_runner_core/lib/src/asset/writer.dart
@@ -11,4 +11,10 @@ typedef OnDelete = void Function(AssetId id);
 
 abstract class RunnerAssetWriter implements AssetWriter {
   Future delete(AssetId id);
+
+  /// Called after each completed build.
+  ///
+  /// Some [RunnerAssetWriter] implementations may buffer completed writes
+  /// internally and flush them in [completeBuild].
+  Future<void> completeBuild();
 }
diff --git a/build_runner_core/lib/src/environment/io_environment.dart b/build_runner_core/lib/src/environment/io_environment.dart
diff --git a/build_runner_core/lib/src/generate/build_impl.dart b/build_runner_core/lib/src/generate/build_impl.dart
diff --git a/build_runner_core/pubspec.yaml b/build_runner_core/pubspec.yaml
diff --git a/build_runner_core/test/asset/batch_test.dart b/build_runner_core/test/asset/batch_test.dart
diff --git a/pubspec.yaml b/pubspec.yaml

Original file line number	Diff line number	Diff line change
`@@ -38,4 +38,7 @@ class InMemoryRunnerAssetWriter extends InMemoryAssetWriter`
`38`	`38`	`FakeWatcher.notifyWatchers(WatchEvent(`
`39`	`39`	`ChangeType.REMOVE, p.absolute(id.package, p.fromUri(id.path))));`
`40`	`40`	`}`
	`41`	`+`
	`42`	`+ @override`
	`43`	`+ Future<void> completeBuild() async {}`
`41`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,4 +20,7 @@ class RunnerAssetWriterSpy extends AssetWriterSpy implements RunnerAssetWriter {`
`20`	`20`	`_assetsDeleted.add(id);`
`21`	`21`	`return _delegate.delete(id);`
`22`	`22`	`}`
	`23`	`+`
	`24`	`+ @override`
	`25`	`+ Future<void> completeBuild() async {}`
`23`	`26`	`}`