Skip expensive hasInstance() type-checks in overloads

Skip expensive hasInstance() type-checks in overloads

From overload resolution callbacks, pass the index of the argument that
was type-checked (or -1 if no argument was strictly type-checked) to the
actual overload's callback, to let it in turn skip its type-checks on
that argument, if possible.

[Jens Widell, 2014-10-14 12:13:43]

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

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

This is an alternative approach to https://codereview.chromium.org/611953003/
that I wanted to explore, and see what your opinions about are.

This moves the decision whether to type check to runtime (instead of
compile-time) which would be a down-side. Or at least would seem to do. In
practice, the individual overload methods are often inlined into the overload
resolution method, so the compiler's optimizer will be able to make the decision
compile-time still. And even if it doesn't, we ought not introduce too much
cost, and still get the runtime gain of not calling hasInstance().

On the plus side, this implementation doesn't have to be conservative; it will
really skip the check whenever possible. For complex overloads where an argument
is sometimes type-checked and sometimes not, it will skip the check (runtime)
for those calls where it was type-checked earlier.

https://codereview.chromium.org/657523002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/657523002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestObject.cpp:9822: if (info.Length() > 0
&& !V8Node::hasInstance(info[0], info.GetIsolate())) {
In the context of this CL, please ignore this hasInstance(). It would be dealt
with otherwise.

[haraken, 2014-10-14 13:54:07]

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

https://codereview.chromium.org/657523002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:211:
convert_v8_value_to_local_cpp_value_unsafe = v8_value_to_local_cpp_value(

convert_v8_value_to_local_cpp_value_unsafe =>
convert_v8_value_to_local_cpp_value_without_type_check ?

https://codereview.chromium.org/657523002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:212: argument, index, type_checked=True,
return_promise=method.returns_promise)

I'd rename type_checked to needs_type_check and flip the true/false.

https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:181: if (type_checked_argument_index ==
{{argument.index}})

Help me understand: I'm curious about the relationship between
argument.is_distinguishing_argument and 'type_checked_argument_index ==
{{argument.index}}'. Is it possible that the argument is an distinguishing
argument but 'type_checked_argument_index == {{argument.index}}' does not hold?

I was expecting that type is always checked if the argument is an distinguishing
argument.

https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:562: static void {{name}}(const
v8::FunctionCallbackInfo<v8::Value>& info{% if constructor.overload_index %},
int type_checked_argument_index{% endif %})

type_checked_argument_index => typeCheckedArgumentIndex

Style checker is (intentionally) not enabled for auto-generated code.

[Jens Widell, 2014-10-14 14:37:57]

https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:181: if (type_checked_argument_index ==
{{argument.index}})
On 2014/10/14 13:54:07, haraken wrote:
> 
> Help me understand: I'm curious about the relationship between
> argument.is_distinguishing_argument and 'type_checked_argument_index ==
> {{argument.index}}'. Is it possible that the argument is an distinguishing
> argument but 'type_checked_argument_index == {{argument.index}}' does not
hold?

Yes, it's possible. The key complexity here (which makes the other approach
intricate) is that different arguments (to the same method) can theoretically be
the distinguishing argument, depending on the number of arguments in the call.

I could have called 'is_distinguishing_argument' something better, I think.
Maybe 'is_ever_distinguishing_argument'. It's really just an optimization;
skipping the runtime check for arguments that are never the distinguishing
argument (which we can trivially know compile-time.)

> I was expecting that type is always checked if the argument is an
distinguishing
> argument.

An easy mistake to make. :-)

Consider HTMLSelectElement.add(), which has these overloads:

  void add(HTMLElement element, optional HTMLElement? before = null);
  void add(HTMLElement element, long before);

The first one could be called with the argument lists

  (anElement)
  (anElement, null)
  (anElement, anotherElement)

In the last case, 'before' is type-checked and add1Method() is called with
type_checked_argument_index=1. In the first two cases, no relevant type-checking
will have been made before add1Method() is called, and add1Method() is called
with type_checked_argument_index=-1, which means it uses toImplWithTypeCheck()
(unless info.Length()<2, which uses the default value code path, of course.)

And add2Method() is always called with no prior (relevant) type-checking.

[haraken, 2014-10-14 15:18:22]

On 2014/10/14 14:37:57, Jens Widell wrote:
>
https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/657523002/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:181: if (type_checked_argument_index ==
> {{argument.index}})
> On 2014/10/14 13:54:07, haraken wrote:
> > 
> > Help me understand: I'm curious about the relationship between
> > argument.is_distinguishing_argument and 'type_checked_argument_index ==
> > {{argument.index}}'. Is it possible that the argument is an distinguishing
> > argument but 'type_checked_argument_index == {{argument.index}}' does not
> hold?
> 
> Yes, it's possible. The key complexity here (which makes the other approach
> intricate) is that different arguments (to the same method) can theoretically
be
> the distinguishing argument, depending on the number of arguments in the call.
> 
> I could have called 'is_distinguishing_argument' something better, I think.
> Maybe 'is_ever_distinguishing_argument'. It's really just an optimization;
> skipping the runtime check for arguments that are never the distinguishing
> argument (which we can trivially know compile-time.)
> 
> > I was expecting that type is always checked if the argument is an
> distinguishing
> > argument.
> 
> An easy mistake to make. :-)
> 
> Consider HTMLSelectElement.add(), which has these overloads:
> 
>   void add(HTMLElement element, optional HTMLElement? before = null);
>   void add(HTMLElement element, long before);
> 
> The first one could be called with the argument lists
> 
>   (anElement)
>   (anElement, null)
>   (anElement, anotherElement)
> 
> In the last case, 'before' is type-checked and add1Method() is called with
> type_checked_argument_index=1. In the first two cases, no relevant
type-checking
> will have been made before add1Method() is called, and add1Method() is called
> with type_checked_argument_index=-1, which means it uses toImplWithTypeCheck()
> (unless info.Length()<2, which uses the default value code path, of course.)
> 
> And add2Method() is always called with no prior (relevant) type-checking.

Thanks for the clarification, now the CL makes sense to me.

The final question is whether this optimization is worth doing or not. I'd like
to make sure that it's worth introducing the complexity to the IDL compiler :)

[Jens Widell, 2014-10-14 15:31:07]

On 2014/10/14 15:18:22, haraken wrote:
> The final question is whether this optimization is worth doing or not. I'd
like
> to make sure that it's worth introducing the complexity to the IDL compiler :)

I think the numbers presented in https://codereview.chromium.org/611953003/
indicate that this is an inefficiency worth eliminating.

The argument to drawImage() is supposed to be a union type, so for drawImage()
at least, the problem there will go away with full union type support. The union
type argument will then never be the distinguishing argument, so no
hasInstance() calls will be in the overload resolution method.

So, we could wait for full union type support instead, if we think all important
overloads are actually cases that should use union types instead (which seems
plausible.)

[haraken, 2014-10-14 15:33:25]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org

[haraken, 2014-10-14 15:33:25]

+bashi-san

Let's ask bashi-san about the union type support.

Jens: Thanks for the numbers. The implementation looks good, so I'll LG once it
turns out this is a right way to go.

[bashi, 2014-10-15 00:01:59]

On 2014/10/14 15:33:25, haraken wrote:
> +bashi-san
> 
> Let's ask bashi-san about the union type support.
> 
> Jens: Thanks for the numbers. The implementation looks good, so I'll LG once
it
> turns out this is a right way to go.

I have a half-baked WIP CL about union type support and will upload it today.
I'm not sure we can use union types for all cases (esp. performance sensitive
ones like this) so I think it would be worth considering this CL to land.

yunchao@, how did you get these numbers? I'd like to measure drawImage()
performance with my CL.

[bashi, 2014-10-15 00:02:12]

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

https://codereview.chromium.org/657523002/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:685: [(test, method)]
Could you update this comment?

[yunchao, 2014-10-15 06:24:28]

On 2014/10/14 12:13:44, Jens Widell wrote:
> This is an alternative approach to https://codereview.chromium.org/611953003/
> that I wanted to explore, and see what your opinions about are.
> 
> This moves the decision whether to type check to runtime (instead of
> compile-time) which would be a down-side. Or at least would seem to do. In
> practice, the individual overload methods are often inlined into the overload
> resolution method, so the compiler's optimizer will be able to make the
decision
> compile-time still. And even if it doesn't, we ought not introduce too much
> cost, and still get the runtime gain of not calling hasInstance().
> 
> On the plus side, this implementation doesn't have to be conservative; it will
> really skip the check whenever possible. For complex overloads where an
argument
> is sometimes type-checked and sometimes not, it will skip the check (runtime)
> for those calls where it was type-checked earlier.
> 
>
https://codereview.chromium.org/657523002/diff/1/Source/bindings/tests/result...
> File Source/bindings/tests/results/core/V8TestObject.cpp (right):
> 
>
https://codereview.chromium.org/657523002/diff/1/Source/bindings/tests/result...
> Source/bindings/tests/results/core/V8TestObject.cpp:9822: if (info.Length() >
0
> && !V8Node::hasInstance(info[0], info.GetIsolate())) {
> In the context of this CL, please ignore this hasInstance(). It would be dealt
> with otherwise.

Hi, Jens, could you sumbit 2 patches as following:
In these 2 patch sets, test cases of overload resolution methods in
TestObject.idl are added, and CanvasRenderingContext2D.idl is added into this
test suite. 

Then one patch set keeps the original auto-generated cpp code, the other patch
set applied this change and get the new generated code. Then we can compare the
difference much more easier.

I will appreciate this.

IMHO, this change differs from mine at
https://codereview.chromium.org/611953003/.

[yunchao, 2014-10-15 06:38:56]

On 2014/10/15 00:01:59, bashi1 wrote:
> On 2014/10/14 15:33:25, haraken wrote:
> > +bashi-san
> > 
> > Let's ask bashi-san about the union type support.
> > 
> > Jens: Thanks for the numbers. The implementation looks good, so I'll LG once
> it
> > turns out this is a right way to go.
> 
> I have a half-baked WIP CL about union type support and will upload it today.
> I'm not sure we can use union types for all cases (esp. performance sensitive
> ones like this) so I think it would be worth considering this CL to land.
> 
> yunchao@, how did you get these numbers? I'd like to measure drawImage()
> performance with my CL.

bashi@, you can follow the instructions at
(1)http://code.google.com/p/chromium/wiki/ProfilingContentShellOnAndroid.
Basically, I takes these important steps:
1. add "profiling=1" into GYP_DEFINES in chromium.gyp_env, then build
content_shell for android:
https://code.google.com/p/chromium/wiki/AndroidBuildInstructions. 
2. Install content_shell on device, say, Nexus7.
3. Download AOSP and build it, then build perf(perf depends on some libs in
AOSP, so you need to build AOSP at first, who will take some time), install perf
on Nexus7. This perf is the perf guest.
4. Download Linux kernel, build perf (you may need to install some pkgs), This
is the perf on host machine. Seems that the default perf in Linux(I use Ubuntu
14.04) is not the latest one, it will miss some info.
5. Set symbols for perf host accroding to (1), Run benchmarks(SpeedReading,
FishIETank, GM3, etc), use 'perf record' to profiling on Nexus7, adb push the
profile data to host machine, and use 'perf report' to get the profiling data.

That's all. Contact me if you have some issues.

[Jens Widell, 2014-10-15 06:52:40]

On 2014/10/15 06:24:28, yunchao wrote:
> Hi, Jens, could you sumbit 2 patches as following:
> In these 2 patch sets, test cases of overload resolution methods in
> TestObject.idl are added, and CanvasRenderingContext2D.idl is added into this
> test suite. 
> 
> Then one patch set keeps the original auto-generated cpp code, the other patch
> set applied this change and get the new generated code. Then we can compare
the
> difference much more easier.
> 
> I will appreciate this.

Test suite impact visualized here: https://codereview.chromium.org/654183002/

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

On 2014/10/15 06:52:40, Jens Widell wrote:
> On 2014/10/15 06:24:28, yunchao wrote:
> > Hi, Jens, could you sumbit 2 patches as following:
> > In these 2 patch sets, test cases of overload resolution methods in
> > TestObject.idl are added, and CanvasRenderingContext2D.idl is added into
this
> > test suite. 
> > 
> > Then one patch set keeps the original auto-generated cpp code, the other
patch
> > set applied this change and get the new generated code. Then we can compare
> the
> > difference much more easier.
> > 
> > I will appreciate this.
> 
> Test suite impact visualized here: https://codereview.chromium.org/654183002/

Why the 2nd patch set is not based on the 1st one? 

It doesn't matter. If you look into the impact, you will get the conclusion that
this change mixed my 2 patches: https://codereview.chromium.org/611953003/ and
https://codereview.chromium.org/608853008/ and applied the rule to non
[TypeChecking=Interface] methods with overload resolution. 

hasInstance() is called 3 times for every drawImage (or any other similar
methods do overload resolution, with [TyepChecking=Interface]): 
(1) the 1st hasInstance is called by overload resolution. 
(2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
overloading variants
(3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
variants.

my patch at https://codereview.chromium.org/611953003/ removes the 2nd call of
hasInstance if the argument has been checked by overloading resolution. 
my patch at https://codereview.chromium.org/608853008/ removes the 3nd call of
hasInstance if the argument is in [TypeChecking=Interface] method, sometimes,
this hasInstance is the 2nd one if it is not checked by overloading resolution.
But no matter the argument is checked by overload resolution or not, it has been
checked already by (1) or by (2). The exception is that the argument is nullable
or optional undefined.

this change checked the overloading resolution, and removes the 3rd
type-checking in (3) if the argument has been checked by overloading, *no matter
the method has [TypeChecking=Interface]*. So this change is an enhancement based
on my 2 CLs listed above. If affects type-checks for all overloading arguments,
not only these methods with [TypeChecking=Interface] like canvas::drawImage.

Please correct me if I am wrong.

[yunchao, 2014-10-15 07:28:08]

On 2014/10/15 07:25:20, yunchao wrote:
> On 2014/10/15 06:52:40, Jens Widell wrote:
> > On 2014/10/15 06:24:28, yunchao wrote:
> > > Hi, Jens, could you sumbit 2 patches as following:
> > > In these 2 patch sets, test cases of overload resolution methods in
> > > TestObject.idl are added, and CanvasRenderingContext2D.idl is added into
> this
> > > test suite. 
> > > 
> > > Then one patch set keeps the original auto-generated cpp code, the other
> patch
> > > set applied this change and get the new generated code. Then we can
compare
> > the
> > > difference much more easier.
> > > 
> > > I will appreciate this.
> > 
> > Test suite impact visualized here:
https://codereview.chromium.org/654183002/
> 
> Why the 2nd patch set is not based on the 1st one? 
> 
> It doesn't matter. If you look into the impact, you will get the conclusion
that
> this change mixed my 2 patches: https://codereview.chromium.org/611953003/ and
> https://codereview.chromium.org/608853008/ and applied the rule to non
> [TypeChecking=Interface] methods with overload resolution. 
> 
> hasInstance() is called 3 times for every drawImage (or any other similar
> methods do overload resolution, with [TyepChecking=Interface]): 
> (1) the 1st hasInstance is called by overload resolution. 
> (2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
> overloading variants
> (3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
> variants.
> 
> my patch at https://codereview.chromium.org/611953003/ removes the 2nd call of
> hasInstance if the argument has been checked by overloading resolution. 
> my patch at https://codereview.chromium.org/608853008/ removes the 3nd call of
> hasInstance if the argument is in [TypeChecking=Interface] method, sometimes,
> this hasInstance is the 2nd one if it is not checked by overloading
resolution.
> But no matter the argument is checked by overload resolution or not, it has
been
> checked already by (1) or by (2). The exception is that the argument is
nullable
> or optional undefined.
> 
> this change checked the overloading resolution, and removes the 3rd
> type-checking in (3) if the argument has been checked by overloading, *no
matter
> the method has [TypeChecking=Interface]*. So this change is an enhancement
based
> on my 2 CLs listed above. If affects type-checks for all overloading
arguments,
  on my 2 CLs listed above. It affects ......
                           (typo)

> not only these methods with [TypeChecking=Interface] like canvas::drawImage.
> 
> Please correct me if I am wrong.

[yunchao, 2014-10-15 07:31:04]

On 2014/10/15 07:28:08, yunchao wrote:
> On 2014/10/15 07:25:20, yunchao wrote:
> > On 2014/10/15 06:52:40, Jens Widell wrote:
> > > On 2014/10/15 06:24:28, yunchao wrote:
> > > > Hi, Jens, could you sumbit 2 patches as following:
> > > > In these 2 patch sets, test cases of overload resolution methods in
> > > > TestObject.idl are added, and CanvasRenderingContext2D.idl is added into
> > this
> > > > test suite. 
> > > > 
> > > > Then one patch set keeps the original auto-generated cpp code, the other
> > patch
> > > > set applied this change and get the new generated code. Then we can
> compare
> > > the
> > > > difference much more easier.
> > > > 
> > > > I will appreciate this.
> > > 
> > > Test suite impact visualized here:
> https://codereview.chromium.org/654183002/
> > 
> > Why the 2nd patch set is not based on the 1st one? 
> > 
> > It doesn't matter. If you look into the impact, you will get the conclusion
> that
> > this change mixed my 2 patches: https://codereview.chromium.org/611953003/
and
> > https://codereview.chromium.org/608853008/ and applied the rule to non
> > [TypeChecking=Interface] methods with overload resolution. 
> > 
> > hasInstance() is called 3 times for every drawImage (or any other similar
> > methods do overload resolution, with [TyepChecking=Interface]): 
> > (1) the 1st hasInstance is called by overload resolution. 
> > (2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
> > overloading variants
> > (3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
> > variants.
> > 
> > my patch at https://codereview.chromium.org/611953003/ removes the 2nd call
of
> > hasInstance if the argument has been checked by overloading resolution. 
> > my patch at https://codereview.chromium.org/608853008/ removes the 3nd call
of
> > hasInstance if the argument is in [TypeChecking=Interface] method,
sometimes,
> > this hasInstance is the 2nd one if it is not checked by overloading
> resolution.
> > But no matter the argument is checked by overload resolution or not, it has
> been
> > checked already by (1) or by (2). The exception is that the argument is
> nullable
> > or optional undefined.
> > 
> > this change checked the overloading resolution, and removes the 3rd
> > type-checking in (3) if the argument has been checked by overloading, *no
> matter
> > the method has [TypeChecking=Interface]*. So this change is an enhancement
> based
> > on my 2 CLs listed above. If affects type-checks for all overloading
> arguments,
>   on my 2 CLs listed above. It affects ......
>                            (typo)
> 
> > not only these methods with [TypeChecking=Interface] like canvas::drawImage.
> > 
> > Please correct me if I am wrong.

But index checking in (3) for arguments in methods with [TypeChecking=Interface]
is redundant. These arguments are safe to call toImpl instead of
toImplWithTypeCheck.

[yunchao, 2014-10-15 07:46:33]

On 2014/10/15 07:31:04, yunchao wrote:
> On 2014/10/15 07:28:08, yunchao wrote:
> > On 2014/10/15 07:25:20, yunchao wrote:
> > > On 2014/10/15 06:52:40, Jens Widell wrote:
> > > > On 2014/10/15 06:24:28, yunchao wrote:
> > > > > Hi, Jens, could you sumbit 2 patches as following:
> > > > > In these 2 patch sets, test cases of overload resolution methods in
> > > > > TestObject.idl are added, and CanvasRenderingContext2D.idl is added
into
> > > this
> > > > > test suite. 
> > > > > 
> > > > > Then one patch set keeps the original auto-generated cpp code, the
other
> > > patch
> > > > > set applied this change and get the new generated code. Then we can
> > compare
> > > > the
> > > > > difference much more easier.
> > > > > 
> > > > > I will appreciate this.
> > > > 
> > > > Test suite impact visualized here:
> > https://codereview.chromium.org/654183002/
> > > 
> > > Why the 2nd patch set is not based on the 1st one? 
> > > 
> > > It doesn't matter. If you look into the impact, you will get the
conclusion
> > that
> > > this change mixed my 2 patches: https://codereview.chromium.org/611953003/
> and
> > > https://codereview.chromium.org/608853008/ and applied the rule to non
> > > [TypeChecking=Interface] methods with overload resolution. 
> > > 
> > > hasInstance() is called 3 times for every drawImage (or any other similar
> > > methods do overload resolution, with [TyepChecking=Interface]): 
> > > (1) the 1st hasInstance is called by overload resolution. 
> > > (2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
> > > overloading variants
> > > (3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
> > > variants.
> > > 
> > > my patch at https://codereview.chromium.org/611953003/ removes the 2nd
call
> of
> > > hasInstance if the argument has been checked by overloading resolution. 
> > > my patch at https://codereview.chromium.org/608853008/ removes the 3nd
call
> of
> > > hasInstance if the argument is in [TypeChecking=Interface] method,
> sometimes,
> > > this hasInstance is the 2nd one if it is not checked by overloading
> > resolution.
> > > But no matter the argument is checked by overload resolution or not, it
has
> > been
> > > checked already by (1) or by (2). The exception is that the argument is
> > nullable
> > > or optional undefined.
> > > 
> > > this change checked the overloading resolution, and removes the 3rd
> > > type-checking in (3) if the argument has been checked by overloading, *no
> > matter
> > > the method has [TypeChecking=Interface]*. So this change is an enhancement
> > based
> > > on my 2 CLs listed above. If affects type-checks for all overloading
> > arguments,
> >   on my 2 CLs listed above. It affects ......
> >                            (typo)
> > 
> > > not only these methods with [TypeChecking=Interface] like
canvas::drawImage.
> > > 
> > > Please correct me if I am wrong.
> 
> But index checking in (3) for arguments in methods with
[TypeChecking=Interface]
> is redundant. These arguments are safe to call toImpl instead of
> toImplWithTypeCheck.

I think you can submit this change for arguments in methods *without*
[TypeChecking=Interface], handle v8_value to cpp_value for those argumets when
*not has_type_checking_interface*. From my perspective, it looks good for that
situation.

[Jens Widell, 2014-10-15 07:53:08]

On 2014/10/15 07:25:20, yunchao wrote:
> It doesn't matter. If you look into the impact, you will get the conclusion
that
> this change mixed my 2 patches: https://codereview.chromium.org/611953003/ and
> https://codereview.chromium.org/608853008/ and applied the rule to non
> [TypeChecking=Interface] methods with overload resolution. 

This is an alternative approach to solving the problem that
https://codereview.chromium.org/611953003/ solves, and thus replaces it. IOW, we
would not land both of these. We may land neither, if we decide a third approach
(e.g. union types) is a better way forward.

It is based on https://codereview.chromium.org/608853008/ since that's on
master. It reverts parts of it, and reuses other parts of it.

> hasInstance() is called 3 times for every drawImage (or any other similar
> methods do overload resolution, with [TyepChecking=Interface]): 
> (1) the 1st hasInstance is called by overload resolution. 
> (2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
> overloading variants
> (3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
> variants.
> 
> my patch at https://codereview.chromium.org/611953003/ removes the 2nd call of
> hasInstance if the argument has been checked by overloading resolution.

Yes, and this patch doesn't, but as I commented: please ignore that in the
context of this CL. *That* hasInstance() is not really about overloading, so I
just chose to not address it in *this* CL. It will be dealt with, and the
changes in this CL would not make that harder in any way.

> my patch at https://codereview.chromium.org/608853008/ removes the 3nd call of
> hasInstance if the argument is in [TypeChecking=Interface] method, sometimes,
> this hasInstance is the 2nd one if it is not checked by overloading
resolution.
> But no matter the argument is checked by overload resolution or not, it has
been
> checked already by (1) or by (2). The exception is that the argument is
nullable
> or optional undefined.
> 
> this change checked the overloading resolution, and removes the 3rd
> type-checking in (3) if the argument has been checked by overloading, *no
matter
> the method has [TypeChecking=Interface]*. So this change is an enhancement
based
> on my 2 CLs listed above. If affects type-checks for all overloading
arguments,
> not only these methods with [TypeChecking=Interface] like canvas::drawImage.
> 
> Please correct me if I am wrong.

For the overload resolution interaction, this patch does cover a bit more, yes,
since it doesn't need to be conservative. It doesn't need to make any
"difficult" decisions compile-time (i.e. in the Python scripts) since it's
really just about propagating information so that the decision can be made
trivially at runtime instead. (And then we hope that the C++ compiler optimizes
so that the decision actually mostly happens compile-time still.)

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

On 2014/10/15 07:53:08, Jens Widell wrote:
> On 2014/10/15 07:25:20, yunchao wrote:
> > It doesn't matter. If you look into the impact, you will get the conclusion
> that
> > this change mixed my 2 patches: https://codereview.chromium.org/611953003/
and
> > https://codereview.chromium.org/608853008/ and applied the rule to non
> > [TypeChecking=Interface] methods with overload resolution. 
> 
> This is an alternative approach to solving the problem that
> https://codereview.chromium.org/611953003/ solves, and thus replaces it. IOW,
we
> would not land both of these. We may land neither, if we decide a third
approach
> (e.g. union types) is a better way forward.
> 
> It is based on https://codereview.chromium.org/608853008/ since that's on
> master. It reverts parts of it, and reuses other parts of it.
> 
> > hasInstance() is called 3 times for every drawImage (or any other similar
> > methods do overload resolution, with [TyepChecking=Interface]): 
> > (1) the 1st hasInstance is called by overload resolution. 
> > (2) the 2nd hasInstance is called because of [TypeChecking=Interface] in
> > overloading variants
> > (3) the 3rd hasInstance is called in toImplWithTypeCheck() in overloading
> > variants.
> > 
> > my patch at https://codereview.chromium.org/611953003/ removes the 2nd call
of
> > hasInstance if the argument has been checked by overloading resolution.
> 
> Yes, and this patch doesn't, but as I commented: please ignore that in the
> context of this CL. *That* hasInstance() is not really about overloading, so I
> just chose to not address it in *this* CL. It will be dealt with, and the
> changes in this CL would not make that harder in any way.
> 
> > my patch at https://codereview.chromium.org/608853008/ removes the 3nd call
of
> > hasInstance if the argument is in [TypeChecking=Interface] method,
sometimes,
> > this hasInstance is the 2nd one if it is not checked by overloading
> resolution.
> > But no matter the argument is checked by overload resolution or not, it has
> been
> > checked already by (1) or by (2). The exception is that the argument is
> nullable
> > or optional undefined.
> > 
> > this change checked the overloading resolution, and removes the 3rd
> > type-checking in (3) if the argument has been checked by overloading, *no
> matter
> > the method has [TypeChecking=Interface]*. So this change is an enhancement
> based
> > on my 2 CLs listed above. If affects type-checks for all overloading
> arguments,
> > not only these methods with [TypeChecking=Interface] like canvas::drawImage.
> > 
> > Please correct me if I am wrong.
> 
> For the overload resolution interaction, this patch does cover a bit more,
yes,
> since it doesn't need to be conservative. It doesn't need to make any
> "difficult" decisions compile-time (i.e. in the Python scripts) since it's
> really just about propagating information so that the decision can be made
> trivially at runtime instead. (And then we hope that the C++ compiler
optimizes
> so that the decision actually mostly happens compile-time still.)

You are correct. This change covers a bit more for overload resolution. it
extends to arguments in [TypeChecking=Interface] methods with nullable and
optional undefined arguments, more important, to arguments in
non-[TypeChecking=Interface]-methods, by checking the index of overloading at
runtime. But for the arguments in [TypeChecking=Interface] methods without
nullable and optional undefined arguments, the runtime check is unnecessary. And
it handles the redundant call of hasInstance introduced by toImplWithTypeCheck,
like the one at https://codereview.chromium.org/608853008/, not handle the
redundant call of hasInstance introduced by [TyepChecking=Interface], like the
one at https://codereview.chromium.org/611953003/.

[Jens Widell, 2014-11-13 07:22:55]

Since https://codereview.chromium.org/703783004/ has landed, which solves the
overloading part of the hasInstance() issue for drawImage(), I'm closing this
CL. Was never very happy with it.

If we find another performance issue caused by redundant hasInstance()
type-checks in overloaded methods, we can revisit this.