Summary: | GC should be able to remove things from the DFG worklist and cancel on-going compilations if it knows that the compilation would already be invalidated | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | atrick, barraclough, commit-queue, ggaren, juergen, mark.lam, mhahnenberg, msaboff, nrotem, oliver, sam | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 132167 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2014-04-24 20:45:01 PDT
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 |