1390568 - Consider making nsQueryReferent not be an nsCOMPtr_helper

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[(no longer active)]

See bug 1390527 for example.  Right now checking weak pointers through do_QueryReferent() takes 2 virtual calls.  It'd be nice if we made nsQueryReferent not be an nsCOMPtr_helper and eliminate the virtual nsQueryReferent::operator() call by making nsCOMPtr and friends know about nsQueryReferent in the same way that they know about nsCOMPtr_helper.  That way they can call its operator() without having to make an indirect call.

Comment 1

[Nathan Froyd [:froydnj]]

nsQueryReferent is marked as `final` under the assumption that the compiler is able to devirtualize.  But nsCOMPtr::assign_from_helper, which handles general assignment from nsCOMPtr_helper subclasses, isn't inlined (at least not with GCC and I can see other compilers not inlining it as well), so the compiler doesn't get a chance to see the opportunity for devirtualization.  So `final` doesn't really help in this case, and we'd have to special-case assigning from nsQueryReferent in nsCOMPtr.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: manually de-virtualize do_QueryReferent

nsQueryReferent is defined as an nsCOMPtr_helper, which implies that
calling its operator() method requires a virtual call.  While
nsQueryReferent is marked `final`, compiler inlining decisions make it
impossible to de-virtualize the call to operator().  However, we have
many other classes returned by do_* functions that nsCOMPtr handles
directly, requiring no extra virtual calls, and we can give
nsQueryReferent the same treatment.

It's stupid that the ->QueryReferent method call inside
nsQueryReferent::operator() is going to require a virtual call too, but maybe
removing a virtual call from the call chain is virtuous in and of itself?  We
can address deCOMing nsIWeakReference (as a step to merging it with
mozilla::WeakPtr or whatever) in a followup bug).

Comment 3

[(no longer active)]

Comment on attachment 8900860 [details] [diff] [review]
manually de-virtualize do_QueryReferent

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

::: mfbt/RefPtr.h
@@ +148,5 @@
>      // construct from |Move(RefPtr<SomeSubclassOfT>)|.
>    {
>    }
>  
> +  MOZ_IMPLICIT RefPtr(const nsQueryReferent& aHelper);

Out of curiosity, why are these not marked as inline?  Do we want them to be?  I am totally unsure how much compilers care about this of course.

(Could be follow-up work of course.)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e6ab8c91bc
manually de-virtualize do_QueryReferent; r=ehsan

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/20e6ab8c91bc