TypeProfiler and running GC collection on another thread don't play nicely with each...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Nov 2016 22:02:20 +0000 (22:02 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Nov 2016 22:02:20 +0000 (22:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164441
<rdar://problem/29132174>

Reviewed by Geoffrey Garen.

JSTests:

* typeProfiler/type-profiler-gc.js: Added.
(bar):
(foo):

Source/JavaScriptCore:

This fix here is simple: we now treat the type profiler log as a GC root.
GC will make sure that we mark any values/structures that are in the log.
It's easy to reason about the correctness of this, and it also solves
the problem that we were clearing the log on the GC thread. Clearing the
log on the GC thread was a problem because when we clear the log, we may
allocate, which we're not allowed to do from the GC thread.

* heap/Heap.cpp:
(JSC::Heap::markRoots):
(JSC::Heap::visitTypeProfiler):
(JSC::Heap::collectInThread):
* heap/Heap.h:
* runtime/TypeProfilerLog.cpp:
(JSC::TypeProfilerLog::processLogEntries):
(JSC::TypeProfilerLog::visit):
* runtime/TypeProfilerLog.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208483 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/typeProfiler/type-profiler-gc.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/runtime/TypeProfilerLog.cpp
Source/JavaScriptCore/runtime/TypeProfilerLog.h

index d7a6312..6bb544d 100644 (file)
@@ -1,3 +1,15 @@
+2016-11-09  Saam Barati  <sbarati@apple.com>
+
+        TypeProfiler and running GC collection on another thread don't play nicely with each other
+        https://bugs.webkit.org/show_bug.cgi?id=164441
+        <rdar://problem/29132174>
+
+        Reviewed by Geoffrey Garen.
+
+        * typeProfiler/type-profiler-gc.js: Added.
+        (bar):
+        (foo):
+
 2016-11-04  Mark Lam  <mark.lam@apple.com>
 
         Error description code should be able to handle Symbol values.
diff --git a/JSTests/typeProfiler/type-profiler-gc.js b/JSTests/typeProfiler/type-profiler-gc.js
new file mode 100644 (file)
index 0000000..791e8bd
--- /dev/null
@@ -0,0 +1,25 @@
+load("./driver/driver.js");
+
+// The goal of this test is to just ensure that we don't crash the type profiler.
+
+function bar(o) {
+    o[Math.random() + "foo"] = new Array(100);
+    return o;
+}
+noInline(bar);
+
+function foo(o) {
+    let x = bar(o);
+    return x;
+}
+noInline(foo);
+
+let o = {};
+for (let i = 0; i < 2000; i++) {
+    if (i % 5 === 0)
+        o = {};
+    foo(o);
+    for (let i = 0; i < 20; i++) {
+        new Array(100);
+    }
+}
index 2e3cf95..b68d76e 100644 (file)
@@ -1,3 +1,28 @@
+2016-11-09  Saam Barati  <sbarati@apple.com>
+
+        TypeProfiler and running GC collection on another thread don't play nicely with each other
+        https://bugs.webkit.org/show_bug.cgi?id=164441
+        <rdar://problem/29132174>
+
+        Reviewed by Geoffrey Garen.
+
+        This fix here is simple: we now treat the type profiler log as a GC root.
+        GC will make sure that we mark any values/structures that are in the log.
+        It's easy to reason about the correctness of this, and it also solves
+        the problem that we were clearing the log on the GC thread. Clearing the
+        log on the GC thread was a problem because when we clear the log, we may
+        allocate, which we're not allowed to do from the GC thread.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+        (JSC::Heap::visitTypeProfiler):
+        (JSC::Heap::collectInThread):
+        * heap/Heap.h:
+        * runtime/TypeProfilerLog.cpp:
+        (JSC::TypeProfilerLog::processLogEntries):
+        (JSC::TypeProfilerLog::visit):
+        * runtime/TypeProfilerLog.h:
+
 2016-11-09  JF Bastien  <jfbastien@apple.com>
 
         WebAssembly: Silence noisy warning
index 9412f94..611d53d 100644 (file)
@@ -539,6 +539,7 @@ void Heap::markRoots(double gcStartTime)
         visitStrongHandles(heapRootVisitor);
         visitHandleStack(heapRootVisitor);
         visitSamplingProfiler();
+        visitTypeProfiler();
         visitShadowChicken();
         traceCodeBlocksAndJITStubRoutines();
         m_slotVisitor.drainFromShared(SlotVisitor::MasterDrain);
@@ -777,6 +778,16 @@ void Heap::visitSamplingProfiler()
 #endif // ENABLE(SAMPLING_PROFILER)
 }
 
+void Heap::visitTypeProfiler()
+{
+    if (vm()->typeProfiler()) {
+        vm()->typeProfilerLog()->visit(m_slotVisitor);
+        if (Options::logGC() == GCLogging::Verbose)
+            dataLog("Type Profiler visit data:\n", m_slotVisitor);
+        m_slotVisitor.donateAndDrain();
+    }
+}
+
 void Heap::visitShadowChicken()
 {
     m_vm->shadowChicken().visitChildren(m_slotVisitor);
@@ -1091,11 +1102,6 @@ void Heap::collectInThread()
     
     double gcStartTime;
     
-    if (vm()->typeProfiler()) {
-        DeferGCForAWhile awhile(*this);
-        vm()->typeProfilerLog()->processLogEntries(ASCIILiteral("GC"));
-    }
-    
 #if ENABLE(JIT)
     {
         DeferGCForAWhile awhile(*this);
index eb5a3e2..16e8cfb 100644 (file)
@@ -443,6 +443,7 @@ private:
     void visitStrongHandles(HeapRootVisitor&);
     void visitHandleStack(HeapRootVisitor&);
     void visitSamplingProfiler();
+    void visitTypeProfiler();
     void visitShadowChicken();
     void traceCodeBlocksAndJITStubRoutines();
     void visitWeakHandles(HeapRootVisitor&);
index 60d14e4..2f0fac3 100644 (file)
@@ -30,6 +30,7 @@
 #include "TypeProfilerLog.h"
 
 #include "JSCInlines.h"
+#include "SlotVisitor.h"
 #include "TypeLocation.h"
 #include <wtf/CurrentTime.h>
 
@@ -87,6 +88,11 @@ void TypeProfilerLog::processLogEntries(const String& reason)
         entry++;
     }
 
+    // Note that we don't update this cursor until we're done processing the log.
+    // This allows us to have a sane story in case we have to mark the log
+    // while processing through it. We won't be iterating over the log while
+    // marking it, but we may be in the middle of iterating over when the mutator
+    // pauses and causes the collector to mark the log.
     m_currentLogEntryPtr = m_logStartPtr;
 
     if (verbose) {
@@ -95,4 +101,15 @@ void TypeProfilerLog::processLogEntries(const String& reason)
     }
 }
 
+void TypeProfilerLog::visit(SlotVisitor& visitor)
+{
+    for (LogEntry* entry = m_logStartPtr; entry != m_currentLogEntryPtr; ++entry) {
+        visitor.appendUnbarrieredReadOnlyValue(entry->value);
+        if (StructureID id = entry->structureID) {
+            Structure* structure = visitor.heap()->structureIDTable().get(id); 
+            visitor.appendUnbarrieredReadOnlyPointer(structure);
+        }
+    }
+}
+
 } // namespace JSC
index 0bb26d3..01f6ae6 100644 (file)
@@ -64,6 +64,8 @@ public:
     JS_EXPORT_PRIVATE void processLogEntries(const String&);
     LogEntry* logEndPtr() const { return m_logEndPtr; }
 
+    void visit(SlotVisitor&);
+
     static ptrdiff_t logStartOffset() { return OBJECT_OFFSETOF(TypeProfilerLog, m_logStartPtr); }
     static ptrdiff_t currentLogEntryOffset() { return OBJECT_OFFSETOF(TypeProfilerLog, m_currentLogEntryPtr); }