JS binding: remove redundant type check in attr setter for TypeChecking interface

JS binding: remove redundant type check in attr setter for TypeChecking interface

A [TypeChecking=interface] attribute has been checked already in its setter. 
If it is not nullable, just call toImpl instead of toImplWithTypeCheck to avoid 
redundant type check.

This change can improve performance and save power.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183717

[yunchao, 2014-10-13 07:55:38]

yunchao.he@intel.com changed reviewers:
+ haraken@chromium.org, jl@opera.com

[yunchao, 2014-10-13 07:56:42]

A fix like https://codereview.chromium.org/608853008/. 

PTAL. Thanks!

[Jens Widell, 2014-10-13 10:40:24]

LGTM

AFAICT, this only affects CanvasRenderingContext2D.currentTransform and
AudioBufferSourceNode.buffer. Since this just extends use of a mechanism we
already have, seems fine to do it even though currently it doesn't really
matter.

It occurs to me now that there's an alternative way of implementing this
optimization:

  Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
  if (!cppValue) {
    // throw error
  }

This could be implemented in the templates only, with no need to figure out in
advance whether the optimization is valid, and no risk of crashing in Cast() or
doing invalid C++ casts in toImpl() if we get it wrong.

[haraken, 2014-10-13 15:06:41]

On 2014/10/13 10:40:24, Jens Widell wrote:
> LGTM
> 
> AFAICT, this only affects CanvasRenderingContext2D.currentTransform and
> AudioBufferSourceNode.buffer. Since this just extends use of a mechanism we
> already have, seems fine to do it even though currently it doesn't really
> matter.
> 
> It occurs to me now that there's an alternative way of implementing this
> optimization:
> 
>   Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
>   if (!cppValue) {
>     // throw error
>   }
> 
> This could be implemented in the templates only, with no need to figure out in
> advance whether the optimization is valid, and no risk of crashing in Cast()
or
> doing invalid C++ casts in toImpl() if we get it wrong.

Sounds nicer.

Also it would be helpful to add ASSERT(V8T::hasInstance()) to toImpl() to verify
that toImpl() doesn't operate on incorrect type of objects.

[yunchao, 2014-10-14 02:49:56]

On 2014/10/13 15:06:41, haraken wrote:
> On 2014/10/13 10:40:24, Jens Widell wrote:
> > LGTM
> > 
> > AFAICT, this only affects CanvasRenderingContext2D.currentTransform and
> > AudioBufferSourceNode.buffer. Since this just extends use of a mechanism we
> > already have, seems fine to do it even though currently it doesn't really
> > matter.
> > 
> > It occurs to me now that there's an alternative way of implementing this
> > optimization:
> > 
> >   Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
> >   if (!cppValue) {
> >     // throw error
> >   }
> > 
> > This could be implemented in the templates only, with no need to figure out
in
> > advance whether the optimization is valid, and no risk of crashing in Cast()
> or
> > doing invalid C++ casts in toImpl() if we get it wrong.
> 
> Sounds nicer.
> 
> Also it would be helpful to add ASSERT(V8T::hasInstance()) to toImpl() to
verify
> that toImpl() doesn't operate on incorrect type of objects.

jl@ and haraken@, thanks for your suggestion. I suppose that your proposal is
that don't call hasInstance for [TypeChecking=Interface] attributes and
arguments before toImplWithTypeCheck. Just call toImplWithTypeCheck, and raise
exception if necessary. 

The original one:
if (!V8Foo::hasInstance(v8Value)) {
    // throw error
}
Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value)

My change:
if (!V8Foo::hasInstance(v8Value)) {
    // throw error
}
Foo* cppValue = V8Foo::toImpl(v8Handle<V8::Object>::Case(v8Value));

Your proposal:
Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
if (!cppValue) {    
    // throw error
}

(please correct me if I get you wrong)

I also considered the last one you proposed. But you know that there is another
patch at https://codereview.chromium.org/611953003/. For
[TypeChecking=Interface] methods and attributes, It is sure that type check has
been done by either overloading (who will call hasInstance() to distinguish
among variants) or this code snippet:
if (!V8Foo::hasInstance(v8Value)) {
    // throw error
}
So It is safe to call toImpl instead of toImplWithTypeCheck.

Because overloading is a must, who may have called hasInstance() already, so I
prefer to keep toImpl. Just let overloading or hasInstance to do checking work
for *all* [TypeChecking=Interface] attributes and arguments. Don't mixed all
these three together.

(Well, you may think it is a corner case for JS binding that overloading,
[TypeChecking=Interface] and toImplWithTypeCheck all exist, but canvas.drawImage
is really an important API. Without drawImage (and good performance of
drawImage), the <canvas> tag is nonsense. )

[Jens Widell, 2014-10-14 06:41:09]

On 2014/10/14 02:49:56, yunchao wrote:
> On 2014/10/13 15:06:41, haraken wrote:
> > On 2014/10/13 10:40:24, Jens Widell wrote:
> > > LGTM
> > > 
> > > AFAICT, this only affects CanvasRenderingContext2D.currentTransform and
> > > AudioBufferSourceNode.buffer. Since this just extends use of a mechanism
we
> > > already have, seems fine to do it even though currently it doesn't really
> > > matter.
> > > 
> > > It occurs to me now that there's an alternative way of implementing this
> > > optimization:
> > > 
> > >   Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
> > >   if (!cppValue) {
> > >     // throw error
> > >   }
> > > 
> > > This could be implemented in the templates only, with no need to figure
out
> in
> > > advance whether the optimization is valid, and no risk of crashing in
Cast()
> > or
> > > doing invalid C++ casts in toImpl() if we get it wrong.
> > 
> > Sounds nicer.
> > 
> > Also it would be helpful to add ASSERT(V8T::hasInstance()) to toImpl() to
> verify
> > that toImpl() doesn't operate on incorrect type of objects.
> 
> jl@ and haraken@, thanks for your suggestion. I suppose that your proposal is
> that don't call hasInstance for [TypeChecking=Interface] attributes and
> arguments before toImplWithTypeCheck. Just call toImplWithTypeCheck, and raise
> exception if necessary. 
> 
> The original one:
> if (!V8Foo::hasInstance(v8Value)) {
>     // throw error
> }
> Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value)
> 
> My change:
> if (!V8Foo::hasInstance(v8Value)) {
>     // throw error
> }
> Foo* cppValue = V8Foo::toImpl(v8Handle<V8::Object>::Case(v8Value));
> 
> Your proposal:
> Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
> if (!cppValue) {    
>     // throw error
> }
> 
> (please correct me if I get you wrong)

That's correct.

> I also considered the last one you proposed. But you know that there is
another
> patch at https://codereview.chromium.org/611953003/. For
> [TypeChecking=Interface] methods and attributes, It is sure that type check
has
> been done by either overloading (who will call hasInstance() to distinguish
> among variants) or this code snippet:
> if (!V8Foo::hasInstance(v8Value)) {
>     // throw error
> }
> So It is safe to call toImpl instead of toImplWithTypeCheck.
> 
> Because overloading is a must, who may have called hasInstance() already, so I
> prefer to keep toImpl. Just let overloading or hasInstance to do checking work
> for *all* [TypeChecking=Interface] attributes and arguments. Don't mixed all
> these three together.
> 
> (Well, you may think it is a corner case for JS binding that overloading,
> [TypeChecking=Interface] and toImplWithTypeCheck all exist, but
canvas.drawImage
> is really an important API. Without drawImage (and good performance of
> drawImage), the <canvas> tag is nonsense. )

Keeping the broader, non-overloading fix simple(r) is worth-while even if we
reintroduce the complexity specifically for the overloading case, I think. And
they don't conflict; in the template, the simplified optimization would still be

  Foo* cppValue = {{something-from-the-Python-script}};
  {% if type-checking-interface and not
type-already-checked-by-overload-resolution %}
  if (!cppValue) {
    {{throw-error}}
  }
  {% endif %}

so the overloading fix would (still) just need to affect the
something-from-the-Python-script and type-already-checked-by-overload-resolution
parts. IOW, the overloading fix's impact is the same, but the non-overloading
fix becomes simpler.

Or, put differently: We don't need to keep the non-overloading fix complicated
just because the overloading fix will be complicated.

Also, I'm not particularly happy about the overloading fix. I tried the same
thing myself a while back, and ended up not happy about it then either. It seems
nice and simple at first, but inevitably turns out a bit too intricate for
comfort. As Nils suggested in that CL, union types would largely that problem
for us too, without introducing complexity just for the sake of optimizing.

[yunchao, 2014-10-14 07:31:12]

On 2014/10/14 06:41:09, Jens Widell wrote:
> On 2014/10/14 02:49:56, yunchao wrote:
> > On 2014/10/13 15:06:41, haraken wrote:
> > > On 2014/10/13 10:40:24, Jens Widell wrote:
> > > > LGTM
> > > > 
> > > > AFAICT, this only affects CanvasRenderingContext2D.currentTransform and
> > > > AudioBufferSourceNode.buffer. Since this just extends use of a mechanism
> we
> > > > already have, seems fine to do it even though currently it doesn't
really
> > > > matter.
> > > > 
> > > > It occurs to me now that there's an alternative way of implementing this
> > > > optimization:
> > > > 
> > > >   Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
> > > >   if (!cppValue) {
> > > >     // throw error
> > > >   }
> > > > 
> > > > This could be implemented in the templates only, with no need to figure
> out
> > in
> > > > advance whether the optimization is valid, and no risk of crashing in
> Cast()
> > > or
> > > > doing invalid C++ casts in toImpl() if we get it wrong.
> > > 
> > > Sounds nicer.
> > > 
> > > Also it would be helpful to add ASSERT(V8T::hasInstance()) to toImpl() to
> > verify
> > > that toImpl() doesn't operate on incorrect type of objects.
> > 
> > jl@ and haraken@, thanks for your suggestion. I suppose that your proposal
is
> > that don't call hasInstance for [TypeChecking=Interface] attributes and
> > arguments before toImplWithTypeCheck. Just call toImplWithTypeCheck, and
raise
> > exception if necessary. 
> > 
> > The original one:
> > if (!V8Foo::hasInstance(v8Value)) {
> >     // throw error
> > }
> > Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value)
> > 
> > My change:
> > if (!V8Foo::hasInstance(v8Value)) {
> >     // throw error
> > }
> > Foo* cppValue = V8Foo::toImpl(v8Handle<V8::Object>::Case(v8Value));
> > 
> > Your proposal:
> > Foo* cppValue = V8Foo::toImplWithTypeCheck(v8Value);
> > if (!cppValue) {    
> >     // throw error
> > }
> > 
> > (please correct me if I get you wrong)
> 
> That's correct.
> 
> > I also considered the last one you proposed. But you know that there is
> another
> > patch at https://codereview.chromium.org/611953003/. For
> > [TypeChecking=Interface] methods and attributes, It is sure that type check
> has
> > been done by either overloading (who will call hasInstance() to distinguish
> > among variants) or this code snippet:
> > if (!V8Foo::hasInstance(v8Value)) {
> >     // throw error
> > }
> > So It is safe to call toImpl instead of toImplWithTypeCheck.
> > 
> > Because overloading is a must, who may have called hasInstance() already, so
I
> > prefer to keep toImpl. Just let overloading or hasInstance to do checking
work
> > for *all* [TypeChecking=Interface] attributes and arguments. Don't mixed all
> > these three together.
> > 
> > (Well, you may think it is a corner case for JS binding that overloading,
> > [TypeChecking=Interface] and toImplWithTypeCheck all exist, but
> canvas.drawImage
> > is really an important API. Without drawImage (and good performance of
> > drawImage), the <canvas> tag is nonsense. )
> 
> Keeping the broader, non-overloading fix simple(r) is worth-while even if we
> reintroduce the complexity specifically for the overloading case, I think. And
> they don't conflict; in the template, the simplified optimization would still
be
> 
>   Foo* cppValue = {{something-from-the-Python-script}};
>   {% if type-checking-interface and not
> type-already-checked-by-overload-resolution %}
>   if (!cppValue) {
>     {{throw-error}}
>   }
>   {% endif %}
> 
> so the overloading fix would (still) just need to affect the
> something-from-the-Python-script and
type-already-checked-by-overload-resolution
> parts. IOW, the overloading fix's impact is the same, but the non-overloading
> fix becomes simpler.
> 
> Or, put differently: We don't need to keep the non-overloading fix complicated
> just because the overloading fix will be complicated.
> 

Hi, Jens, thank you very much. I know that these two solutions are both OK.
Another reason I don't do it in that way is because python script used to select
between toImplWithTypeCheck and toImpl is called in attribute_context and
method_context, who is done before overload resolution. So it doesn't know the
status of overload resolution. 

So, we need to do overload resolution before attribute_context and
method_context. However, overload resolution itself depend on information from
attribute_context and method_context. (Maybe it is also feasible, but it seems
to be complicated). And casting an object is trivial. It is not a hotspot at all
from Linux perf tool. So I prefer to this solution. What do you think? 

> Also, I'm not particularly happy about the overloading fix. I tried the same
> thing myself a while back, and ended up not happy about it then either. It
seems
> nice and simple at first, but inevitably turns out a bit too intricate for
> comfort. As Nils suggested in that CL, union types would largely that problem
> for us too, without introducing complexity just for the sake of optimizing.

Understood. I just find it from the hotspots reported by perf from benchmarks
and try to fix it. Maybe union type is indeed a better solution. Anyway, we hope
that chrome and chrome on android are competitive projects/products, we all try
to contribute and promote them.

[Jens Widell, 2014-10-14 07:43:18]

On 2014/10/14 07:31:12, yunchao wrote:
> On 2014/10/14 06:41:09, Jens Widell wrote:
> > Also, I'm not particularly happy about the overloading fix. I tried the same
> > thing myself a while back, and ended up not happy about it then either. It
> seems
> > nice and simple at first, but inevitably turns out a bit too intricate for
> > comfort. As Nils suggested in that CL, union types would largely that
problem
> > for us too, without introducing complexity just for the sake of optimizing.
> 
> Understood. I just find it from the hotspots reported by perf from benchmarks
> and try to fix it. Maybe union type is indeed a better solution. Anyway, we
hope
> that chrome and chrome on android are competitive projects/products, we all
try
> to contribute and promote them.

And your effort is greatly appreciated!

I of course agree that we should generate efficient code, but at the same time
the code generator needs to be understandable and maintainable as well, or we
risk making it harder to generate good code in the future, or, indeed, even to
generate correct code.

[yunchao, 2014-10-14 07:47:54]

On 2014/10/14 07:43:18, Jens Widell wrote: 
> I of course agree that we should generate efficient code, but at the same time
> the code generator needs to be understandable and maintainable as well, or we
> risk making it harder to generate good code in the future, or, indeed, even to
> generate correct code.

Totally agree!

[yunchao, 2014-10-14 09:30:06]

Hi, jl@ and haraken@, if no objection, I will commit this change tomorrow.

[yunchao, 2014-10-15 02:49:10]

The CQ bit was checked by yunchao.he@intel.com

[commit-bot: I haz the power, 2014-10-15 02:49:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654473002/200001

[commit-bot: I haz the power, 2014-10-15 04:27:48]

Committed patchset #3 (id:200001) as 183717