Specialize closures for 0 and 2 arguments

Change-Id: Ia21249cb9671960eb3e0d96afef5e5efb8ba7ee2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105200
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Joshua Litt <joshualitt@google.com>
diff --git a/pkg/compiler/lib/src/common_elements.dart b/pkg/compiler/lib/src/common_elements.dart
index a57232b..91d8067 100644
--- a/pkg/compiler/lib/src/common_elements.dart
+++ b/pkg/compiler/lib/src/common_elements.dart
@@ -335,6 +335,8 @@
   FunctionEntity findHelperFunction(String name);
 
   ClassEntity get closureClass;
+  ClassEntity get closureClass0Args;
+  ClassEntity get closureClass2Args;
 
   ClassEntity get boundClosureClass;
 
@@ -1536,6 +1538,16 @@
   @override
   ClassEntity get closureClass => _closureClass ??= _findHelperClass('Closure');
 
+  ClassEntity _closureClass0Args;
+  @override
+  ClassEntity get closureClass0Args =>
+      _closureClass0Args ??= _findHelperClass('Closure0Args');
+
+  ClassEntity _closureClass2Args;
+  @override
+  ClassEntity get closureClass2Args =>
+      _closureClass2Args ??= _findHelperClass('Closure2Args');
+
   ClassEntity _boundClosureClass;
   @override
   ClassEntity get boundClosureClass =>
diff --git a/pkg/compiler/lib/src/elements/entities.dart b/pkg/compiler/lib/src/elements/entities.dart
index f136327..b0f12cf 100644
--- a/pkg/compiler/lib/src/elements/entities.dart
+++ b/pkg/compiler/lib/src/elements/entities.dart
@@ -291,6 +291,12 @@
   static const ParameterStructure zeroArguments =
       ParameterStructure._(0, 0, [], {}, 0);
 
+  static const ParameterStructure oneArgument =
+      ParameterStructure._(1, 1, [], {}, 0);
+
+  static const ParameterStructure twoArguments =
+      ParameterStructure._(2, 2, [], {}, 0);
+
   static const List<ParameterStructure> _simple = [
     ParameterStructure._(0, 0, [], {}, 0),
     ParameterStructure._(1, 1, [], {}, 0),
diff --git a/pkg/compiler/lib/src/js_backend/backend_impact.dart b/pkg/compiler/lib/src/js_backend/backend_impact.dart
index cb72411..e1de963 100644
--- a/pkg/compiler/lib/src/js_backend/backend_impact.dart
+++ b/pkg/compiler/lib/src/js_backend/backend_impact.dart
@@ -574,8 +574,11 @@
   BackendImpact _functionClass;
 
   BackendImpact get functionClass {
-    return _functionClass ??=
-        new BackendImpact(globalClasses: [_commonElements.closureClass]);
+    return _functionClass ??= new BackendImpact(globalClasses: [
+      _commonElements.closureClass,
+      _commonElements.closureClass0Args,
+      _commonElements.closureClass2Args,
+    ]);
   }
 
   BackendImpact _mapClass;
diff --git a/pkg/compiler/lib/src/js_emitter/model.dart b/pkg/compiler/lib/src/js_emitter/model.dart
index fd4296e..156ccb1 100644
--- a/pkg/compiler/lib/src/js_emitter/model.dart
+++ b/pkg/compiler/lib/src/js_emitter/model.dart
@@ -241,7 +241,14 @@
   final bool onlyForConstructor;
   final bool isDirectlyInstantiated;
   final bool isNative;
-  final bool isClosureBaseClass; // Common base class for closures.
+
+  /// `true` if this is the one class that is the root of all 'Closure' classes.
+  final bool isClosureBaseClass;
+
+  /// If non-null, this class is used as a base class for closures with a fixed
+  /// small number of arguments in order to inherit `Function.apply`
+  /// metadata. The value is the fixed number of arguments.
+  final int sharedClosureApplyMetadata;
 
   final bool isMixinApplicationWithMembers;
 
@@ -279,6 +286,7 @@
       this.isDirectlyInstantiated,
       this.isNative,
       this.isClosureBaseClass,
+      this.sharedClosureApplyMetadata,
       this.isMixinApplicationWithMembers}) {
     assert(onlyForRti != null);
     assert(onlyForConstructor != null);
@@ -492,6 +500,8 @@
   /// functions that can be torn off.
   final bool isClosureCallMethod;
 
+  final bool inheritsApplyMetadata;
+
   /// True if the interceptor calling convention is used for this method.
   final bool isIntercepted;
 
@@ -512,6 +522,7 @@
     int requiredParameterCount,
     /* List | Map */ optionalParameterDefaultValues,
     this.isClosureCallMethod,
+    this.inheritsApplyMetadata,
     this.isIntercepted,
     js.Expression functionType,
     int applyIndex,
diff --git a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
index 6c581a5..2a8ae0a 100644
--- a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
+++ b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
@@ -557,7 +557,18 @@
     bool onlyForRti = _nativeData.isJsInteropClass(cls);
     bool hasRtiField = _rtiNeed.classNeedsTypeArguments(cls);
     bool onlyForConstructorOrRti = onlyForConstructor || onlyForRti;
+
+    // Recognize the specialized base classes for closures.
     bool isClosureBaseClass = cls == _commonElements.closureClass;
+    int sharedClosureApplyMetadata;
+    if (cls == _commonElements.closureClass) {
+      // The root base class has metadata for single-argument closures.
+      sharedClosureApplyMetadata = 1;
+    } else if (cls == _commonElements.closureClass0Args) {
+      sharedClosureApplyMetadata = 0;
+    } else if (cls == _commonElements.closureClass2Args) {
+      sharedClosureApplyMetadata = 2;
+    }
 
     List<Method> methods = [];
     List<StubMethod> callStubs = [];
@@ -712,6 +723,7 @@
       assert(!_nativeData.isNativeClass(cls));
       assert(methods.isEmpty);
       assert(!isClosureBaseClass);
+      assert(sharedClosureApplyMetadata == null);
 
       result = MixinApplication(cls, typeData, name, instanceFields, callStubs,
           checkedSetters, gettersSetters, isChecks, typeTests.functionTypeIndex,
@@ -738,6 +750,7 @@
           onlyForConstructor: onlyForConstructor,
           isNative: _nativeData.isNativeClass(cls),
           isClosureBaseClass: isClosureBaseClass,
+          sharedClosureApplyMetadata: sharedClosureApplyMetadata,
           isMixinApplicationWithMembers: isMixinApplicationWithMembers);
     }
     _classes[cls] = result;
@@ -814,6 +827,7 @@
     bool tearOffNeedsDirectAccess = false;
     js.Name tearOffName;
     bool isClosureCallMethod = false;
+    bool inheritsApplyMetadata = false;
     bool isNotApplyTarget =
         !element.isFunction || element.isGetter || element.isSetter;
 
@@ -829,6 +843,17 @@
       if (element.enclosingClass.isClosure) {
         canTearOff = false;
         isClosureCallMethod = true;
+        ClassEntity superclass =
+            _elementEnvironment.getSuperClass(element.enclosingClass);
+        if (superclass == _commonElements.closureClass &&
+                element.parameterStructure == ParameterStructure.oneArgument ||
+            superclass == _commonElements.closureClass0Args &&
+                element.parameterStructure ==
+                    ParameterStructure.zeroArguments ||
+            superclass == _commonElements.closureClass2Args &&
+                element.parameterStructure == ParameterStructure.twoArguments) {
+          inheritsApplyMetadata = true;
+        }
       } else {
         // Careful with operators.
         bool needsSuperGetter = _codegenWorld.methodsNeedsSuperGetter(element);
@@ -881,6 +906,7 @@
         tearOffName: tearOffName,
         tearOffNeedsDirectAccess: tearOffNeedsDirectAccess,
         isClosureCallMethod: isClosureCallMethod,
+        inheritsApplyMetadata: inheritsApplyMetadata,
         isIntercepted: isIntercepted,
         aliasName: aliasName,
         canBeApplied: canBeApplied,
diff --git a/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart b/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
index 9f40195..bf57340 100644
--- a/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
+++ b/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
@@ -592,6 +592,9 @@
       _call1Name ??= _namer.getNameForJsGetName(null, JsGetName.CALL_PREFIX1);
   js.Name get call2Name =>
       _call2Name ??= _namer.getNameForJsGetName(null, JsGetName.CALL_PREFIX2);
+  List<js.Name> _callNamesByArity;
+  List<js.Name> get callNamesByArity =>
+      _callNamesByArity ??= [call0Name, call1Name, call2Name];
 
   FragmentEmitter(
       this._options,
@@ -1068,26 +1071,31 @@
     allMethods.forEach((Method method) {
       emitInstanceMethod(method)
           .forEach((js.Expression name, js.Expression code) {
-        var prop = js.Property(name, code);
-        registerEntityAst(method.element, prop);
-        properties.add(prop);
+        final property = js.Property(name, code);
+        registerEntityAst(method.element, property);
+        properties.add(property);
       });
     });
 
-    if (cls.isClosureBaseClass) {
-      // Closures extend a common base class, so we can put properties on the
-      // prototype for common values.
+    // Closures have metadata that is often the same. We avoid repeated metadata
+    // by putting it on a shared superclass. It is overridden in the subclass if
+    // necessary.
 
-      // Closures taking exactly one argument are common.
+    int arity = cls.sharedClosureApplyMetadata;
+    if (arity != null) {
+      // This is a closure base class that has the specialized `Function.apply`
+      // metadata for functions taking exactly [arity] arguments.
       properties.add(js.Property(js.string(_namer.fixedNames.callCatchAllName),
-          js.quoteName(call1Name)));
+          js.quoteName(callNamesByArity[arity])));
       properties.add(js.Property(
-          js.string(_namer.fixedNames.requiredParameterField), js.number(1)));
+          js.string(_namer.fixedNames.requiredParameterField),
+          js.number(arity)));
+    }
 
+    if (cls.isClosureBaseClass) {
       // Most closures have no optional arguments.
       properties.add(js.Property(
-          js.string(_namer.fixedNames.defaultValuesField),
-          new js.LiteralNull()));
+          js.string(_namer.fixedNames.defaultValuesField), js.LiteralNull()));
     }
 
     return new js.ObjectInitializer(properties);
@@ -1112,35 +1120,27 @@
       }
 
       if (method.isClosureCallMethod && method.canBeApplied) {
-        // TODO(sra): We should also add these properties for the user-defined
-        // `call` method on classes. Function.apply is currently broken for
-        // complex cases. [forceAdd] might be true when this is fixed.
-        bool forceAdd = !method.isClosureCallMethod;
+        // The `call` method might flow to `Function.apply`, so the metadata for
+        // `Function.apply` is needed.
 
-        // Common case of "call*": "call$1" is stored on the Closure class.
-        if (method.applyIndex != 0 ||
-            method.parameterStubs.isNotEmpty ||
-            method.requiredParameterCount != 1 ||
-            forceAdd) {
+        // Avoid adding the metadata if a superclass has the same metadata.
+        if (!method.inheritsApplyMetadata) {
           js.Name applyName = method.applyIndex == 0
               ? method.name
               : method.parameterStubs[method.applyIndex - 1].name;
           properties[js.string(_namer.fixedNames.callCatchAllName)] =
               js.quoteName(applyName);
-        }
-        // Common case of '1' is stored on the Closure class.
-        if (method.requiredParameterCount != 1 || forceAdd) {
           properties[js.string(_namer.fixedNames.requiredParameterField)] =
               js.number(method.requiredParameterCount);
-        }
 
-        js.Expression defaultValues =
-            _encodeOptionalParameterDefaultValues(method);
-        // Default values property of `null` is stored on the common JS
-        // superclass.
-        if (defaultValues is! js.LiteralNull || forceAdd) {
-          properties[js.string(_namer.fixedNames.defaultValuesField)] =
-              defaultValues;
+          js.Expression defaultValues =
+              _encodeOptionalParameterDefaultValues(method);
+          // Default values property of `null` is stored on the common JS
+          // superclass.
+          if (defaultValues is! js.LiteralNull) {
+            properties[js.string(_namer.fixedNames.defaultValuesField)] =
+                defaultValues;
+          }
         }
       }
     }
diff --git a/pkg/compiler/lib/src/js_model/js_world_builder.dart b/pkg/compiler/lib/src/js_model/js_world_builder.dart
index c07ccea..8a4928d 100644
--- a/pkg/compiler/lib/src/js_model/js_world_builder.dart
+++ b/pkg/compiler/lib/src/js_model/js_world_builder.dart
@@ -417,7 +417,8 @@
       Map<ir.VariableDeclaration, JRecordField> boxedVariables,
       KernelScopeInfo info,
       {bool createSignatureMethod}) {
-    ClassEntity superclass = _commonElements.closureClass;
+    ClassEntity superclass =
+        _chooseClosureSuperclass(originalClosureFunctionNode);
 
     JsClosureClassInfo closureClassInfo = _elementMap.constructClosureClass(
         member,
@@ -440,6 +441,21 @@
     return closureClassInfo;
   }
 
+  ClassEntity _chooseClosureSuperclass(ir.FunctionNode node) {
+    // Choose a superclass so that similar closures can share the metadata used
+    // by `Function.apply`.
+    int requiredParameterCount = node.requiredParameterCount;
+    if (node.typeParameters.isEmpty &&
+        node.namedParameters.isEmpty &&
+        requiredParameterCount == node.positionalParameters.length) {
+      if (requiredParameterCount == 0) return _commonElements.closureClass0Args;
+      if (requiredParameterCount == 2) return _commonElements.closureClass2Args;
+    }
+    // Note that the base closure class has specialized metadata for the common
+    // case of single-argument functions.
+    return _commonElements.closureClass;
+  }
+
   OutputUnitData _convertOutputUnitData(JsToFrontendMapImpl map,
       OutputUnitData data, ClosureData closureDataLookup) {
     // Convert front-end maps containing K-class and K-local function keys to a
diff --git a/sdk/lib/_internal/js_runtime/lib/js_helper.dart b/sdk/lib/_internal/js_runtime/lib/js_helper.dart
index 4ef1735..f06c29a 100644
--- a/sdk/lib/_internal/js_runtime/lib/js_helper.dart
+++ b/sdk/lib/_internal/js_runtime/lib/js_helper.dart
@@ -2342,6 +2342,12 @@
   return Closure.fromTearOff(parameters);
 }
 
+/// Base class for closures with no arguments.
+abstract class Closure0Args extends Closure {}
+
+/// Base class for closures with two positional arguments.
+abstract class Closure2Args extends Closure {}
+
 /// Represents an implicit closure of a function.
 abstract class TearOffClosure extends Closure {}
 
diff --git a/tests/web/internal/lax_runtime_type_closure_to_string1_test.dart b/tests/web/internal/lax_runtime_type_closure_to_string1_test.dart
index aed91ee..220d3c6 100644
--- a/tests/web/internal/lax_runtime_type_closure_to_string1_test.dart
+++ b/tests/web/internal/lax_runtime_type_closure_to_string1_test.dart
@@ -18,7 +18,7 @@
   var toString = '${local1.runtimeType}';
   if ('$Object' == 'Object') {
     // `true` if non-minified.
-    Expect.equals("Closure", toString);
+    Expect.equals("Closure0Args", toString);
   }
   print(toString);
   local2(0);
diff --git a/tests/web_2/internal/lax_runtime_type_closure_to_string1_test.dart b/tests/web_2/internal/lax_runtime_type_closure_to_string1_test.dart
index 51d90d5..210d320 100644
--- a/tests/web_2/internal/lax_runtime_type_closure_to_string1_test.dart
+++ b/tests/web_2/internal/lax_runtime_type_closure_to_string1_test.dart
@@ -20,7 +20,7 @@
   var toString = '${local1.runtimeType}';
   if ('$Object' == 'Object') {
     // `true` if non-minified.
-    Expect.equals("Closure", toString);
+    Expect.equals("Closure0Args", toString);
   }
   print(toString);
   local2(0);