This will be fun.
Created attachment 230233 [details] I am afraid to try running this patch
Created attachment 230253 [details] possibly done
This appears to help but the fact that we may have a running plan hurts us. It would be even cooler if we could cancel the running plans.
Created attachment 230262 [details] more Adding cancellation to DFG::Safepoint
Created attachment 230265 [details] all the pieces are in place
Created attachment 230269 [details] might actually be done
*** Bug 132236 has been marked as a duplicate of this bug. ***
Created attachment 230273 [details] and now, plans are cancellable at *any* time There is no escape from the GC cancellation monster.
Created attachment 230276 [details] the patch
Attachment 230276 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:127: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 230276 [details] the patch yay!
Comment on attachment 230276 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=230276&action=review What are some good stress tests for this stuff? gbemu? Our compiler-heavy benchmarks on platforms with <= 2 cores? Maybe something with a bunch of workers all doing compilation? > Source/JavaScriptCore/dfg/DFGSafepoint.cpp:105 > + return true; // We were cancelled during a previous GC, so let's not mess with it this time around - pretend it's live and move on. I don't understand this. Can you elaborate? > Source/JavaScriptCore/dfg/DFGWorklist.cpp:272 > + Deque<RefPtr<Plan>> newQueue; > + while (!m_queue.isEmpty()) { > + RefPtr<Plan> plan = m_queue.takeFirst(); > + if (plan->stage != Plan::Cancelled) > + continue; > + newQueue.append(plan); > + } > + m_queue.swap(newQueue); Hmm, I'm confused by this code. What I see is: (1) Create a new queue with all of the plans whose stage == Cancelled. (2) Replace the old work queue with just the new queue of cancelled items. I would think we would want to throw away the Cancelled plans and keep the non-cancelled ones. Is this what we're doing and I'm just being dense? > Source/JavaScriptCore/heap/Heap.cpp:638 > + GCPHASE(FinalizerDFGWorklists); s/FinalizerDFGWorklists/FinalizeDFGWorklists/ ?
(In reply to comment #12) > (From update of attachment 230276 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230276&action=review > > What are some good stress tests for this stuff? gbemu? Our compiler-heavy benchmarks on platforms with <= 2 cores? Maybe something with a bunch of workers all doing compilation? Our stress testing infrastructure can already do this, more-or-less. > > > Source/JavaScriptCore/dfg/DFGSafepoint.cpp:105 > > + return true; // We were cancelled during a previous GC, so let's not mess with it this time around - pretend it's live and move on. > > I don't understand this. Can you elaborate? The GC doesn't remove safepoints. The GC cannot force forward progress in the compiler thread. So, after we cancel the plan associated with a safepoint (i.e. cancel the safepoint), a subsequent GC may still see the same safepoint - in a cancelled state - with a cancelled plan. This code gracefully ignores such safepoints since there is nothing left to do for them and they do not reference any GC memory anymore. > > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:272 > > + Deque<RefPtr<Plan>> newQueue; > > + while (!m_queue.isEmpty()) { > > + RefPtr<Plan> plan = m_queue.takeFirst(); > > + if (plan->stage != Plan::Cancelled) > > + continue; > > + newQueue.append(plan); > > + } > > + m_queue.swap(newQueue); > > Hmm, I'm confused by this code. What I see is: > > (1) Create a new queue with all of the plans whose stage == Cancelled. > (2) Replace the old work queue with just the new queue of cancelled items. > > I would think we would want to throw away the Cancelled plans and keep the non-cancelled ones. Is this what we're doing and I'm just being dense? Yeah, the logic was inverted. Thanks for catching it! > > > Source/JavaScriptCore/heap/Heap.cpp:638 > > + GCPHASE(FinalizerDFGWorklists); > > s/FinalizerDFGWorklists/FinalizeDFGWorklists/ ? Lol I will change.
Landed in http://trac.webkit.org/changeset/167897