[kernel] Optimize reading of metadata further

This optimization is based on the assumption (which at least holds true
for the dill file extracted from the mentioned bug) that only very few
nodes actually have metadata associated with it. For the test file, more
than two million node-offsets is looked up, and only ~7.5 thousand nodes
actually have metadata on it.
What is done here is to assume that most often we ask offsets in order
and that most often the answer is that there's no metadata (when we fall
into this case we can return `false` quickly) and we can skip looking up
the actual metadata. This furthermore - although not benchmark here -
should make it (a lot) faster in the case where we have multiple
subsections as we here only lookup in in one index. Only upon a `true`
response from that we have to lookup in all subsections to find and read
the actual metadata to associate.

Reading the test file without metadata, taking an average of averages
(average of 5 runs of the binary benchmarks output for
"AstFromBinaryEager") takes: 374481.4 us.

Before https://dart-review.googlesource.com/c/sdk/+/391761 reading the
test file with metadata takes: 627120.68 us.
This was about a quarter of a second, or 67+% slower.

With https://dart-review.googlesource.com/c/sdk/+/391761 this number
became: 441784.44 us.
A reduction of ~185 ms, or almost 30%, making the tax of reading
metadata "only" ~18%.

With this CL this number goes to 398011.44 us.
This is a further reduction of ~43 ms, or almost 10% (for a combined
reduction of ~230 ms or 36+%).
The tax of reading metadata is now ~6.3%.

Bug: flutter/flutter#156713
Change-Id: I20bbabecfc8976293fe897528d05fc2145e2a8c0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/392021
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/kernel/lib/binary/ast_from_binary.dart

@@ -4162,6 +4162,8 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
   /// List of metadata subsections that have corresponding [MetadataRepository]
   /// and are awaiting to be parsed and attached to nodes.
   List<_MetadataSubsection>? _subsections;
+  List<int>? _allKnownMetadataKeys;
+  int? _previousMetadataLookupKey;
 
   BinaryBuilderWithMetadata(List<int> bytes,
       {String? filename,
@@ -4180,6 +4182,7 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
     // If reading a component with several sub-components there's no reason to
     // lookup in old ones.
     _subsections = null;
+    _allKnownMetadataKeys = null;
 
     // At the beginning of this function _byteOffset points right past
     // metadataMappings to string table.
@@ -4204,11 +4207,13 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
         // Read nodeOffsetToMetadataOffset mapping.
         final Map<int, int> mapping = <int, int>{};
         _byteOffset = mappingStart;
+        List<int> allKnownMetadataKeys = _allKnownMetadataKeys ??= [];
         for (int j = 0; j < mappingLength; j++) {
           final int nodeOffset = _componentStartOffset + readUint32();
           final int metadataOffset =
               binaryOffsetForMetadataPayloads + readUint32();
           mapping[nodeOffset] = metadataOffset;
+          allKnownMetadataKeys.add(nodeOffset);
         }
 
         (_subsections ??= <_MetadataSubsection>[])
@@ -4218,6 +4223,7 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
       // Start of the subsection and the end of the previous one.
       endOffset = mappingStart - 4;
     }
+    _allKnownMetadataKeys?.sort();
   }
 
   Object _readMetadata(Node node, MetadataRepository repository, int offset) {
@@ -4230,12 +4236,47 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
     return metadata;
   }
 
+  bool _hasMetadata(int nodeOffset) {
+    List<int>? allKnownMetadataKeys = _allKnownMetadataKeys;
+    if (allKnownMetadataKeys == null) return false;
+    if (allKnownMetadataKeys.isEmpty) return false;
+
+    int? prevIndex = _previousMetadataLookupKey;
+    if (prevIndex != null) {
+      if (prevIndex >= 0 && prevIndex < allKnownMetadataKeys.length - 1) {
+        int prevOffset = allKnownMetadataKeys[prevIndex];
+        int nextOffset = allKnownMetadataKeys[prevIndex + 1];
+        if (prevOffset < nodeOffset && nextOffset > nodeOffset) {
+          // Common case: This is between the previously found metadata
+          // and the next metadata (and doesn't itself have metadata).
+          return false;
+        }
+      }
+    }
+
+    _previousMetadataLookupKey = null;
+    int low = 0, high = allKnownMetadataKeys.length - 1;
+    while (low < high) {
+      int mid = high - ((high - low) >> 1); // Get middle, rounding up.
+      int pivot = allKnownMetadataKeys[mid];
+      if (pivot <= nodeOffset) {
+        low = mid;
+      } else {
+        high = mid - 1;
+      }
+    }
+    this._previousMetadataLookupKey = low;
+    if (allKnownMetadataKeys[low] == nodeOffset) {
+      return true;
+    }
+    return false;
+  }
+
   @override
   T _associateMetadata<T extends Node>(T node, int nodeOffset) {
     if (_subsections == null) {
       return node;
     }
-
     for (_MetadataSubsection subsection in _subsections!) {
       // First check if there is any metadata associated with this node.
       final int? metadataOffset = subsection.mapping[nodeOffset];
@@ -4251,137 +4292,156 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
   @override
   DartType readDartType({bool forSupertype = false}) {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final DartType result = super.readDartType(forSupertype: forSupertype);
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Library readLibrary(Component component, int endOffset) {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Library result = super.readLibrary(component, endOffset);
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Typedef readTypedef() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Typedef result = super.readTypedef();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Class readClass(int endOffset) {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Class result = super.readClass(endOffset);
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Extension readExtension() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Extension result = super.readExtension();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   ExtensionTypeDeclaration readExtensionTypeDeclaration() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final ExtensionTypeDeclaration result =
         super.readExtensionTypeDeclaration();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Field readField() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Field result = super.readField();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Constructor readConstructor() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Constructor result = super.readConstructor();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Procedure readProcedure(int endOffset) {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Procedure result = super.readProcedure(endOffset);
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Initializer readInitializer() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Initializer result = super.readInitializer();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   FunctionNode readFunctionNode(
       {bool lazyLoadBody = false, int outerEndOffset = -1}) {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final FunctionNode result = super.readFunctionNode(
         lazyLoadBody: lazyLoadBody, outerEndOffset: outerEndOffset);
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Expression readExpression() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Expression result = super.readExpression();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Arguments readArguments() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Arguments result = super.readArguments();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   NamedExpression readNamedExpression() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final NamedExpression result = super.readNamedExpression();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   VariableDeclaration readVariableDeclaration() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final VariableDeclaration result = super.readVariableDeclaration();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Statement readStatement() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Statement result = super.readStatement();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Combinator readCombinator() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Combinator result = super.readCombinator();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   LibraryDependency readLibraryDependency() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final LibraryDependency result = super.readLibraryDependency();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   LibraryPart readLibraryPart() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final LibraryPart result = super.readLibraryPart();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
@@ -4399,18 +4459,20 @@ class BinaryBuilderWithMetadata extends BinaryBuilder implements BinarySource {
   @override
   Supertype readSupertype() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     InterfaceType type =
         super.readDartType(forSupertype: true) as InterfaceType;
-    return _associateMetadata(
-        new Supertype.byReference(type.classReference, type.typeArguments),
-        nodeOffset);
+    Supertype result =
+        new Supertype.byReference(type.classReference, type.typeArguments);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 
   @override
   Name readName() {
     final int nodeOffset = _byteOffset;
+    final bool hasMetadata = _hasMetadata(_byteOffset);
     final Name result = super.readName();
-    return _associateMetadata(result, nodeOffset);
+    return hasMetadata ? _associateMetadata(result, nodeOffset) : result;
   }
 }
 

pkg/kernel/test/binary_bench2.dart

@@ -201,8 +201,7 @@ Component _fromBinary(List<int> bytes,
         filename: 'filename', disableLazyReading: eager);
     builder.readComponent(component);
     if (verbose) {
-      // print("#lookups: ${builder.lookups}");
-      // print("#good lookups: ${builder.goodLookups}");
+      // No current verbose output.
     }
   } else {
     new BinaryBuilder(bytes, filename: 'filename', disableLazyReading: eager)