Short circuit CC QIs for a few classes

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

2 years ago
Blocks: 1384717
(Assignee)

Comment 1

2 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

2 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

2 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

2 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+
(Assignee)

Updated

2 years ago
Blocks: 1395636
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 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.

Comment 13

2 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

2 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

2 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

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