- Revision
- 179319
- Author
- [email protected]
- Date
- 2015-01-28 16:58:13 -0800 (Wed, 28 Jan 2015)
Log Message
Removed fastMallocForbid / fastMallocAllow
https://bugs.webkit.org/show_bug.cgi?id=141012
Reviewed by Mark Hahnenberg.
Source/_javascript_Core:
Copy non-current thread stacks before scanning them instead of scanning
them in-place.
This operation is uncommon (i.e., never in the web content process),
and even in a stress test with 4 threads it only copies about 27kB,
so I think the performance cost is OK.
Scanning in-place requires a complex dance where we constrain our GC
data structures not to use malloc, free, or any other interesting functions
that might acquire locks. We've gotten this wrong many times in the past,
and I just got it wrong again yesterday. Since this code path is rarely
tested, I want it to just make sense, and not depend on or constrain the
details of the rest of the GC heap's design.
* heap/MachineStackMarker.cpp:
(JSC::otherThreadStack): Factored out a helper function for dealing with
unaligned and/or backwards pointers.
(JSC::MachineThreads::tryCopyOtherThreadStack): This is now the only
constrained function, and it only calls memcpy and low-level thread APIs.
(JSC::MachineThreads::tryCopyOtherThreadStacks): The design here is that
you do one pass over all the threads to compute their combined size,
and then a second pass to do all the copying. In theory, the threads may
grow in between passes, in which case you'll continue until the threads
stop growing. In practice, you never continue.
(JSC::growBuffer): Helper function for growing.
(JSC::MachineThreads::gatherConservativeRoots):
(JSC::MachineThreads::gatherFromOtherThread): Deleted.
* heap/MachineStackMarker.h: Updated for interface changes.
Source/WTF:
Removed the forbid / allow API because it is no longer used.
* wtf/FastMalloc.cpp:
(WTF::tryFastMalloc):
(WTF::fastMalloc):
(WTF::tryFastCalloc):
(WTF::fastCalloc):
(WTF::fastFree):
(WTF::tryFastRealloc):
(WTF::fastRealloc):
(WTF::TCMalloc_ThreadCache::CreateCacheIfNecessary):
(WTF::isForbidden): Deleted.
(WTF::fastMallocForbid): Deleted.
(WTF::fastMallocAllow): Deleted.
(WTF::initializeIsForbiddenKey): Deleted.
* wtf/FastMalloc.h:
Tools:
Fixed a test bug I noticed while testing.
* DumpRenderTree/_javascript_Threading.cpp:
(stopJavaScriptThreads): Lock the _javascript_Threads lock before
accessing _javascript_Threads -- otherwise, you'll ASSERT.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (179318 => 179319)
--- trunk/Source/_javascript_Core/ChangeLog 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-01-29 00:58:13 UTC (rev 179319)
@@ -1,3 +1,43 @@
+2015-01-28 Geoffrey Garen <[email protected]>
+
+ Removed fastMallocForbid / fastMallocAllow
+ https://bugs.webkit.org/show_bug.cgi?id=141012
+
+ Reviewed by Mark Hahnenberg.
+
+ Copy non-current thread stacks before scanning them instead of scanning
+ them in-place.
+
+ This operation is uncommon (i.e., never in the web content process),
+ and even in a stress test with 4 threads it only copies about 27kB,
+ so I think the performance cost is OK.
+
+ Scanning in-place requires a complex dance where we constrain our GC
+ data structures not to use malloc, free, or any other interesting functions
+ that might acquire locks. We've gotten this wrong many times in the past,
+ and I just got it wrong again yesterday. Since this code path is rarely
+ tested, I want it to just make sense, and not depend on or constrain the
+ details of the rest of the GC heap's design.
+
+ * heap/MachineStackMarker.cpp:
+ (JSC::otherThreadStack): Factored out a helper function for dealing with
+ unaligned and/or backwards pointers.
+
+ (JSC::MachineThreads::tryCopyOtherThreadStack): This is now the only
+ constrained function, and it only calls memcpy and low-level thread APIs.
+
+ (JSC::MachineThreads::tryCopyOtherThreadStacks): The design here is that
+ you do one pass over all the threads to compute their combined size,
+ and then a second pass to do all the copying. In theory, the threads may
+ grow in between passes, in which case you'll continue until the threads
+ stop growing. In practice, you never continue.
+
+ (JSC::growBuffer): Helper function for growing.
+
+ (JSC::MachineThreads::gatherConservativeRoots):
+ (JSC::MachineThreads::gatherFromOtherThread): Deleted.
+ * heap/MachineStackMarker.h: Updated for interface changes.
+
2015-01-28 Brian J. Burg <[email protected]>
Web Inspector: remove CSS.setPropertyText, CSS.toggleProperty and related dead code
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp (179318 => 179319)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-01-29 00:58:13 UTC (rev 179319)
@@ -413,57 +413,91 @@
#endif
}
-void MachineThreads::gatherFromOtherThread(ConservativeRoots& conservativeRoots, Thread* thread, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks)
+static std::pair<void*, size_t> otherThreadStack(void* stackBase, const PlatformThreadRegisters& registers)
{
- PlatformThreadRegisters regs;
- size_t regSize = getPlatformThreadRegisters(thread->platformThread, regs);
+ void* begin = stackBase;
+ void* end = reinterpret_cast<void*>(
+ WTF::roundUpToMultipleOf<sizeof(void*)>(
+ reinterpret_cast<uintptr_t>(
+ otherThreadStackPointer(registers))));
+ if (begin > end)
+ std::swap(begin, end);
+ return std::make_pair(begin, static_cast<char*>(end) - static_cast<char*>(begin));
+}
- conservativeRoots.add(static_cast<void*>(®s), static_cast<void*>(reinterpret_cast<char*>(®s) + regSize), jitStubRoutines, codeBlocks);
+// This function must not call malloc(), free(), or any other function that might
+// acquire a lock. Since 'thread' is suspended, trying to acquire a lock
+// will deadlock if 'thread' holds that lock.
+void MachineThreads::tryCopyOtherThreadStack(Thread* thread, void* buffer, size_t capacity, size_t* size)
+{
+ PlatformThreadRegisters registers;
+ size_t registersSize = getPlatformThreadRegisters(thread->platformThread, registers);
+ std::pair<void*, size_t> stack = otherThreadStack(thread->stackBase, registers);
- void* stackPointer = otherThreadStackPointer(regs);
- void* stackBase = thread->stackBase;
- stackPointer = reinterpret_cast<void*>(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackPointer)));
- conservativeRoots.add(stackPointer, stackBase, jitStubRoutines, codeBlocks);
+ bool canCopy = *size + registersSize + stack.second <= capacity;
- freePlatformThreadRegisters(regs);
+ if (canCopy)
+ memcpy(static_cast<char*>(buffer) + *size, ®isters, registersSize);
+ *size += registersSize;
+
+ if (canCopy)
+ memcpy(static_cast<char*>(buffer) + *size, stack.first, stack.second);
+ *size += stack.second;
+
+ freePlatformThreadRegisters(registers);
}
-void MachineThreads::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackCurrent, RegisterState& registers)
+bool MachineThreads::tryCopyOtherThreadStacks(MutexLocker&, void* buffer, size_t capacity, size_t* size)
{
- gatherFromCurrentThread(conservativeRoots, jitStubRoutines, codeBlocks, stackCurrent, registers);
+ *size = 0;
- if (m_threadSpecific) {
- PlatformThread currentPlatformThread = getCurrentPlatformThread();
+ PlatformThread currentPlatformThread = getCurrentPlatformThread();
+ for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+ if (!equalThread(thread->platformThread, currentPlatformThread))
+ suspendThread(thread->platformThread);
+ }
- MutexLocker lock(m_registeredThreadsMutex);
+ for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+ if (!equalThread(thread->platformThread, currentPlatformThread))
+ tryCopyOtherThreadStack(thread, buffer, capacity, size);
+ }
-#ifndef NDEBUG
- // Forbid malloc during the gather phase. The gather phase suspends
- // threads, so a malloc during gather would risk a deadlock with a
- // thread that had been suspended while holding the malloc lock.
- fastMallocForbid();
-#endif
- for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
- if (!equalThread(thread->platformThread, currentPlatformThread))
- suspendThread(thread->platformThread);
- }
+ for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+ if (!equalThread(thread->platformThread, currentPlatformThread))
+ resumeThread(thread->platformThread);
+ }
- // It is safe to access the registeredThreads list, because we earlier asserted that locks are being held,
- // and since this is a shared heap, they are real locks.
- for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
- if (!equalThread(thread->platformThread, currentPlatformThread))
- gatherFromOtherThread(conservativeRoots, thread, jitStubRoutines, codeBlocks);
- }
+ return *size <= capacity;
+}
- for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
- if (!equalThread(thread->platformThread, currentPlatformThread))
- resumeThread(thread->platformThread);
- }
+static void growBuffer(size_t size, void** buffer, size_t* capacity)
+{
+ if (*buffer)
+ fastFree(*buffer);
-#ifndef NDEBUG
- fastMallocAllow();
-#endif
- }
+ *capacity = WTF::roundUpToMultipleOf(WTF::pageSize(), size * 2);
+ *buffer = fastMalloc(*capacity);
}
+void MachineThreads::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackCurrent, RegisterState& currentThreadRegisters)
+{
+ gatherFromCurrentThread(conservativeRoots, jitStubRoutines, codeBlocks, stackCurrent, currentThreadRegisters);
+
+ if (!m_threadSpecific)
+ return;
+
+ size_t size;
+ size_t capacity = 0;
+ void* buffer = nullptr;
+ MutexLocker lock(m_registeredThreadsMutex);
+ while (!tryCopyOtherThreadStacks(lock, buffer, capacity, &size))
+ growBuffer(size, &buffer, &capacity);
+
+ if (!buffer)
+ return;
+
+ conservativeRoots.add(buffer, static_cast<char*>(buffer) + size, jitStubRoutines, codeBlocks);
+ fastFree(buffer);
+}
+
} // namespace JSC
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.h (179318 => 179319)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-01-29 00:58:13 UTC (rev 179319)
@@ -48,15 +48,16 @@
JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
private:
+ class Thread;
+
void gatherFromCurrentThread(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackCurrent, RegisterState& registers);
- class Thread;
+ void tryCopyOtherThreadStack(Thread*, void*, size_t capacity, size_t*);
+ bool tryCopyOtherThreadStacks(MutexLocker&, void*, size_t capacity, size_t*);
static void removeThread(void*);
void removeCurrentThread();
- void gatherFromOtherThread(ConservativeRoots&, Thread*, JITStubRoutineSet&, CodeBlockSet&);
-
Mutex m_registeredThreadsMutex;
Thread* m_registeredThreads;
WTF::ThreadSpecificKey m_threadSpecific;
Modified: trunk/Source/WTF/ChangeLog (179318 => 179319)
--- trunk/Source/WTF/ChangeLog 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/WTF/ChangeLog 2015-01-29 00:58:13 UTC (rev 179319)
@@ -1,3 +1,27 @@
+2015-01-28 Geoffrey Garen <[email protected]>
+
+ Removed fastMallocForbid / fastMallocAllow
+ https://bugs.webkit.org/show_bug.cgi?id=141012
+
+ Reviewed by Mark Hahnenberg.
+
+ Removed the forbid / allow API because it is no longer used.
+
+ * wtf/FastMalloc.cpp:
+ (WTF::tryFastMalloc):
+ (WTF::fastMalloc):
+ (WTF::tryFastCalloc):
+ (WTF::fastCalloc):
+ (WTF::fastFree):
+ (WTF::tryFastRealloc):
+ (WTF::fastRealloc):
+ (WTF::TCMalloc_ThreadCache::CreateCacheIfNecessary):
+ (WTF::isForbidden): Deleted.
+ (WTF::fastMallocForbid): Deleted.
+ (WTF::fastMallocAllow): Deleted.
+ (WTF::initializeIsForbiddenKey): Deleted.
+ * wtf/FastMalloc.h:
+
2015-01-28 Dana Burkart <[email protected]>
Move ASan flag settings from DebugRelease.xcconfig to Base.xcconfig
Modified: trunk/Source/WTF/wtf/FastMalloc.cpp (179318 => 179319)
--- trunk/Source/WTF/wtf/FastMalloc.cpp 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/WTF/wtf/FastMalloc.cpp 2015-01-29 00:58:13 UTC (rev 179319)
@@ -117,74 +117,8 @@
// Use a background thread to periodically scavenge memory to release back to the system
#define USE_BACKGROUND_THREAD_TO_SCAVENGE_MEMORY 1
-#ifndef NDEBUG
namespace WTF {
-#if OS(WINDOWS)
-
-static DWORD isForibiddenTlsIndex = TLS_OUT_OF_INDEXES;
-static const LPVOID kTlsAllowValue = reinterpret_cast<LPVOID>(0); // Must be zero.
-static const LPVOID kTlsForbiddenValue = reinterpret_cast<LPVOID>(1);
-
-#if !ASSERT_DISABLED
-static bool isForbidden()
-{
- // By default, fastMalloc is allowed so we don't allocate the
- // tls index unless we're asked to make it forbidden. If TlsSetValue
- // has not been called on a thread, the value returned by TlsGetValue is 0.
- return (isForibiddenTlsIndex != TLS_OUT_OF_INDEXES) && (TlsGetValue(isForibiddenTlsIndex) == kTlsForbiddenValue);
-}
-#endif
-
-void fastMallocForbid()
-{
- if (isForibiddenTlsIndex == TLS_OUT_OF_INDEXES)
- isForibiddenTlsIndex = TlsAlloc(); // a little racey, but close enough for debug only
- TlsSetValue(isForibiddenTlsIndex, kTlsForbiddenValue);
-}
-
-void fastMallocAllow()
-{
- if (isForibiddenTlsIndex == TLS_OUT_OF_INDEXES)
- return;
- TlsSetValue(isForibiddenTlsIndex, kTlsAllowValue);
-}
-
-#else // !OS(WINDOWS)
-
-static pthread_key_t isForbiddenKey;
-static pthread_once_t isForbiddenKeyOnce = PTHREAD_ONCE_INIT;
-static void initializeIsForbiddenKey()
-{
- pthread_key_create(&isForbiddenKey, 0);
-}
-
-#if !ASSERT_DISABLED
-static bool isForbidden()
-{
- pthread_once(&isForbiddenKeyOnce, initializeIsForbiddenKey);
- return !!pthread_getspecific(isForbiddenKey);
-}
-#endif
-
-void fastMallocForbid()
-{
- pthread_once(&isForbiddenKeyOnce, initializeIsForbiddenKey);
- pthread_setspecific(isForbiddenKey, &isForbiddenKey);
-}
-
-void fastMallocAllow()
-{
- pthread_once(&isForbiddenKeyOnce, initializeIsForbiddenKey);
- pthread_setspecific(isForbiddenKey, 0);
-}
-#endif // OS(WINDOWS)
-
-} // namespace WTF
-#endif // NDEBUG
-
-namespace WTF {
-
void* fastZeroedMalloc(size_t n)
{
void* result = fastMalloc(n);
@@ -230,15 +164,11 @@
TryMallocReturnValue tryFastMalloc(size_t n)
{
- ASSERT(!isForbidden());
-
return malloc(n);
}
void* fastMalloc(size_t n)
{
- ASSERT(!isForbidden());
-
void* result = malloc(n);
if (!result)
CRASH();
@@ -248,14 +178,11 @@
TryMallocReturnValue tryFastCalloc(size_t n_elements, size_t element_size)
{
- ASSERT(!isForbidden());
return calloc(n_elements, element_size);
}
void* fastCalloc(size_t n_elements, size_t element_size)
{
- ASSERT(!isForbidden());
-
void* result = calloc(n_elements, element_size);
if (!result)
CRASH();
@@ -265,19 +192,16 @@
void fastFree(void* p)
{
- ASSERT(!isForbidden());
free(p);
}
TryMallocReturnValue tryFastRealloc(void* p, size_t n)
{
- ASSERT(!isForbidden());
return realloc(p, n);
}
void* fastRealloc(void* p, size_t n)
{
- ASSERT(!isForbidden());
void* result = realloc(p, n);
if (!result)
CRASH();
@@ -320,7 +244,6 @@
void* fastMalloc(size_t size)
{
- ASSERT(!isForbidden());
return bmalloc::api::malloc(size);
}
@@ -4017,10 +3940,6 @@
static ALWAYS_INLINE void* do_malloc(size_t size) {
void* ret = NULL;
-#ifdef WTF_CHANGES
- ASSERT(!isForbidden());
-#endif
-
// The following call forces module initialization
TCMalloc_ThreadCache* heap = TCMalloc_ThreadCache::GetCache();
#ifndef WTF_CHANGES
Modified: trunk/Source/WTF/wtf/FastMalloc.h (179318 => 179319)
--- trunk/Source/WTF/wtf/FastMalloc.h 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Source/WTF/wtf/FastMalloc.h 2015-01-29 00:58:13 UTC (rev 179319)
@@ -76,11 +76,6 @@
WTF_EXPORT_PRIVATE void fastFree(void*);
-#ifndef NDEBUG
- WTF_EXPORT_PRIVATE void fastMallocForbid();
- WTF_EXPORT_PRIVATE void fastMallocAllow();
-#endif
-
WTF_EXPORT_PRIVATE void releaseFastMallocFreeMemory();
struct FastMallocStatistics {
@@ -109,11 +104,6 @@
using WTF::tryFastRealloc;
using WTF::tryFastZeroedMalloc;
-#ifndef NDEBUG
-using WTF::fastMallocForbid;
-using WTF::fastMallocAllow;
-#endif
-
#if COMPILER(GCC) && OS(DARWIN)
#define WTF_PRIVATE_INLINE __private_extern__ inline __attribute__((always_inline))
#elif COMPILER(GCC)
Modified: trunk/Tools/ChangeLog (179318 => 179319)
--- trunk/Tools/ChangeLog 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Tools/ChangeLog 2015-01-29 00:58:13 UTC (rev 179319)
@@ -1,3 +1,16 @@
+2015-01-28 Geoffrey Garen <[email protected]>
+
+ Removed fastMallocForbid / fastMallocAllow
+ https://bugs.webkit.org/show_bug.cgi?id=141012
+
+ Reviewed by Mark Hahnenberg.
+
+ Fixed a test bug I noticed while testing.
+
+ * DumpRenderTree/_javascript_Threading.cpp:
+ (stopJavaScriptThreads): Lock the _javascript_Threads lock before
+ accessing _javascript_Threads -- otherwise, you'll ASSERT.
+
2015-01-28 Dana Burkart <[email protected]>
asan.xcconfig should use CLANG_ADDRESS_SANITIZER=YES instead of -fsanitize=address
Modified: trunk/Tools/DumpRenderTree/_javascript_Threading.cpp (179318 => 179319)
--- trunk/Tools/DumpRenderTree/_javascript_Threading.cpp 2015-01-29 00:50:53 UTC (rev 179318)
+++ trunk/Tools/DumpRenderTree/_javascript_Threading.cpp 2015-01-29 00:58:13 UTC (rev 179319)
@@ -147,7 +147,10 @@
for (size_t i = 0; i < _javascript_ThreadsCount; ++i)
waitForThreadCompletion(threads[i]);
- _javascript_Threads().clear();
+ {
+ MutexLocker locker(_javascript_ThreadsMutex());
+ _javascript_Threads().clear();
+ }
JSContextGroupRelease(_javascript_ThreadsGroup);
}
