Closed
Bug 1384821
Opened 7 years ago
Closed 7 years ago
Consider optimizing inherited QIs for nsCycleCollectionISupports
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8890691 -
Flags: review?(peterv)
Comment 2•7 years ago
|
||
Another possibility if we're willing to break binary compatibility of nsISupports is add an explicit nsISupports.getCycleCollectionParticipant method and bypass QI altogether.
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Comment 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
FWIW bug 1385459 should cut the number of QIs in half that we call when destroying an object.
Assignee | ||
Comment 9•7 years ago
|
||
(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)
Comment 10•7 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/26e332a412c7 Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports; r=peterv
Comment 11•7 years ago
|
||
Backout by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3377c3b7a86d Backout for assertion failures
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
Ah, true, I guess we can't assert. Though it'd be better if it didn't happen :-/.
Comment 14•7 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25d87deedcab Optimize inherited cycle-collectible QueryInterface() implementations for nsCycleCollectionISupports; r=peterv
Updated•7 years ago
|
Assignee: nobody → ehsan
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25d87deedcab
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•