Incremental compiler and incremental serializer do not support kernel metadata

[alexmarkov]

1. If front-end server is used to compile dart2js in non-incremental mode, and resulting dill file is dumped (using pkg/vm/bin/dump_kernel.dart, as it recognizes VM metadata), then there are @vm.call-site-attributes.metadata. This is the expected behavior.

tools/sdks/dart-sdk/bin/dart pkg/frontend_server/bin/frontend_server_starter.dart --target=vm --sdk-root=out/ReleaseX64 --platform=vm_platform_strong.dill --output-dill=foo.dill --packages=.packages pkg/compiler/bin/dart2js.dart
tools/sdks/dart-sdk/bin/dart pkg/vm/bin/dump_kernel.dart foo.dill foo.dump

library from "package:_fe_analyzer_shared/src/flow_analysis/flow_analysis.dart" as flo {
...
    method beginNode() → void {
      [@vm.call-site-attributes.metadata=receiverType:dart.core::List<#lib1::AssignedVariablesNodeInfo<#lib1::AssignedVariables::Variable*>*>*] this.{flo::AssignedVariables::_stack}.{core::List::add}(new flo::AssignedVariablesNodeInfo::•<flo::AssignedVariables::Variable*>());
    }

2. In incremental mode (--incremental) there is no metadata:

rm foo.dill
tools/sdks/dart-sdk/bin/dart pkg/frontend_server/bin/frontend_server_starter.dart --incremental --target=vm --sdk-root=out/ReleaseX64 --platform=vm_platform_strong.dill --output-dill=foo.dill --packages=.packages pkg/compiler/bin/dart2js.dart
tools/sdks/dart-sdk/bin/dart pkg/vm/bin/dump_kernel.dart foo.dill foo.dump

    method beginNode() → void {
      this.{flo::AssignedVariables::_stack}.{core::List::add}(new flo::AssignedVariablesNodeInfo::•<flo::AssignedVariables::Variable*>());
    }

There is no metadata even if incremental serialization is disabled with --no-incremental-serialization. So it looks like incremental compiler does not keep metadata.

3. To check if there are problems with serialization, we can also annotate kernel component after incremental compilation is finished:

diff --git a/pkg/frontend_server/lib/frontend_server.dart b/pkg/frontend_server/lib/frontend_server.dart
index 10eab7b4c3..a8d66bccac 100644
--- a/pkg/frontend_server/lib/frontend_server.dart
+++ b/pkg/frontend_server/lib/frontend_server.dart
@@ -36,6 +36,8 @@ import 'package:vm/bytecode/gen_bytecode.dart'
 import 'package:vm/bytecode/options.dart' show BytecodeOptions;
 import 'package:vm/incremental_compiler.dart' show IncrementalCompiler;
 import 'package:vm/kernel_front_end.dart';
+import 'package:vm/transformations/call_site_annotator.dart'
+    as call_site_annotator;
 
 import 'src/javascript_bundle.dart';
 import 'src/strong_components.dart';
@@ -455,6 +457,12 @@ class FrontendCompiler implements CompilerInterface {
     if (results.component != null) {
       transformer?.transform(results.component);
 
+      call_site_annotator.transformLibraries(
+          results.component,
+          results.component.libraries,
+          results.coreTypes,
+          results.classHierarchy);
+
       if (_compilerOptions.target.name == 'dartdevc') {
         await writeJavascriptBundle(results, _kernelBinaryFilename);
       } else {

By default, there is still no metadata.

rm foo.dill
tools/sdks/dart-sdk/bin/dart pkg/frontend_server/bin/frontend_server_starter.dart --incremental --target=vm --sdk-root=out/ReleaseX64 --platform=vm_platform_strong.dill --output-dill=foo.dill --packages=.packages pkg/compiler/bin/dart2js.dart

However, without incremental serialization (--no-incremental-serialization) there is metadata:

rm foo.dill
tools/sdks/dart-sdk/bin/dart pkg/frontend_server/bin/frontend_server_starter.dart --no-incremental-serialization --incremental --target=vm --sdk-root=out/ReleaseX64 --platform=vm_platform_strong.dill --output-dill=foo.dill --packages=.packages pkg/compiler/bin/dart2js.dart

So it looks like incremental serialization doesn't keep metadata as well.

/cc @jensjoha @aam

[jensjoha]

Another reason (along with #38078 (comment)) why the current metadata system doesn't work.
I think it should be re-done.

Does this fix it?:

diff --git a/pkg/front_end/lib/src/fasta/incremental_serializer.dart b/pkg/front_end/lib/src/fasta/incremental_serializer.dart
index 59d84880a4f..54ad29d95c4 100644
--- a/pkg/front_end/lib/src/fasta/incremental_serializer.dart
+++ b/pkg/front_end/lib/src/fasta/incremental_serializer.dart
@@ -10,7 +10,8 @@ import 'package:kernel/binary/ast_from_binary.dart' show SubComponentView;
 
 import 'package:kernel/binary/ast_to_binary.dart' show BinaryPrinter;
 
-import 'package:kernel/kernel.dart' show Component, Library, LibraryDependency;
+import 'package:kernel/kernel.dart'
+    show Component, Library, LibraryDependency, MetadataRepository;
 
 import 'kernel/utils.dart' show ByteSink;
 
@@ -302,6 +303,9 @@ class IncrementalSerializer {
         libraries: libraries,
         uriToSource: component.uriToSource,
         nameRoot: component.root);
+    for (MetadataRepository repository in component.metadata.values) {
+      singlePackageLibraries.addMetadataRepository(repository);
+    }
 
     ByteSink byteSink = new ByteSink();
     final BinaryPrinter printer = new BinaryPrinter(byteSink);

[alexmarkov]

@jensjoha With your patch, along with annotating kernel Component before serialization (case 3 above) dumping dill file crashes in the following way (tried at b5110a5):

Unhandled exception:
type 'InterfaceType' is not a subtype of type 'TreeNode'
#0      BinaryBuilderWithMetadata._associateMetadata (package:kernel/binary/ast_from_binary.dart:2417:39)
#1      BinaryBuilderWithMetadata.readDartType (package:kernel/binary/ast_from_binary.dart:2429:12)
#2      BinaryBuilder.readFunctionNode (package:kernel/binary/ast_from_binary.dart:1506:22)
#3      BinaryBuilderWithMetadata.readFunctionNode (package:kernel/binary/ast_from_binary.dart:2499:26)
#4      BinaryBuilder.readConstructor (package:kernel/binary/ast_from_binary.dart:1313:20)
#5      BinaryBuilderWithMetadata.readConstructor (package:kernel/binary/ast_from_binary.dart:2470:26)
#6      BinaryBuilder.readClassPartialContent.<anonymous closure> (package:kernel/binary/ast_from_binary.dart:1208:47)
#7      BinaryBuilder._fillTreeNodeList (package:kernel/binary/ast_from_binary.dart:376:36)
#8      BinaryBuilder._mergeNamedNodeList (package:kernel/binary/ast_from_binary.dart:409:7)
#9      BinaryBuilder.readClassPartialContent (package:kernel/binary/ast_from_binary.dart:1207:5)
#10     BinaryBuilder.readClass (package:kernel/binary/ast_from_binary.dart:1129:7)
#11     BinaryBuilderWithMetadata.readClass (package:kernel/binary/ast_from_binary.dart:2449:26)
#12     BinaryBuilder.readLibrary.<anonymous closure> (package:kernel/binary/ast_from_binary.dart:953:14)
#13     BinaryBuilder._fillTreeNodeList (package:kernel/binary/ast_from_binary.dart:376:36)
#14     BinaryBuilder._mergeNamedNodeList (package:kernel/binary/ast_from_binary.dart:409:7)
#15     BinaryBuilder.readLibrary (package:kernel/binary/ast_from_binary.dart:951:5)
#16     BinaryBuilderWithMetadata.readLibrary (package:kernel/binary/ast_from_binary.dart:2435:26)
#17     BinaryBuilder._readOneComponent (package:kernel/binary/ast_from_binary.dart:731:25)
#18     BinaryBuilder.readComponent.<anonymous closure> (package:kernel/binary/ast_from_binary.dart:499:33)
#19     Timeline.timeSync (dart:developer/timeline.dart:161:22)
#20     BinaryBuilder.readComponent (package:kernel/binary/ast_from_binary.dart:472:21)
#21     main (file:///usr/local/google/home/alexmarkov/work/dart/sdk/pkg/vm/bin/dump_kernel.dart:47:40)
<asynchronous suspension>
#22     _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:303:32)
#23     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:172:12)

We don't really annotate type nodes. I think this crash might be related to incorrect calculation of node offsets in case of multi-component dill files.

[mkustermann]

We should identify all places where the JIT needs metadata and see if we can get the needed information baked into the normal Kernel AST (and work with CFE team is there's something missing).

That would simplify the incremental compiler as well as the VM itself.

[alexmarkov]

We might be able to remove call sites metadata after fully switching to the new invocation AST nodes, so there will be no uses of metadata in JIT mode. However, it is still useful to have a mechanism like metadata for passing arbitrary data from kernel to the VM without modifying AST nodes. For example, consider many uses of metadata in AOT - it would be superfluous to add all that information to AST nodes.

[alexmarkov]

It turns out new invocation nodes do not have sufficient information to replace call sites metadata. Filed #46340 to address that.

[johnniwinther]

@jensjoha We probably need to support this if we going to use the incremental compiler for one-off compiles.