[turbofan] Split CodeGenerator::GenerateCode into AssembleCode and FinishCodeObject.

[turbofan] Split CodeGenerator::GenerateCode into AssembleCode and FinishCodeObject.

This CL splits CodeGenerator::GenerateCode into two new functions:
AssembleCode and FinishCodeObject. AssembleCode does not access or
modify the JS heap, which means that AssembleCode can be executed on
background threads. FinishCodeObject allocates the generated code object
on the JS heap and therefore has to be executed on the main thread.

Implementation details:
The GenerateCode function has been split just before out-of-line code is
assembled. The reason is that code stubs may be generated when
out-of-line code is assembled, which potentially allocates these code
stubs on the heap.

- Parts of initialization of the CodeGenerator has been moved from the
constructor to an Initialize function so that we can instantiate an empty
CodeGenerator object in PipelineData.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org

Committed: https://crrev.com/03058a2187e32cc4080612181802086527c116a2
Cr-Commit-Position: refs/heads/master@{#38604}

[ahaas, 2016-08-10 12:17:20]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 12:17:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 12:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 12:33:58]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Michael Starzinger, 2016-08-11 14:16:26]

LGTM with comments.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:77: // Open a frame scope to indicate that there
is a frame on the stack.  The
nit: Empty newline after DCHECK for readability.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator.h
File src/compiler/code-generator.h (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:51: explicit CodeGenerator(Zone* zone,
CompilationInfo* info);
nit: Doesn't need to be "explicit" with more than one argument.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:53: void Initialize(Frame* frame, Linkage*
linkage, InstructionSequence* code);
nit: Can we add a "// Initialize before calling assembling or finishing code
object."

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:54: // Generate native code.
nit: Empty whitespace between declarations for readability. Also lets adapt the
comment to make clear that this can be run off the main thread.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:56: // Finishes the code object generated in
AssembleCode.
nit: Empty whitespace between declarations for readability. Also it might be
good to mention that this will return an empty handle when AssembleCode failed
or wasn't called (to clarify that is is safe to call).

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:98: code_generator_(instruction_zone_, info),
As discussed offline: Lets put this into the outer_zone for now to get this
landed without having to worry about lifetime issues and still get the
off-thread advantages. Then we can tackle life time issues in a separate CL.
Likewise below in the other constructors.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:356: CodeGenerator code_generator_;
nit: Can be move up to the other fields not living in a specific zone (e.g.
around line 330) to avoid confusion for now.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:661: return FAILED;
Can we unconditionally run both, "AssembleCode" and "FinishCodeObject" here?
That way we get the proper handling when no bailout reason is given below.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1825: Handle<Code> PipelineImpl::FinishCodeObject() {
nit: Empty newline between functions.

[ahaas, 2016-08-11 14:54:12]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 14:54:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-08-11 14:54:20]

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:77: // Open a frame scope to indicate that there
is a frame on the stack.  The
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Empty newline after DCHECK for readability.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator.h
File src/compiler/code-generator.h (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:51: explicit CodeGenerator(Zone* zone,
CompilationInfo* info);
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Doesn't need to be "explicit" with more than one argument.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:53: void Initialize(Frame* frame, Linkage*
linkage, InstructionSequence* code);
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Can we add a "// Initialize before calling assembling or finishing code
object."

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:54: // Generate native code.
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Empty whitespace between declarations for readability. Also lets adapt
the comment to make clear that this can be run off the main thread.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.h:56: // Finishes the code object generated in
AssembleCode.
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Empty whitespace between declarations for readability. Also it might be
good to mention that this will return an empty handle when AssembleCode failed
or wasn't called (to clarify that is is safe to call).

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:98: code_generator_(instruction_zone_, info),
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> As discussed offline: Lets put this into the outer_zone for now to get this
landed without having to worry about lifetime issues and still get the
off-thread advantages. Then we can tackle life time issues in a separate CL.
Likewise below in the other constructors.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:356: CodeGenerator code_generator_;
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Can be move up to the other fields not living in a specific zone (e.g.
around line 330) to avoid confusion for now.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:661: return FAILED;
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> Can we unconditionally run both, "AssembleCode" and "FinishCodeObject" here?
That way we get the proper handling when no bailout reason is given below.

Done.

https://codereview.chromium.org/2229243003/diff/1/src/compiler/pipeline.cc#ne...
src/compiler/pipeline.cc:1825: Handle<Code> PipelineImpl::FinishCodeObject() {
On 2016/08/11 at 14:16:26, Michael Starzinger wrote:
> nit: Empty newline between functions.

Done.

[commit-bot: I haz the power, 2016-08-11 15:14:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 15:14:06]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10823)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[ahaas, 2016-08-12 09:19:07]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 09:19:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 09:42:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 09:42:16]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-08-12 09:48:47]

Thanks. Still LGTM.

[ahaas, 2016-08-12 09:48:54]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-08-12 09:49:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 09:50:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-12 09:51:10]

Description was changed from

==========
[turbofan] Split CodeGenerator::GenerateCode into AssembleCode and
FinishCodeObject.

This CL splits CodeGenerator::GenerateCode into two new functions:
AssembleCode and FinishCodeObject. AssembleCode does not access or
modify the JS heap, which means that AssembleCode can be executed on
background threads. FinishCodeObject allocates the generated code object
on the JS heap and therefore has to be executed on the main thread.

Implementation details:
The GenerateCode function has been split just before out-of-line code is
assembled. The reason is that code stubs may be generated when
out-of-line code is assembled, which potentially allocates these code
stubs on the heap.

- Parts of initialization of the CodeGenerator has been moved from the
constructor to an Initialize function so that we can instantiate an empty
CodeGenerator object in PipelineData.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

to

==========
[turbofan] Split CodeGenerator::GenerateCode into AssembleCode and
FinishCodeObject.

This CL splits CodeGenerator::GenerateCode into two new functions:
AssembleCode and FinishCodeObject. AssembleCode does not access or
modify the JS heap, which means that AssembleCode can be executed on
background threads. FinishCodeObject allocates the generated code object
on the JS heap and therefore has to be executed on the main thread.

Implementation details:
The GenerateCode function has been split just before out-of-line code is
assembled. The reason is that code stubs may be generated when
out-of-line code is assembled, which potentially allocates these code
stubs on the heap.

- Parts of initialization of the CodeGenerator has been moved from the
constructor to an Initialize function so that we can instantiate an empty
CodeGenerator object in PipelineData.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org

Committed: https://crrev.com/03058a2187e32cc4080612181802086527c116a2
Cr-Commit-Position: refs/heads/master@{#38604}
==========

[commit-bot: I haz the power, 2016-08-12 09:51:11]

Patchset 3 (id:??) landed as
https://crrev.com/03058a2187e32cc4080612181802086527c116a2
Cr-Commit-Position: refs/heads/master@{#38604}

[ahaas, 2016-08-12 10:59:17]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2240523003/ by ahaas@chromium.org.

The reason for reverting is: There is a data race in the initialization of the
Isolate::random_number_generator().