Canvas2D Performance: fix the bottleneck of hasInstance during JS binding -- overloading

Canvas2D Performance: fix the bottleneck of hasInstance during JS binding -- overloading

If web app invokes hundreds of drawImage per frame,  hasInstance(called by drawImageMethod) 
will become a bottleneck. Here is test data from content shell on Nexus7 by Linux perf tool:
benchmark                              ratio that hasInstance costs in the whole renderer
my local benchmark(draw 1000 sprites)             6.9%
speedReading                                      4.4%
FishIETank(1000 fishes)                           2.6%
GUIMark3 Bitmap                                   4.0%

drawImageMethod is in out/Release/gen/blink/bindings/core/v8/V8CanvasRenderingContext2D.cpp, 
which is an auto-generated file. It will call hasInstance(eg V8HTMLImageElement::hasInstance) 
3 times for every call to drawImage. One is in drawImageMethod directly, the other two are in 
drawImage*Method, say drawImage1Method. The 2nd call site is redundant if type check has been 
done by overloading during JS binding. 

This change removes the 2nd call of hasInstance in drawImageMethod to fix the performance 
bottleneck. It also benefits similar cases during JS binding. 

This change improves performance and/or saves power.

BUG=416393

[yunchao, 2014-09-29 09:43:11]

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

[yunchao, 2014-09-29 09:43:49]

PTAL. Thanks a lot!

[Jens Widell, 2014-09-29 09:50:58]

Quick comment: Please run

  $ Tools/Scripts/run-bindings-tests --reset-results

and add the modifications it does to the patch.

[yunchao, 2014-09-29 09:58:05]

On 2014/09/29 09:50:58, Jens Widell wrote:
> Quick comment: Please run
> 
>   $ Tools/Scripts/run-bindings-tests --reset-results
> 
> and add the modifications it does to the patch.

Oh... The document about JS binding told me this... my fault. 
Thank you, Jens.

[Jens Widell, 2014-09-29 10:04:46]

If I may make a suggestion: Split this CL into two; one which addresses the case
of

  if (!V8SomeInterface::hasInstance(...) {
    /* throw error */
  }
  arg = V8SomeInterface::toImplWithTypeCheck(...);

issue (where toImplWithTypeCheck() doesn't need to be used) and one which
addresses the case of overloaded methods (where the overload resolution
sometimes will have done all the necessary type-checking.)

The latter is quite a bit more complicated, and also narrower and thus less
important (since far from all methods are overloaded.)

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_i...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:779: argument['type_checked_already'] =
True
This is not correct. Only an argument that is directly used for overload
resolution will be type checked by the overload resolution functions.

An argument is "directly used" if it's the last argument for some value of
"number of arguments" with which the method can legally be called, and for which
there is at least one other overload that can also be legally called. Consider
for instance

  f(SomeInterface a)
  f(long x, long y)

Here, the 'a' argument is never type-checked by the overload resolution, because
if f() is called with one argument, f(SomeInterface a) is the only viable
candidate. In the similar case

  f(SomeInterface a)
  f(long x, optional long y)

the 'a' argument is type-checked. And in the case

  f(SomeInterface a, SomeInterface b)
  f(SomeInterface a, long x)

the 'b' argument to the first overload will be checked, but the 'a' argument to
either overload will never have been checked.

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:239: 'type_checked_already': False,
Nit: Keep this list alphabetically sorted.

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/611953003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:537:
'V8{idl_type}::toImpl(v8::Handle<v8::Object>::Cast({v8_value}))')
So what you did here is remove the only type-check for a whole lot of arguments.
That's not a valid change.

This is only valid if the argument was already type-checked because of
[TypeChecking=Interface] *or* if the argument took part in overload resolution
(and was thus type-checked by the overload resolution function.)

And even then, it's also sometimes possible for the argument to be null or
undefined, in which case the cast here is still invalid.

[Jens Widell, 2014-09-29 10:24:22]

On 2014/09/29 10:04:46, Jens Widell wrote:
> The latter is quite a bit more complicated, and also narrower and thus less
> important (since far from all methods are overloaded.)

BTW, long ago I submitted a patch that essentially took care of the latter, but
which was rejected/dropped: https://codereview.chromium.org/289843006/

The comments on that issue contains some information that's relevant.

[yunchao, 2014-09-30 09:17:29]

Quick comment: Thanks for your review, Jens. I will submit a new patch set
tomorrow.

https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
resolution_tests_methods has filtered a lot of arguments before overloads, or
returned early in this function if only one argument that is directly used for
overload resolution. So the arguments in the cases your listed  are not in the
'arguments' here. They will not be marked as 'type_checked_already'.

[Jens Widell, 2014-09-30 09:38:22]

https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
On 2014/09/30 09:17:29, yunchao wrote:
> resolution_tests_methods has filtered a lot of arguments before overloads, or
> returned early in this function if only one argument that is directly used for
> overload resolution. So the arguments in the cases your listed  are not in the
> 'arguments' here. They will not be marked as 'type_checked_already'. 

Ah, yes, that's true. Note though that this function can be called multiple
times with the same method in the |effective_overloads| list, and that it thus
can end up setting 'type_checked_already' to true for multiple arguments of that
one method.

In the case

  f(long a)
  f(SomeInterface b, optional SomeInterface c, optional SomeInterface d)
  f(SomeInterface e, long f)

for instance, resolution_tests_methods() will be called once for the
len(argv)==1 case, and will consider the first two, using their first arguments,
and mark |b| as type checked. Then it will be called again for the len(argv)==2
case, and will then consider the last two, using their second arguments, and
mark |c| as type checked. (It will also be called once for the len(argv)==3
case, but then it stops early, having only one viable candidate.)

So both of the second method's arguments would be marked as type checked, but
when it's actually called, at most one of the arguments will actually have been
type checked. None of them will have been type checked if it's called with three
arguments.

[yunchao, 2014-10-02 10:46:19]

On 2014/09/30 09:38:22, Jens Widell wrote:
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_interface.py (right):
> 
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
> On 2014/09/30 09:17:29, yunchao wrote:
> > resolution_tests_methods has filtered a lot of arguments before overloads,
or
> > returned early in this function if only one argument that is directly used
for
> > overload resolution. So the arguments in the cases your listed  are not in
the
> > 'arguments' here. They will not be marked as 'type_checked_already'. 
> 
> Ah, yes, that's true. Note though that this function can be called multiple
> times with the same method in the |effective_overloads| list, and that it thus
> can end up setting 'type_checked_already' to true for multiple arguments of
that
> one method.
> 
> In the case
> 
>   f(long a)
>   f(SomeInterface b, optional SomeInterface c, optional SomeInterface d)
>   f(SomeInterface e, long f)
> 
> for instance, resolution_tests_methods() will be called once for the
> len(argv)==1 case, and will consider the first two, using their first
arguments,
> and mark |b| as type checked. Then it will be called again for the
len(argv)==2
> case, and will then consider the last two, using their second arguments, and
> mark |c| as type checked. (It will also be called once for the len(argv)==3
> case, but then it stops early, having only one viable candidate.)
> 
> So both of the second method's arguments would be marked as type checked, but
> when it's actually called, at most one of the arguments will actually have
been
> type checked. None of them will have been type checked if it's called with
three
> arguments.

Thank you very much, Jens. I was aware of this case too. There is an interface
::clip() in CanvasRenderingContext2D.idl. It has the same problem. So I want to
do a conservative optimization, I just remove hasInstance if there is no
optional argument(s) in overload methods. 

As you know, I found this bottleneck in CanvasRenderingContext2D::drawImage.
This API calls hasInstance 3 times. And It has no optional argument(s) at all.
This is an easy case, and the original auto-generated code does hurt performance
in many real cases and benchmarks of Canvas2D. This change can remove the
bottleneck.

As a result, for wrapper type arguments: 
1) some are checked because of overloading. 
2) If the method is marked as typecheck, we only check those arguments who are
not checked by overloading. For security reason, we will check all wrapper type
arguments if the method has optional argument(s). 
3) If the method is marked as typecheck, we use toImpl directly, all arguments
has been typechecked in 1) and/or 2). Otherwise, use toImplWithTypeCheck to do
typecheck.

What do you think?

[Jens Widell, 2014-10-02 10:51:41]

On 2014/10/02 10:46:19, yunchao wrote:
> On 2014/09/30 09:38:22, Jens Widell wrote:
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > File Source/bindings/scripts/v8_interface.py (right):
> > 
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
> > On 2014/09/30 09:17:29, yunchao wrote:
> > > resolution_tests_methods has filtered a lot of arguments before overloads,
> or
> > > returned early in this function if only one argument that is directly used
> for
> > > overload resolution. So the arguments in the cases your listed  are not in
> the
> > > 'arguments' here. They will not be marked as 'type_checked_already'. 
> > 
> > Ah, yes, that's true. Note though that this function can be called multiple
> > times with the same method in the |effective_overloads| list, and that it
thus
> > can end up setting 'type_checked_already' to true for multiple arguments of
> that
> > one method.
> > 
> > In the case
> > 
> >   f(long a)
> >   f(SomeInterface b, optional SomeInterface c, optional SomeInterface d)
> >   f(SomeInterface e, long f)
> > 
> > for instance, resolution_tests_methods() will be called once for the
> > len(argv)==1 case, and will consider the first two, using their first
> arguments,
> > and mark |b| as type checked. Then it will be called again for the
> len(argv)==2
> > case, and will then consider the last two, using their second arguments, and
> > mark |c| as type checked. (It will also be called once for the len(argv)==3
> > case, but then it stops early, having only one viable candidate.)
> > 
> > So both of the second method's arguments would be marked as type checked,
but
> > when it's actually called, at most one of the arguments will actually have
> been
> > type checked. None of them will have been type checked if it's called with
> three
> > arguments.
> 
> Thank you very much, Jens. I was aware of this case too. There is an interface
> ::clip() in CanvasRenderingContext2D.idl. It has the same problem. So I want
to
> do a conservative optimization, I just remove hasInstance if there is no
> optional argument(s) in overload methods. 
> 
> As you know, I found this bottleneck in CanvasRenderingContext2D::drawImage.
> This API calls hasInstance 3 times. And It has no optional argument(s) at all.
> This is an easy case, and the original auto-generated code does hurt
performance
> in many real cases and benchmarks of Canvas2D. This change can remove the
> bottleneck.
> 
> As a result, for wrapper type arguments: 
> 1) some are checked because of overloading. 
> 2) If the method is marked as typecheck, we only check those arguments who are
> not checked by overloading. For security reason, we will check all wrapper
type
> arguments if the method has optional argument(s). 
> 3) If the method is marked as typecheck, we use toImpl directly, all arguments
> has been typechecked in 1) and/or 2). Otherwise, use toImplWithTypeCheck to do
> typecheck.
> 
> What do you think?

I think that might work. :-)

I still think we should do this in two steps, though; first the
TypeChecking=Interface thing, disregarding overloading. When we're happy with
that, we look into the overloading case, which is significantly more tricky to
get right.

[yunchao, 2014-10-02 10:51:56]

https://codereview.chromium.org/611953003/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:757: if
argument['idl_type_object'].is_wrapper_type and
method['number_of_required_arguments'] == method['number_of_arguments']:
both number_of_required_arguments and number_of_arguments are elements in
method_context. But It reports an error for method['number_of_arguments'],
although method['number_of_required_arguments'] is OK. Three days ago, both of
method['number_of_required_arguments'] and method['number_of_arguments'] are OK.
Do you know why?

[yunchao, 2014-10-02 11:14:48]

On 2014/10/02 10:51:41, Jens Widell wrote:
> On 2014/10/02 10:46:19, yunchao wrote:
> > On 2014/09/30 09:38:22, Jens Widell wrote:
> > >
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > > File Source/bindings/scripts/v8_interface.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > > Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
> > > On 2014/09/30 09:17:29, yunchao wrote:
> > > > resolution_tests_methods has filtered a lot of arguments before
overloads,
> > or
> > > > returned early in this function if only one argument that is directly
used
> > for
> > > > overload resolution. So the arguments in the cases your listed  are not
in
> > the
> > > > 'arguments' here. They will not be marked as 'type_checked_already'. 
> > > 
> > > Ah, yes, that's true. Note though that this function can be called
multiple
> > > times with the same method in the |effective_overloads| list, and that it
> thus
> > > can end up setting 'type_checked_already' to true for multiple arguments
of
> > that
> > > one method.
> > > 
> > > In the case
> > > 
> > >   f(long a)
> > >   f(SomeInterface b, optional SomeInterface c, optional SomeInterface d)
> > >   f(SomeInterface e, long f)
> > > 
> > > for instance, resolution_tests_methods() will be called once for the
> > > len(argv)==1 case, and will consider the first two, using their first
> > arguments,
> > > and mark |b| as type checked. Then it will be called again for the
> > len(argv)==2
> > > case, and will then consider the last two, using their second arguments,
and
> > > mark |c| as type checked. (It will also be called once for the
len(argv)==3
> > > case, but then it stops early, having only one viable candidate.)
> > > 
> > > So both of the second method's arguments would be marked as type checked,
> but
> > > when it's actually called, at most one of the arguments will actually have
> > been
> > > type checked. None of them will have been type checked if it's called with
> > three
> > > arguments.
> > 
> > Thank you very much, Jens. I was aware of this case too. There is an
interface
> > ::clip() in CanvasRenderingContext2D.idl. It has the same problem. So I want
> to
> > do a conservative optimization, I just remove hasInstance if there is no
> > optional argument(s) in overload methods. 
> > 
> > As you know, I found this bottleneck in CanvasRenderingContext2D::drawImage.
> > This API calls hasInstance 3 times. And It has no optional argument(s) at
all.
> > This is an easy case, and the original auto-generated code does hurt
> performance
> > in many real cases and benchmarks of Canvas2D. This change can remove the
> > bottleneck.
> > 
> > As a result, for wrapper type arguments: 
> > 1) some are checked because of overloading. 
> > 2) If the method is marked as typecheck, we only check those arguments who
are
> > not checked by overloading. For security reason, we will check all wrapper
> type
> > arguments if the method has optional argument(s). 
> > 3) If the method is marked as typecheck, we use toImpl directly, all
arguments
> > has been typechecked in 1) and/or 2). Otherwise, use toImplWithTypeCheck to
do
> > typecheck.
> > 
> > What do you think?
> 
> I think that might work. :-)
> 
> I still think we should do this in two steps, though; first the
> TypeChecking=Interface thing, disregarding overloading. When we're happy with
> that, we look into the overloading case, which is significantly more tricky to
> get right.

Agree. This two things are not related to each other. Two patches will make it
much clear. See https://codereview.chromium.org/608853008/ to handle
TypeChecking=Interface thing. This one will handle the overloading case, which
is a little complicated.

[yunchao, 2014-10-10 03:03:00]

On 2014/10/02 11:14:48, yunchao wrote:
> On 2014/10/02 10:51:41, Jens Widell wrote:
> > On 2014/10/02 10:46:19, yunchao wrote:
> > > On 2014/09/30 09:38:22, Jens Widell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > > > File Source/bindings/scripts/v8_interface.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/611953003/diff/20001/Source/bindings/scripts/...
> > > > Source/bindings/scripts/v8_interface.py:777: for argument in arguments:
> > > > On 2014/09/30 09:17:29, yunchao wrote:
> > > > > resolution_tests_methods has filtered a lot of arguments before
> overloads,
> > > or
> > > > > returned early in this function if only one argument that is directly
> used
> > > for
> > > > > overload resolution. So the arguments in the cases your listed  are
not
> in
> > > the
> > > > > 'arguments' here. They will not be marked as 'type_checked_already'. 
> > > > 
> > > > Ah, yes, that's true. Note though that this function can be called
> multiple
> > > > times with the same method in the |effective_overloads| list, and that
it
> > thus
> > > > can end up setting 'type_checked_already' to true for multiple arguments
> of
> > > that
> > > > one method.
> > > > 
> > > > In the case
> > > > 
> > > >   f(long a)
> > > >   f(SomeInterface b, optional SomeInterface c, optional SomeInterface d)
> > > >   f(SomeInterface e, long f)
> > > > 
> > > > for instance, resolution_tests_methods() will be called once for the
> > > > len(argv)==1 case, and will consider the first two, using their first
> > > arguments,
> > > > and mark |b| as type checked. Then it will be called again for the
> > > len(argv)==2
> > > > case, and will then consider the last two, using their second arguments,
> and
> > > > mark |c| as type checked. (It will also be called once for the
> len(argv)==3
> > > > case, but then it stops early, having only one viable candidate.)
> > > > 
> > > > So both of the second method's arguments would be marked as type
checked,
> > but
> > > > when it's actually called, at most one of the arguments will actually
have
> > > been
> > > > type checked. None of them will have been type checked if it's called
with
> > > three
> > > > arguments.
> > > 
> > > Thank you very much, Jens. I was aware of this case too. There is an
> interface
> > > ::clip() in CanvasRenderingContext2D.idl. It has the same problem. So I
want
> > to
> > > do a conservative optimization, I just remove hasInstance if there is no
> > > optional argument(s) in overload methods. 
> > > 
> > > As you know, I found this bottleneck in
CanvasRenderingContext2D::drawImage.
> > > This API calls hasInstance 3 times. And It has no optional argument(s) at
> all.
> > > This is an easy case, and the original auto-generated code does hurt
> > performance
> > > in many real cases and benchmarks of Canvas2D. This change can remove the
> > > bottleneck.
> > > 
> > > As a result, for wrapper type arguments: 
> > > 1) some are checked because of overloading. 
> > > 2) If the method is marked as typecheck, we only check those arguments who
> are
> > > not checked by overloading. For security reason, we will check all wrapper
> > type
> > > arguments if the method has optional argument(s). 
> > > 3) If the method is marked as typecheck, we use toImpl directly, all
> arguments
> > > has been typechecked in 1) and/or 2). Otherwise, use toImplWithTypeCheck
to
> do
> > > typecheck.
> > > 
> > > What do you think?
> > 
> > I think that might work. :-)
> > 
> > I still think we should do this in two steps, though; first the
> > TypeChecking=Interface thing, disregarding overloading. When we're happy
with
> > that, we look into the overloading case, which is significantly more tricky
to
> > get right.
> 
> Agree. This two things are not related to each other. Two patches will make it
> much clear. See https://codereview.chromium.org/608853008/ to handle
> TypeChecking=Interface thing. This one will handle the overloading case, which
> is a little complicated.

The code is updated to fix a small bug. And test cases of type checking
interface for overloading have been added into TestObject.idl. Please take a
look when you have time. Thanks a lot! 

For your convenient, I upload the auto-generated cpp code in patchset 5 w/o the
change, and patchset 6 w/ the change.

[Jens Widell, 2014-10-10 10:14:53]

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:756: for argument, method in
arguments_methods:
Could you add a comment before this code explaining what we're doing, and
specifically under what circumstances it's safe/not safe to say "type already
checked"?

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:757: if
argument['idl_type_object'].is_wrapper_type and
method['number_of_required_arguments'] == method['number_of_arguments']:
You can use argument['is_wrapper_type'] here instead.

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:920: 'number_of_arguments':
len(constructor.arguments),
Move up a bit; the list should be sorted alphabetically.

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:239: 'type_checked_already': False,
Move this up a bit; the list should be sorted alphabetically.

[yunchao, 2014-10-10 11:40:49]

Thanks for your review, Jens. I have updated the code accordingly. Please take a
look when you have time. Thanks in advance!

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:756: for argument, method in
arguments_methods:
On 2014/10/10 10:14:53, Jens Widell wrote:
> Could you add a comment before this code explaining what we're doing, and
> specifically under what circumstances it's safe/not safe to say "type already
> checked"?

Done.

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:757: if
argument['idl_type_object'].is_wrapper_type and
method['number_of_required_arguments'] == method['number_of_arguments']:
On 2014/10/10 10:14:53, Jens Widell wrote:
> You can use argument['is_wrapper_type'] here instead.

Acknowledged.

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:920: 'number_of_arguments':
len(constructor.arguments),
On 2014/10/10 10:14:53, Jens Widell wrote:
> Move up a bit; the list should be sorted alphabetically.

Done.

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/611953003/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:239: 'type_checked_already': False,
On 2014/10/10 10:14:53, Jens Widell wrote:
> Move this up a bit; the list should be sorted alphabetically.

Done.

[Nils Barth (inactive), 2014-10-13 14:36:56]

nbarth@chromium.org changed reviewers:
+ bashi@chromium.org, nbarth@chromium.org

[Nils Barth (inactive), 2014-10-13 14:36:57]

+bashi, who's working on union type support.

Thanks for submitting this Yunchao!
Could you give data/benchmarks for how much this patch improves performance?
To reiterate comments I made earlier:
https://codereview.chromium.org/289843006/#msg4


I appreciate performance concerns, but...
it would be *much* better to simply support a union type here.
My biggest concern is that skipping checks can lead to very hard-to-debug
errors.
Further, supporting a union type here would probably speed up the code (due to
delegating correctly immediately), and much simplify the IDLs: it would be a
huge win.

I'll defer to more active developers about whether this should be included or
not.

bashi, looking at
CanvasRenderingContext2D.drawImage
how feasible to you think supporting a union type for method dispatch would be?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Also a few style points.)

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:763: # no optional arguments, for safe
reason.
"...for safety."

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:765: if argument['is_wrapper_type'] and
method['number_of_required_arguments'] == method['number_of_arguments']:
line length
Could you use (...) and break, so:
if (argument['is_wrapper_type'] and
    ...):
?

More simply:
argument['type_checked_already'] = (argument['is_wrapper_type'] and
 ...)

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:235: 'type_checked_already': False,
Is this needed?
(Absent variables default to None/False.)

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:112: {% if
argument.has_type_checking_interface and not argument.is_variadic_wrapper_type
and not argument.type_checked_already %}
Line break please.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/tests/i...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/tests/i...
Source/bindings/tests/idls/core/TestObject.idl:556: // remove redundant type
checking
Could you move these up to the other [TypeChecking] examples?

[yunchao, 2014-10-14 06:58:49]

Thanks for you review, nbarth@. I have updated the code accordingly.

This change and another similar one at
https://codereview.chromium.org/608853008/ removed two thirds call sites of
hasInstance. The ratio that hasInstance() costs decreased about 40% (not 67%
because of temporal locality) in the benchmarks I listed. As a result, I can see
the FPS is improved in these two benchmarks: 
my local case: see attachment in
http://code.google.com/p/chromium/issues/detail?id=416393
speedReading:
http://ie.microsoft.com/testdrive/Performance/SpeedReading/Default.html 
The FPS is reported by the script src/build/android/surface_stat.py on Nexus7.

It can save power if the FPS is already very high.

As you know, canvas.drawImage is a very important API, without it, the <canvas>
tag is basically nonsense. Its performance is critical for web *game*
developers, among other web developers.

BTW, I think hasInstance costs too much time even it is only called one time for
every drawImage. For a web game based on Canvas, the render process will 1)
execute JS code, 2) binding to Blink, 3) picture recording, 4) layout, 5)
commit, 6) sync between LayerTree and LayerTreeImpl, 7) rasterization in raster
worker, 8) compositing, and so on. There are so many complicated and heavy work.
Binding to Blink is a glue layer. It is quite easy. It should be lightweight. I
think the logic of hasInstance() should be optimized too if possible.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:763: # no optional arguments, for safe
reason.
On 2014/10/13 14:36:56, Nils Barth (inactive) wrote:
> "...for safety."

Done.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:765: if argument['is_wrapper_type'] and
method['number_of_required_arguments'] == method['number_of_arguments']:
On 2014/10/13 14:36:56, Nils Barth (inactive) wrote:
> line length
> Could you use (...) and break, so:
> if (argument['is_wrapper_type'] and
>     ...):
> ?
> 
> More simply:
> argument['type_checked_already'] = (argument['is_wrapper_type'] and
>  ...)

Acknowledged.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:235: 'type_checked_already': False,
On 2014/10/13 14:36:56, Nils Barth (inactive) wrote:
> Is this needed?
> (Absent variables default to None/False.)

Acknowledged.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:112: {% if
argument.has_type_checking_interface and not argument.is_variadic_wrapper_type
and not argument.type_checked_already %}
On 2014/10/13 14:36:57, Nils Barth (inactive) wrote:
> Line break please.

Done.

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/tests/i...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/611953003/diff/210001/Source/bindings/tests/i...
Source/bindings/tests/idls/core/TestObject.idl:556: // remove redundant type
checking
On 2014/10/13 14:36:57, Nils Barth (inactive) wrote:
> Could you move these up to the other [TypeChecking] examples?

Done.

[Nils Barth (inactive), 2014-10-14 23:00:08]

Thanks for explaining Yunchao!
Question:
* I don't see the *bottom line* impact: what is the FPS before and after your
change?
(This looks like a ~1.5% improvement, if it's 40% of 4%.)

I appreciate graphics as being very performance-sensitive, so minimizing impact
of bindings on this is important, but without causing huge headaches for others.
I'm fine with tweaking this, but let's be careful.
(I.e., I'm not saying "no", but I'm pushing back so we get the best outcome.)

haraken, bashi, Jens: thoughts on:
1. What we can do to improve this performance case?
(We may be able to do other things instead or as well.)
2. Cleanest way to do it?
(Jens proposed an alternative that should have the same effect but may be
simpler.)

There is inevitably going to be *some* delegation cost, and given that the
method is polymorphic, that's not going to be trivial, but it should be
minimizable.

[yunchao, 2014-10-15 08:17:05]

On 2014/10/14 23:00:08, Nils Barth (inactive) wrote:
> Thanks for explaining Yunchao!
> Question:
> * I don't see the *bottom line* impact: what is the FPS before and after your
> change?
> (This looks like a ~1.5% improvement, if it's 40% of 4%.)
> 
> I appreciate graphics as being very performance-sensitive, so minimizing
impact
> of bindings on this is important, but without causing huge headaches for
others.
> I'm fine with tweaking this, but let's be careful.
> (I.e., I'm not saying "no", but I'm pushing back so we get the best outcome.)
> 
> haraken, bashi, Jens: thoughts on:
> 1. What we can do to improve this performance case?
> (We may be able to do other things instead or as well.)
> 2. Cleanest way to do it?
> (Jens proposed an alternative that should have the same effect but may be
> simpler.)
> 
> There is inevitably going to be *some* delegation cost, and given that the
> method is polymorphic, that's not going to be trivial, but it should be
> minimizable.

Hi, Nils, please see the comment at https://codereview.chromium.org/657523002/.
Jens' patch extends the scenario of skipping redundant hasInstance during JS
binding to non-[TypeChecking=Interface] methods. It is an enhancement of this
change and https://codereview.chromium.org/608853008/. IMHO, I think this change
and Jens' are both OK.

[yunchao, 2014-10-20 07:25:25]

On 2014/10/14 23:00:08, Nils Barth (inactive) wrote:
> Thanks for explaining Yunchao!
> Question:
> * I don't see the *bottom line* impact: what is the FPS before and after your
> change?
> (This looks like a ~1.5% improvement, if it's 40% of 4%.)
> 

Take my local benchmark(draw 1000 sprites per frame) for example, the FPS w/o
this patch is 26.6 on Nexus7, while it is 27.1 with this patch.

It is not a big improvement for the final result. Because chrome is so
complicated, hot spots are scattered here and there, optimization for one hot
spot will not get a big surprise. And a series of improvements can got pretty
good result. Besides, for JS binding itself, this change improves a lot.

[Jens Widell, 2014-11-14 12:12:26]

yunchao@: with https://codereview.chromium.org/703783004/, drawImage() uses a
union type for the first argument, so is only overloaded for different number of
arguments. This patch thus no longer affects drawImage() at all, and the number
of hasInstance() checks should now be optimized to one.

You may want to rerun your benchmarks (with no patches applied), and close this
CL.

[yunchao, 2014-11-17 06:20:56]

On 2014/11/14 12:12:26, Jens Widell wrote:
> yunchao@: with https://codereview.chromium.org/703783004/, drawImage() uses a
> union type for the first argument, so is only overloaded for different number
of
> arguments. This patch thus no longer affects drawImage() at all, and the
number
> of hasInstance() checks should now be optimized to one.
> 
> You may want to rerun your benchmarks (with no patches applied), and close
this
> CL.

Thanks for your patch, Jens. I will double check the number of call sites to
hasInstance several days later when I have time.