Closed Bug 1384821 Opened 7 years ago Closed 7 years ago

Consider optimizing inherited QIs for nsCycleCollectionISupports

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

QIs for nsCycleCollectionISupports show up a lot in profiles from <https://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/xpcom/base/nsCycleCollector.cpp#2372> for example.

Today we discussed special casing the inherited CC interface map/table macros for this interface.  I did some measurements, and this helps a lot by avoiding having to scan all of the table in the child class(es) before figuring out in the base class what our canonical nsISupports is.

The cost of this special casing is of course imposed on QIs for other interfaces.  As a mitigation, we decided on special casing the IIDs of this interface and nsXPCOMCycleCollectionParticipant to avoid having to do more comparisons than strictly required when comparing the IIDs in the special case fast paths in the QI implementations.
Another possibility if we're willing to break binary compatibility of nsISupports is add an explicit nsISupports.getCycleCollectionParticipant method and bypass QI altogether.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> Another possibility if we're willing to break binary compatibility of
> nsISupports is add an explicit nsISupports.getCycleCollectionParticipant
> method and bypass QI altogether.

See also bug 1384717.
See Also: → 1384717
Comment on attachment 8890691 [details] [diff] [review]
Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports

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

::: xpcom/base/nsCycleCollectionParticipant.h
@@ +322,5 @@
>    nsISupports* foundInterface; \
>    NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(_class)
>  
> +// The IIDs for nsXPCOMCycleCollectionParticipant and nsCycleCollectionISupports
> +

Empty line.

@@ +330,5 @@
> +// deal with.
> +// It would be nice to have a similar optimization for the
> +// NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED case above as well, but
> +// NS_TableDrivenQI isn't aware of this special case so we don't have an easy
> +// option there.

The last sentence doesn't make sense to me, since NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED doesn't use NS_TableDrivenQI?
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED is used way more than NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED. I might be missing something, but couldn't we add the optimization to  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION, and call that in NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED?

@@ +342,5 @@
> +      if (aIID.LowWordEquals(NS_GET_IID(nsXPCOMCycleCollectionParticipant))) { \
> +        *aInstancePtr = NS_CYCLE_COLLECTION_PARTICIPANT(_class);              \
> +        return NS_OK;                                                         \
> +      }                                                                       \
> +      if (aIID.LowWordEquals(NS_GET_IID(nsCycleCollectionISupports))) {       \

We could just assert here that aIID.LowWordEquals(NS_GET_IID(nsCycleCollectionISupports)).

::: xpcom/base/nsID.h
@@ +80,5 @@
> +  inline bool LowWordEquals(const nsID& aOther) const
> +  {
> +    return (((uint32_t*)&m0)[3] == ((uint32_t*)&aOther.m0)[3]);
> +  }
> +

I'd be ok with just adding these somewhere in the CC header (as static functions taking nsIDs). Unless we think we might use this hack for other things?
Attachment #8890691 - Flags: review?(peterv) → review+
(In reply to Peter Van der Beken [:peterv] from comment #4)
> @@ +330,5 @@
> > +// deal with.
> > +// It would be nice to have a similar optimization for the
> > +// NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED case above as well, but
> > +// NS_TableDrivenQI isn't aware of this special case so we don't have an easy
> > +// option there.
> 
> The last sentence doesn't make sense to me, since
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED doesn't use
> NS_TableDrivenQI?
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED is used way more than
> NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED. I might be missing
> something, but couldn't we add the optimization to 
> NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION, and call that in
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED?

Hmm, yes you are totally right, I have no idea what I was thinking there...

> ::: xpcom/base/nsID.h
> @@ +80,5 @@
> > +  inline bool LowWordEquals(const nsID& aOther) const
> > +  {
> > +    return (((uint32_t*)&m0)[3] == ((uint32_t*)&aOther.m0)[3]);
> > +  }
> > +
> 
> I'd be ok with just adding these somewhere in the CC header (as static
> functions taking nsIDs). Unless we think we might use this hack for other
> things?

Will do, except I should probably make them inline functions instead to avoid getting one copy of them per translation unit.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58cfaca894a9
Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports; r=peterv
Backed out for bustage at nsCycleCollectionParticipant.h:312: 'const nsIID' has no member named 'LowWordEquals':

https://hg.mozilla.org/integration/mozilla-inbound/rev/a431c842cc50cfd446af8d48593bfda68a9e127f

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=58cfaca894a989a829b9982743171b57a4b1014a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=119626718&repo=mozilla-inbound

[task 2017-07-31T16:55:38.851855Z] 16:55:38     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/dist/include/jspubtd.h:14:0,
[task 2017-07-31T16:55:38.852178Z] 16:55:38     INFO -                   from /home/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionTraversalCallback.h:10,
[task 2017-07-31T16:55:38.852987Z] 16:55:38     INFO -                   from /home/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionNoteChild.h:13,
[task 2017-07-31T16:55:38.853543Z] 16:55:38     INFO -                   from /home/worker/workspace/build/src/xpcom/ds/nsCOMArray.h:15,
[task 2017-07-31T16:55:38.854150Z] 16:55:38     INFO -                   from /home/worker/workspace/build/src/xpcom/ds/nsPersistentProperties.cpp:9,
[task 2017-07-31T16:55:38.854933Z] 16:55:38     INFO -                   from /home/worker/workspace/build/src/obj-firefox/xpcom/ds/Unified_cpp_xpcom_ds1.cpp:2:
[task 2017-07-31T16:55:38.856023Z] 16:55:38     INFO -  /home/worker/workspace/build/src/xpcom/ds/nsVariant.cpp: In member function 'virtual nsresult nsVariantCC::QueryInterface(const nsIID&, void**)':
[task 2017-07-31T16:55:38.856880Z] 16:55:38     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:312:21: error: 'const nsIID' has no member named 'LowWordEquals'
Flags: needinfo?(ehsan)
FWIW bug 1385459 should cut the number of QIs in half that we call when destroying an object.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> FWIW bug 1385459 should cut the number of QIs in half that we call when
> destroying an object.

Awesome!!
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26e332a412c7
Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports; r=peterv
(In reply to Peter Van der Beken [:peterv] from comment #4)
> @@ +342,5 @@
> > +      if (aIID.LowWordEquals(NS_GET_IID(nsXPCOMCycleCollectionParticipant))) { \
> > +        *aInstancePtr = NS_CYCLE_COLLECTION_PARTICIPANT(_class);              \
> > +        return NS_OK;                                                         \
> > +      }                                                                       \
> > +      if (aIID.LowWordEquals(NS_GET_IID(nsCycleCollectionISupports))) {       \
> 
> We could just assert here that
> aIID.LowWordEquals(NS_GET_IID(nsCycleCollectionISupports)).

Actually I'm not sure if this is correct, since the low word could be anything really, there is no guarantees the IID we have here is one of these two.
Ah, true, I guess we can't assert. Though it'd be better if it didn't happen :-/.
Depends on: 1386321
No longer depends on: 1386321
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25d87deedcab
Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports; r=peterv
Assignee: nobody → ehsan
https://hg.mozilla.org/mozilla-central/rev/25d87deedcab
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1390692
You need to log in before you can comment on or make changes to this bug.