Bug 120326

Summary: CodeBlock compilation and installation should be simplified and rationalized
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: abrhm, barraclough, commit-queue, eflews.bot, ggaren, gtk-ews, gyuyoung.kim, kadam, mark.lam, mhahnenberg, msaboff, oliver, ossy, rakuco, rego+ews, sam, webkit-ews, xan.lopez, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 120477    
Bug Blocks: 112838    
Attachments:
Description Flags
work in progress
none
almost there
none
more
none
almost compiles
none
almost
webkit-ews: commit-queue-
the patch
none
the patch oliver: review+

Description Filip Pizlo 2013-08-26 14:56:49 PDT
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.
Comment 1 Filip Pizlo 2013-08-26 14:57:41 PDT
Created attachment 209681 [details]
work in progress
Comment 2 Filip Pizlo 2013-08-27 13:48:09 PDT
Created attachment 209794 [details]
almost there
Comment 3 Filip Pizlo 2013-08-27 13:54:29 PDT
Created attachment 209797 [details]
more
Comment 4 Filip Pizlo 2013-08-27 18:53:43 PDT
Created attachment 209834 [details]
almost compiles
Comment 5 Filip Pizlo 2013-08-27 19:43:38 PDT
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.
Comment 6 WebKit Commit Bot 2013-08-27 19:45:49 PDT
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.
Comment 7 Filip Pizlo 2013-08-27 19:49:56 PDT
(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.
Comment 8 Early Warning System Bot 2013-08-27 19:51:19 PDT
Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605112
Comment 9 Early Warning System Bot 2013-08-27 19:53:10 PDT
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
Comment 10 EFL EWS Bot 2013-08-27 20:02:29 PDT
Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1619121
Comment 11 EFL EWS Bot 2013-08-27 20:07:08 PDT
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
Comment 12 kov's GTK+ EWS bot 2013-08-27 20:47:23 PDT
Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1626109
Comment 13 Build Bot 2013-08-27 21:29:56 PDT
Comment on attachment 209837 [details]
almost

Attachment 209837 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1623130
Comment 14 Filip Pizlo 2013-08-27 21:46:03 PDT
Created attachment 209845 [details]
the patch
Comment 15 WebKit Commit Bot 2013-08-27 21:48:03 PDT
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.
Comment 16 Filip Pizlo 2013-08-27 21:48:33 PDT
Created attachment 209846 [details]
the patch
Comment 17 WebKit Commit Bot 2013-08-27 21:51:35 PDT
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.
Comment 18 Oliver Hunt 2013-08-28 08:39:47 PDT
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?
Comment 19 Filip Pizlo 2013-08-28 13:21:59 PDT
(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.
Comment 20 Filip Pizlo 2013-08-28 21:02:22 PDT
Landed in http://trac.webkit.org/changeset/154804
Comment 21 WebKit Commit Bot 2013-08-29 09:35:52 PDT
Re-opened since this is blocked by bug 120477
Comment 22 Filip Pizlo 2013-08-29 11:24:55 PDT
Relanded in http://trac.webkit.org/changeset/154824