Return-value-optimization when return type is RefPtr<T>

[jww...@mozilla.com]

For

RefPtr<T> GetFoo() {
RefPtr<T> foo;
// ...
}

should we:

1. return foo and expect RVO to kick in to eliminate additional AddRef/Release
2. return foo.forget()
3. return Move(foo)

Which one is preferred?

ps: I find gcc is able to apply RVO even with "-O0". Not sure if it is also true for other compilers.

[Kan-Ru Chen (陳侃如)]

I think the current practice is to use already_AddRefed<T> as return
type and return foo.forget()

Kanru

[jww...@mozilla.com]

Kan-Ru Chen (陳侃如)於 2016年5月24日星期二 UTC+8下午4時00分12秒寫道：

> I think the current practice is to use already_AddRefed<T> as return
> type and return foo.forget()
>
> Kanru

I think that is the legacy when we don't have move semantics. Returning RefPtr<t> won't incur ref-counting overhead and is more expressive and functional.

[Gerald Squelart]

Coincidentally: (?)
https://bugzilla.mozilla.org/show_bug.cgi?id=1280296
"remove already_AddRefed"
:-)


For your original question, I would vote for RVO when possible, and Move() otherwise.

It feels like static analysis should be able to detect cases where RVO is possible and suggest it if missing; and also detect cases where it's not possible and warn about a missing Move().

[Boris Zbarsky]

On 6/16/16 3:15 AM, jww...@mozilla.com wrote:
> I think that is the legacy when we don't have move semantics. Returning RefPtr<t> won't incur ref-counting overhead and is more expressive and functional.

Except for the footgun described in
<https://bugzilla.mozilla.org/show_bug.cgi?id=1280296#c0>, yes?

-Boris

[Eric Rescorla]

Is it worth reconsidering removing implicit conversion from RefPtr<T> to T*?

-Ekr

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Michael Layzell]

We pass T* as an argument to a function too often for this to e practical.
The best solution is probably to remove the conversion from RefPtr<T>&& to
T* which is I believe what froydnj is planning to do.

This requires ref qualifiers for methods, which isn't supported in MSVC
2013, but is supported in 2015. See
https://bugzilla.mozilla.org/show_bug.cgi?id=1280295

On Thu, Jun 16, 2016 at 12:27 PM, Eric Rescorla <e...@rtfm.com> wrote:

> Is it worth reconsidering removing implicit conversion from RefPtr<T> to
> T*?
>
> -Ekr
>
>
> On Thu, Jun 16, 2016 at 9:50 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
>

[Eric Rescorla]

On Thu, Jun 16, 2016 at 2:15 PM, Michael Layzell <mic...@thelayzells.com>
wrote:

> We pass T* as an argument to a function too often for this to e practical.
>

Can you explain why? Is your point here to avoid people having to type
.get() or the mass
conversion from the current code? The former seems like it's a feature
rather than a bug...

-Ekr

The best solution is probably to remove the conversion from RefPtr<T>&& to
> T* which is I believe what froydnj is planning to do.
>
> This requires ref qualifiers for methods, which isn't supported in MSVC
> 2013, but is supported in 2015. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1280295
>
> On Thu, Jun 16, 2016 at 12:27 PM, Eric Rescorla <e...@rtfm.com> wrote:
>
>> Is it worth reconsidering removing implicit conversion from RefPtr<T> to
>> T*?
>>
>> -Ekr
>>
>>
>> On Thu, Jun 16, 2016 at 9:50 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
>>

[smaug]

Can we somehow guarantee that RVO kicks in? It feels quite error prone to rely on compiler optimization when dealing with
possibly slow Addref/Release calls.
https://en.wikipedia.org/wiki/Return_value_optimization#Compiler_support hints that there are cases when RVO might not happen.


-Olli

[smaug]

I've been told we can't guarantee RVO to kick in, which means, I think, that we should assume we don't have RVO ever and need to
rely on other coding patterns which are guaranteed to optimize out extra addref/release.
Currently having already_AddRefed as the return type is one such pattern.

What are the other options?


At least we can't start using RefPtr<T> as return type before we know it won't be causing performance regressions.
So I would recommend using already_AddRefed for now.

[Gerald Squelart]

On Friday, June 17, 2016 at 2:31:15 PM UTC+1, smaug wrote:
> On 06/16/2016 06:40 PM, smaug wrote:

From what *I* understand, RVO is guaranteed (or at least supported everywhere?) when there is only one stack variable that is returned, e.g.:
C foo()
{
C rv;
// ... (put stuff in rv)
return rv;
}
In this case, the caller function stack can host 'rv' directly, no copies needed.

RVO won't happen when there may be multiple variables in the function, only one of which will be returned, e.g.:
C foo()
{
C rv1, rv2; bool b;
// ... (assign a value to b)
return b ? rv1 : rv2;
}
The caller cannot know which of rv1 or rv2 will be returned, so no return value can be lifted.

(Any actual expert who knows the truth?)

[Gerald Squelart]

(sorry for the spam)

If I'm correct, then the question becomes: Can we trust developers to always make the right choice? Or better: Can we write static analyzers that will catch RVO-hostile code?

[Gabriele Svelto]

On 17/06/2016 16:57, Gerald Squelart wrote:
> From what *I* understand, RVO is guaranteed (or at least supported everywhere?) when there is only one stack variable that is returned, e.g.:
> C foo()
> {
> C rv;
> // ... (put stuff in rv)
> return rv;
> }
> In this case, the caller function stack can host 'rv' directly, no copies needed.

Sounds like the compiler needs visibility into the called function for
this to work so it'll either be limited to functions compiled as part of
the same invocation or when LTO is enabled. Also what about non-leaf
calls? I.e.

C foo()
{
C rv;
// ... (put stuff in rv)
return rv;
}

C bar()
{
// ...
return foo();
}

Would it still work? Do we care about it?

> (Any actual expert who knows the truth?)

That's most likely dependent on a combination of the actual code,
compiler, compiler version and flags used when compiling (and possibly
even target dependent if it interacts with other things like register
promotion of stack variables, etc...).

Gabriele

[smaug]

No. And reviewers won't catch it either.

Or better: Can we write static analyzers that will catch RVO-hostile code?
>

So this is the question.


And what then when the code is RVO hostile? developer is forced to write possibly uglier but non-RVO-hostile code? I could live with that I guess,
hoping that it happens rarely.


-Olli

[Aryeh Gregor]

Sorry for the necroposting, but some points that I think are still
worth correcting:

This is fixed for (ns)RefPtr in
<https://bugzilla.mozilla.org/show_bug.cgi?id=1193298>, and nsCOMPtr
in <https://bugzilla.mozilla.org/show_bug.cgi?id=1193762>. The
footgun code no longer compiles, because Foo() returns RefPtr<T>&& and
will not convert implicitly to T*.

On Thu, Jun 16, 2016 at 5:25 PM, Eric Rescorla <e...@rtfm.com> wrote:
> On Thu, Jun 16, 2016 at 2:15 PM, Michael Layzell <mic...@thelayzells.com>
> wrote:
>
>> We pass T* as an argument to a function too often for this to e practical.
>>
>
> Can you explain why? Is your point here to avoid people having to type
> .get() or the mass
> conversion from the current code? The former seems like it's a feature
> rather than a bug...

.get() is basically dangerous and IMO people should not be used to
writing it everywhere. Passing the pointer from a local RefPtr to a
function without addreffing is not dangerous, because the callee
cannot remove your local strong reference and the caller is guaranteed
to hold the reference until the callee returns. We should not have to
use .get() everywhere just because the compiler isn't smart enough to
figure out automatically that the operation is actually safe.

I had a proposal (with implementation) for some new parameter types to
replace raw pointers, which would hopefully allow complete removal of
automatic conversion to raw pointer without requiring extensive
.get(), and would also enforce other safety constraints (prohibiting
passing pointers that you don't hold a strong reference to).
<https://bugzilla.mozilla.org/show_bug.cgi?id=1194195>
<https://bugzilla.mozilla.org/show_bug.cgi?id=1194215> But froydnj
decided he wasn't willing to take them at the present time (and I'm
not sure I disagree).

All of these features are built into Rust, incidentally, because it
tracks lifetimes and will automatically figure out which usages are
safe.

Mass conversion is not an absolute blocker if the change is valuable
enough. Somebody might know how to automatically rewrite the code,
and if not, I've done pretty large conversions by hand in the past.
(It's just hours of repeatedly recompiling and fixing the same thing
over and over again.)

On Thu, Jun 16, 2016 at 8:40 PM, smaug <sm...@welho.com> wrote:
> Can we somehow guarantee that RVO kicks in? It feels quite error prone to
> rely on compiler optimization when dealing with
> possibly slow Addref/Release calls.

No, RVO is never guaranteed, but move constructors are guaranteed to
be used instead of copy constructors whenever you're returning a local
variable. Thus we only need to make sure that all cases have move
constructors (including things like moving nsCOMPtr<T> to RefPtr<U>).
We can write tests to make sure that there are no unexpected
AddRef/Release. I think the required move constructors don't yet
exist, or at least they didn't last I checked, but they could be added
easily.

IMO, it makes sense to move ahead with this.

[Aryeh Gregor]

On Mon, Aug 15, 2016 at 3:55 PM, Aryeh Gregor <a...@aryeh.name> wrote:
> IMO, it makes sense to move ahead with this.

One more point I forgot to (re-?)mention: with proper move
constructors, it's not just already_AddRefed<T> return values that
should be changed to RefPtr<T>, but also T* return values, in the case
where the returning function already had a RefPtr<T> lying around.
This will *reduce* addref/release in common cases, like:

T*
Foo()
{
RefPtr<T> t = DoStuff();
return t;
}

RefPtr<T> t = Foo();

Currently this does a release when Foo() returns, and a new addref
when assigning to the new variable. If Foo() instead returned
RefPtr<T> with proper move constructors, these would be removed. You
also could not assign the return value of Foo() to a raw pointer
(without .get()), which improves security against use-after-free
without any extra addref/release. (The release is just handled by the
caller instead of callee.)

It could inadvertently introduce extra addref/release if a function
that doesn't hold a reference on some code paths has its return type
changed to RefPtr, like:

T* Bar();

T*
Foo()
{
return Bar();
}

Foo()->Method();

If Foo()'s return type was changed to RefPtr<T>, this would silently
add an addref/release. I don't know how to prevent this. Would
making the relevant constructor explicit do it? If so, would it cause
issues in other places?

Also -- now I'm remembering more of the issues here -- this would also
require .get() to pass a function's return value directly as a
function parameter, unless bug 1194215 is fixed (see also bug
1194195). It's not *so* common to do this, but it's common enough
that we don't want to require .get(), I think.

So this isn't such a no-brainer, but I think it is where we ideally want to be.