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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: froydnj, Assigned: aidin)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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?
Assignee: nobody → aidin
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.
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.
It seems that it would be a bit hard to back to the nsISupports version, since there are several places need a manual upcast.
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.
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 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/
Attachment #8719234 - Attachment description: MozReview Request: Bug 1247972 - Removing inline from NS_ProxyRelease to reduce the size of the output. r?bobbyholley → MozReview Request: Bug 1247972 - Reducing size of the build, with reducing lines of code of the template classes. r?bobbyholley
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.
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(nfroyd)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(nfroyd)
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 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/
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
(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.
Flags: needinfo?(bobbyholley)
(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.
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.)
(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. :-)
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.
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
Attachment #8764730 - Flags: review?(erahm)
Attachment #8719234 - Attachment is obsolete: true
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?
Attachment #8764730 - Flags: review?(erahm) → review+
(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.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a8beff2575
specialize NS_ProxyRelease for nsISupports to be out-of-line; r=erahm
https://hg.mozilla.org/mozilla-central/rev/a5a8beff2575
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1283552
You need to log in before you can comment on or make changes to this bug.