Changeset 19209 in webkit for trunk/JavaScriptCore

Timestamp:

Jan 28, 2007, 10:17:38 PM (18 years ago)

Author:

ggaren

Message:

Reviewed by Maciej Stachowiak.

First step in fixing <rdar://problem/4485644> REGRESSION: JavaScriptCore
has init routines

Don't rely on a static initializer to store the main thread's ID (which
we would use to detect allocations on secondary threads). Instead, require
the caller to notify fastMalloc if it might allocate on a secondary thread.

Also fixed what seemed like a race condition in do_malloc.

tcmalloc_unittest and my custom versions of JS iBench and PLT show no
regressions.

• wtf/FastMalloc.cpp: (WTF::fastMallocSetIsMultiThreaded): (1) Renamed from "fastMallocRegisterThread", which was a misleading name because not all threads need to register with fastMalloc -- only secondary threads need to, and only for the purpose of disabling its single-threaded optimization.

(2) Use the pageheap_lock instead of a custom one, since we need to synchronize
with the read of isMultiThreaded inside CreateCacheIfNecessary. This is a new
requirement, now that we can't guarantee that the first call to CreateCacheIfNecessary
will occur on the main thread at init time, before any other threads have been created.

(WTF::TCMalloc_ThreadCache::CreateCacheIfNecessary):
(WTF::do_malloc): Reverted WTF change only to call GetCache() if size <= kMaxSize.
The WTF code would read phinited without holding the pageheap_lock, which
seemed like a race condition. Regardless, calling GetCache reduces the number
of code paths to module initialization, which will help in writing the
final fix for this bug.

Location:

trunk/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• kjs/collector.cpp (modified) (1 diff)
• wtf/FastMalloc.cpp (modified) (6 diffs)
• wtf/FastMallocInternal.h (modified) (1 diff)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2007-01-28  Geoffrey Garen  <[email protected]>
+
+        Reviewed by Maciej Stachowiak.
+        
+        First step in fixing <rdar://problem/4485644> REGRESSION: JavaScriptCore 
+        has init routines
+        
+        Don't rely on a static initializer to store the main thread's ID (which
+        we would use to detect allocations on secondary threads). Instead, require 
+        the caller to notify fastMalloc if it might allocate on a secondary thread.
+        
+        Also fixed what seemed like a race condition in do_malloc.
+        
+        tcmalloc_unittest and my custom versions of JS iBench and PLT show no
+        regressions.
+
+        * wtf/FastMalloc.cpp:
+        (WTF::fastMallocSetIsMultiThreaded): 
+        (1) Renamed from "fastMallocRegisterThread", which was a misleading name because 
+        not all threads need to register with fastMalloc -- only secondary threads 
+        need to, and only for the purpose of disabling its single-threaded optimization. 
+
+        (2) Use the pageheap_lock instead of a custom one, since we need to synchronize
+        with the read of isMultiThreaded inside CreateCacheIfNecessary. This is a new
+        requirement, now that we can't guarantee that the first call to CreateCacheIfNecessary
+        will occur on the main thread at init time, before any other threads have been created.
+
+        (WTF::TCMalloc_ThreadCache::CreateCacheIfNecessary):
+        (WTF::do_malloc): Reverted WTF change only to call GetCache() if size <= kMaxSize.
+        The WTF code would read phinited without holding the pageheap_lock, which
+        seemed like a race condition. Regardless, calling GetCache reduces the number 
+        of code paths to module initialization, which will help in writing the 
+        final fix for this bug.
+
 2007-01-28  David Kilzer  <[email protected]>
 

trunk/JavaScriptCore/kjs/collector.cpp

@@ -262 +262 @@
 
   if (!pthread_getspecific(registeredThreadKey)) {
+    if (!pthread_main_np())
+        WTF::fastMallocSetIsMultiThreaded();
     pthread_t pthread = pthread_self();
-    WTF::fastMallocRegisterThread(pthread);
     Collector::Thread *thread = new Collector::Thread(pthread, pthread_mach_thread_np(pthread));
     thread->next = registeredThreads;

trunk/JavaScriptCore/wtf/FastMalloc.cpp

@@ -104 +104 @@
 
 #if !PLATFORM(WIN_OS)
-void fastMallocRegisterThread(pthread_t) 
+void fastMallocSetIsMultiThreaded() 
 {
 }
@@ -382 +382 @@
  private:
   // How much to allocate from system at a time
-  static const int kAllocIncrement = 32 << 10;
+  static const size_t kAllocIncrement = 32 << 10;
 
   // Aligned size of T
@@ -1394 +1394 @@
 bool isMultiThreaded;
 TCMalloc_ThreadCache *mainThreadCache;
-pthread_t mainThreadID;
-static SpinLock multiThreadedLock = SPINLOCK_INITIALIZER;
-
-void fastMallocRegisterThread(pthread_t thread) 
+
+void fastMallocSetIsMultiThreaded() 
 {
-    if (thread != mainThreadID) {
-        // We lock when writing isMultiThreaded but not when reading it.
-        // It's ok if the main thread gets it wrong - for the main thread, the
-        // global variable cache is the same as the thread-specific cache.
-        // And other threads can't get it wrong because they must have gone through 
-        // this function before allocating so they've synchronized.
-        // Also, mainThreadCache is only set when isMultiThreaded is false, 
-        // to save a branch in some cases.
-        SpinLockHolder lock(&multiThreadedLock);
-        isMultiThreaded = true;
-        mainThreadCache = 0;
-    }
+    // We lock when writing mainThreadCache but not when reading it. It's OK if
+    // the main thread reads a stale, non-NULL value for mainThreadCache because
+    // mainThreadCache is the same as the main thread's thread-specific cache.
+    // Other threads can't read a stale, non-NULL value for mainThreadCache because
+    // clients must call this function before allocating on other threads, so they'll 
+    // have synchronized before reading mainThreadCache.
+
+    // mainThreadCache is only set when isMultiThreaded is false, to save a 
+    // branch in some cases.
+
+    SpinLockHolder lock(&pageheap_lock);
+    isMultiThreaded = true;
+    mainThreadCache = 0;
 }
 
@@ -1524 +1523 @@
       thread_heap_count++;
       RecomputeThreadCacheSize(); 
-      if (!isMultiThreaded) {
-          mainThreadCache = heap;
-          mainThreadID = pthread_self();
-      }
+      if (!isMultiThreaded)
+        mainThreadCache = heap;
     }
   }
@@ -1857 +1854 @@
 static ALWAYS_INLINE void* do_malloc(size_t size) {
 
+#ifdef WTF_CHANGES
+    ASSERT(isMultiThreaded || pthread_main_np());
+#endif
+
 #ifndef WTF_CHANGES
   if (TCMallocDebug::level >= TCMallocDebug::kVerbose) 
     MESSAGE("In tcmalloc do_malloc(%" PRIuS")\n", size);
 #endif
-
-#ifndef WTF_CHANGES
   // The following call forces module initialization
   TCMalloc_ThreadCache* heap = TCMalloc_ThreadCache::GetCache();
+#ifndef WTF_CHANGES
   if (heap->SampleAllocation(size)) {
     Span* span = DoSampledAllocation(size);
@@ -1873 +1873 @@
   if (size > kMaxSize) {
     // Use page-level allocator
-    if (!tsd_inited && !phinited)
-      TCMalloc_ThreadCache::InitModule();
-
     SpinLockHolder h(&pageheap_lock);
-
-
     Span* span = pageheap->New(pages(size));
     if (span == NULL) return NULL;
     return reinterpret_cast<void*>(span->start << kPageShift);
   } else {
-#ifdef WTF_CHANGES
-      TCMalloc_ThreadCache* heap = TCMalloc_ThreadCache::GetCache();
-#endif
-      return heap->Allocate(size);
+    return heap->Allocate(size);
   }
 }

trunk/JavaScriptCore/wtf/FastMallocInternal.h

@@ -29 +29 @@
 
 namespace WTF {
-    void fastMallocRegisterThread(pthread_t thread);
+    // Clients must call this function before allocating memory on a secondary thread.
+    void fastMallocSetIsMultiThreaded();
 }