Title: [208483] trunk
- Revision
- 208483
- Author
- sbar...@apple.com
- Date
- 2016-11-09 14:02:20 -0800 (Wed, 09 Nov 2016)
Log Message
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.
JSTests:
* typeProfiler/type-profiler-gc.js: Added.
(bar):
(foo):
Source/_javascript_Core:
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:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (208482 => 208483)
--- trunk/JSTests/ChangeLog 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/JSTests/ChangeLog 2016-11-09 22:02:20 UTC (rev 208483)
@@ -1,3 +1,15 @@
+2016-11-09 Saam Barati <sbar...@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....@apple.com>
Error description code should be able to handle Symbol values.
Added: trunk/JSTests/typeProfiler/type-profiler-gc.js (0 => 208483)
--- trunk/JSTests/typeProfiler/type-profiler-gc.js (rev 0)
+++ trunk/JSTests/typeProfiler/type-profiler-gc.js 2016-11-09 22:02:20 UTC (rev 208483)
@@ -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);
+ }
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (208482 => 208483)
--- trunk/Source/_javascript_Core/ChangeLog 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-11-09 22:02:20 UTC (rev 208483)
@@ -1,3 +1,28 @@
+2016-11-09 Saam Barati <sbar...@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 <jfbast...@apple.com>
WebAssembly: Silence noisy warning
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (208482 => 208483)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2016-11-09 22:02:20 UTC (rev 208483)
@@ -539,6 +539,7 @@
visitStrongHandles(heapRootVisitor);
visitHandleStack(heapRootVisitor);
visitSamplingProfiler();
+ visitTypeProfiler();
visitShadowChicken();
traceCodeBlocksAndJITStubRoutines();
m_slotVisitor.drainFromShared(SlotVisitor::MasterDrain);
@@ -777,6 +778,16 @@
#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 @@
double gcStartTime;
- if (vm()->typeProfiler()) {
- DeferGCForAWhile awhile(*this);
- vm()->typeProfilerLog()->processLogEntries(ASCIILiteral("GC"));
- }
-
#if ENABLE(JIT)
{
DeferGCForAWhile awhile(*this);
Modified: trunk/Source/_javascript_Core/heap/Heap.h (208482 => 208483)
--- trunk/Source/_javascript_Core/heap/Heap.h 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2016-11-09 22:02:20 UTC (rev 208483)
@@ -443,6 +443,7 @@
void visitStrongHandles(HeapRootVisitor&);
void visitHandleStack(HeapRootVisitor&);
void visitSamplingProfiler();
+ void visitTypeProfiler();
void visitShadowChicken();
void traceCodeBlocksAndJITStubRoutines();
void visitWeakHandles(HeapRootVisitor&);
Modified: trunk/Source/_javascript_Core/runtime/TypeProfilerLog.cpp (208482 => 208483)
--- trunk/Source/_javascript_Core/runtime/TypeProfilerLog.cpp 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/Source/_javascript_Core/runtime/TypeProfilerLog.cpp 2016-11-09 22:02:20 UTC (rev 208483)
@@ -30,6 +30,7 @@
#include "TypeProfilerLog.h"
#include "JSCInlines.h"
+#include "SlotVisitor.h"
#include "TypeLocation.h"
#include <wtf/CurrentTime.h>
@@ -87,6 +88,11 @@
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::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
Modified: trunk/Source/_javascript_Core/runtime/TypeProfilerLog.h (208482 => 208483)
--- trunk/Source/_javascript_Core/runtime/TypeProfilerLog.h 2016-11-09 21:42:33 UTC (rev 208482)
+++ trunk/Source/_javascript_Core/runtime/TypeProfilerLog.h 2016-11-09 22:02:20 UTC (rev 208483)
@@ -64,6 +64,8 @@
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); }
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
