Closed
Bug 1390660
Opened 7 years ago
Closed 7 years ago
Short circuit CC QIs for a few classes
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(3 files)
Ehsan's Speedometer profiles consistently show QI overhead (needed CC-related stuff) from specific classes. nsStyledElement and nsTextNode are the most common, and they both use NS_IMPL_QUERY_INTERFACE_INHERITED. We could short circuit evaluation for the CC QIs, by using the macros we usually use in classes that implement their own CC participant to reduce this overhead. It looks like about 10 classes account for 80% of the table driven QI overhead.
Assignee | ||
Comment 1•7 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #0) > It looks like about 10 classes account for 80% of the table > driven QI overhead. This estimate is likely high, because some of the top entries are not actually related to CC. But I think it is still quite a lot of the total.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8898072 [details] Bug 1390660, part 1 - Clean up some uses of the table-to-map segue. https://reviewboard.mozilla.org/r/169374/#review176228 ::: commit-message-a9086:7 (Diff revision 1) > + > +In a number of places, there's no substantial use of maps any more > +after the segue. > + > +The ELEMENT segue tries the FragmentOrElement QI, but that is > +redundant with the Element QI. This doesn't seem relevant to this commit? ::: dom/xbl/XBLChildrenElement.cpp:59 (Diff revision 1) > NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAnonymousContentList) > NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAnonymousContentList) > > NS_INTERFACE_TABLE_HEAD(nsAnonymousContentList) > NS_WRAPPERCACHE_INTERFACE_TABLE_ENTRY > NS_INTERFACE_TABLE_INHERITED(nsAnonymousContentList, nsINodeList, While you're here, please make this use NS_INTERFACE_TABLE (and remove the entry for nsISupports below).
Attachment #8898072 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #5) > This doesn't seem relevant to this commit? That's for this part: - NS_ELEMENT_INTERFACE_TABLE_TO_MAP_SEGUE -NS_INTERFACE_MAP_END_INHERITING(Element)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8898073 [details] Bug 1390660, part 2 - Define and use a new macro for CC isupports. https://reviewboard.mozilla.org/r/169376/#review179962 Can you file a bug about converting everything that can be to the new macros?
Attachment #8898073 -
Flags: review?(peterv) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
For part 3, two of the classes don't implement a new nsIFoo interface any more, so I had to define a new NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED0 macro. This in turn revealed that NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED does not initialize rv, so I fixed that.
Assignee | ||
Comment 12•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4842308772cd7c9ee7b18b80e6e3a8347a3be13
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8898074 [details] Bug 1390660, part 3 - Make QIing to a few CCed classes faster. https://reviewboard.mozilla.org/r/169378/#review181200 ::: dom/html/HTMLLIElement.cpp:27 (Diff revision 2) > { > } > > -NS_IMPL_ISUPPORTS_INHERITED0(HTMLLIElement, nsGenericHTMLElement) > +// Use the CC variant of this, even though this class does not define > +// a new CC participant, to make QIing to the CC interfaces faster. > +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED0(HTMLLIElement, nsGenericHTMLElement) Hmm, personally I'd rather remove the NS_IMPL_ISUPPORTS_INHERITED0 instead. I know we lose a bit of flexibility in refcount logging, but I don't think it's worth it if it's a performance hit (not just in QI but also in AddRef/Release).
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8898074 [details] Bug 1390660, part 3 - Make QIing to a few CCed classes faster. https://reviewboard.mozilla.org/r/169378/#review181206
Attachment #8898074 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #13) > Hmm, personally I'd rather remove the NS_IMPL_ISUPPORTS_INHERITED0 instead. > I know we lose a bit of flexibility in refcount logging, but I don't think > it's worth it if it's a performance hit (not just in QI but also in > AddRef/Release). I'll just leave these two classes alone, and remove NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED0 from my patch. When the measurements were previously done, there were additional interfaces on these subclasses, so the QI had to call into table-driven QI, which was showing up on profiles, but now they will directly call into the parent QI, which shouldn't be enough overhead to worry about. I should have thought about that when I was updating my patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/279b7e7e46b0 part 1 - Clean up some uses of the table-to-map segue. r=peterv https://hg.mozilla.org/integration/autoland/rev/7fcbb4f25c5c part 2 - Define and use a new macro for CC isupports. r=peterv https://hg.mozilla.org/integration/autoland/rev/1c4eec5a5b96 part 3 - Make QIing to a few CCed classes faster. r=peterv
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/279b7e7e46b0 https://hg.mozilla.org/mozilla-central/rev/7fcbb4f25c5c https://hg.mozilla.org/mozilla-central/rev/1c4eec5a5b96
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•