Make type canonicalization thread safe

runtime/vm/object.cc

Index: runtime/vm/object.cc
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 11de55fb370d6ae092d04b3d981a094e0e1b0343..a23d0a004a3b520ef94a9814310dd70c6ffa9b9d 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -4134,8 +4134,9 @@ RawLibraryPrefix* Class::LookupLibraryPrefix(const String& name) const {
 // conrtinue search for canonical type at the last index visited.
 RawAbstractType* Class::LookupOrAddCanonicalType(
     const AbstractType& lookup_type, intptr_t start_index) const {
-  intptr_t index = start_index;
-  Zone* zone = Thread::Current()->zone();
+  Thread* thread = Thread::Current();
+  Zone* zone = thread->zone();
+  Isolate* isolate = thread->isolate();
   AbstractType& type = Type::Handle(zone);
   Array& canonical_types = Array::Handle(zone);
   canonical_types ^= this->canonical_types();
@@ -4143,7 +4144,8 @@ RawAbstractType* Class::LookupOrAddCanonicalType(
     canonical_types = empty_array().raw();
   }
   ASSERT(canonical_types.IsArray());
-  const intptr_t length = canonical_types.Length();
+  intptr_t length = canonical_types.Length();
+  intptr_t index = start_index;
   while (index < length) {
     type ^= canonical_types.At(index);
     if (type.IsNull()) {
@@ -4157,20 +4159,42 @@ RawAbstractType* Class::LookupOrAddCanonicalType(
     index++;
   }

[siva, 2016/03/01 16:57:40]

Should we hoist the while loop as a helper method 
RawType* LookupType(const Array& canonical_types);

and then we could do

canonical_types ^= this->canonical_types();
type^ = LookupType(canonical_types);
if (!type.IsNull()) {
  return type.raw();
}
{
 Lock...
 ....
 canonical_types ^= ...;
 type ^= LookupType(canonical_types);
 if (!type.IsNull()) {
   return type.raw();
 }
 lookup_type.SetCanonical();
 ...
 ...
}

[srdjan, 2016/03/01 19:28:13]

Done.

 
-  lookup_type.SetCanonical();
+  {
+    SafepointMutexLocker ml(isolate->type_canonicalization_mutex());
+    // Lookup again, in case the canonicalization array changed.
+    canonical_types ^= this->canonical_types();
+    if (canonical_types.IsNull()) {
+      canonical_types = empty_array().raw();
+    }
+    length = canonical_types.Length();
+    while (index < length) {
+      type ^= canonical_types.At(index);
+      if (type.IsNull()) {
+        break;
+      }
+      ASSERT(type.IsFinalized());
+      if (lookup_type.Equals(type)) {
+        ASSERT(type.IsCanonical());
+        return type.raw();
+      }
+      index++;
+    }
 
-  // The type needs to be added to the list. Grow the list if it is full.
-  if (index >= length) {
-    ASSERT((index == length) || ((index == 1) && (length == 0)));
-    const intptr_t new_length = (length > 64) ?
-        (length + 64) :
-        ((length == 0) ? 2 : (length * 2));
-    const Array& new_canonical_types = Array::Handle(
-        zone, Array::Grow(canonical_types, new_length, Heap::kOld));
-    new_canonical_types.SetAt(index, lookup_type);
-    this->set_canonical_types(new_canonical_types);
-  } else {
-    canonical_types.SetAt(index, lookup_type);
+    lookup_type.SetCanonical();
+
+    // The type needs to be added to the list. Grow the list if it is full.
+    if (index >= length) {
+      ASSERT((index == length) || ((index == 1) && (length == 0)));
+      const intptr_t new_length = (length > 64) ?
+          (length + 64) :
+          ((length == 0) ? 2 : (length * 2));
+      const Array& new_canonical_types = Array::Handle(
+          zone, Array::Grow(canonical_types, new_length, Heap::kOld));
+      new_canonical_types.SetAt(index, lookup_type);
+      this->set_canonical_types(new_canonical_types);
+    } else {
+      canonical_types.SetAt(index, lookup_type);
+    }
   }
   return lookup_type.raw();
 }
@@ -15793,9 +15817,14 @@ RawAbstractType* Type::Canonicalize(TrailPtr trail) const {
       set_arguments(type_args);
       type = cls.CanonicalType();  // May be set while canonicalizing type args.
       if (type.IsNull()) {
-        cls.set_canonical_types(*this);
-        SetCanonical();
-        return this->raw();
+        MutexLocker ml(isolate->type_canonicalization_mutex());
+        // Recheck if type exists.
+        type = cls.CanonicalType();
+        if (type.IsNull()) {
+          SetCanonical();
+          cls.set_canonical_types(*this);
+          return this->raw();
+        }
       }
     }
     ASSERT(this->Equals(type));