1247972 - reduce the codesize hit from changing NS_ProxyRelease to take already_AddRefed<T>

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

Bug 1164581 changed NS_ProxyRelease to accept already_AddRefed<T>, which eliminates footguns and is generally a welcome change.  The only problem is that the templates used are rather large (NS_ProxyRelease itself especially, and nsProxyReleaseEvent will generate a decent amount of code).

Codesize on x86-64 Linux went up by ~60K after this change, which is a hefty amount.  (That's spread over ~100 NS_ReleaseOnMainThread/NS_ProxyRelease calls, so ~600 bytes/call, which is too much code to get inlined for non-hot path code, IMHO.)

We can at least try to make common out-of-line codepaths for nsISupports things.  I don't have any good suggestions for handling general T arguments, short of some dodgy casts and manually mucking around with vtables.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I don't think it was a good idea to move those functions from .cpp to .h for that change.

We can probably revert the change on nsProxyRelease.{h,cpp} and create the template function with a non-inline specialization for nsISupports?

Comment 2

[Aidin Gharibnavaz]

Attachment: MozReview Request: Bug 1247972 - Reducing size of the build, with reducing lines of code of the template classes. r?bobbyholley

Review commit: https://reviewboard.mozilla.org/r/34915/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34915/

Comment 3

[Aidin Gharibnavaz]

The problem is I made NS_ProxyRelease inline. It wasn't before. Only its overloads was inline. I removed the inline, and seems it fixed the problem.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

I don't really believe. The keyword "inline" shouldn't really have much effect on compilers' code generation.

Also the long function's instantiation for each type involving would also cause unnecessary bloat on code size I suppose.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

It seems that it would be a bit hard to back to the nsISupports version, since there are several places need a manual upcast.

Comment 6

[Bobby Holley (:bholley)]

Could we have an explicit specialization for things that unambiguously inherit nsISupport (which would just cast the thing to nsISupports and then invoke the other code)? Possibly leveraging SFINAE somehow?

Worst comes to worst, we can either leave it, or go back to requiring nsISupports and cast again in the callers. That does mean that at least one callsite will no longer be able to use this though.

Comment 7

[Andrew McCreight [:mccr8]]

I don't know if this helps, but cycle collected classes that ambiguously inherit from nsISupports have a ToSupports() overload to do the cast.

Comment 8

[Aidin Gharibnavaz]

Comment on attachment 8719234 [details]
MozReview Request: Bug 1247972 - Reducing size of the build, with reducing lines of code of the template classes. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34915/diff/1-2/

Comment 9

[Aidin Gharibnavaz]

How about this solution?
Note that this is a quick, dirty patch, just to check what you think about this solution. Comments and code cleanups will be add later.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

I think you forgot to include the newly added "nsProxyRelease.cpp" file in the commit. But otherwise, it looks like the right way to go.

One nit, I'd prefer "NS_ProxyReleaseInternal", which seems to match our naming elsewhere, over "NS_InternalProxyRelease". And probably we should add a comment above that function warns people not to use it unless they know what they are doing.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

One coding style question around the parameter here, which is worth a discussion with froydnj and bholley here.

I suggest we use "already_AddRefed<>&&" rather than "already_AddRefed<>" in parameters (and "UniquePtr<>&&" as well), although they are effectively equivalent. I explain the reasons in bug 1221823 comment 10.

What do you two think about this suggestion? I think whatever we decide, we should record it in the coding style document.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #10)
> One nit, I'd prefer "NS_ProxyReleaseInternal", which seems to match our
> naming elsewhere, over "NS_InternalProxyRelease". And probably we should add
> a comment above that function warns people not to use it unless they know
> what they are doing.

Using NS_ProxyReleaseInternal would be fine; another alternative is to do:

namespace mozilla {
namespace detail {

nsresult
ProxyRelease(...)
{
}

}
}

and rely on the detail:: prefixing necessary to scare people away.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #11)
> I suggest we use "already_AddRefed<>&&" rather than "already_AddRefed<>" in
> parameters (and "UniquePtr<>&&" as well), although they are effectively
> equivalent. I explain the reasons in bug 1221823 comment 10.

We have advice on this in UniquePtr.h, though I think the advice given there distinguishing between uncondtional transfers and conditional transfers is quite subtle.  As I have been moving things to UniquePtr and away from nsAuto*Ptr, I have been using |UniquePtr<T>| arguments per the comment in UniquePtr.h.  It's more consistent with other objects to use |UniquePtr<T>&&|, I think.

I don't understand the bit about "disjointed objects", but perhaps that's just terminology I haven't heard before.

Comment 13

[Aidin Gharibnavaz]

Comment on attachment 8719234 [details]
MozReview Request: Bug 1247972 - Reducing size of the build, with reducing lines of code of the template classes. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34915/diff/2-3/

Comment 14

[Aidin Gharibnavaz]

Comment on attachment 8719234 [details]
MozReview Request: Bug 1247972 - Reducing size of the build, with reducing lines of code of the template classes. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34915/diff/3-4/

Comment 15

[Aidin Gharibnavaz]

All done.

But, there's a Deadlock error which I couldn't fix.

In mozStorageService.cpp, |unregisterConnection| will be called twice. The second time is when NS_ProxyRelease calls the Release method of the Connection pointer.

A failed log:
https://treeherder.mozilla.org/logviewer.html#?job_id=17009073&repo=try

Comment 16

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #11)
> One coding style question around the parameter here, which is worth a
> discussion with froydnj and bholley here.
> 
> I suggest we use "already_AddRefed<>&&" rather than "already_AddRefed<>" in
> parameters (and "UniquePtr<>&&" as well), although they are effectively
> equivalent. I explain the reasons in bug 1221823 comment 10.
> 
> What do you two think about this suggestion? I think whatever we decide, we
> should record it in the coding style document.

I think we should stick with already_AddRefed<>, personally. This is a long-standing convention and pretty common throughout the codebase. I don't think we should introduce inconsistency, and I don't think it's worth the churn to change it everywhere.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Bobby Holley (busy) from comment #16)
> I think we should stick with already_AddRefed<>, personally. This is a
> long-standing convention and pretty common throughout the codebase. I don't
> think we should introduce inconsistency, and I don't think it's worth the
> churn to change it everywhere.

Before we have rvalue reference, we cannot do |already_AddRefed<>| in parameters, so it is not that common in our codebase. We are able to use this at the same time we can use |already_AddRefed<>&&|. Old code uses |already_AddRefed<>&| which is the only thing usable in the past, but it is clearly undesired nowadays given its inconvenience for callsites. So it doesn't really hurt consistency.

If you wonder, you can search |(already_AddRefed<| and |, already_AddRefed<| though the code. |already_AddRefed<>&| is the absolute majority, and the other two forms are about the same.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Searching on DXR with |regexp:"(\(|, )already_AddRefed<.+> "| gives 111 results, and that with "|regexp:"(\(|, )already_AddRefed<.+>&& "| gives 144 results. Which indicates the |already_AddRefed<>&&| form is actually used more widely than the |already_AddRefed<>| form.

(FWIW, |regexp:"(\(|, )already_AddRefed<.+>& "| gives 281 results.)

Comment 19

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #17)
> (In reply to Bobby Holley (busy) from comment #16)
> > I think we should stick with already_AddRefed<>, personally. This is a
> > long-standing convention and pretty common throughout the codebase. I don't
> > think we should introduce inconsistency, and I don't think it's worth the
> > churn to change it everywhere.
> 
> Before we have rvalue reference, we cannot do |already_AddRefed<>| in
> parameters, so it is not that common in our codebase. We are able to use
> this at the same time we can use |already_AddRefed<>&&|. Old code uses
> |already_AddRefed<>&| which is the only thing usable in the past, but it is
> clearly undesired nowadays given its inconvenience for callsites. So it
> doesn't really hurt consistency.

Ah, good point.

I think I still prefer the non-rvalue-reference version though. already_AddRefed<T> is already word-sized and already means "a single owning reference", so I don't think that that "a unique reference to a single owning reference" adds any semantic value (and probably detracts a bit).

The caller needs to do the Move() dance in either case. Adding the &&-wart to the callee just hurts readability (especially for people without a great grasp of rvalue references), so I think we should abstract that detail away into the move constructor for already_AddRefed.

That's just my opinion though, and I confess to not really understanding the rationale for && in this case. If you want to discuss it further, dev-platform is probably the place to do it. :-)

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

After thinking a bit deeper, I agree that we should prefer pass-by-value. I think pass-by-value should be prefer in the majority of cases for both move-only and non-move-only types, unless you are writing a move-ctor or you want to explicitly forbid the callsite to pass a lvalue (or you want conditional move, but that could make the interface confusing I suppose).

The reason behind is that, pass-by-value could actually give compilers more opportunity to optimize, since it is unconditional. For example, given the following code
> {
>   UniquePtr<SomeType> someObject = ...;
>   doSomething(Move(someObject));
> }
assuming the function "doSomething" is not inlinable, if it takes "UniquePtr<SomeType>&&", then the compiler would have to call the destructor anyway at the end of the scope, because it does not know whether the ownership has been transfered. However, if it takes "UniquePtr<SomeType>" instead, a smart enough compiler could remove the destructor completely.

We should probably convert most of already_AddRefed<>&& back to already_AddRefed<> (also UniquePtr<>&& to UniquePtr<>) to keep consistency.

Comment 21

[Nathan Froyd [:froydnj]]

Attachment: specialize NS_ProxyRelease for nsISupports to be out-of-line

Here's a patch that compiles for me and seems to pass the problematic unit
tests.  It's a little more complicated than Aidin's previous patch because
NS_ProxyRelease isn't just used for nsISupports things (!?), but the net effect
is the same.

Let's see what try thinks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32cdb1d7a017

Comment 22

[Eric Rahm [:erahm]]

Comment on attachment 8764730 [details] [diff] [review]
specialize NS_ProxyRelease for nsISupports to be out-of-line

Review of attachment 8764730 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one bit of clarification requested though.

::: xpcom/glue/nsProxyRelease.h
@@ +91,5 @@
> +  static void ProxyRelease(nsIEventTarget* aTarget,
> +                           already_AddRefed<T> aDoomed,
> +                           bool aAlwaysProxy)
> +  {
> +    ProxyReleaseISupports(aTarget, ToSupports(aDoomed.take()), aAlwaysProxy);

So does this mean we need a specialized ToSupports for every class type derived from nsISupports that we pass to this?

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #22)
> So does this mean we need a specialized ToSupports for every class type
> derived from nsISupports that we pass to this?

No.  There is a catch-all ToSupports definition:

http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#89

and the compiler will use that if the cast to nsISupports* is unambiguous.  We only need to implement ToSupports for cases where the cast to nsISupports* is ambiguous.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a8beff2575
specialize NS_ProxyRelease for nsISupports to be out-of-line; r=erahm

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5a8beff2575