Bug 128291 – GC blocks on FTL and then badness

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Mark Hahnenberg]

*** Bug 125518 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 223548 [details]
work in progress

[Filip Pizlo]

Created attachment 223635 [details]
the patch

I think it's ready, but I'm not setting r? because I have no tests to do.

[Filip Pizlo]

Created attachment 223637 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 223638 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 223639 [details]
the patch

[Filip Pizlo]

Created attachment 223640 [details]
the patch

[Filip Pizlo]

Created attachment 223642 [details]
the patch

[Filip Pizlo]

Created attachment 223643 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 223644 [details]
the patch

[Filip Pizlo]

Created attachment 223645 [details]
the ptch

[Filip Pizlo]

Created attachment 223646 [details]
the patch

[Filip Pizlo]

Created attachment 223663 [details]
the patch

[Filip Pizlo]

Created attachment 223664 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/163765

[Mark Hahnenberg]

*** Bug 125518 has been marked as a duplicate of this bug. ***