Consider making nsQueryReferent not be an nsCOMPtr_helper

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: froydnj)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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).
Attachment #8900860 - Flags: review?(ehsan)
Assignee: nobody → nfroyd
(Reporter)

Comment 3

2 years ago
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.)
Attachment #8900860 - Flags: review?(ehsan) → review+

Comment 4

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e6ab8c91bc
manually de-virtualize do_QueryReferent; r=ehsan
https://hg.mozilla.org/mozilla-central/rev/20e6ab8c91bc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.