[ignition] Optimize JSGenerator creation

[ignition] Optimize JSGenerator creation

As a first step towards improving generator creation, create a builtin
that can do it without a call to the runtime. Thread that builtin
into the interpreter via an intrinsic.

BUG=v8:6352
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2861983002
Cr-Commit-Position: refs/heads/master@{#45145}
Committed: https://chromium.googlesource.com/v8/v8/+/c3f0e2a5b0eb8371ebb99868e14f038a00e9b669

[mvstanton, 2017-05-04 13:14:48]

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

[commit-bot: I haz the power, 2017-05-04 13:14:59]

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, 2017-05-04 13:21:34]

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

[commit-bot: I haz the power, 2017-05-04 13:21:35]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)

[mvstanton, 2017-05-04 16:26:52]

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

[commit-bot: I haz the power, 2017-05-04 16:27:01]

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, 2017-05-04 16:55:21]

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

[commit-bot: I haz the power, 2017-05-04 16:55:22]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-05-04 18:47:30]

mvstanton@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[mvstanton, 2017-05-04 18:47:31]

Hi Benedikt, hi Ross,

Here is the first step in improving CreateJSGeneratorObject.

Ross, could you look at the interpreter changes...you'll see those weakening of
asserts we chatted about. I should probably make that cleaner with a predicate?

Benedikt, I want to be sure I've got your renaming suggestions correct -- you
would advocate 's/CreateJSGeneratorObject/CreateGeneratorObject' right? So I
should rename that runtime function, eh?

Thanks much,
--Michael

[rmcilroy, 2017-05-05 09:44:11]

LGTM with a couple of comments, thanks.

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:755: bytecode == Bytecode::kInvokeIntrinsic);
please remove this and the kInvokeIntrinsic in the switch below, we shouldn't be
calling this with Bytecode::kInvokeIntrinic

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:752: bytecode_ ==
Bytecode::kInvokeIntrinsic);
please also remove this, InvokeIntrinsic shouldn't be calling a JS function.

[Benedikt Meurer, 2017-05-05 09:56:28]

LGTM modulo Ross' comments.

[mvstanton, 2017-05-05 11:03:47]

Description was changed from

==========
Optimize JSGenerator creation

As a first step towards improving generator creation, create a builtin
that can do it without a call to the runtime. Thread that builtin
into the interpreter via an intrinsic.

BUG=v8:6352
R=bmeurer@chromium.org
==========

to

==========
[ignition] Optimize JSGenerator creation

As a first step towards improving generator creation, create a builtin
that can do it without a call to the runtime. Thread that builtin
into the interpreter via an intrinsic.

BUG=v8:6352
R=bmeurer@chromium.org
==========

[mvstanton, 2017-05-05 11:50:27]

Thanks for the comments!

Ross, I get an assert in snapshot generation if I don't weaken the assert in
question to allow kInvokeIntrinsic (presumably for this single case). 

Have a look at the call stack and see if the allowance makes sense.
Thanks again,
--Michael

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:755: bytecode == Bytecode::kInvokeIntrinsic);
On 2017/05/05 09:44:11, rmcilroy wrote:
> please remove this and the kInvokeIntrinsic in the switch below, we shouldn't
be
> calling this with Bytecode::kInvokeIntrinic

This is a side-effect of the location identified with your other comment. The
Call() code wants to get the receiver mode, and therefore I weaken this assert
in the same way.

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:752: bytecode_ ==
Bytecode::kInvokeIntrinsic);
On 2017/05/05 09:44:11, rmcilroy wrote:
> please also remove this, InvokeIntrinsic shouldn't be calling a JS function.

Drat, it does so, in this call stack in bytecode handler generation:


#2  v8::internal::interpreter::InterpreterAssembler::CallJS ()
    at ../../src/interpreter/interpreter-assembler.cc:751
#3  v8::internal::interpreter::IntrinsicsGenerator::Call ()
    at ../../src/interpreter/interpreter-intrinsics-generator.cc:311
#4  v8::internal::interpreter::IntrinsicsGenerator::InvokeIntrinsic () 
    at ../../src/interpreter/interpreter-intrinsics-generator.cc:98
#5  v8::internal::interpreter::GenerateInvokeIntrinsic () 
    at ../../src/interpreter/interpreter-intrinsics-generator.cc:63
#6  v8::internal::interpreter::InvokeIntrinsicAssembler::GenerateImpl () 
    at ../../src/interpreter/interpreter-generator.cc:1997
#7  v8::internal::interpreter::InvokeIntrinsicAssembler::Generate ()
    at ../../src/interpreter/interpreter-generator.cc:1992
#8  v8::internal::interpreter::GenerateBytecodeHandler ()
    ...

The bytecode_ is kInvokeIntrinsic. It looks like in the act of generating the
bytecode handler to invoke an intrinsic we run into this. A bootstrapping
problem? That's why I allowed the exception -- perhaps there is another way?

[rmcilroy, 2017-05-05 13:50:32]

LGTM, I'd forgotten there was an intrinsic to call a JS function. Thanks for
checking.

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:755: bytecode == Bytecode::kInvokeIntrinsic);
On 2017/05/05 11:50:26, mvstanton wrote:
> On 2017/05/05 09:44:11, rmcilroy wrote:
> > please remove this and the kInvokeIntrinsic in the switch below, we
shouldn't
> be
> > calling this with Bytecode::kInvokeIntrinic
> 
> This is a side-effect of the location identified with your other comment. The
> Call() code wants to get the receiver mode, and therefore I weaken this assert
> in the same way.

Right, makes sense, thanks for checking.

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2861983002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:752: bytecode_ ==
Bytecode::kInvokeIntrinsic);
On 2017/05/05 11:50:26, mvstanton wrote:
> On 2017/05/05 09:44:11, rmcilroy wrote:
> > please also remove this, InvokeIntrinsic shouldn't be calling a JS function.
> 
> Drat, it does so, in this call stack in bytecode handler generation:
> 
> 
> #2  v8::internal::interpreter::InterpreterAssembler::CallJS ()
>     at ../../src/interpreter/interpreter-assembler.cc:751
> #3  v8::internal::interpreter::IntrinsicsGenerator::Call ()
>     at ../../src/interpreter/interpreter-intrinsics-generator.cc:311
> #4  v8::internal::interpreter::IntrinsicsGenerator::InvokeIntrinsic () 
>     at ../../src/interpreter/interpreter-intrinsics-generator.cc:98
> #5  v8::internal::interpreter::GenerateInvokeIntrinsic () 
>     at ../../src/interpreter/interpreter-intrinsics-generator.cc:63
> #6  v8::internal::interpreter::InvokeIntrinsicAssembler::GenerateImpl () 
>     at ../../src/interpreter/interpreter-generator.cc:1997
> #7  v8::internal::interpreter::InvokeIntrinsicAssembler::Generate ()
>     at ../../src/interpreter/interpreter-generator.cc:1992
> #8  v8::internal::interpreter::GenerateBytecodeHandler ()
>     ...
> 
> The bytecode_ is kInvokeIntrinsic. It looks like in the act of generating the
> bytecode handler to invoke an intrinsic we run into this. A bootstrapping
> problem? That's why I allowed the exception -- perhaps there is another way?


Ahh yeah, I forgot we had the Runtime::kInlineCall intrinsic which performs a JS
call to another function.  Yeah we should add this to allow Intrinsic to CallJS
in that case, thanks for checking.

[mvstanton, 2017-05-05 15:38:17]

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

[commit-bot: I haz the power, 2017-05-05 15:38:25]

Dry run: CQ is trying da patch.

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

[mvstanton, 2017-05-05 15:40:52]

mvstanton@chromium.org changed reviewers:
+ neis@chromium.org

[mvstanton, 2017-05-05 15:40:53]

Right on, thanks Ross!
 Georg could you have a look at the src/parsing change?
Best,
--Michael

[neis, 2017-05-05 16:04:38]

src/parsing/ lgtm

[commit-bot: I haz the power, 2017-05-05 16:20:00]

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

[commit-bot: I haz the power, 2017-05-05 16:20:01]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-05-05 18:31:05]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-05-05 18:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2861983002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-05 18:31:17]

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, 2017-05-05 18:33:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494009065311050,
"parent_rev": "1b064d9cd015bfa8878e190243ca7cbdfe69482b", "commit_rev":
"c3f0e2a5b0eb8371ebb99868e14f038a00e9b669"}

[commit-bot: I haz the power, 2017-05-05 18:33:08]

Description was changed from

==========
[ignition] Optimize JSGenerator creation

As a first step towards improving generator creation, create a builtin
that can do it without a call to the runtime. Thread that builtin
into the interpreter via an intrinsic.

BUG=v8:6352
R=bmeurer@chromium.org
==========

to

==========
[ignition] Optimize JSGenerator creation

As a first step towards improving generator creation, create a builtin
that can do it without a call to the runtime. Thread that builtin
into the interpreter via an intrinsic.

BUG=v8:6352
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2861983002
Cr-Commit-Position: refs/heads/master@{#45145}
Committed:
https://chromium.googlesource.com/v8/v8/+/c3f0e2a5b0eb8371ebb99868e14f038a00e...
==========

[commit-bot: I haz the power, 2017-05-05 18:33:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/c3f0e2a5b0eb8371ebb99868e14f038a00e...