Simplify and fix implicit closure check, speed up Closure_equals

runtime/lib/function.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/bootstrap_natives.h"
 
 #include "vm/compiler.h"
 #include "vm/dart_entry.h"
 #include "vm/exceptions.h"
 #include "vm/native_entry.h"
@@ skipping 17 matching lines @@
     Exceptions::PropagateError(Error::Cast(result));
   }
   return result.raw();
 }
 
 DEFINE_NATIVE_ENTRY(Closure_equals, 2) {
   const Closure& receiver =
       Closure::CheckedHandle(zone, arguments->NativeArgAt(0));
   GET_NATIVE_ARGUMENT(Instance, other, arguments->NativeArgAt(1));
   ASSERT(!other.IsNull());
-  if (receiver.raw() == other.raw()) return Bool::True().raw();
-  if (other.IsClosure()) {
-    const Function& func_a = Function::Handle(zone, receiver.function());
-    const Function& func_b =
-        Function::Handle(zone, Closure::Cast(other).function());
-    if (func_a.raw() == func_b.raw()) {
-      ASSERT(!func_a.IsImplicitStaticClosureFunction());
-      if (func_a.IsImplicitInstanceClosureFunction()) {
+  // For implicit instance closures compare receiver instance and function's
+  // name and owner (multiple function objects could exist for the same
+  // function due to hot reload).
+  // Objects of other closure kinds are unique, so use identity comparison.
+  if (receiver.raw() == other.raw()) {
+    return Bool::True().raw();
+  }
+  const Function& func_a = Function::Handle(zone, receiver.function());

[siva, 2017/07/21 21:47:17]

It probably makes sense to avoid creation of this handle
if other.IsClosure() is false.
The old code was doing that.

[alexmarkov, 2017/07/21 22:02:43]

My gut feeling tells me that comparing a closure with non-closure is more rare
than comparing 2 closures. By moving IsClosure() check after
IsImplicitClosureFunction() I optimized cases of explicit and implicit static
closures (identity comparison is sufficient for them).

Do you think my assumption is wrong and comparison of closure with non-closure
happens more frequently than comparison of 2 closures?

[siva, 2017/07/21 22:51:24]

Not sure depends on the program being run, you should be able to figure that out
by running some flutter programs (posse, gallery etc.). 

other.IsClosure() is a very cheap function anyway (a virtual call)

[alexmarkov, 2017/07/24 16:11:47]

Reverted to the earlier testing other.IsClosure(), in the next review
(https://codereview.chromium.org/2987703002/).

+  if (func_a.IsImplicitInstanceClosureFunction()) {
+    if (other.IsClosure()) {
+      const Closure& other_closure = Closure::Cast(other);
+      const Function& func_b = Function::Handle(zone, other_closure.function());
+      if (func_b.IsImplicitInstanceClosureFunction()) {
         const Context& context_a = Context::Handle(zone, receiver.context());
         const Context& context_b =
-            Context::Handle(zone, Closure::Cast(other).context());
-        const Object& receiver_a = Object::Handle(zone, context_a.At(0));
-        const Object& receiver_b = Object::Handle(zone, context_b.At(0));
-        if (receiver_a.raw() == receiver_b.raw()) return Bool::True().raw();
-      }
-    } else if (func_a.IsImplicitInstanceClosureFunction() &&
-               func_b.IsImplicitInstanceClosureFunction()) {
-      // TODO(rmacnak): Patch existing tears off during reload instead.
-      const Context& context_a = Context::Handle(zone, receiver.context());
-      const Context& context_b =
-          Context::Handle(zone, Closure::Cast(other).context());
-      const Object& receiver_a = Object::Handle(zone, context_a.At(0));
-      const Object& receiver_b = Object::Handle(zone, context_b.At(0));
-      if ((receiver_a.raw() == receiver_b.raw()) &&
-          (func_a.name() == func_b.name()) &&
-          (func_a.Owner() == func_b.Owner())) {
-        return Bool::True().raw();
+            Context::Handle(zone, other_closure.context());
+        RawObject* receiver_a = context_a.At(0);
+        RawObject* receiver_b = context_b.At(0);
+        if ((receiver_a == receiver_b) &&
+            ((func_a.raw() == func_b.raw()) ||
+             ((func_a.name() == func_b.name()) &&
+              (func_a.Owner() == func_b.Owner())))) {
+          return Bool::True().raw();
+        }
       }
     }
   }
   return Bool::False().raw();
 }
 
 DEFINE_NATIVE_ENTRY(Closure_computeHash, 1) {
   const Closure& receiver =
       Closure::CheckedHandle(zone, arguments->NativeArgAt(0));
   return Smi::New(receiver.ComputeHash());
@@ skipping 14 matching lines @@
   Object& instance = Object::Handle(zone);
   for (int i = 0; i < context.num_variables(); i++) {
     instance = context.At(i);
     cloned_context.SetAt(i, instance);
   }
   return Closure::New(instantiator_type_arguments, function_type_arguments,
                       function, cloned_context);
 }
 
 }  // namespace dart