Skip to content

Commit

Permalink
Fix expression evaluation failure after hot restart
Browse files Browse the repository at this point in the history
The lifetime of program compilers in JavaScriptBundler is too short,
moving the program compiler cache to FrontendCompiler instead
so the ProgramCompilers created during main compilation could be reused
for expression evaluation until the next compilation.

Closes: #45266
Change-Id: I7d41476c7064f16cab2783f88c49114a08a0c038
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190301
Commit-Queue: Anna Gringauze <annagrin@google.com>
Reviewed-by: Jonah Williams <jonahwilliams@google.com>
Reviewed-by: Nicholas Shahan <nshahan@google.com>
Reviewed-by: Mark Zhou <markzipan@google.com>
  • Loading branch information
Anna Gringauze authored and commit-bot@chromium.org committed Mar 11, 2021
1 parent 494aaaa commit f132854
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 16 deletions.
25 changes: 18 additions & 7 deletions pkg/frontend_server/lib/frontend_server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import 'dart:io' hide FileSystemEntity;

import 'package:args/args.dart';
import 'package:dev_compiler/dev_compiler.dart'
show DevCompilerTarget, ExpressionCompiler, parseModuleFormat;
show
DevCompilerTarget,
ExpressionCompiler,
parseModuleFormat,
ProgramCompiler;

// front_end/src imports below that require lint `ignore_for_file`
// are a temporary state of things until frontend team builds better api
Expand Down Expand Up @@ -631,14 +635,15 @@ class FrontendCompiler implements CompilerInterface {
final sourceMapsFileSink = sourceMapsFile.openWrite();
final metadataFileSink =
emitDebugMetadata ? metadataFile.openWrite() : null;
await _bundler.compile(
final kernel2JsCompilers = await _bundler.compile(
results.classHierarchy,
results.coreTypes,
results.loadedLibraries,
sourceFileSink,
manifestFileSink,
sourceMapsFileSink,
metadataFileSink);
cachedProgramCompilers.addAll(kernel2JsCompilers);
await Future.wait([
sourceFileSink.close(),
manifestFileSink.close(),
Expand Down Expand Up @@ -806,6 +811,12 @@ class FrontendCompiler implements CompilerInterface {
}
}

/// Program compilers per module.
///
/// Produced suring initial compilation of the module to JavaScript,
/// cached to be used for expression compilation in [compileExpressionToJs].
final Map<String, ProgramCompiler> cachedProgramCompilers = {};

@override
Future<Null> compileExpressionToJs(
String libraryUri,
Expand All @@ -822,7 +833,7 @@ class FrontendCompiler implements CompilerInterface {
reportError('JavaScript bundler is null');
return;
}
if (!_bundler.compilers.containsKey(moduleName)) {
if (!cachedProgramCompilers.containsKey(moduleName)) {
reportError('Cannot find kernel2js compiler for $moduleName.');
return;
}
Expand All @@ -833,13 +844,13 @@ class FrontendCompiler implements CompilerInterface {
_processedOptions.ticker
.logMs('Compiling expression to JavaScript in $moduleName');

var kernel2jsCompiler = _bundler.compilers[moduleName];
final kernel2jsCompiler = cachedProgramCompilers[moduleName];
Component component = _generator.lastKnownGoodComponent;
component.computeCanonicalNames();

_processedOptions.ticker.logMs('Computed component');

var expressionCompiler = new ExpressionCompiler(
final expressionCompiler = new ExpressionCompiler(
_compilerOptions,
parseModuleFormat(_options['dartdevc-module-format'] as String),
errors,
Expand All @@ -848,10 +859,10 @@ class FrontendCompiler implements CompilerInterface {
component,
);

var procedure = await expressionCompiler.compileExpressionToJs(
final procedure = await expressionCompiler.compileExpressionToJs(
libraryUri, line, column, jsFrameValues, expression);

var result = errors.length > 0 ? errors[0] : procedure;
final result = errors.length > 0 ? errors[0] : procedure;

// TODO(annagrin): kernelBinaryFilename is too specific
// rename to _outputFileName?
Expand Down
14 changes: 7 additions & 7 deletions pkg/frontend_server/lib/src/javascript_bundle.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ class JavaScriptBundler {
this.emitDebugMetadata = false,
this.soundNullSafety = false,
String moduleFormat})
: compilers = <String, ProgramCompiler>{},
_moduleFormat = parseModuleFormat(moduleFormat ?? 'amd') {
: _moduleFormat = parseModuleFormat(moduleFormat ?? 'amd') {
_summaries = <Component>[];
_summaryUris = <Uri>[];
_moduleImportForSummary = <Uri, String>{};
Expand Down Expand Up @@ -64,7 +63,6 @@ class JavaScriptBundler {
final PackageConfig _packageConfig;
final bool useDebuggerModuleNames;
final bool emitDebugMetadata;
final Map<String, ProgramCompiler> compilers;
final ModuleFormat _moduleFormat;
final bool soundNullSafety;

Expand All @@ -75,7 +73,7 @@ class JavaScriptBundler {
Map<Uri, Component> _uriToComponent;

/// Compile each component into a single JavaScript module.
Future<void> compile(
Future<Map<String, ProgramCompiler>> compile(
ClassHierarchy classHierarchy,
CoreTypes coreTypes,
Set<Library> loadedLibraries,
Expand All @@ -88,6 +86,7 @@ class JavaScriptBundler {
var metadataOffset = 0;
final manifest = <String, Map<String, List<int>>>{};
final Set<Uri> visited = <Uri>{};
final Map<String, ProgramCompiler> kernel2JsCompilers = {};

final importToSummary = Map<Library, Component>.identity();
final summaryToModule = Map<Component, String>.identity();
Expand Down Expand Up @@ -150,9 +149,8 @@ class JavaScriptBundler {
// so it can map dart symbols to js symbols
// [issue 40273](https://github.com/dart-lang/sdk/issues/40273)

// program compiler is used by ExpressionCompiler to evaluate expressions
// on demand
compilers[moduleName] = compiler;
// Save program compiler to reuse for expression evaluation.
kernel2JsCompilers[moduleName] = compiler;

final moduleUrl = urlForComponentUri(moduleUri);
String sourceMapBase;
Expand Down Expand Up @@ -201,6 +199,8 @@ class JavaScriptBundler {
};
}
manifestSink.add(utf8.encode(json.encode(manifest)));

return kernel2JsCompilers;
}
}

Expand Down
13 changes: 11 additions & 2 deletions pkg/frontend_server/test/src/javascript_bundle_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,14 @@ void main() {
final metadataSink = _MemorySink();
final coreTypes = CoreTypes(testComponent);

await javaScriptBundler.compile(ClassHierarchy(testComponent, coreTypes),
coreTypes, {}, codeSink, manifestSink, sourcemapSink, metadataSink);
final compilers = await javaScriptBundler.compile(
ClassHierarchy(testComponent, coreTypes),
coreTypes,
{},
codeSink,
manifestSink,
sourcemapSink,
metadataSink);

final Map manifest = json.decode(utf8.decode(manifestSink.buffer));
final String code = utf8.decode(codeSink.buffer);
Expand All @@ -122,6 +128,9 @@ void main() {

// verify source map url is correct.
expect(code, contains('sourceMappingURL=c.dart.lib.js.map'));

// verify program compilers are created.
expect(compilers.keys, equals([urlForComponentUri(library.importUri)]));
});

test('converts package: uris into /packages/ uris', () async {
Expand Down

0 comments on commit f132854

Please sign in to comment.