1268314 - Convert most of already_AddRefed<>&& parameters to take already_AddRefed<> instead

Status: NEW

(Core :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Using the plain type in parameter with a move constructor actually gives compilers move chance to optimize theoretically because it guarantees that the moving always happens.

Conditional move could usually be confusing, and should be avoid as much as possible. And for already_AddRefed, we almost never want conditional move at all.

Comment 1

[Daniel Holbert [:dholbert]]

Can/should we just convert most of these parameters to just take a RefPtr<>&& (or a nsCOMPtr<>&&) instead?  Is there any benefit to using already_AddRefed as a param?

(IMO, already_AddRefed is awkward as a parameter. I think we used it traditionally to avoid an extra addref/release, when we intended to transfer ownership into a function; but now that we have move semantics, we don't need it for that anymore.)

Also, note that (as filed) this bug seems like it's proposing to revert bug 967364 parts 4 and 4.1...

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #0)
> Using the plain type in parameter with a move constructor actually gives
> compilers move chance to optimize theoretically because it guarantees that
> the moving always happens.

Is the optimizing happening on the caller side or the callee side?  What is the compiler optimizing away in this case?

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Can/should we just convert most of these parameters to just take a
> RefPtr<>&& (or a nsCOMPtr<>&&) instead?  Is there any benefit to using
> already_AddRefed as a param?
>
> (IMO, already_AddRefed is awkward as a parameter. I think we used it
> traditionally to avoid an extra addref/release, when we intended to transfer
> ownership into a function; but now that we have move semantics, we don't
> need it for that anymore.)

One awkwardness--which you might have been alluding to--is that you have to transfer the already_AddRefed parameter into a smart pointer to do anything useful with it, which clutters the code a bit.  RefPtr<>&& would help avoid that.

I do fear people using RefPtr<> (value) parameters, whether by accident or just thinking the && is unnecessary baggage, and then getting sloppy with code, which leads to extra AddRef/Release traffic.  Maybe static analysis could help here.

Comment 3

[Nathan Froyd [:froydnj]]

bz's point on dev-platform that already_AddRefed will assert that people take the reference is a good one as well.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> (In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #0)
> > Using the plain type in parameter with a move constructor actually gives
> > compilers move chance to optimize theoretically because it guarantees that
> > the moving always happens.
> 
> Is the optimizing happening on the caller side or the callee side?  What is
> the compiler optimizing away in this case?

The optimizating happens on the caller side, because the compiler would know that the reference would always be taken, so that it can safely remove any check of the value of the raw pointer inside after the calling.

It doesn't have much difference on already_AddRefed, though, as its destructor is trivial on release build, and would assert in debug build if it is not taken.

But this really affects other smart pointers like RefPtr and UniquePtr, and we may see unnecessary refcount (or ownership) check in the callsite.

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Can/should we just convert most of these parameters to just take a
> RefPtr<>&& (or a nsCOMPtr<>&&) instead?  Is there any benefit to using
> already_AddRefed as a param?

If we want to do this, I think we don't want to use RefPtr<>&& because it removes an optimization chance on reducing possible useless refcount check. I'd prefer using RefPtr<> directly, but using RefPtr<> directly could hide another set of unnecessary refcount changes as far as it has copy-ctor.

Could we mark copy-ctor of RefPtr "= delete", and add a method called "Clone" so that we never implicitly addref?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #4)
> If we want to do this, I think we don't want to use RefPtr<>&& because it
> removes an optimization chance on reducing possible useless refcount check.
> I'd prefer using RefPtr<> directly,

(to be clear, here xidorn means we'd make the param have type RefPtr<Foo>, and the caller would pass in Move(myFoo), and the param would take ownership. This has the benefit that myFoo is guaranteed to be nulled out.)

> but using RefPtr<> directly could hide
> another set of unnecessary refcount changes as far as it has copy-ctor. 
> Could we mark copy-ctor of RefPtr "= delete", and add a method called
> "Clone" so that we never implicitly addref?

Maybe someday! But until we can do some major reworking like that, though, I'm now convinced that already_AddRefed is worth keeping as a parameter type; sorry for the brief diversion I've caused here about replacing it with another smart pointer type.