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)

enhancement
Not set
normal

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.
Blocks: 1384717
(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 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+
(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 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+
Blocks: 1395636
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.
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 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+
(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.
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.