Simplify and fix implicit closure check, speed up Closure_equals

runtime/lib/function.cc

Index: runtime/lib/function.cc
diff --git a/runtime/lib/function.cc b/runtime/lib/function.cc
index 0bd2c70d429b5d098b97b05524b2a60a447c9335..af6969d1a912bb1bbbe5a43b2871bca77dc4ebbd 100644
--- a/runtime/lib/function.cc
+++ b/runtime/lib/function.cc
@@ -35,33 +35,30 @@ DEFINE_NATIVE_ENTRY(Closure_equals, 2) {
       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();
+        }
       }
     }
   }