Call JS functions via native context instead of js builtins object.

Call JS functions via native context instead of js builtins object.

We look up %-functions in the context if not found in the runtime.

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

Committed: https://crrev.com/299f775cf4a936c82e0eead5342defe85d312507
Cr-Commit-Position: refs/heads/master@{#30379}

[Yang, 2015-08-25 13:56:20]

I'd like some comments on the ia32 port.

[Michael Starzinger, 2015-08-25 18:12:09]

Essentially one high-level comment/suggestion. Happy to discuss in person and/or
be convinced otherwise. Would also be interested in what Benedikt thinks about
this.

https://codereview.chromium.org/1306993003/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1306993003/diff/1/src/ast.h#newcode2052
src/ast.h:2052: const Runtime::Function* function_;
If below comment is addressed then we would need two fields here again, either
Runtime::Function or Context::Index. Still a memory saving from what was here
before, and if size really becomes the argument here, then there are (other)
ways to compress it down I think.

https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h#newco...
src/runtime/runtime.h:995: enum IntrinsicType { RUNTIME, INLINE, CONTEXT };
High-level feedback: I am not sure whether overloading the Runtime::Function
table like this is the way to go. Basically what we need is a mapping (name ->
context-index) IIUC. The other fields (e.g. nargs, result_size) make no sense
for "imported context functions". Also them being implemented in JavaScript as
opposed to C++ makes them different. Would it be worth it to have a separate
lookup functionality outside of the Runtime class?

[Yang, 2015-08-25 18:30:01]

On 2015/08/25 18:12:09, Michael Starzinger wrote:
> Essentially one high-level comment/suggestion. Happy to discuss in person
and/or
> be convinced otherwise. Would also be interested in what Benedikt thinks about
> this.
> 
> https://codereview.chromium.org/1306993003/diff/1/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/1306993003/diff/1/src/ast.h#newcode2052
> src/ast.h:2052: const Runtime::Function* function_;
> If below comment is addressed then we would need two fields here again, either
> Runtime::Function or Context::Index. Still a memory saving from what was here
> before, and if size really becomes the argument here, then there are (other)
> ways to compress it down I think.
> 
> https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h
> File src/runtime/runtime.h (right):
> 
>
https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h#newco...
> src/runtime/runtime.h:995: enum IntrinsicType { RUNTIME, INLINE, CONTEXT };
> High-level feedback: I am not sure whether overloading the Runtime::Function
> table like this is the way to go. Basically what we need is a mapping (name ->
> context-index) IIUC. The other fields (e.g. nargs, result_size) make no sense
> for "imported context functions". Also them being implemented in JavaScript as
> opposed to C++ makes them different. Would it be worth it to have a separate
> lookup functionality outside of the Runtime class?

I'm somewhat torn about this as well, and initially actually had the lookup
implemented separately. However, the separate implementation would share a lot
of the same code, and, if done via hash table lookup, use the same data
structure as well. Furthermore, the difference between C++ runtime functions to
JS builtin functions is no larger than the difference between C++ runtime
functions and inlined %_ functions. Common to all of them is that they can be
addressed via % syntax. So I think grouping the three of them together is
alright.

But I can be convince otherwise, if you feel strongly about this.

[Yang, 2015-08-26 08:58:19]

On 2015/08/25 18:30:01, Yang wrote:
> On 2015/08/25 18:12:09, Michael Starzinger wrote:
> > Essentially one high-level comment/suggestion. Happy to discuss in person
> and/or
> > be convinced otherwise. Would also be interested in what Benedikt thinks
about
> > this.
> > 
> > https://codereview.chromium.org/1306993003/diff/1/src/ast.h
> > File src/ast.h (right):
> > 
> > https://codereview.chromium.org/1306993003/diff/1/src/ast.h#newcode2052
> > src/ast.h:2052: const Runtime::Function* function_;
> > If below comment is addressed then we would need two fields here again,
either
> > Runtime::Function or Context::Index. Still a memory saving from what was
here
> > before, and if size really becomes the argument here, then there are (other)
> > ways to compress it down I think.
> > 
> > https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h
> > File src/runtime/runtime.h (right):
> > 
> >
>
https://codereview.chromium.org/1306993003/diff/1/src/runtime/runtime.h#newco...
> > src/runtime/runtime.h:995: enum IntrinsicType { RUNTIME, INLINE, CONTEXT };
> > High-level feedback: I am not sure whether overloading the Runtime::Function
> > table like this is the way to go. Basically what we need is a mapping (name
->
> > context-index) IIUC. The other fields (e.g. nargs, result_size) make no
sense
> > for "imported context functions". Also them being implemented in JavaScript
as
> > opposed to C++ makes them different. Would it be worth it to have a separate
> > lookup functionality outside of the Runtime class?
> 
> I'm somewhat torn about this as well, and initially actually had the lookup
> implemented separately. However, the separate implementation would share a lot
> of the same code, and, if done via hash table lookup, use the same data
> structure as well. Furthermore, the difference between C++ runtime functions
to
> JS builtin functions is no larger than the difference between C++ runtime
> functions and inlined %_ functions. Common to all of them is that they can be
> addressed via % syntax. So I think grouping the three of them together is
> alright.
> 
> But I can be convince otherwise, if you feel strongly about this.

I separated the context slot lookup. And ported to all platforms. Please review.

[Michael Starzinger, 2015-08-26 09:43:22]

LGTM if all comments are addressed from my end. Please also wait on Benedikt's
sign-off. I like this a lot. Thanks!

https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1847: native_context()->set_reflect_construct(*construct);
Woot! Thanks!

https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2552:
environment()->Push(jsgraph()->UndefinedConstant());
nit: Let's add a trailing "// receiver" comment, or alternatively use a local
variable called "receiver_value", both are fine with me.

https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h
File src/contexts.h (left):

https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h#oldcode132
src/contexts.h:132: #define NATIVE_CONTEXT_IMPORTED_FIELDS_FOR_PROXY(V)       \
Yay! I like that this is gone and has been merged into the above list.

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:1102: os << function()->name << " ";
Is "this" guaranteed to point to a runtime call or do we need "is_jsruntime()"
here?

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc#newcode...
src/hydrogen.cc:11102: if (call->function()->function_id !=
Runtime::kInlineClassOf) return false;
Is "call" guaranteed to point to a runtime function here or do we need a
"!call->is_jsruntime()" here to guard that? Because ClusterFuzz will find that
one for sure. :)

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc
File src/prettyprinter.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:798: Print("%%%s\n", node->function()->name);
Is node guaranteed to have a runtime function or is "is_jsruntime()" needed?

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:1526: Print("NAME %s\n", node->function()->name);
Likewise.

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc#...
src/runtime/runtime.cc:84: if (kIntrinsicFunctions[i].intrinsic_type == RUNTIME
&&
Is this needed? This looks fishy to me!

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:9: #include "src/contexts.h"
nit: Include shouldn't be needed anymore, let's drop it.

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:1003: // For CONTEXT functions this is the native context
index.
nit: Last line of the comment is obsolete now, let's drop it.

[Yang, 2015-08-26 10:27:30]

Addressed comments.

https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1847: native_context()->set_reflect_construct(*construct);
On 2015/08/26 09:43:21, Michael Starzinger wrote:
> Woot! Thanks!

Acknowledged.

https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2552:
environment()->Push(jsgraph()->UndefinedConstant());
On 2015/08/26 09:43:21, Michael Starzinger wrote:
> nit: Let's add a trailing "// receiver" comment, or alternatively use a local
> variable called "receiver_value", both are fine with me.

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h
File src/contexts.h (left):

https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h#oldcode132
src/contexts.h:132: #define NATIVE_CONTEXT_IMPORTED_FIELDS_FOR_PROXY(V)       \
On 2015/08/26 09:43:21, Michael Starzinger wrote:
> Yay! I like that this is gone and has been merged into the above list.

Acknowledged.

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:1102: os << function()->name << " ";
On 2015/08/26 09:43:21, Michael Starzinger wrote:
> Is "this" guaranteed to point to a runtime call or do we need "is_jsruntime()"
> here?

HCallRuntime does not match CallRuntime one-to-one. HCallRuntime only does C++
runtime calls. There is no is_jsruntime() here.

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc#newcode...
src/hydrogen.cc:11102: if (call->function()->function_id !=
Runtime::kInlineClassOf) return false;
On 2015/08/26 09:43:21, Michael Starzinger wrote:
> Is "call" guaranteed to point to a runtime function here or do we need a
> "!call->is_jsruntime()" here to guard that? Because ClusterFuzz will find that
> one for sure. :)

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc
File src/prettyprinter.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:798: Print("%%%s\n", node->function()->name);
On 2015/08/26 09:43:22, Michael Starzinger wrote:
> Is node guaranteed to have a runtime function or is "is_jsruntime()" needed?

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:1526: Print("NAME %s\n", node->function()->name);
On 2015/08/26 09:43:22, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc#...
src/runtime/runtime.cc:84: if (kIntrinsicFunctions[i].intrinsic_type == RUNTIME
&&
On 2015/08/26 09:43:22, Michael Starzinger wrote:
> Is this needed? This looks fishy to me!

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:9: #include "src/contexts.h"
On 2015/08/26 09:43:22, Michael Starzinger wrote:
> nit: Include shouldn't be needed anymore, let's drop it.

Done.

https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:1003: // For CONTEXT functions this is the native context
index.
On 2015/08/26 09:43:22, Michael Starzinger wrote:
> nit: Last line of the comment is obsolete now, let's drop it.

Done.

[Yang, 2015-08-26 10:28:04]

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

[commit-bot: I haz the power, 2015-08-26 10:28:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993003/40001

[commit-bot: I haz the power, 2015-08-26 10:29:15]

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

[commit-bot: I haz the power, 2015-08-26 10:29:16]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/8973)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9083)

[Benedikt Meurer, 2015-08-26 10:38:47]

LGTM.

[Yang, 2015-08-26 10:47:28]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-08-26 10:47:28]

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

[commit-bot: I haz the power, 2015-08-26 10:47:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993003/60001

[commit-bot: I haz the power, 2015-08-26 11:16:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-26 11:17:08]

Patchset 4 (id:??) landed as
https://crrev.com/299f775cf4a936c82e0eead5342defe85d312507
Cr-Commit-Position: refs/heads/master@{#30379}