move constructors to new style callbacks

move constructors to new style callbacks

R=jochen@chromium.org
BUG=243656
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151239

[jochen (gone - plz use gerrit), 2013-05-23 14:20:19]

lgtm

you'll need to update the bindings test baselines, no?

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
Source/bindings/scripts/CodeGeneratorV8.pm:2265: if ("old" eq $style) {
nit $style eq "old"

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
Source/bindings/scripts/CodeGeneratorV8.pm:2293: my $isOld = "old" eq $style;
same here

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
Source/bindings/scripts/CodeGeneratorV8.pm:2471: 
why the empty line?

[haraken, 2013-05-23 14:39:36]

What is the overall plan of the replacement?

[dcarney, 2013-05-23 14:44:21]

On 2013/05/23 14:39:36, haraken wrote:
> What is the overall plan of the replacement?

the old style callbacks will be deprecated and removed in v8
the new style callbacks allow direct return from Persistent

[haraken, 2013-05-23 15:04:38]

Chatted with jochen@ offline and understood the rationale for the change. It
would be better if you could describe the rationale in the "Description" or some
meta bug so that others can follow your changes. (e.g. kojih@ and nbarth@ are
also actively working on cleaning up/rewriting V8 bindings.)

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
Source/bindings/scripts/CodeGeneratorV8.pm:2774: static void
${v8ClassName}ConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>&
args)

Are you going to use FunctionCallbackInfo for DOM attributes as well? The
difference between AccessorInfo and Arguments has complicated the current V8
bindings. It would be better if we can unify them.

https://chromiumcodereview.appspot.com/15877002/diff/1/Source/bindings/script...
Source/bindings/scripts/CodeGeneratorV8.pm:2833:
args.GetReturnValue().Set(wrapper);

Can't we write this as args.SetReturnValue(wrapper)?

[haraken, 2013-05-24 01:33:00]

Correct me if I'm wrong: In my understanding, this change is not related to
killing copy constructors of persistents. This change is for performance, i.e.
it allows us to return handles to V8 without creating new local handles.

How much will it improve performance (for example for Dromaeo)? (I don't intend
to focus on Dromaeo performance. I just want to confirm that the optimization
has impacts on micro benchmarks at least.)

[jochen (gone - plz use gerrit), 2013-05-24 06:55:31]

On 2013/05/24 01:33:00, haraken wrote:
> Correct me if I'm wrong: In my understanding, this change is not related to
> killing copy constructors of persistents. This change is for performance, i.e.
> it allows us to return handles to V8 without creating new local handles.

it's related to killing the common base class between local and persistent.

If you return a weak persistent handle from a callback, there's nothing that
guarantees that the object is still alive by the time it's used. To be on the
safe side, you first have to create a strong reference, i.e. a local handle.

Now creating a local handle instead of just casting the persistent to handle is
a performance hit.

This change is needed to avoid the performance hit. The goal is that this is
performance neutral - but in contrast to the old callback style actually
correct.

> How much will it improve performance (for example for Dromaeo)? (I don't
intend
> to focus on Dromaeo performance. I just want to confirm that the optimization
> has impacts on micro benchmarks at least.)

it should be performance neutral

[haraken, 2013-05-24 07:00:59]

> Now creating a local handle instead of just casting the persistent to handle
is
> a performance hit.
> This change is needed to avoid the performance hit. The goal is that this is
> performance neutral - but in contrast to the old callback style actually
> correct.

Thanks for the clarification. Makes great sense!

[dcarney, 2013-05-24 08:07:32]

The first series of changes will be performance neutral, but after we are using
the new style, I will continue to change the return paths to have direct return
for primitive values, eliminating the small integer cache, which should be a win
in some places.

[dcarney, 2013-05-24 08:11:47]

https://codereview.chromium.org/15877002/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15877002/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:2774: static void
${v8ClassName}ConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>&
args)
On 2013/05/23 15:04:38, haraken wrote:
> 
> Are you going to use FunctionCallbackInfo for DOM attributes as well?

No.

Unification is not easy, since they are very different structures, but they have
a similar interfaces, which means they can be passed easily to templates that
use that interface, as is done in toV8Fast.  There is no reason the differences
should complicate bindings much.

https://codereview.chromium.org/15877002/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:2833:
args.GetReturnValue().Set(wrapper);
On 2013/05/23 15:04:38, haraken wrote:
> 
> Can't we write this as args.SetReturnValue(wrapper)?

The ReturnValue is mean to be stack passable, and has 5 or 6 Set functions.  We
wouldn't get much out of copying all the setters to FunctionCallbackInfo and
PropertyCallbackInfo

[haraken, 2013-05-24 08:46:30]

> > Are you going to use FunctionCallbackInfo for DOM attributes as well?
> 
> No.
> 
> Unification is not easy, since they are very different structures, but they
have
> a similar interfaces, which means they can be passed easily to templates that
> use that interface, as is done in toV8Fast.  There is no reason the
differences
> should complicate bindings much.

Makes sense.

>
https://codereview.chromium.org/15877002/diff/1/Source/bindings/scripts/CodeG...
> Source/bindings/scripts/CodeGeneratorV8.pm:2833:
> args.GetReturnValue().Set(wrapper);
> On 2013/05/23 15:04:38, haraken wrote:
> > 
> > Can't we write this as args.SetReturnValue(wrapper)?
> 
> The ReturnValue is mean to be stack passable, and has 5 or 6 Set functions. 
We
> wouldn't get much out of copying all the setters to FunctionCallbackInfo and
> PropertyCallbackInfo

hmm, I'm neutral to this. Rather than writing .ReturnValue.Set() n times in
embedders, I'd prefer duplicating the 8 Set() APIs in v8.h. (though I don't know
the V8 team likes the idea.)

[commit-bot: I haz the power, 2013-05-28 07:03:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcarney@chromium.org/15877002/19001

[commit-bot: I haz the power, 2013-05-28 09:27:32]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[haraken, 2013-05-28 09:33:07]

> args.GetReturnValue().Set(wrapper);

Bikeshedding: This idiom sounds very VM-ish to me (i.e. Let's get a slot for a
return value, and then set a value on the slot. V8 bindings are for web platform
developers.) Even if we don't want to add V8 APIs for args.SetReturnValue(), we
might want to add helper functions in V8Binding.h so that we can write this like
setReturnValue(args, wrapper).

[dcarney, 2013-05-28 10:41:32]

Committed patchset #3 manually as r151239 (presubmit successful).