Extend locking of GCInfo table to include slot allocation.

Source/platform/heap/Visitor.cpp

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 25 matching lines @@
 
 namespace blink {
 
 // GCInfo indices start from 1 for heap objects, with 0 being treated
 // specially as the index for freelist entries and large heap objects.
 int GCInfoTable::s_gcInfoIndex = 0;
 
 size_t GCInfoTable::s_gcInfoTableSize = 0;
 GCInfo const** s_gcInfoTable = nullptr;
 
-size_t GCInfoTable::allocateGCInfoSlot()
+bool GCInfoTable::allocateGCInfoSlot(size_t& gcInfoIndexSlot)

[haraken, 2015/01/10 08:37:24]

Nit: I'd slightly prefer a raw pointer for an output parameter. (That's a
convention in the chromium side, and no convention in the Blink side.)

[sof, 2015/01/10 08:54:23]

Done.

 {
-    // FIXME: if multiple threads attempt to allocate the initial object
-    // for a given type, there'll be a write race on updating the 'static'
-    // holding its index. Duplicate GCInfo slots might be written for the
-    // object, which is benign, but verify that the index update race is
-    // acceptable.
-    int index = atomicIncrement(&s_gcInfoIndex);
+    // Keep a global GCInfoTable lock while allocating a new slot.
+    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
+    MutexLocker locker(mutex);
+
+    // If more than one thread ends up allocating a slot for
+    // the same GCInfo, have later threads reuse the slot
+    // allocated by the first.
+    if (gcInfoIndexSlot)
+        return false;
+
+    int index = ++s_gcInfoIndex;
     size_t gcInfoIndex = static_cast<size_t>(index);
     ASSERT(gcInfoIndex < GCInfoTable::maxIndex);
     if (gcInfoIndex >= s_gcInfoTableSize)
-        resize(gcInfoIndex);
+        resize();
 
-    return gcInfoIndex;
+    gcInfoIndexSlot = gcInfoIndex;
+    return true;
 }
 
-void GCInfoTable::resize(size_t index)
+void GCInfoTable::resize()
 {
-    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
-    MutexLocker locker(mutex);
-
-    // Keep a lock while expanding the shared table; check
-    // if another thread have already resized.
-    if (index < s_gcInfoTableSize)
-        return;
-
     // (Light) experimentation suggests that Blink doesn't need
     // more than this while handling content on popular web properties.
     const size_t initialSize = 512;
 
     size_t newSize = s_gcInfoTableSize ? 2 * s_gcInfoTableSize : initialSize;
     ASSERT(newSize < GCInfoTable::maxIndex);
     s_gcInfoTable = reinterpret_cast<GCInfo const**>(realloc(s_gcInfoTable, newSize * sizeof(GCInfo)));
     ASSERT(s_gcInfoTable);
     memset(reinterpret_cast<uint8_t*>(s_gcInfoTable) + s_gcInfoTableSize * sizeof(GCInfo), 0, (newSize - s_gcInfoTableSize) * sizeof(GCInfo));
     s_gcInfoTableSize = newSize;
 }
 
 void GCInfoTable::init()
 {
     RELEASE_ASSERT(!s_gcInfoTable);
-    resize(0);
+    resize();
 }
 
 void GCInfoTable::shutdown()
 {
     free(s_gcInfoTable);
     s_gcInfoTable = nullptr;
 }
 
 StackFrameDepth* Visitor::m_stackFrameDepth = nullptr;
 
 #if ENABLE(ASSERT)
 void assertObjectHasGCInfo(const void* payload, size_t gcInfoIndex)
 {
     HeapObjectHeader::fromPayload(payload)->checkHeader();
 #if !defined(COMPONENT_BUILD)
     // On component builds we cannot compare the gcInfos as they are statically
     // defined in each of the components and hence will not match.
     BaseHeapPage* page = pageFromObject(payload);
     ASSERT(page->orphaned() || HeapObjectHeader::fromPayload(payload)->gcInfoIndex() == gcInfoIndex);
 #endif
 }
 #endif
 
 }