Improve precompilation (#4005)
diff --git a/doc/cache_layout.md b/doc/cache_layout.md
index 5c58d36..47c6305 100644
--- a/doc/cache_layout.md
+++ b/doc/cache_layout.md
@@ -194,7 +194,6 @@
│ └── mono_repo.dart-3.0.0-55.0.dev.snapshot
├── .dart_tool/
│ └── package_config.json
- ├── incremental
└── pubspec.lock
```
@@ -213,9 +212,6 @@
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.
diff --git a/doc/dart_tool_layout.md b/doc/dart_tool_layout.md
new file mode 100644
index 0000000..94f34be
--- /dev/null
+++ b/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.
diff --git a/lib/src/dart.dart b/lib/src/dart.dart
index 4e1c4d1..85e3095 100644
--- a/lib/src/dart.dart
+++ b/lib/src/dart.dart
@@ -96,10 +96,13 @@
///
/// 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 @@
/// 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 @@
// 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(
diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart
index efba10c..b149008 100644
--- a/lib/src/entrypoint.dart
+++ b/lib/src/entrypoint.dart
@@ -222,10 +222,6 @@
/// 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,
@@ -524,7 +520,6 @@
await dart.precompile(
executablePath: await resolveExecutable(executable),
outputPath: pathOfExecutable(executable),
- incrementalDillPath: incrementalDillPathOfExecutable(executable),
packageConfigPath: packageConfigPath,
name: '$package:${p.basenameWithoutExtension(executable.relativePath)}',
additionalSources: additionalSources,
@@ -555,20 +550,6 @@
);
}
- 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(
diff --git a/pubspec.yaml b/pubspec.yaml
index 4e034b8..a9d1321 100644
--- a/pubspec.yaml
+++ b/pubspec.yaml
@@ -9,6 +9,7 @@
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
diff --git a/test/precompilation_test.dart b/test/precompilation_test.dart
new file mode 100644
index 0000000..7ed02ea
--- /dev/null
+++ b/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);
+ });
+}
diff --git a/tool/test.dart b/tool/test.dart
index d0ffe95..2eb8adc 100755
--- a/tool/test.dart
+++ b/tool/test.dart
@@ -30,16 +30,16 @@
});
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 @@
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();
}
}
