Merge r181060 - GC should compute stack bounds and dump registers at …

…the earliest opportunity. <https://webkit.org/b/142310> <rdar://problem/20045624> Reviewed by Geoffrey Garen. Make Heap::collect() a wrapper function around a collectImpl() where the work is actually done. The wrapper function that grabs a snapshot of the current stack boundaries and register values on entry, and sanitizes the stack on exit. This is a speculative fix for what appears to be overly conservative behavior in the garbage collector following r178364 which caused a measurable regression in memory usage on Membuster. The theory being that we were putting pointers to dead things on the stack before scanning it, and by doing that ended up marking things that we'd otherwise discover to be garbage. * heap/Heap.cpp: (JSC::Heap::markRoots): (JSC::Heap::gatherStackRoots): (JSC::Heap::collect): (JSC::Heap::collectImpl): * heap/Heap.h: * heap/MachineStackMarker.cpp: (JSC::MachineThreads::gatherFromCurrentThread): (JSC::MachineThreads::gatherConservativeRoots): * heap/MachineStackMarker.h:
WebKit · Mar 8, 2015 · 40d76e9 · 40d76e9
1 parent ee1127f
commit 40d76e9
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 22 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,31 @@
+2015-03-04  Andreas Kling  <akling@apple.com>
+
+        GC should compute stack bounds and dump registers at the earliest opportunity.
+        <https://webkit.org/b/142310>
+        <rdar://problem/20045624>
+
+        Reviewed by Geoffrey Garen.
+
+        Make Heap::collect() a wrapper function around a collectImpl() where the work is actually done.
+        The wrapper function that grabs a snapshot of the current stack boundaries and register values
+        on entry, and sanitizes the stack on exit.
+
+        This is a speculative fix for what appears to be overly conservative behavior in the garbage
+        collector following r178364 which caused a measurable regression in memory usage on Membuster.
+        The theory being that we were putting pointers to dead things on the stack before scanning it,
+        and by doing that ended up marking things that we'd otherwise discover to be garbage.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+        (JSC::Heap::gatherStackRoots):
+        (JSC::Heap::collect):
+        (JSC::Heap::collectImpl):
+        * heap/Heap.h:
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::gatherFromCurrentThread):
+        (JSC::MachineThreads::gatherConservativeRoots):
+        * heap/MachineStackMarker.h:
+
 2015-03-04  Debarshi Ray  <debarshir@gnome.org>
 
         Silence GCC's -Wstrict-prototypes

diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
@@ -498,7 +498,7 @@ void Heap::getConservativeRegisterRoots(HashSet<JSCell*>& roots)
     }
 }
 
-void Heap::markRoots(double gcStartTime)
+void Heap::markRoots(double gcStartTime, void* stackOrigin, void* stackTop, MachineThreads::RegisterState& calleeSavedRegisters)
 {
     SamplingRegion samplingRegion("Garbage Collection: Marking");
 
@@ -519,15 +519,11 @@ void Heap::markRoots(double gcStartTime)
 
     // We gather conservative roots before clearing mark bits because conservative
     // gathering uses the mark bits to determine whether a reference is valid.
-    void* dummy;
-    ALLOCATE_AND_GET_REGISTER_STATE(registers);
     ConservativeRoots conservativeRoots(&m_objectSpace.blocks(), &m_storageSpace);
-    gatherStackRoots(conservativeRoots, &dummy, registers);
+    gatherStackRoots(conservativeRoots, stackOrigin, stackTop, calleeSavedRegisters);
     gatherJSStackRoots(conservativeRoots);
     gatherScratchBufferRoots(conservativeRoots);
 
-    sanitizeStackForVM(m_vm);
-
     clearLivenessData();
 
     m_sharedData.didStartMarking();
@@ -583,11 +579,11 @@ void Heap::copyBackingStores()
         m_storageSpace.doneCopying();
 }
 
-void Heap::gatherStackRoots(ConservativeRoots& roots, void** dummy, MachineThreads::RegisterState& registers)
+void Heap::gatherStackRoots(ConservativeRoots& roots, void* stackOrigin, void* stackTop, MachineThreads::RegisterState& calleeSavedRegisters)
 {
     GCPHASE(GatherStackRoots);
     m_jitStubRoutines.clearMarks();
-    m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
+    m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, stackOrigin, stackTop, calleeSavedRegisters);
 }
 
 void Heap::gatherJSStackRoots(ConservativeRoots& roots)
@@ -988,7 +984,17 @@ void Heap::collectAllGarbage()
 
 static double minute = 60.0;
 
-void Heap::collect(HeapOperation collectionType)
+NEVER_INLINE void Heap::collect(HeapOperation collectionType)
+{
+    void* stackTop;
+    ALLOCATE_AND_GET_REGISTER_STATE(registers);
+
+    collectImpl(collectionType, wtfThreadData().stack().origin(), &stackTop, registers);
+
+    sanitizeStackForVM(m_vm);
+}
+
+NEVER_INLINE void Heap::collectImpl(HeapOperation collectionType, void* stackOrigin, void* stackTop, MachineThreads::RegisterState& calleeSavedRegisters)
 {
 #if ENABLE(ALLOCATION_LOGGING)
     dataLogF("JSC GC starting collection.\n");
@@ -1033,7 +1039,7 @@ void Heap::collect(HeapOperation collectionType)
     stopAllocation();
     flushWriteBarrierBuffer();
 
-    markRoots(gcStartTime);
+    markRoots(gcStartTime, stackOrigin, stackTop, calleeSavedRegisters);
 
     if (m_verifier) {
         m_verifier->gatherLiveObjects(HeapVerifier::Phase::AfterMarking);

diff --git a/Source/JavaScriptCore/heap/Heap.h b/Source/JavaScriptCore/heap/Heap.h
@@ -267,15 +267,17 @@ class Heap {
     JS_EXPORT_PRIVATE bool isValidAllocation(size_t);
     JS_EXPORT_PRIVATE void reportExtraMemoryCostSlowCase(size_t);
 
+    void collectImpl(HeapOperation, void* stackOrigin, void* stackTop, MachineThreads::RegisterState&);
+
     void suspendCompilerThreads();
     void willStartCollection(HeapOperation collectionType);
     void deleteOldCode(double gcStartTime);
     void flushOldStructureIDTables();
     void flushWriteBarrierBuffer();
     void stopAllocation();
 
-    void markRoots(double gcStartTime);
-    void gatherStackRoots(ConservativeRoots&, void** dummy, MachineThreads::RegisterState& registers);
+    void markRoots(double gcStartTime, void* stackOrigin, void* stackTop, MachineThreads::RegisterState&);
+    void gatherStackRoots(ConservativeRoots&, void* stackOrigin, void* stackTop, MachineThreads::RegisterState&);
     void gatherJSStackRoots(ConservativeRoots&);
     void gatherScratchBufferRoots(ConservativeRoots&);
     void clearLivenessData();

diff --git a/Source/JavaScriptCore/heap/MachineStackMarker.cpp b/Source/JavaScriptCore/heap/MachineStackMarker.cpp
@@ -276,15 +276,13 @@ void MachineThreads::removeThreadIfFound(PlatformThread platformThread)
     }
 }
 
-void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackCurrent, RegisterState& registers)
+void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackOrigin, void* stackTop, RegisterState& calleeSavedRegisters)
 {
-    void* registersBegin = &registers;
-    void* registersEnd = reinterpret_cast<void*>(roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(&registers + 1)));
+    void* registersBegin = &calleeSavedRegisters;
+    void* registersEnd = reinterpret_cast<void*>(roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(&calleeSavedRegisters + 1)));
     conservativeRoots.add(registersBegin, registersEnd, jitStubRoutines, codeBlocks);
 
-    void* stackBegin = stackCurrent;
-    void* stackEnd = wtfThreadData().stack().origin();
-    conservativeRoots.add(stackBegin, stackEnd, jitStubRoutines, codeBlocks);
+    conservativeRoots.add(stackTop, stackOrigin, jitStubRoutines, codeBlocks);
 }
 
 static inline bool suspendThread(const PlatformThread& platformThread)
@@ -614,9 +612,9 @@ static void growBuffer(size_t size, void** buffer, size_t* capacity)
     *buffer = fastMalloc(*capacity);
 }
 
-void MachineThreads::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackCurrent, RegisterState& currentThreadRegisters)
+void MachineThreads::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks, void* stackOrigin, void* stackTop, RegisterState& calleeSavedRegisters)
 {
-    gatherFromCurrentThread(conservativeRoots, jitStubRoutines, codeBlocks, stackCurrent, currentThreadRegisters);
+    gatherFromCurrentThread(conservativeRoots, jitStubRoutines, codeBlocks, stackOrigin, stackTop, calleeSavedRegisters);
 
     size_t size;
     size_t capacity = 0;

diff --git a/Source/JavaScriptCore/heap/MachineStackMarker.h b/Source/JavaScriptCore/heap/MachineStackMarker.h
@@ -42,14 +42,14 @@ namespace JSC {
         MachineThreads(Heap*);
         ~MachineThreads();
 
-        void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackCurrent, RegisterState& registers);
+        void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackOrigin, void* stackTop, RegisterState& calleeSavedRegisters);
 
         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);
+        void gatherFromCurrentThread(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackOrigin, void* stackTop, RegisterState& calleeSavedRegisters);
 
         void tryCopyOtherThreadStack(Thread*, void*, size_t capacity, size_t*);
         bool tryCopyOtherThreadStacks(MutexLocker&, void*, size_t capacity, size_t*);