Improve precompilation

doc/cache_layout.md

@@ -194,7 +194,6 @@ $PUB_CACHE/global_packages/
     │   └── mono_repo.dart-3.0.0-55.0.dev.snapshot
     ├── .dart_tool/
     │   └── package_config.json
-    ├── incremental
     └── pubspec.lock
 ```
 
@@ -213,9 +212,6 @@ activated package is used by several sdk-versions (TODO: This does have some
 limitations, and we should probably rethink this). A re-activation of the
 package will delete all the existing snapshots.
 
-The `incremental` is used while compiling them. (TODO: We should probably remove
-this after successful compilation https://github.com/dart-lang/pub/issues/3896).
-
 For packages activated with `--source=path` the lockfile is special-cased to just point
 to the activated path, and `.dart_tool/package_config.json`, snapshots are
 stored in that folder.

doc/dart_tool_layout.md

@@ -0,0 +1,64 @@
+# Layout of the `.dart_tool/pub` folder
+
+The pub client creates `.dart_tool/package_config.json` as described by
+[https://github.com/dart-lang/language/blob/main/accepted/2.8/language-versioning/package-config-file-v2.md].
+
+But furthermore pub can use a folder called `.dart_tool/pub` for storing
+artifacts. The organization of that folder is what this document is trying to describe.
+
+The information in this document is informational, and can be used for
+understanding the cache, but we strongly encourage all manipulation of the
+`.dart_tool/pub` folder happens though the `dart pub`/`flutter pub` commands to
+avoid relying on accidental properties that might be broken in the future.
+
+## Precompilation cache
+
+```tree
+.dart_tool/
+├── package_config.json
+├── pub
+│   ├── bin
+│   │   ├── pub
+│   │   │   └── pub.dart-3.1.0.snapshot.incremental
+│   │   └── test
+│   │       └── test.dart-3.2.0-36.0.dev.snapshot
+
+```
+
+When `dart run <package>:<executable>` is called, pub will try to find `<executable>` in
+the package `<package>` and compile it as a "dill" file (using
+`package:frontend_server_client`).
+
+The output will be stored in The dill file will be stored in
+`.dart_tool/pub/bin/<package>/<executable>.dart-<sdk-version>.snapshot`.
+
+This can be used to run the executable by invoking (done implicitly by `dart run`):
+
+```
+dart .dart_tool/pub/bin/<package>/<executable>.dart-<sdk-version>.snapshot
+```
+
+But the dill-file is also fed to the compiler for incremental compilation. This
+can in many cases greatly speed up the compilation when no change has happened.
+
+If the compilation fails, pub avoids leaving a `.snapshot` file, but instead leaves a
+`.dart_tool/pub/bin/<package>/<executable>.dart-<sdk-version>.snapshot.incremental` file.
+
+This file cannot be executed. But it can still give the benefit of incremental
+compilation when changes have happened to the code.
+
+Earlier versions of the dart sdk would put this "incremental" file in:
+
+`.dart_tool/pub/incremental/<package>/<executable>.dart-incremental.dill`.
+
+As we don't expect many of those files to linger, we don't attempt to clean them up.
+
+We use the `<sdk-version>` to enable different sdk-versions to each have their
+own snapshot, so they don't step on each others toes when you switch from one
+sdk to another. The downside is that there is no mechanism for deleting
+snapshots of old sdks. We might want change that logic.
+
+One could argue that a "snapshot", is a different thing from a "dill" file in
+Dart VM terms. But both can be invoked by the VM, and run rather quickly without
+much more pre-compilation. In the future we might want to use true "snapshots"
+for executables from immutable packages, as they don't benefit from incremental compilation.

lib/src/dart.dart

@@ -96,10 +96,13 @@ class AnalyzerErrorGroup implements Exception {
 ///
 /// If the compilation succeeds it is saved to a kernel file at [outputPath].
 ///
-/// If compilation fails, the output is cached at [incrementalDillOutputPath].
+/// If compilation fails, the output is cached at "[outputPath].incremental".
 ///
-/// Whichever of [incrementalDillOutputPath] and [outputPath] already exists is
-/// used to initialize the compiler run.
+/// Whichever of "[outputPath].incremental" and [outputPath] already exists is
+/// used to initialize the compiler run. To avoid the potential for
+/// race-conditions, it is first copied to a temporary location, and atomically
+/// moved to either [outputPath] or "[outputPath].incremental" depending on the
+/// result of compilation.
 ///
 /// The [packageConfigPath] should point at the package config file to be used
 /// for `package:` uri resolution.
@@ -114,31 +117,29 @@ class AnalyzerErrorGroup implements Exception {
 /// assets map.
 Future<void> precompile({
   required String executablePath,
-  required String incrementalDillPath,
   required String name,
   required String outputPath,
   required String packageConfigPath,
   List<String> additionalSources = const [],
   String? nativeAssets,
 }) async {
-  ensureDir(p.dirname(outputPath));
-  ensureDir(p.dirname(incrementalDillPath));
-
   const platformDill = 'lib/_internal/vm_platform_strong.dill';
   final sdkRoot = p.relative(p.dirname(p.dirname(Platform.resolvedExecutable)));
   String? tempDir;
   FrontendServerClient? client;
   try {
-    tempDir = createTempDir(p.dirname(incrementalDillPath), 'tmp');
+    ensureDir(p.dirname(outputPath));
+    final incrementalDillPath = '$outputPath.incremental';
+    tempDir = createTempDir(p.dirname(outputPath), 'tmp');
     // To avoid potential races we copy the incremental data to a temporary file
     // for just this compilation.
     final temporaryIncrementalDill =
         p.join(tempDir, '${p.basename(incrementalDillPath)}.temp');
     try {
-      if (fileExists(incrementalDillPath)) {
-        copyFile(incrementalDillPath, temporaryIncrementalDill);
-      } else if (fileExists(outputPath)) {
+      if (fileExists(outputPath)) {
         copyFile(outputPath, temporaryIncrementalDill);
+      } else if (fileExists(incrementalDillPath)) {
+        copyFile(incrementalDillPath, temporaryIncrementalDill);
       }
     } on FileSystemException {
       // Not able to copy existing file, compilation will start from scratch.
@@ -172,11 +173,14 @@ Future<void> precompile({
       // By using rename we ensure atomicity. An external observer will either
       // see the old or the new snapshot.
       renameFile(temporaryIncrementalDill, outputPath);
+      // Any old incremental data is deleted in case we started from a file on
+      // [incrementalDillPath].
+      deleteEntry(incrementalDillPath);
     } else {
       // By using rename we ensure atomicity. An external observer will either
       // see the old or the new snapshot.
       renameFile(temporaryIncrementalDill, incrementalDillPath);
-      // If compilation failed we don't want to leave an incorrect snapshot.
+      // If compilation failed, don't leave an incorrect snapshot.
       tryDeleteEntry(outputPath);
 
       throw ApplicationException(

lib/src/entrypoint.dart

@@ -220,10 +220,6 @@ class Entrypoint {
   /// The path to the directory containing dependency executable snapshots.
   String get _snapshotPath => p.join(cachePath, 'bin');
 
-  /// The path to the directory containing previous dill files for incremental
-  /// builds.
-  String get _incrementalDillsPath => p.join(cachePath, 'incremental');
-
   Entrypoint._(
     this.rootDir,
     this._lockFile,
@@ -522,7 +518,6 @@ To update `$lockFilePath` run `$topLevelProgram pub get`$suffix without
     await dart.precompile(
       executablePath: await resolveExecutable(executable),
       outputPath: pathOfExecutable(executable),
-      incrementalDillPath: incrementalDillPathOfExecutable(executable),
       packageConfigPath: packageConfigPath,
       name: '$package:${p.basenameWithoutExtension(executable.relativePath)}',
       additionalSources: additionalSources,
@@ -553,20 +548,6 @@ To update `$lockFilePath` run `$topLevelProgram pub get`$suffix without
           );
   }
 
-  String incrementalDillPathOfExecutable(Executable executable) {
-    assert(p.isRelative(executable.relativePath));
-    return isGlobal
-        ? p.join(
-            _incrementalDillsPath,
-            '${p.basename(executable.relativePath)}.incremental.dill',
-          )
-        : p.join(
-            _incrementalDillsPath,
-            executable.package,
-            '${p.basename(executable.relativePath)}.incremental.dill',
-          );
-  }
-
   /// The absolute path of [executable] resolved relative to [this].
   Future<String> resolveExecutable(Executable executable) async {
     return p.join(

pubspec.yaml

@@ -9,6 +9,7 @@ dependencies:
   analyzer: ^5.1.0
   args: ^2.4.0
   async: ^2.6.1
+  checks: ^0.2.2
   cli_util: ^0.4.0
   collection: ^1.15.0
   convert: ^3.0.2

test/precompilation_test.dart

@@ -0,0 +1,111 @@
+import 'dart:io';
+
+import 'package:checks/checks.dart';
+import 'package:pub/src/dart.dart';
+import 'package:pub/src/exceptions.dart';
+import 'package:pub/src/log.dart';
+import 'package:test/test.dart';
+
+import 'descriptor.dart';
+
+String outputPath() => '$sandbox/output/snapshot';
+String incrementalDillPath() => '${outputPath()}.incremental';
+
+// A quite big program is needed for the caching to be an actual advantage.
+FileDescriptor foo = file('foo.dart', '''
+foo() {
+  ${List.generate(500000, (index) => 'print("$index");').join('\n')}
+}
+  ''');
+
+FileDescriptor workingMain = file(
+  'main.dart',
+  '''
+import 'foo.dart';
+
+main() async {
+  foo();
+}
+''',
+);
+
+FileDescriptor brokenMain = file(
+  'main.dart',
+  '''
+import 'foo.dart';
+yadda yadda
+main() asyncc {
+  foo();
+}
+''',
+);
+
+Future<Duration> timeCompilation(
+  String executable, {
+  bool fails = false,
+}) async {
+  final s = Stopwatch()..start();
+  verbosity = Verbosity.none;
+  Future<void> compile() async {
+    await precompile(
+      executablePath: executable,
+      name: 'abc',
+      outputPath: outputPath(),
+      packageConfigPath: path('app/.dart_tool/package_config.json'),
+    );
+  }
+
+  if (fails) {
+    await check(compile()).throws<ApplicationException>();
+  } else {
+    await compile();
+  }
+  verbosity = Verbosity.normal;
+  return s.elapsed;
+}
+
+void main() {
+  test('Precompilation is much faster second time and removes old artifacts',
+      () async {
+    await dir('app', [
+      workingMain,
+      foo,
+      packageConfigFile([]),
+    ]).create();
+    final first = await timeCompilation(path('app/main.dart'));
+    check(
+      because: 'Should not leave a stray directory.',
+      File(incrementalDillPath()).existsSync(),
+    ).isFalse();
+    check(File(outputPath()).existsSync()).isTrue();
+
+    // Do a second compilation to compare the compile times, it should be much
+    // faster because it can reuse the compiled data in the dill file.
+    final second = await timeCompilation(path('app/main.dart'));
+    check(first).isGreaterThan(second * 2);
+
+    // Now create an error to test that the output is placed at a different
+    // location.
+    await dir('app', [
+      brokenMain,
+      foo,
+      packageConfigFile([]),
+    ]).create();
+    final afterErrors =
+        await timeCompilation(path('app/main.dart'), fails: true);
+    check(File(incrementalDillPath()).existsSync()).isTrue();
+    check(File(outputPath()).existsSync()).isFalse();
+    check(first).isGreaterThan(afterErrors * 2);
+
+    // Fix the error, and check that we still use the cached output to improve
+    // compile times.
+    await dir('app', [
+      workingMain,
+    ]).create();
+    final afterFix = await timeCompilation(path('app/main.dart'));
+    // The output from the failed compilation should now be gone.
+    check(File('${outputPath()}.incremental').existsSync()).isFalse();
+    check(File(outputPath()).existsSync()).isTrue();
+    check(first).isGreaterThan(afterFix * 2);
+  });
+}

tool/test.dart

@@ -30,16 +30,16 @@ Future<void> main(List<String> args) async {
   });
   final pubSnapshotFilename =
       path.absolute(path.join('.dart_tool', '_pub', 'pub.dart.snapshot.dart2'));
-  final pubSnapshotIncrementalFilename = '$pubSnapshotFilename.incremental';
   try {
-    stderr.writeln('Building snapshot');
+    final stopwatch = Stopwatch()..start();
+    stderr.write('Building snapshot...');
     await precompile(
       executablePath: path.join('bin', 'pub.dart'),
       outputPath: pubSnapshotFilename,
-      incrementalDillPath: pubSnapshotIncrementalFilename,
       name: 'bin/pub.dart',
       packageConfigPath: path.join('.dart_tool', 'package_config.json'),
     );
+    stderr.writeln(' (${stopwatch.elapsed.inMilliseconds}ms)');
     testProcess = await Process.start(
       Platform.resolvedExecutable,
       ['run', 'test', ...args],
@@ -51,11 +51,6 @@ Future<void> main(List<String> args) async {
     stderr.writeln('Failed building snapshot: $e');
     exitCode = 1;
   } finally {
-    try {
-      await File(pubSnapshotFilename).delete();
-      await sub.cancel();
-    } on Exception {
-      // snapshot didn't exist.
-    }
+    await sub.cancel();
   }
 }