Adds the possibility of setting a Code object as the callback of a FunctionTemplate.

Adds the possibility of setting a Code object as the callback of a FunctionTemplate.

BUG=

Committed: https://crrev.com/4307e448997d4a5ab6f4f0bd1216e7910177fa3f
Cr-Commit-Position: refs/heads/master@{#32213}

[vogelheim, 2015-11-10 00:44:23]

vogelheim@chromium.org changed reviewers:
+ vogelheim@chromium.org

[vogelheim, 2015-11-10 00:44:24]

This looks really nice; certainly much nicer than my initial stab at that
problem.

The only problem I see is that the new v8::FunctionTemplate::New overload should
have a more specific signature than Local<Value>; but since the suitable type
does not yet exists, I think that's a TODO for when I actually write the
AccessorBuilder thingy.

[epertoso, 2015-11-10 17:36:17]

On 2015/11/10 at 00:44:24, vogelheim wrote:
> This looks really nice; certainly much nicer than my initial stab at that
problem.
> 
> The only problem I see is that the new v8::FunctionTemplate::New overload
should have a more specific signature than Local<Value>; but since the suitable
type does not yet exists, I think that's a TODO for when I actually write the
AccessorBuilder thingy.

Added a TODO.

[epertoso, 2015-11-10 17:36:36]

epertoso@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-11-10 19:52:35]

can you split the revert of the old api and the addition of the new api in two
CLs?

[epertoso, 2015-11-10 20:40:19]

On 2015/11/10 at 19:52:35, jochen wrote:
> can you split the revert of the old api and the addition of the new api in two
CLs?

http://crrev.com/1434693008

[epertoso, 2015-11-10 22:05:08]

Patchset #3 (id:40001) has been deleted

[epertoso, 2015-11-11 01:22:04]

Description was changed from

==========
Adds the possibility of adding a Code object as the callback of a
FunctionTemplate.

BUG=
==========

to

==========
Adds the possibility of setting a Code object as the callback of a
FunctionTemplate.

BUG=
==========

[jochen (gone - plz use gerrit), 2015-11-11 07:55:25]

lgtm

[epertoso, 2015-11-11 17:36:03]

epertoso@chromium.org changed reviewers:
+ bmeurer@chromium.org

[epertoso, 2015-11-11 17:37:08]

epertoso@chromium.org changed required reviewers:
+ bmeurer@chromium.org

[epertoso, 2015-11-11 17:37:09]

Benedikt, could you take a look at this CL and see if there's anything wrong
with it before I go ahead with the implementation for the other archs?

[Benedikt Meurer, 2015-11-12 04:31:20]

bmeurer@chromium.org changed reviewers:
+ verwaest@chromium.org

[Benedikt Meurer, 2015-11-12 04:31:20]

bmeurer@chromium.org changed required reviewers:
+ verwaest@chromium.org

[Benedikt Meurer, 2015-11-12 04:31:21]

We should probably consider wrapping the Code object into something Callable or
find another suitable abstraction, otherwise this will become hard to maintain
at some point.

Also the naming is confusing me; why is this called V8_JS_ACCESSORS? And why is
it called JSApiCall? Because the code object can be anything, and doesn't have
to be a JS thing, right? Can we maybe name this FastApiCall / V8_FAST_ACCESSORS
or something like that?

Also Toon should have a look before we move on.

https://codereview.chromium.org/1407313004/diff/60001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/execution.cc#newcod...
src/execution.cc:139: bool is_callback = false;
Please add a TODO here, that this code will need cleanup once we know exactly
what we want in the end.

https://codereview.chromium.org/1407313004/diff/60001/src/ic/x64/handler-comp...
File src/ic/x64/handler-compiler-x64.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/ic/x64/handler-comp...
src/ic/x64/handler-compiler-x64.cc:193: __ Move(rdx,
handle(Code::cast(api_call_info->callback())));
You should be able to use __ Jump(code, RelocInfo::CODE_TARGET) instead.

https://codereview.chromium.org/1407313004/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/objects.cc#newcode7769
src/objects.cc:7769: getter->IsCode());
This will be hard to maintain everywhere. AFAIR there are various places where
we expect getters/setters to be callable (or null/undefined). How about wrapping
the code object into a helper object that is callable?

[epertoso, 2015-11-13 01:39:28]

https://codereview.chromium.org/1407313004/diff/60001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/execution.cc#newcod...
src/execution.cc:139: bool is_callback = false;
On 2015/11/12 at 04:31:21, Benedikt Meurer wrote:
> Please add a TODO here, that this code will need cleanup once we know exactly
what we want in the end.

I refactored the code a bit.

I'm not sure I understand your comment. With this patch, we will have two
possible callbacks: the regular FunctionCallback, and the 'fast' one which is
passed around as a code object. Do you foresee any way in which this might
change?

https://codereview.chromium.org/1407313004/diff/60001/src/ic/x64/handler-comp...
File src/ic/x64/handler-compiler-x64.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/ic/x64/handler-comp...
src/ic/x64/handler-compiler-x64.cc:193: __ Move(rdx,
handle(Code::cast(api_call_info->callback())));
On 2015/11/12 at 04:31:21, Benedikt Meurer wrote:
> You should be able to use __ Jump(code, RelocInfo::CODE_TARGET) instead.

Done.

https://codereview.chromium.org/1407313004/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1407313004/diff/60001/src/objects.cc#newcode7769
src/objects.cc:7769: getter->IsCode());
On 2015/11/12 at 04:31:21, Benedikt Meurer wrote:
> This will be hard to maintain everywhere. AFAIR there are various places where
we expect getters/setters to be callable (or null/undefined). How about wrapping
the code object into a helper object that is callable?

Actually we don't need it, at this point the FunctionTemplateInfo has already
been instantiated and we get a JSFunction. My bad, it was left over from some
previous experiment.

[Benedikt Meurer, 2015-11-13 05:41:25]

So Toon told me that he suggested a LazyAccessorPair mechanism earlier, which
seems to address some of the problems you (will) face, although as far as I
understood, it's also somewhat orthogonal to your concrete problem.

The low level bits look good to me; but for the accessor related stuff, Toon
should definitely do the review and decide how to move forward there.

[Toon Verwaest, 2015-11-13 09:55:49]

I'm glad that the objects.cc change was a stray change :)

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc#newcod...
src/execution.cc:145: !CallHandlerInfo::cast(call_code)->callback()->IsCode()) {
Don't we want the ability to stay in C++ if we are there? Hence, shouldn't "fast
api callbacks" that can stay in JS when we are there, also have the C++ fallback
in case we call from C++? That would obviate this change.

[epertoso, 2015-11-13 19:06:10]

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc#newcod...
src/execution.cc:145: !CallHandlerInfo::cast(call_code)->callback()->IsCode()) {
On 2015/11/13 at 09:55:49, Toon Verwaest wrote:
> Don't we want the ability to stay in C++ if we are there? Hence, shouldn't
"fast api callbacks" that can stay in JS when we are there, also have the C++
fallback in case we call from C++? That would obviate this change.

Well, not necessarily. The fast callback, on the current state of things, is
executed without leaving javascript, but it might bail-out for any reason (e.g.
in the case of Blink, V8 wrappers for DOM nodes are created lazily the first
time they're accessed. The fast path checks that a wrapper is present, if not it
will bailout to C++).

You're correct in saying that in this case we might have up to two more
transitions (from C++ to JS and in the worse case scenario to C++ again).

Daniel, what do you think?

[Benedikt Meurer, 2015-11-16 04:43:56]

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc#newcod...
src/execution.cc:145: !CallHandlerInfo::cast(call_code)->callback()->IsCode()) {
I think there should always be a C++ backup implementation, which is complete;
hence we should just leave the "fast C++ path" here. It's definitely cheaper
than going through JavaScript land first.

[epertoso, 2015-11-17 10:32:04]

Patchset #5 (id:100001) has been deleted

[epertoso, 2015-11-17 10:32:43]

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1407313004/diff/80001/src/execution.cc#newcod...
src/execution.cc:145: !CallHandlerInfo::cast(call_code)->callback()->IsCode()) {
On 2015/11/16 at 04:43:55, Benedikt Meurer wrote:
> I think there should always be a C++ backup implementation, which is complete;
hence we should just leave the "fast C++ path" here. It's definitely cheaper
than going through JavaScript land first.

Done. Added a new field to CallHandlerInfo, as discussed offline.

[Benedikt Meurer, 2015-11-17 10:34:27]

Thanks. LGTM from my side.

[Toon Verwaest, 2015-11-18 08:51:46]

lgtm with nit

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc#new...
src/api-natives.cc:443: !handle(CallHandlerInfo::cast(obj->call_code()))
Why is this wrapped in a handle before calling fast_handler?

[vogelheim, 2015-11-18 17:30:05]

lgtm. More nitpicks, though. :)

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h#newcode4428
include/v8.h:4428: Isolate* isolate, Local<Value> fast_handler, FunctionCallback
callback,
nitpick: Wouldn't it make more sense to have fast_handler be an optional
argument, following callback? That would also be consistent with the
SetCallHandler signature.

I guess then we can't really ambiguate the two overloads, so maybe just make it
an optional parameter for the existing FunctionTemplate::New?

(Decide as you see fit, though. This is #ifdef-ed and trivial to change later
on, when we have a better idea how the API should look like.)

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h#newcode4445
include/v8.h:4445: Local<Value> fast_handler = Local<Value>());
The FunctionTemplate::New version is #ifdef-ed, but SetCallHandler isn't. 
That's kinda weird.

Maybe either provide two overloads, or just #ifdef the implementation [1], or
maybe just #ifdef one parameter [2], like so:

[1]: #ifdef ... #else CRASH() #endif
[2] void SetCallHandler(..., ...
    #ifdef
       , Local<Value> ...
    #endif

Not sure what our default strategy for this kind of thing is...

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc#new...
src/api-natives.cc:445: ->IsUndefined()) {
nitpick: Why !..->IsUndefined(), instead of ...->IsCode() ?

(They do the same thing, it's just with the negation being 2 lines above the
thing to be negated I at first just overlooked it.)

[epertoso, 2015-11-20 15:27:14]

Resolved the comments and added a few other archs.

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h#newcode4428
include/v8.h:4428: Isolate* isolate, Local<Value> fast_handler, FunctionCallback
callback,
On 2015/11/18 at 17:30:05, vogelheim wrote:
> nitpick: Wouldn't it make more sense to have fast_handler be an optional
argument, following callback? That would also be consistent with the
SetCallHandler signature.
> 
> I guess then we can't really ambiguate the two overloads, so maybe just make
it an optional parameter for the existing FunctionTemplate::New?
> 
> (Decide as you see fit, though. This is #ifdef-ed and trivial to change later
on, when we have a better idea how the API should look like.)

Resolved according to the offline discussion we had.

https://codereview.chromium.org/1407313004/diff/120001/include/v8.h#newcode4445
include/v8.h:4445: Local<Value> fast_handler = Local<Value>());
On 2015/11/18 at 17:30:05, vogelheim wrote:
> The FunctionTemplate::New version is #ifdef-ed, but SetCallHandler isn't. 
That's kinda weird.
> 
> Maybe either provide two overloads, or just #ifdef the implementation [1], or
maybe just #ifdef one parameter [2], like so:
> 
> [1]: #ifdef ... #else CRASH() #endif
> [2] void SetCallHandler(..., ...
>     #ifdef
>        , Local<Value> ...
>     #endif
> 
> Not sure what our default strategy for this kind of thing is...

Removed the #ifdefs.

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc
File src/api-natives.cc (right):

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc#new...
src/api-natives.cc:443: !handle(CallHandlerInfo::cast(obj->call_code()))
On 2015/11/18 at 08:51:46, Toon Verwaest wrote:
> Why is this wrapped in a handle before calling fast_handler?

Removed.

https://codereview.chromium.org/1407313004/diff/120001/src/api-natives.cc#new...
src/api-natives.cc:445: ->IsUndefined()) {
On 2015/11/18 at 17:30:05, vogelheim wrote:
> nitpick: Why !..->IsUndefined(), instead of ...->IsCode() ?
> 
> (They do the same thing, it's just with the negation being 2 lines above the
thing to be negated I at first just overlooked it.)

Done.

[epertoso, 2015-11-24 10:40:24]

Patchset #7 (id:160001) has been deleted

[epertoso, 2015-11-24 10:48:29]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2015-11-24 10:48:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
bmeurer@chromium.org, verwaest@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1407313004/#ps180001
(title: "Update.")

[epertoso, 2015-11-24 10:48:32]

The CQ bit was unchecked by epertoso@chromium.org

[epertoso, 2015-11-24 10:48:46]

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

[commit-bot: I haz the power, 2015-11-24 10:49:01]

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

[commit-bot: I haz the power, 2015-11-24 11:30:42]

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

[commit-bot: I haz the power, 2015-11-24 11:30:43]

Dry run: This issue passed the CQ dry run.

[epertoso, 2015-11-24 13:57:37]

bmeurer@, verwaest@: does this still LGTY?

[Benedikt Meurer, 2015-11-24 14:05:10]

Yep.

[Toon Verwaest, 2015-11-24 14:29:52]

lgtm

[epertoso, 2015-11-24 14:30:53]

The CQ bit was checked by epertoso@chromium.org

[commit-bot: I haz the power, 2015-11-24 14:31:02]

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

[commit-bot: I haz the power, 2015-11-24 14:32:45]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-11-24 14:33:28]

Patchset 7 (id:??) landed as
https://crrev.com/4307e448997d4a5ab6f4f0bd1216e7910177fa3f
Cr-Commit-Position: refs/heads/master@{#32213}