From 7007567685e0639271eca55faccdae7cc6dd42c3 Mon Sep 17 00:00:00 2001 From: Jens Johansen Date: Wed, 10 Oct 2018 06:35:49 +0000 Subject: [PATCH] Fix issue with --unsafe-package-serialization --unsafe-package-serialization saves the frontend_server from having to re-serialize everything all the time. Before this CL, though, things could go wrong: If you had a situation where you had previously compiled a file that depend on a package A, and where (some of) package A depends on package B this would happen: - All of package A was serialized together. - All of package B was serialized together. When later, you compile something that depend less on package A - namely on only on parts that does not depend on package B, but is included in the previously serialized package A, the following would happen: - The new (non-package) libraries would be serialized. - Package A serialization would be reused. This is basically fine: Running the app would be fine, everything it actually depend on is there. However, if the VM is forced to compile everything it now also compiles stuff that was included - but really unused - from package A - namely also the libraries that depend on package B --- which is not included. This CL changes the last part by also including package B. The result is that even more unused libraries are included, but that the VM can survive a forceful compile. Note that the --unsafe-package-serialization is only used for tests, so the "including even more unused" part is probably not a big deal, and all in all there's still a big speed advantage to doing this. Change-Id: Iac06ba6f40c2caaacce641c5853e9491496dce53 Reviewed-on: https://dart-review.googlesource.com/c/78541 Reviewed-by: Vyacheslav Egorov Commit-Queue: Jens Johansen --- pkg/vm/lib/frontend_server.dart | 42 ++++++++- pkg/vm/test/frontend_server_test.dart | 122 ++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 3 deletions(-) diff --git a/pkg/vm/lib/frontend_server.dart b/pkg/vm/lib/frontend_server.dart index 7cdc7d7099d..abb66619b87 100644 --- a/pkg/vm/lib/frontend_server.dart +++ b/pkg/vm/lib/frontend_server.dart @@ -506,6 +506,14 @@ class FrontendCompiler implements CompilerInterface { /// [writePackagesToSinkAndTrimComponent]. Map> cachedPackageLibraries = new Map>(); + /// Map of dependencies for already serialized dill data. + /// E.g. if blob1 dependents on blob2, but only using a single file from blob1 + /// that does not dependent on blob2, blob2 would not be included leaving the + /// dill file in a weird state that could cause the VM to crash if asked to + /// forcefully compile everything. Used by + /// [writePackagesToSinkAndTrimComponent]. + Map> cachedPackageDependencies = new Map>(); + writePackagesToSinkAndTrimComponent( Component deltaProgram, Sink> ioSink) { if (deltaProgram == null) return; @@ -528,12 +536,21 @@ class FrontendCompiler implements CompilerInterface { Map> newPackages = new Map>(); Set> alreadyAdded = new Set>(); + + addDataAndDependentData(List data, Uri uri) { + if (alreadyAdded.add(data)) { + ioSink.add(data); + // Now also add all dependencies. + for (Uri dep in cachedPackageDependencies[uri]) { + addDataAndDependentData(cachedPackageLibraries[dep], dep); + } + } + } + for (Library lib in packageLibraries) { List data = cachedPackageLibraries[lib.fileUri]; if (data != null) { - if (alreadyAdded.add(data)) { - ioSink.add(data); - } + addDataAndDependentData(data, lib.fileUri); } else { String package = lib.importUri.pathSegments.first; newPackages[package] ??= []; @@ -550,9 +567,28 @@ class FrontendCompiler implements CompilerInterface { ByteSink byteSink = new ByteSink(); final BinaryPrinter printer = printerFactory.newBinaryPrinter(byteSink); printer.writeComponentFile(singleLibrary); + + // Record things this package blob dependent on. + Set libraryUris = new Set(); + for (Library lib in libraries) { + libraryUris.add(lib.fileUri); + } + Set deps = new Set(); + for (Library lib in libraries) { + for (LibraryDependency dep in lib.dependencies) { + Library dependencyLibrary = dep.importedLibraryReference.asLibrary; + if (dependencyLibrary.importUri.scheme != "package") continue; + Uri dependencyLibraryUri = + dep.importedLibraryReference.asLibrary.fileUri; + if (libraryUris.contains(dependencyLibraryUri)) continue; + deps.add(dependencyLibraryUri); + } + } + List data = byteSink.builder.takeBytes(); for (Library lib in libraries) { cachedPackageLibraries[lib.fileUri] = data; + cachedPackageDependencies[lib.fileUri] = new List.from(deps); } ioSink.add(data); } diff --git a/pkg/vm/test/frontend_server_test.dart b/pkg/vm/test/frontend_server_test.dart index efef8e841e4..f049e27870d 100644 --- a/pkg/vm/test/frontend_server_test.dart +++ b/pkg/vm/test/frontend_server_test.dart @@ -792,6 +792,128 @@ true inputStreamController.close(); }); + test('unsafe-package-serialization', () async { + // Package A. + var file = new File('${tempDir.path}/pkgA/a.dart') + ..createSync(recursive: true); + file.writeAsStringSync("pkgA() {}"); + + // Package B. + file = new File('${tempDir.path}/pkgB/.packages') + ..createSync(recursive: true); + file.writeAsStringSync("pkgA: ../pkgA"); + file = new File('${tempDir.path}/pkgB/a.dart') + ..createSync(recursive: true); + file.writeAsStringSync("pkgB_a() {}"); + file = new File('${tempDir.path}/pkgB/b.dart') + ..createSync(recursive: true); + file.writeAsStringSync("import 'package:pkgA/a.dart';" + "pkgB_b() { pkgA(); }"); + + // Application. + file = new File('${tempDir.path}/app/.packages') + ..createSync(recursive: true); + file.writeAsStringSync("pkgA:../pkgA\n" + "pkgB:../pkgB"); + + // Entry point A uses both package A and B. + file = new File('${tempDir.path}/app/a.dart') + ..createSync(recursive: true); + file.writeAsStringSync("import 'package:pkgB/b.dart';" + "import 'package:pkgB/a.dart';" + "appA() { pkgB_a(); pkgB_b(); }"); + + // Entry point B uses only package B. + var fileB = new File('${tempDir.path}/app/B.dart') + ..createSync(recursive: true); + fileB.writeAsStringSync("import 'package:pkgB/a.dart';" + "appB() { pkgB_a(); }"); + + // Other setup. + var dillFile = new File('${tempDir.path}/app.dill'); + expect(dillFile.existsSync(), equals(false)); + + // First compile app entry point A. + final List args = [ + '--sdk-root=${sdkRoot.toFilePath()}', + '--strong', + '--incremental', + '--platform=${platformKernel.path}', + '--output-dill=${dillFile.path}', + '--unsafe-package-serialization', + ]; + + final StreamController> inputStreamController = + new StreamController>(); + final StreamController> stdoutStreamController = + new StreamController>(); + final IOSink ioSink = new IOSink(stdoutStreamController.sink); + StreamController receivedResults = new StreamController(); + + String boundaryKey; + stdoutStreamController.stream + .transform(utf8.decoder) + .transform(const LineSplitter()) + .listen((String s) { + const String RESULT_OUTPUT_SPACE = 'result '; + if (boundaryKey == null) { + if (s.startsWith(RESULT_OUTPUT_SPACE)) { + boundaryKey = s.substring(RESULT_OUTPUT_SPACE.length); + } + } else { + if (s.startsWith(boundaryKey)) { + receivedResults.add(s.substring(boundaryKey.length + 1)); + boundaryKey = null; + } + } + }); + Future result = + starter(args, input: inputStreamController.stream, output: ioSink); + inputStreamController.add('compile ${file.path}\n'.codeUnits); + int count = 0; + receivedResults.stream.listen((String outputFilenameAndErrorCount) { + CompilationResult result = + new CompilationResult.parse(outputFilenameAndErrorCount); + switch (count) { + case 0: + expect(dillFile.existsSync(), equals(true)); + expect(result.filename, dillFile.path); + expect(result.errorsCount, 0); + count += 1; + inputStreamController.add('accept\n'.codeUnits); + inputStreamController.add('reset\n'.codeUnits); + + inputStreamController.add('recompile ${fileB.path} abc\n' + '${fileB.path}\n' + 'abc\n' + .codeUnits); + break; + case 1: + expect(result.filename, dillFile.path); + expect(result.errorsCount, 0); + inputStreamController.add('quit\n'.codeUnits); + + // Loadable. + Component component = loadComponentFromBinary(dillFile.path); + + // Contains (at least) the 2 files we want. + component.libraries + .where((l) => + l.importUri.toString() == "package:pkgB/a.dart" || + l.fileUri.toString().contains(fileB.path)) + .length == + 2; + + // Verifiable (together with the platform file). + component = + loadComponentFromBinary(platformKernel.toFilePath(), component); + verifyComponent(component); + } + }); + expect(await result, 0); + inputStreamController.close(); + }); + test('compile and recompile report non-zero error count', () async { var file = new File('${tempDir.path}/foo.dart')..createSync(); file.writeAsStringSync("main() { foo(); bar(); }\n");