Reapply "Improve hashCode for closure objects" with fixes.

This CL includes the following fixes:
* Fix for incorrect non-nullable assumption about _Closure._hash field.
* Add error handling into BecomeMapTraits::Hash.
* Correct assertions for validating layout of Closure objects.
* Add identityHashCode to the list of VM entry points in precompiler.

Closes #30211.

Original code review:

https://codereview.chromium.org/2983823002/

Original CL description:

This performance improvement is inspired by Flutter listeners stored in
the HashSet (see ObserverList) and frequently checked using
HashSet.contains(). If there are many such listeners and they are
implicit instance closures (for example, created by
'new Listenable.merge(...)'), HashSet.contains() becomes very slow.
It spends a lot of time in Closure_equals native method due to hash
collisions between closure objects with same function
but different receivers.

This CL improves hashCode() calculation for implicit instance closures
by mixing function hashcode with identity hashcode of the receiver.
For explicit closures and static implicit closures hashCode() is
improved by using identityHashCode() of a closure object.

Also, hashcode is calculated once and cached in each closure instance.
The size of a closure instance doesn't grow up because there was unused
word-size padding both on 32-bit and 64-bit architectures.

The execution time of the following micro-benchmark is reduced from
47665ms to 135ms on my Linux/x64 box.

-------------------------------------

import "dart:collection";

class Foo {
  int _a;
  Foo(this._a);
  void bar() {}
}

main() {
  HashSet hs = new HashSet();
  for (int i = 0; i < 1000; ++i) {
    hs.add(new Foo(i).bar);
  }

  var watch = new Stopwatch()..start();

  for (int i = 0; i < 1000; ++i) {
    for (var c in hs) {
      hs.contains(c);
    }
  }

  int time = watch.elapsedMilliseconds;
  print("Time: ${time}ms\n");
}

-------------------------------------

R=zra@google.com

Review-Url: https://codereview.chromium.org/2988493002 .

runtime/lib/function.cc

@@ -68,11 +68,10 @@ DEFINE_NATIVE_ENTRY(Closure_equals, 2) {
   return Bool::False().raw();
 }
 
-DEFINE_NATIVE_ENTRY(Closure_hashCode, 1) {
+DEFINE_NATIVE_ENTRY(Closure_computeHash, 1) {
   const Closure& receiver =
       Closure::CheckedHandle(zone, arguments->NativeArgAt(0));
-  const Function& func = Function::Handle(zone, receiver.function());
-  return func.GetClosureHashCode();
+  return Smi::New(receiver.ComputeHash());
 }
 
 DEFINE_NATIVE_ENTRY(Closure_clone, 1) {

runtime/lib/function.dart

@@ -5,16 +5,23 @@
 class _Closure implements Function {
   bool operator ==(other) native "Closure_equals";
 
-  int get hashCode native "Closure_hashCode";
+  int get hashCode {
+    if (_hash == null) {
+      _hash = _computeHash();
+    }
+    return _hash;
+  }
 
   _Closure get call => this;
 
   _Closure _clone() native "Closure_clone";
 
+  int _computeHash() native "Closure_computeHash";
+
   // No instance fields should be declared before the following 4 fields whose
   // offsets must be identical in Dart and C++.
 
-  // The following 4 fields are declared both in raw_object.h (for direct access
+  // The following fields are declared both in raw_object.h (for direct access
   // from C++ code) and also here so that the offset-to-field map used by
   // deferred objects is properly initialized.
   // Caution: These fields are not Dart instances, but VM objects. Their Dart
@@ -23,4 +30,13 @@ class _Closure implements Function {
   var _function_type_arguments;
   var _function;
   var _context;
+
+  // Note: _Closure objects are created by VM "magically", without invoking
+  // constructor. So, _Closure default constructor is never compiled and
+  // detection of default-initialized fields is not performed.
+  // As a consequence, VM incorrectly assumes that _hash field is not
+  // nullable and may incorrectly remove 'if (_hash == null)' in get:hashCode.
+  // This initializer makes _hash field nullable even without constructor
+  // compilation.
+  var _hash = null;
 }

runtime/vm/bootstrap.cc

@@ -232,7 +232,7 @@ static void Finish(Thread* thread, bool from_kernel) {
 #if defined(DEBUG)
   // Verify that closure field offsets are identical in Dart and C++.
   const Array& fields = Array::Handle(zone, cls.fields());
-  ASSERT(fields.Length() == 4);
+  ASSERT(fields.Length() == 5);
   Field& field = Field::Handle(zone);
   field ^= fields.At(0);
   ASSERT(field.Offset() == Closure::instantiator_type_arguments_offset());
@@ -242,6 +242,8 @@ static void Finish(Thread* thread, bool from_kernel) {
   ASSERT(field.Offset() == Closure::function_offset());
   field ^= fields.At(3);
   ASSERT(field.Offset() == Closure::context_offset());
+  field ^= fields.At(4);
+  ASSERT(field.Offset() == Closure::hash_offset());
 #endif  // defined(DEBUG)
 
   // Eagerly compile Bool class, bool constants are used from within compiler.

runtime/vm/bootstrap_natives.h

@@ -27,7 +27,7 @@ namespace dart {
   V(Object_as, 4)                                                              \
   V(Function_apply, 2)                                                         \
   V(Closure_equals, 2)                                                         \
-  V(Closure_hashCode, 1)                                                       \
+  V(Closure_computeHash, 1)                                                    \
   V(Closure_clone, 1)                                                          \
   V(AbstractType_toString, 1)                                                  \
   V(Identical_comparison, 2)                                                   \

runtime/vm/bootstrap_nocore.cc

@@ -49,7 +49,7 @@ void Finish(Thread* thread, bool from_kernel) {
 #if defined(DEBUG)
   // Verify that closure field offsets are identical in Dart and C++.
   const Array& fields = Array::Handle(zone, cls.fields());
-  ASSERT(fields.Length() == 4);
+  ASSERT(fields.Length() == 5);
   Field& field = Field::Handle(zone);
   field ^= fields.At(0);
   ASSERT(field.Offset() == Closure::instantiator_type_arguments_offset());
@@ -59,6 +59,8 @@ void Finish(Thread* thread, bool from_kernel) {
   ASSERT(field.Offset() == Closure::function_offset());
   field ^= fields.At(3);
   ASSERT(field.Offset() == Closure::context_offset());
+  field ^= fields.At(4);
+  ASSERT(field.Offset() == Closure::hash_offset());
 #endif  // defined(DEBUG)
 
   // Eagerly compile Bool class, bool constants are used from within compiler.

runtime/vm/clustered_snapshot.cc

@@ -764,7 +764,6 @@ class ClosureDataDeserializationCluster : public DeserializationCluster {
       data->ptr()->parent_function_ = static_cast<RawFunction*>(d->ReadRef());
       data->ptr()->signature_type_ = static_cast<RawType*>(d->ReadRef());
       data->ptr()->closure_ = static_cast<RawInstance*>(d->ReadRef());
-      data->ptr()->hash_ = Object::null();
     }
   }
 };

runtime/vm/dart_entry.cc

@@ -509,6 +509,24 @@ RawObject* DartLibraryCalls::Equals(const Instance& left,
   return result.raw();
 }
 
+// On success, returns a RawInstance.  On failure, a RawError.
+RawObject* DartLibraryCalls::IdentityHashCode(const Instance& object) {
+  const int kNumArguments = 1;
+  Thread* thread = Thread::Current();
+  Zone* zone = thread->zone();
+  const Library& libcore = Library::Handle(zone, Library::CoreLibrary());
+  ASSERT(!libcore.IsNull());
+  const Function& function = Function::Handle(
+      zone, libcore.LookupFunctionAllowPrivate(Symbols::identityHashCode()));
+  ASSERT(!function.IsNull());
+  const Array& args = Array::Handle(zone, Array::New(kNumArguments));
+  args.SetAt(0, object);
+  const Object& result =
+      Object::Handle(zone, DartEntry::InvokeFunction(function, args));
+  ASSERT(result.IsInstance() || result.IsError());
+  return result.raw();
+}
+
 RawObject* DartLibraryCalls::LookupHandler(Dart_Port port_id) {
   Thread* thread = Thread::Current();
   Zone* zone = thread->zone();

runtime/vm/dart_entry.h

@@ -183,6 +183,9 @@ class DartLibraryCalls : public AllStatic {
   // On success, returns a RawInstance.  On failure, a RawError.
   static RawObject* Equals(const Instance& left, const Instance& right);
 
+  // On success, returns a RawInstance.  On failure, a RawError.
+  static RawObject* IdentityHashCode(const Instance& object);
+
   // Returns the handler if one has been registered for this port id.
   static RawObject* LookupHandler(Dart_Port port_id);
 

runtime/vm/isolate_reload.cc

@@ -357,7 +357,11 @@ class BecomeMapTraits {
     } else if (obj.IsField()) {
       return String::HashRawSymbol(Field::Cast(obj).name());
     } else if (obj.IsInstance()) {
-      return Smi::Handle(Smi::RawCast(Instance::Cast(obj).HashCode())).Value();
+      Object& hashObj = Object::Handle(Instance::Cast(obj).HashCode());
+      if (hashObj.IsError()) {
+        Exceptions::PropagateError(Error::Cast(hashObj));
+      }
+      return Smi::Cast(hashObj).Value();
     }
     return 0;
   }

runtime/vm/object.cc

@@ -6889,25 +6889,13 @@ RawInstance* Function::ImplicitInstanceClosure(const Instance& receiver) const {
                       *this, context);
 }
 
-RawSmi* Function::GetClosureHashCode() const {
+intptr_t Function::ComputeClosureHash() const {
   ASSERT(IsClosureFunction());
-  const Object& obj = Object::Handle(raw_ptr()->data_);
-  ASSERT(!obj.IsNull());
-  if (ClosureData::Cast(obj).hash() != Object::null()) {
-    return Smi::RawCast(ClosureData::Cast(obj).hash());
-  }
-  // Hash not yet computed. Compute and cache it.
   const Class& cls = Class::Handle(Owner());
   intptr_t result = String::Handle(name()).Hash();
   result += String::Handle(Signature()).Hash();
   result += String::Handle(cls.Name()).Hash();
-  // Finalize hash value like for strings so that it fits into a smi.
-  result += result << 3;
-  result ^= result >> 11;
-  result += result << 15;
-  result &= ((static_cast<intptr_t>(1) << String::kHashBits) - 1);
-  ClosureData::Cast(obj).set_hash(result);
-  return Smi::New(result);
+  return result;
 }
 
 RawString* Function::BuildSignature(NameVisibility name_visibility) const {
@@ -7327,10 +7315,6 @@ void ClosureData::set_implicit_static_closure(const Instance& closure) const {
   StorePointer(&raw_ptr()->closure_, closure.raw());
 }
 
-void ClosureData::set_hash(intptr_t value) const {
-  StorePointer(&raw_ptr()->hash_, static_cast<RawObject*>(Smi::New(value)));
-}
-
 void ClosureData::set_parent_function(const Function& value) const {
   StorePointer(&raw_ptr()->parent_function_, value.raw());
 }
@@ -14880,6 +14864,10 @@ RawObject* Instance::HashCode() const {
   return DartLibraryCalls::HashCode(*this);
 }
 
+RawObject* Instance::IdentityHashCode() const {
+  return DartLibraryCalls::IdentityHashCode(*this);
+}
+
 bool Instance::CanonicalizeEquals(const Instance& other) const {
   if (this->raw() == other.raw()) {
     return true;  // "===".
@@ -21669,6 +21657,37 @@ const char* Closure::ToCString() const {
                      from, fun_desc);
 }
 
+int64_t Closure::ComputeHash() const {
+  Zone* zone = Thread::Current()->zone();
+  const Function& func = Function::Handle(zone, function());
+  uint32_t result = 0;
+  if (func.IsImplicitInstanceClosureFunction()) {
+    // Implicit instance closures are not unqiue, so combine function's hash
+    // code with identityHashCode of cached receiver.
+    result = static_cast<uint32_t>(func.ComputeClosureHash());
+    const Context& context = Context::Handle(zone, this->context());
+    const Object& receiver = Object::Handle(zone, context.At(0));
+    const Object& receiverHash =
+        Object::Handle(zone, Instance::Cast(receiver).IdentityHashCode());
+    if (receiverHash.IsError()) {
+      Exceptions::PropagateError(Error::Cast(receiverHash));
+      UNREACHABLE();
+    }
+    result = CombineHashes(
+        result, Integer::Cast(receiverHash).AsTruncatedUint32Value());
+  } else {
+    // Explicit closures and implicit static closures are unique,
+    // so identityHashCode of closure object is good enough.
+    const Object& identityHash = Object::Handle(zone, this->IdentityHashCode());
+    if (identityHash.IsError()) {
+      Exceptions::PropagateError(Error::Cast(identityHash));
+      UNREACHABLE();
+    }
+    result = Integer::Cast(identityHash).AsTruncatedUint32Value();
+  }
+  return FinalizeHash(result, String::kHashBits);
+}
+
 RawClosure* Closure::New(const TypeArguments& instantiator_type_arguments,
                          const TypeArguments& function_type_arguments,
                          const Function& function,

runtime/vm/object.h

@@ -2410,7 +2410,7 @@ class Function : public Object {
 
   RawInstance* ImplicitInstanceClosure(const Instance& receiver) const;
 
-  RawSmi* GetClosureHashCode() const;
+  intptr_t ComputeClosureHash() const;
 
   // Redirection information for a redirecting factory.
   bool IsRedirectingFactory() const;
@@ -3081,9 +3081,6 @@ class ClosureData : public Object {
   RawInstance* implicit_static_closure() const { return raw_ptr()->closure_; }
   void set_implicit_static_closure(const Instance& closure) const;
 
-  RawObject* hash() const { return raw_ptr()->hash_; }
-  void set_hash(intptr_t value) const;
-
   static RawClosureData* New();
 
   FINAL_HEAP_OBJECT_IMPLEMENTATION(ClosureData, Object);
@@ -5541,6 +5538,9 @@ class Instance : public Object {
   // Equivalent to invoking hashCode on this instance.
   virtual RawObject* HashCode() const;
 
+  // Equivalent to invoking identityHashCode with this instance.
+  RawObject* IdentityHashCode() const;
+
   static intptr_t InstanceSize() {
     return RoundedAllocationSize(sizeof(RawInstance));
   }
@@ -8356,6 +8356,9 @@ class Closure : public Instance {
   RawContext* context() const { return raw_ptr()->context_; }
   static intptr_t context_offset() { return OFFSET_OF(RawClosure, context_); }
 
+  RawSmi* hash() const { return raw_ptr()->hash_; }
+  static intptr_t hash_offset() { return OFFSET_OF(RawClosure, hash_); }
+
   static intptr_t InstanceSize() {
     return RoundedAllocationSize(sizeof(RawClosure));
   }
@@ -8367,6 +8370,8 @@ class Closure : public Instance {
     return true;
   }
 
+  int64_t ComputeHash() const;
+
   static RawClosure* New(const TypeArguments& instantiator_type_arguments,
                          const TypeArguments& function_type_arguments,
                          const Function& function,

runtime/vm/precompiler.cc

@@ -636,6 +636,7 @@ void Precompiler::AddRoots(Dart_QualifiedFunctionName embedder_entry_points[]) {
   Dart_QualifiedFunctionName vm_entry_points[] = {
     // Functions
     {"dart:core", "::", "_completeDeferredLoads"},
+    {"dart:core", "::", "identityHashCode"},
     {"dart:core", "AbstractClassInstantiationError",
      "AbstractClassInstantiationError._create"},
     {"dart:core", "ArgumentError", "ArgumentError."},

runtime/vm/raw_object.h

@@ -919,8 +919,7 @@ class RawClosureData : public RawObject {
   RawObject** to_snapshot() {
     return reinterpret_cast<RawObject**>(&ptr()->closure_);
   }
-  RawObject* hash_;
-  RawObject** to() { return reinterpret_cast<RawObject**>(&ptr()->hash_); }
+  RawObject** to() { return reinterpret_cast<RawObject**>(&ptr()->closure_); }
 
   friend class Function;
 };
@@ -1799,16 +1798,18 @@ class RawClosure : public RawInstance {
     return reinterpret_cast<RawObject**>(&ptr()->instantiator_type_arguments_);
   }
 
-  // No instance fields should be declared before the following 4 fields whose
+  // No instance fields should be declared before the following fields whose
   // offsets must be identical in Dart and C++.
 
-  // These 4 fields are also declared in the Dart source of class _Closure.
+  // The following fields are also declared in the Dart source of class
+  // _Closure.
   RawTypeArguments* instantiator_type_arguments_;
   RawTypeArguments* function_type_arguments_;
   RawFunction* function_;
   RawContext* context_;
+  RawSmi* hash_;
 
-  RawObject** to() { return reinterpret_cast<RawObject**>(&ptr()->context_); }
+  RawObject** to() { return reinterpret_cast<RawObject**>(&ptr()->hash_); }
 
   // Note that instantiator_type_arguments_ and function_type_arguments_ are
   // used to instantiate the signature of function_ when this closure is

runtime/vm/raw_object_snapshot.cc

@@ -584,7 +584,6 @@ RawClosureData* ClosureData::ReadFrom(SnapshotReader* reader,
   reader->AddBackRef(object_id, &data, kIsDeserialized);
 
   // Set all the object fields.
-  // Cached hash is null-initialized by ClosureData::New()
   READ_OBJECT_FIELDS(data, data.raw()->from(), data.raw()->to_snapshot(),
                      kAsInlinedObject);
 

runtime/vm/symbols.h

@@ -390,6 +390,7 @@ class ObjectPointerVisitor;
   V(_LocalTypeVariableMirror, "_LocalTypeVariableMirror")                      \
   V(_SourceLocation, "_SourceLocation")                                        \
   V(hashCode, "get:hashCode")                                                  \
+  V(identityHashCode, "identityHashCode")                                      \
   V(OptimizedOut, "<optimized out>")                                           \
   V(NotInitialized, "<not initialized>")                                       \
   V(NotNamed, "<not named>")                                                   \

tests/language/language.status

@@ -76,8 +76,6 @@ constructor6_test: Fail # Issue 6422
 
 generic_methods_type_expression_test: RuntimeError # Issue 25869 / 27460
 
-bound_closure_equality_test: Skip # Issue 30211
-
 # Failures related to super call in ctor initializer list
 super_test: Fail, OK
 final_field_initialization_order_test: Fail, OK
@@ -581,3 +579,5 @@ async_star_no_cancel_test: Skip # Flutter Issue 9110
 async_star_cancel_while_paused_test: Skip # Flutter Issue 9110
 await_for_test: Skip # Flutter Issue 9110
 await_for_cancel_test: Skip # Flutter Issue 9110
+
+