Bug 120326 – CodeBlock compilation and installation should be simplified and rationalized

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

[Filip Pizlo]

We currently do a really bizarre dance when creating code blocks.  When compiling, we typically install the compiled code in Executable first and then in the CodeBlock - that's messed up.  We establish the Executable<->CodeBlock relationship at random times depending on code path.  We have duplicated code between the LLInt initialization path, the baseline JIT compilation path, and the DFG/FTL compilation paths.  It's incredibly confusing.  This confusion makes it hard to do things like creating a custom temporary CodeBlock just for DFG->FTL OSR entry, or having the DFG trigger its own replacement compilation (i.e. DFG->FTL tier-up).

It also means that the DFG::Plan and DFG::Worklist have super-special knowledge of the CodeBlock<->Executable relationship.  That makes it hard to establish new such relationships, for example for FTL code.

Instead we should have a simplified system of rules:

1) You can always ask an Executable for a new code block.  It will give you one, and you will own it for now.  You can hold onto it for as long as you like.

2) You can always ask an Executable to install a CodeBlock.  It will do all of the things needed to ensure that calls are redirected from the old one to the new one, and that the GC knows about the increased costs.

3) You can always ask a CodeBlock to prepare itself for execution - i.e. go from being just a thing that holds bytecode to being a thing that has proper entrypoints.  When you make such a request, you should be able to specify which execution engine to use, and the CodeBlock should figure it out.

4) Asking a CodeBlock to prepare for execution should have an optional asynchronous mode, in which case you should pass a callback describing what to do when compilation finishes.

[Filip Pizlo]

Created attachment 209681 [details]
work in progress

[Filip Pizlo]

Created attachment 209794 [details]
almost there

[Filip Pizlo]

Created attachment 209797 [details]
more

[Filip Pizlo]

Created attachment 209834 [details]
almost compiles

[Filip Pizlo]

Created attachment 209837 [details]
almost

Almost done.  I think I just need to write some FTL glue to keep FTL builds working and then I'll be done.

[WebKit Commit Bot]

Attachment 209837 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.h', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.h', u'Source/JavaScriptCore/dfg/DFGFinalizer.h', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.h', u'Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.h', u'Source/JavaScriptCore/dfg/DFGWorklist.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITDriver.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h', u'Source/JavaScriptCore/llint/LLIntEntrypoints.cpp', u'Source/JavaScriptCore/llint/LLIntEntrypoints.h', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/CommonSlowPaths.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.h', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/Executable.h', u'Source/JavaScriptCore/runtime/ExecutionHarness.h']" exit_code: 1
Source/JavaScriptCore/jit/JITStubs.cpp:1058:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/dfg/DFGPlan.h:56:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #6)
> Attachment 209837 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.h', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.h', u'Source/JavaScriptCore/dfg/DFGFinalizer.h', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.h', u'Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.h', u'Source/JavaScriptCore/dfg/DFGWorklist.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITDriver.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h', u'Source/JavaScriptCore/llint/LLIntEntrypoints.cpp', u'Source/JavaScriptCore/llint/LLIntEntrypoints.h', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/CommonSlowPaths.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.h', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/Executable.h', u'Source/JavaScriptCore/runtime/ExecutionHarness.h']" exit_code: 1
> Source/JavaScriptCore/jit/JITStubs.cpp:1058:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

False.

> Source/JavaScriptCore/dfg/DFGPlan.h:56:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]

Fixed.

> Total errors found: 2 in 30 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605112

[Early Warning System Bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1625122

[EFL EWS Bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1619121

[EFL EWS Bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1628129

[kov's GTK+ EWS bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1626109

[Build Bot]

Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1623130

[Filip Pizlo]

Created attachment 209845 [details]
the patch

[WebKit Commit Bot]

Attachment 209845 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.h', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.h', u'Source/JavaScriptCore/dfg/DFGFinalizer.h', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.h', u'Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.h', u'Source/JavaScriptCore/dfg/DFGWorklist.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITDriver.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h', u'Source/JavaScriptCore/llint/LLIntEntrypoints.cpp', u'Source/JavaScriptCore/llint/LLIntEntrypoints.h', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/CommonSlowPaths.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.h', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/Executable.h', u'Source/JavaScriptCore/runtime/ExecutionHarness.h']" exit_code: 1
Source/JavaScriptCore/jit/JITStubs.cpp:1058:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 209846 [details]
the patch

[WebKit Commit Bot]

Attachment 209846 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp', u'Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.h', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGFailedFinalizer.h', u'Source/JavaScriptCore/dfg/DFGFinalizer.h', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.h', u'Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGPlan.h', u'Source/JavaScriptCore/dfg/DFGWorklist.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITDriver.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.cpp', u'Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h', u'Source/JavaScriptCore/llint/LLIntEntrypoints.cpp', u'Source/JavaScriptCore/llint/LLIntEntrypoints.h', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/ArrayPrototype.cpp', u'Source/JavaScriptCore/runtime/CommonSlowPaths.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.cpp', u'Source/JavaScriptCore/runtime/CompilationResult.h', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/Executable.h', u'Source/JavaScriptCore/runtime/ExecutionHarness.h']" exit_code: 1
Source/JavaScriptCore/jit/JITStubs.cpp:1058:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 209846 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209846&action=review

r=me if you switch to having create methods for the new refcounted classes

> Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h:38
> +public:
> +    DeferredCompilationCallback();

Can we give this a protected constructor and a public create method?

> Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h:37
> +public:
> +    JITToDFGDeferredCompilationCallback();

can we make the constructor private, and give this a PassRefPtr returning create() method?

[Filip Pizlo]

(In reply to comment #18)
> (From update of attachment 209846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209846&action=review
> 
> r=me if you switch to having create methods for the new refcounted classes
> 
> > Source/JavaScriptCore/bytecode/DeferredCompilationCallback.h:38
> > +public:
> > +    DeferredCompilationCallback();
> 
> Can we give this a protected constructor and a public create method?

Protected constructor yes - but no need for a create method since it's an abstract base class.

> 
> > Source/JavaScriptCore/jit/JITToDFGDeferredCompilationCallback.h:37
> > +public:
> > +    JITToDFGDeferredCompilationCallback();
> 
> can we make the constructor private, and give this a PassRefPtr returning create() method?

Fixed.

[Filip Pizlo]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120477

[Filip Pizlo]

Relanded in http://trac.webkit.org/changeset/154824