Canvas2D Performance: fix the bottleneck of hasInstance in JS binding -- TypeChecking Interface

Canvas2D Performance: fix the bottleneck of hasInstance in JS binding -- TypeChecking Interface

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 3rd call site is redundant if type check has been 
done by typechecking interface. 

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

This change can improves performance and/or saves power.

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

[yunchao, 2014-10-02 11:10:03]

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

[yunchao, 2014-10-02 11:11:22]

PTAL, thanks a lot. Please see the discussion in
https://codereview.chromium.org/611953003/

[haraken, 2014-10-02 11:52:57]

LGTM

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

https://codereview.chromium.org/608853008/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:213: 'has_type_checking_interface':
type_checking_interface,

Can we rename this to needs_type_check ?

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

https://codereview.chromium.org/608853008/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:566: def
v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value,
variable_name, type_checked=False, index=None, declare_variable=True,
isolate='info.GetIsolate()', used_in_private_script=False,
return_promise=False):

type_checked => needs_type_check (and flip the true/false)

[Jens Widell, 2014-10-02 11:55:27]

On 2014/10/02 11:52:57, haraken wrote:
> LGTM

not lgtm

(Sorry, I have draft comments. Am dealing with some system-administration
emergencies, so a little bit slow.)

[Jens Widell, 2014-10-02 12:18:14]

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

https://codereview.chromium.org/608853008/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:194: type_checking_interface =
(has_extended_attribute_value(interface, 'TypeChecking', 'Interface') or
has_extended_attribute_value(method, 'TypeChecking', 'Interface')) and
idl_type.is_wrapper_type
Please wrap the line (like it used to be wrapped where you copied this from.)

https://codereview.chromium.org/608853008/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:370: name, type_checked, index=index,
declare_variable=False, return_promise=return_promise)
Please pass the new argument as a keyword argument instead of a positional.

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

https://codereview.chromium.org/608853008/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}))')
This isn't safe if the argument is nullable or if it's optional with
[Default=Undefined]. In both cases, the V8 value can be null or undefined, in
which case v8::Handle<v8::Object>::Cast() is invalid to use.

IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also implicitly
checks if the value is an object.

Test case: <script>document.body.insertBefore(new Text, null)</script>

[haraken, 2014-10-02 13:04:46]

On 2014/10/02 11:55:27, Jens Widell wrote:
> On 2014/10/02 11:52:57, haraken wrote:
> > LGTM
> 
> not lgtm
> 
> (Sorry, I have draft comments. Am dealing with some system-administration
> emergencies, so a little bit slow.)

Thanks for the catch!

[yunchao, 2014-10-08 07:04:58]

Hi jl@ and haraken@, I have update the code accordingly. Please take a look.
Thanks a lot.

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

https://codereview.chromium.org/608853008/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}))')
On 2014/10/02 12:18:14, Jens Widell wrote:
> This isn't safe if the argument is nullable or if it's optional with
> [Default=Undefined]. In both cases, the V8 value can be null or undefined, in
> which case v8::Handle<v8::Object>::Cast() is invalid to use.
> 
> IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also implicitly
> checks if the value is an object.
> 
> Test case: <script>document.body.insertBefore(new Text, null)</script>

Hi Jens, thanks for your review. However, nullable or undefined arguments have
been checked already 
if the method is a TypeCheck one. You can see the code in
bindings/templates/methods.cpp:117, 
right before hasInstance is called. For the test case you pointed out:

  <script>document.body.insertBefore(new Text, null)</script>

You can double check the generated code of insertBeforeMethod 
in src/out/Release/gen/blink/bindings/core/v8/V8Node.cpp

[Jens Widell, 2014-10-08 08:11:13]

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

https://codereview.chromium.org/608853008/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}))')
On 2014/10/08 07:04:58, yunchao wrote:
> On 2014/10/02 12:18:14, Jens Widell wrote:
> > This isn't safe if the argument is nullable or if it's optional with
> > [Default=Undefined]. In both cases, the V8 value can be null or undefined,
in
> > which case v8::Handle<v8::Object>::Cast() is invalid to use.
> > 
> > IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also implicitly
> > checks if the value is an object.
> > 
> > Test case: <script>document.body.insertBefore(new Text, null)</script>
> 
> Hi Jens, thanks for your review. However, nullable or undefined arguments have
> been checked already 
> if the method is a TypeCheck one. You can see the code in
> bindings/templates/methods.cpp:117, 
> right before hasInstance is called. For the test case you pointed out:
> 
>   <script>document.body.insertBefore(new Text, null)</script>
> 
> You can double check the generated code of insertBeforeMethod 
> in src/out/Release/gen/blink/bindings/core/v8/V8Node.cpp

Did you try my testcase? With your patch applied to current master, it crashes
for me, just as I'd expect, for the reasons I explained above.

To summarize: Yes, it's type checked already, but it's type checked as "null,
undefined or an object of the correct type." If it was null or undefined,
v8::Handle<v8::Object>::Cast() is invalid and crashes. If it was an object, then
all is good.

For arguments that aren't nullable and aren't optional with [Default=Undefined]
the type check also fails if the value is null or undefined, so for those
arguments, using v8::Handle<v8::Object>::Cast() like you do here is safe.

[yunchao, 2014-10-08 11:23:24]

I updated the code again, please take a look. Thanks a lot!

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

https://codereview.chromium.org/608853008/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}))')
On 2014/10/08 08:11:13, Jens Widell wrote:
> On 2014/10/08 07:04:58, yunchao wrote:
> > On 2014/10/02 12:18:14, Jens Widell wrote:
> > > This isn't safe if the argument is nullable or if it's optional with
> > > [Default=Undefined]. In both cases, the V8 value can be null or undefined,
> in
> > > which case v8::Handle<v8::Object>::Cast() is invalid to use.
> > > 
> > > IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also
implicitly
> > > checks if the value is an object.
> > > 
> > > Test case: <script>document.body.insertBefore(new Text, null)</script>
> > 
> > Hi Jens, thanks for your review. However, nullable or undefined arguments
have
> > been checked already 
> > if the method is a TypeCheck one. You can see the code in
> > bindings/templates/methods.cpp:117, 
> > right before hasInstance is called. For the test case you pointed out:
> > 
> >   <script>document.body.insertBefore(new Text, null)</script>
> > 
> > You can double check the generated code of insertBeforeMethod 
> > in src/out/Release/gen/blink/bindings/core/v8/V8Node.cpp
> 
> Did you try my testcase? With your patch applied to current master, it crashes
> for me, just as I'd expect, for the reasons I explained above.
> 
> To summarize: Yes, it's type checked already, but it's type checked as "null,
> undefined or an object of the correct type." If it was null or undefined,
> v8::Handle<v8::Object>::Cast() is invalid and crashes. If it was an object,
then
> all is good.
> 
> For arguments that aren't nullable and aren't optional with
[Default=Undefined]
> the type check also fails if the value is null or undefined, so for those
> arguments, using v8::Handle<v8::Object>::Cast() like you do here is safe.

You are correct, Jens. So I want to narrow down to arguments who are not
nullable. As you know, I am aim at improving the performance of Canvas2D,
arguments in drawImage API in CanvasRenderingContext2D.idl are not nullable,
they can get improvement by this change. And some other APIs may be also
improved.

[Jens Widell, 2014-10-08 11:51:26]

It would be great if you also added to TestObject.idl two additional
[TypeChecking=Interface] methods, one with a nullable interface argument, and
one with a [Default=Undefined] optional argument, so that it becomes clear what
code we generate in those cases.

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

https://codereview.chromium.org/608853008/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}))')
On 2014/10/08 11:23:23, yunchao wrote:
> On 2014/10/08 08:11:13, Jens Widell wrote:
> > On 2014/10/08 07:04:58, yunchao wrote:
> > > On 2014/10/02 12:18:14, Jens Widell wrote:
> > > > This isn't safe if the argument is nullable or if it's optional with
> > > > [Default=Undefined]. In both cases, the V8 value can be null or
undefined,
> > in
> > > > which case v8::Handle<v8::Object>::Cast() is invalid to use.
> > > > 
> > > > IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also
> implicitly
> > > > checks if the value is an object.
> > > > 
> > > > Test case: <script>document.body.insertBefore(new Text, null)</script>
> > > 
> > > Hi Jens, thanks for your review. However, nullable or undefined arguments
> have
> > > been checked already 
> > > if the method is a TypeCheck one. You can see the code in
> > > bindings/templates/methods.cpp:117, 
> > > right before hasInstance is called. For the test case you pointed out:
> > > 
> > >   <script>document.body.insertBefore(new Text, null)</script>
> > > 
> > > You can double check the generated code of insertBeforeMethod 
> > > in src/out/Release/gen/blink/bindings/core/v8/V8Node.cpp
> > 
> > Did you try my testcase? With your patch applied to current master, it
crashes
> > for me, just as I'd expect, for the reasons I explained above.
> > 
> > To summarize: Yes, it's type checked already, but it's type checked as
"null,
> > undefined or an object of the correct type." If it was null or undefined,
> > v8::Handle<v8::Object>::Cast() is invalid and crashes. If it was an object,
> then
> > all is good.
> > 
> > For arguments that aren't nullable and aren't optional with
> [Default=Undefined]
> > the type check also fails if the value is null or undefined, so for those
> > arguments, using v8::Handle<v8::Object>::Cast() like you do here is safe.
> 
> You are correct, Jens. So I want to narrow down to arguments who are not
> nullable. As you know, I am aim at improving the performance of Canvas2D,
> arguments in drawImage API in CanvasRenderingContext2D.idl are not nullable,
> they can get improvement by this change. And some other APIs may be also
> improved.

This still generates code that crashes in the [Default=Undefined] case.

I don't think we have any overlap between [Default=Undefined] (on interface-type
arguments) and [TypeChecking=Interface] in current IDL files, but this is
largely because [TypeChecking=Interface] isn't widely used. Anyway, we still
shouldn't knowingly add the possibility to introduce a crash bug simply by doing
valid changes to IDL files, so we should fix this.

See comment elsewhere for proposed way to do that.

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

https://codereview.chromium.org/608853008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:238: 'v8_value_to_local_cpp_value':
v8_value_to_local_cpp_value(argument, index, type_checking_interface and not
idl_type.is_nullable, return_promise=method.returns_promise),
I propose to add

  type_checked = (type_checking_interface and
                  # These allow null and undefined values, so a
                  # type-check is still required.
                  not idl_type.is_nullable and
                  not (argument.is_optional and
                       'Default' in extended_attributes))

above, and use 'type_checked' here.

[yunchao, 2014-10-09 04:54:07]

Thanks for your review, Jens. As you suggested, test cases are added into
TestObject.idl.

Please help to have a look. Many thanks!

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

https://codereview.chromium.org/608853008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:238: 'v8_value_to_local_cpp_value':
v8_value_to_local_cpp_value(argument, index, type_checking_interface and not
idl_type.is_nullable, return_promise=method.returns_promise),
On 2014/10/08 11:51:26, Jens Widell wrote:
> I propose to add
> 
>   type_checked = (type_checking_interface and
>                   # These allow null and undefined values, so a
>                   # type-check is still required.
>                   not idl_type.is_nullable and
>                   not (argument.is_optional and
>                        'Default' in extended_attributes))
> 
> above, and use 'type_checked' here.

Done.

[Jens Widell, 2014-10-10 09:26:02]

LGTM, with a few nits.

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

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:201: not idl_type.is_nullable and
Nit: Include the comment I had in my suggestion as well?

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:222: 'has_type_checking_interface':
type_checking_interface,
Nit: haraken@ suggested we rename this 'needs_type_check'. I think that's a
better name for it too, so please do that.

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:556: [TypeChecking=Interface]
void useToImpl4ArgumentsCheckingIfPossibleWithOptionalArg(Node node1, optional
Node node2);
Nit: Move them up to the other [TypeChecking=Interface] methods?

[yunchao, 2014-10-10 10:36:11]

Thanks for your review, Jens. I have updated the code accordingly except one.

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

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:201: not idl_type.is_nullable and
On 2014/10/10 09:26:02, Jens Widell wrote:
> Nit: Include the comment I had in my suggestion as well?

Done.

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:222: 'has_type_checking_interface':
type_checking_interface,
On 2014/10/10 09:26:02, Jens Widell wrote:
> Nit: haraken@ suggested we rename this 'needs_type_check'. I think that's a
> better name for it too, so please do that.

'has_type_checking_interface' is used as a key name in many places. More
important, there is another key called has_type_checking_unrestricted. If
'has_type_checking_interface is renamed, I think maybe
'has_type_checking_unrestricted" should be renamed too. So I prefer to keep it
the same here. BTW, I does renamed the parameters name to 'needs_type_check' in
the callee in v8_types.py.

Well, you can submit patch to rename 'has_type_checking_interface', as well as
'has_type_checking_unrestricted' if they are not appropriate.

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:556: [TypeChecking=Interface]
void useToImpl4ArgumentsCheckingIfPossibleWithOptionalArg(Node node1, optional
Node node2);
On 2014/10/10 09:26:02, Jens Widell wrote:
> Nit: Move them up to the other [TypeChecking=Interface] methods?

Done.

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

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

https://codereview.chromium.org/608853008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:222: 'has_type_checking_interface':
type_checking_interface,
On 2014/10/10 10:36:11, yunchao wrote:
> On 2014/10/10 09:26:02, Jens Widell wrote:
> > Nit: haraken@ suggested we rename this 'needs_type_check'. I think that's a
> > better name for it too, so please do that.
> 
> 'has_type_checking_interface' is used as a key name in many places. More
> important, there is another key called has_type_checking_unrestricted. If
> 'has_type_checking_interface is renamed, I think maybe
> 'has_type_checking_unrestricted" should be renamed too. So I prefer to keep it
> the same here. BTW, I does renamed the parameters name to 'needs_type_check'
in
> the callee in v8_types.py.
> 
> Well, you can submit patch to rename 'has_type_checking_interface', as well as
> 'has_type_checking_unrestricted' if they are not appropriate. 

Okay.

[yunchao, 2014-10-10 10:46:41]

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

[commit-bot: I haz the power, 2014-10-10 10:47:05]

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

[commit-bot: I haz the power, 2014-10-10 11:47:52]

Committed patchset #5 (id:100001) as 183536