Bug 128291

Summary: GC blocks on FTL and then badness
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: atrick, barraclough, bunhere, commit-queue, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, rakuco, sam, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 128297, 128443, 128505    
Bug Blocks: 128039, 128521    
Attachments:
Description Flags
work in progress
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
the ptch
none
the patch
none
the patch
none
the patch oliver: review+

Description Filip Pizlo 2014-02-05 19:07:20 PST
This is the primary reason for the FTL appearing to be a regression on Octane/typescript.  It's probably also the reason for the Octane/gbemu regression.  And it's the reason why we need so many compiler threads.

We should totally fix this.  I suspect that we might be able to make LLVM compilation concurrent with the GC.  But it seems hard-ish.  And if we make it work then we might as well make concurrent compilation work concurrently with GC full stop.  I have a hard time imagining why this wouldn't work, if we just marked everything event slightly reachable (weakly, strongly, or anythingly) from the to-be-compiled code block so long as it was part of a DFG::Plan that is on any worklist.
Comment 1 Mark Hahnenberg 2014-02-05 19:10:09 PST
*** Bug 125518 has been marked as a duplicate of this bug. ***
Comment 2 Filip Pizlo 2014-02-05 22:22:23 PST
Here's my new strawman proposal, consisting of two standalone bugs:

1) When GC starts, don't wait for the worklist to become empty like we do now.  Instead, ask for the current compilations to finish and ask the JIT threads not to dequeue any more work, and then strongly visit all references from all DFG::Plan's still on the worklist.  A DFG::Plan is a CodeBlock plus a few other things.
	
	=> A close alternative would be to just remove any Plans from the worklist that had CodeBlocks that died during GC.  This is doable but seems trickier.

This would be a standalone patch and it would be a speed-up.  It's embarrassing, but in trunk we really do have GC wait for *all* outstanding compilations to finish.  This is a big part of our problems.

2) When the GC asks for the current compilations to finish, make it treat JIT threads that are currently blocked on LLVM as "safe" threads and let them proceed concurrently.  In proper safepointing fashion, this means each thread effectively will have a "right-to-run" mutex.  The thread will acquire this mutex precisely when it is about to do DFG work.  It will release it just before waiting for more work from the worklist, and also just before calling into LLVM.  If the thread every releases the right-to-run mutex while holding onto a DFG::Graph, it will make sure that the DFG::Graph is available to the GC for scanning. The workflow of an FTL thread will thus be:
	
	Mutex rightToRun; // Per-thread variable visible to the GC
	Graph* graph = 0; // Per-thread variable visible to the GC
	for (;;) {
	    RefPtr<Plan> plan = worklist.pop();
	    {
	        Locker locker(rightToRun);
	        DFG::parse(plan);
	        DFG::optimize(plan);
	        FTL::lowerDFGToLLVM(plan);
	        graph = &plan.graph;
	    }
	    LLVM::doThings(plan);
	    {
	        Locker locker(rightToRun);
	        graph = 0;
	        FTL::link(plan);
	        plan.complete();
	    }
	}

The DFG workflow will be simpler:

	Mutex rightToRun; // Per-thread variable visible to the GC
	for (;;) {
	    RefPtr<Plan> plan = worklist.pop();
	    {
	        Locker locker(rightToRun);
	        DFG::parse(plan);
	        DFG::optimize(plan);
	        DFG::generate(plan);
	        plan.complete();
	    }
	}

The GC will acquire the rightToRun of each JIT thread before GC starts, and will release the rightToRun of each JIT thread after GC ends.  While the GC is running, it will inspect each JIT thread's graph - if it's non-null, then it will strongly scan it.
Comment 3 Filip Pizlo 2014-02-07 22:10:45 PST
Created attachment 223548 [details]
work in progress
Comment 4 Filip Pizlo 2014-02-09 11:32:51 PST
Created attachment 223635 [details]
the patch

I think it's ready, but I'm not setting r? because I have no tests to do.
Comment 5 Filip Pizlo 2014-02-09 12:11:13 PST
Created attachment 223637 [details]
the patch
Comment 6 WebKit Commit Bot 2014-02-09 12:13:48 PST
Attachment 223637 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Filip Pizlo 2014-02-09 12:22:29 PST
Created attachment 223638 [details]
the patch
Comment 8 WebKit Commit Bot 2014-02-09 12:25:02 PST
Attachment 223638 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Filip Pizlo 2014-02-09 12:32:25 PST
Created attachment 223639 [details]
the patch
Comment 10 Filip Pizlo 2014-02-09 12:39:41 PST
Created attachment 223640 [details]
the patch
Comment 11 Filip Pizlo 2014-02-09 12:47:01 PST
Created attachment 223642 [details]
the patch
Comment 12 Filip Pizlo 2014-02-09 12:53:49 PST
Created attachment 223643 [details]
the patch
Comment 13 WebKit Commit Bot 2014-02-09 12:55:38 PST
Attachment 223643 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/SamplingTool.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Filip Pizlo 2014-02-09 13:00:40 PST
Created attachment 223644 [details]
the patch
Comment 15 Filip Pizlo 2014-02-09 13:08:49 PST
Created attachment 223645 [details]
the ptch
Comment 16 Filip Pizlo 2014-02-09 13:13:14 PST
Created attachment 223646 [details]
the patch
Comment 17 Filip Pizlo 2014-02-09 18:58:37 PST
Created attachment 223663 [details]
the patch
Comment 18 Filip Pizlo 2014-02-09 19:01:22 PST
Created attachment 223664 [details]
the patch
Comment 19 WebKit Commit Bot 2014-02-09 19:03:27 PST
Attachment 223664 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGThreadData.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSafepoint.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGGraphSafepoint.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Filip Pizlo 2014-02-09 23:16:50 PST
Landed in http://trac.webkit.org/changeset/163765
Comment 21 Mark Hahnenberg 2014-02-20 12:21:48 PST
*** Bug 125518 has been marked as a duplicate of this bug. ***