Make type canonicalization thread safe

runtime/vm/object.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/object.h"
 
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "vm/assembler.h"
 #include "vm/cpu.h"
@@ skipping 4116 matching lines @@
   }
   return LibraryPrefix::null();
 }
 
 
 // Canonicalizing the type arguments may have changed the index, may have
 // grown the table, or may even have canonicalized this type. Therefore
 // 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();
   if (canonical_types.IsNull()) {
     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()) {
       break;
     }
     ASSERT(type.IsFinalized());
     if (lookup_type.Equals(type)) {
       ASSERT(type.IsCanonical());
       return type.raw();
     }
     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();
 }
 
 
 const char* Class::ToCString() const {
   const Library& lib = Library::Handle(library());
   const char* library_name = lib.IsNull() ? "" : lib.ToCString();
   const char* patch_prefix = is_patch() ? "Patch " : "";
   const char* class_name = String::Handle(Name()).ToCString();
@@ skipping 11602 matching lines @@
       TypeArguments& type_args = TypeArguments::Handle(zone, arguments());
       type_args = type_args.Canonicalize(trail);
       if (IsCanonical()) {
         // Canonicalizing type_args canonicalized this type.
         ASSERT(IsRecursive());
         return this->raw();
       }
       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));
     ASSERT(type.IsCanonical());
     return type.raw();
   }
 
   Array& canonical_types = Array::Handle(zone);
   canonical_types ^= cls.canonical_types();
   if (canonical_types.IsNull()) {
@@ skipping 5850 matching lines @@
   return UserTag::null();
 }
 
 
 const char* UserTag::ToCString() const {
   const String& tag_label = String::Handle(label());
   return tag_label.ToCString();
 }
 
 }  // namespace dart