FYI: Compile fixes when always using a local for method return value

FYI: Compile fixes when always using a local for method return value

By always storing the return value in a local, we tend to narrow the
range of acceptable return values on the C++ side, compared to when
passing the return value directly as an argument to a v8SetReturnValue()
function or similar (since those are often overloaded or templates
accepting a multitude of types.)

[Jens Widell, 2014-07-10 16:10:17]

Just a FYI

These changes were needed to compile (on 64-bit Linux) if a local is always used
to store method return values.

Note: I don't propose to commit these changes in a single CL, and I'm not sure
these are the correct fixes for the problems either.

[Jens Widell, 2014-07-10 16:22:12]

On 2014/07/10 16:10:17, Jens Lindström wrote:
> Just a FYI
> 
> These changes were needed to compile (on 64-bit Linux) if a local is always
used
> to store method return values.
> 
> Note: I don't propose to commit these changes in a single CL, and I'm not sure
> these are the correct fixes for the problems either.

Performance-wise, on a micro-benchmark measuring Document.getElementById(),
there might be a 3-4 % slowdown. But this might also just be random noise
(compiler optimizing the two builds differently for some reason.)

[haraken, 2014-07-11 01:14:03]

The change looks reasonable to me.

> Performance-wise, on a micro-benchmark measuring Document.getElementById(),
> there might be a 3-4 % slowdown. But this might also just be random noise
> (compiler optimizing the two builds differently for some reason.)

This would be an important point. Is it a noise, or not?

If it's an actual noise, an alternative idea would be to special-case only
RefPtrs so that they don't use local variables. Special-casing RefPtrs would be
better than special-casing various types (which we currently does).

[Jens Widell, 2014-07-11 06:24:50]

On 2014/07/11 01:14:03, haraken wrote:
> The change looks reasonable to me.
> 
> > Performance-wise, on a micro-benchmark measuring Document.getElementById(),
> > there might be a 3-4 % slowdown. But this might also just be random noise
> > (compiler optimizing the two builds differently for some reason.)
> 
> This would be an important point. Is it a noise, or not?
> 
> If it's an actual noise, an alternative idea would be to special-case only
> RefPtrs so that they don't use local variables. Special-casing RefPtrs would
be
> better than special-casing various types (which we currently does).

Wouldn't special-casing RefPtr like that just lead to even more logic in the
code generator? Even if we optimize the local away specifically (and only) for
RefPtr types, we'd still need to track all the current cases where we really
need a local, and disable the optimization in those cases.

OTOH, once Oilpan is enabled universally, these RefPtrs will typically be raw
pointers instead, in which case there'd be no problem.

[Daniel Bratell, 2014-07-11 12:29:39]

It's just a FYI so I don't know how serious this is, but it looks like the code
will sometimes become less efficient. Would a static_cast accomplish the same
positive effects as the local variable?

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

https://codereview.chromium.org/386613002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/V8TestObject.cpp:5381: long long result =
impl->longLongMethod();
Here it will possibly do two type conversions. First to long long and then from
long long to double. I guess going directly to double would be more optimal.

https://codereview.chromium.org/386613002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/V8TestObject.cpp:5842: RefPtr<TestInterfaceEmpty>
result = impl->testInterfaceEmptyMethod();
Is this equivalent?

https://codereview.chromium.org/386613002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/V8TestObject.cpp:6422: Vector<int> result =
impl->longArrayMethod();
Looks like this might introduce a vector copy which would be unfortunate.

[Jens Widell, 2014-07-11 12:50:04]

On 2014/07/11 12:29:39, Daniel Bratell wrote:
> It's just a FYI so I don't know how serious this is, but it looks like the
code
> will sometimes become less efficient. Would a static_cast accomplish the same
> positive effects as the local variable?

That the code sometimes becomes less efficient is the main worry, yes. I was
thinking mostly about ref-counting churn introduced by an explicit RefPtr<>
local, but clearly, vector copying due to introducing vector locals might be
even worse.

The point of using the local variable is that we sometimes need to have it
anyway, so if we always have it, the code generator becomes simpler. Currently
the code generator has logic for figuring out whether to have a local or not,
and then multiple code paths in multiple places handling the two alternatives.