412300 - Number of calls to nsID::Equals() has increased between FF2 and FF3

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, enhancement)

Description

[Ondrej Brablc]

+++ This bug was initially created as a clone of Bug #406810 +++

(In reply to comment #41)
> >http://www.allpeers.com/download/michal/new/callgraph/FF2/nsID+Equals.png
> >http://www.allpeers.com/download/michal/new/callgraph/FF3/nsID+Equals.png
> 
> It does look like this is mostly QI.  One obvious change here is that the
> number of hits in nsGenericElement::QueryInterface has gone up by 25% or so. 
> Can we get the callers of nsGenericElement::QueryInterface?  Interestingly, the
> number of QIs on nsXULElement hasn't changed much, so the nsGenericElement
> stuff is some other elements.
> 
> There are also a lot more hits in nsStandardURL::QueryInterface (more than
> twice as many as on branch), so perhaps the callers of this would be good too.  
> 
> And BackstagePass::QueryInterface seems to have jumped.
> 
> Between them, those three account for 2/3 or more of the jump in nsID::Equals
> calls.  All might need several levels of callers to get to the actual place
> where the do_QueryInterface happened.
> 
> It might be worth filing a separate bug (blocking this one) for the QI issue so
> that we don't get too many confusing things all going on here at once.  But we
> should definitely look into this!

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Because of the macros aIID.Equals(NS_GET_IID(nsCycleCollectionISupports)
is now the first comparison (if I read the macros correcly). 
Is that really right? I *guess* that - especially during
startup - nsINode, nsIContent and nsPIDOMEventTarget are more common.

Comment 2

[Michal Novotny [:michal]]

(In reply to comment #0)
> > It does look like this is mostly QI.  One obvious change here is that the
> > number of hits in nsGenericElement::QueryInterface has gone up by 25% or so. 
> > Can we get the callers of nsGenericElement::QueryInterface?  Interestingly, the
> > number of QIs on nsXULElement hasn't changed much, so the nsGenericElement
> > stuff is some other elements.

Number of QI on nsGenericElement is in 1.9 even lower than in 1.8. Maybe there
is probably more interfaces and that's whay nsID::Equals() has increased?

http://www.allpeers.com/download/michal/new/callgraph/FF2/nsGenericElement+QueryInterface.png
http://www.allpeers.com/download/michal/new/callgraph/FF3/nsGenericElement+QueryInterface.png

There is one additional interface at:
http://lxr.mozilla.org/mozilla1.8/source/content/base/src/nsGenericElement.cpp#3700
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#3453

And as far as I can see two additional due to
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION macro:
  nsXPCOMCycleCollectionParticipant
  nsCycleCollectionISupports

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Also, quick testing shows that NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContent)
should happen earlier, probably right after NS_INTERFACE_MAP_ENTRY(nsPIDOMEventTarget)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

Comment 5

[Michal Novotny [:michal]]

(In reply to comment #0)
> > There are also a lot more hits in nsStandardURL::QueryInterface (more than
> > twice as many as on branch), so perhaps the callers of this would be good too.  

All relevant callgraphs are:

http://www.allpeers.com/download/michal/new/callgraph/FF2/nsStandardURL+QueryInterface.png
http://www.allpeers.com/download/michal/new/callgraph/FF2/nsCOMPtr_nsIClassInfo_+operator=.png
http://www.allpeers.com/download/michal/new/callgraph/FF2/XPCWrappedNative+GetNewOrUsed.png
http://www.allpeers.com/download/michal/new/callgraph/FF2/nsStandardURL+Equals.png

http://www.allpeers.com/download/michal/new/callgraph/FF3/nsStandardURL+QueryInterface.png
http://www.allpeers.com/download/michal/new/callgraph/FF3/XPCWrappedNative+GetNewOrUsed.png
http://www.allpeers.com/download/michal/new/callgraph/FF3/nsStandardURL+Equals.png

Lot of calls seems to be from JS? I'm not sure that I can get more info about this from profiler :-/

Comment 6

[Benjamin Smedberg]

I don't think that simply tweaking the order of QI is going to help much: data shows that QI has a 70% miss rate in general. See bug 412320 for a thought I had about generated QI that I'll need to test in practice.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> I don't think that simply tweaking the order of QI is going to help much:
Not much, but it is easy way to reduce some .Equals calls.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 297001 [details] [diff] [review]
something like this

Peter, would this make sense to you? This is way simpler than bug 412320, which may take some time before getting fixed.
Doesn't probably affect much, but at least all QIs to nsIContent/etc get a bit faster.

Comment 9

[Peter Van der Beken [:peterv]]

Can you at least show that it improves performance?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 297001 [details] [diff] [review]
something like this

ok, asking review if I figure some testcase for this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Number of QI on nsGenericElement is in 1.9 even lower than in 1.8. Maybe there
> is probably more interfaces and that's whay nsID::Equals() has increased?

That might be.  Or someone may be doing more QIs on interfaces that elements don't implement.  Would it be possible to get one more level on the nsXULElement QIs, perhaps?

> Lot of calls seems to be from JS? 

Hard to say.  There are a lot more calls from nsQueryInteface::operator().  Can we limit the output to things that call nsQueryInteface::operator() which then calls nsStandardURL::QueryInterface?  That might clean up the mess a little bit...

Comment 12

[Michal Novotny [:michal]]

(In reply to comment #11)
> > Number of QI on nsGenericElement is in 1.9 even lower than in 1.8. Maybe there
> > is probably more interfaces and that's whay nsID::Equals() has increased?
> 
> That might be.  Or someone may be doing more QIs on interfaces that elements
> don't implement.  Would it be possible to get one more level on the
> nsXULElement QIs, perhaps?

http://www.allpeers.com/download/michal/new/callgraph/FF2/nsXULElement+QueryInterface.png
http://www.allpeers.com/download/michal/new/callgraph/FF3/nsXULElement+QueryInterface.png

Comment 13

[Michal Novotny [:michal]]

(In reply to comment #11)
> > Lot of calls seems to be from JS? 
> 
> Hard to say.  There are a lot more calls from nsQueryInteface::operator().  Can
> we limit the output to things that call nsQueryInteface::operator() which then
> calls nsStandardURL::QueryInterface?  That might clean up the mess a little
> bit...

AQTime can't do such filtering. I don't find all the callgraphs much useful. I'll try to generate statistics about nsID::Equals() using stackwalk. So I can also do statistics about miss rate etc.

I uploaded to web also callgrind output from linux. Data can be browsed using kcachegrind.

FF2
http://www.allpeers.com/download/michal/callgrind/ff2/callgrind.out.2809.gz

FF3
http://www.allpeers.com/download/michal/callgrind/ff3/callgrind.out.28343.gz

FF3 (with some optimization in fontcache)
http://www.allpeers.com/download/michal/callgrind/ff3/fc-patch/callgrind.out.23583.gz

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, looks like most of the XULElement QIs are coming from XPConnect one way or anther.  Perhaps we should turn this around and see what interfaces nsGenericElement is being QIed to, and for each one how many Equals() calls we end up making.  That might be more illustrative...

Comment 15

[Michal Novotny [:michal]]

(In reply to comment #11)
> Hard to say.  There are a lot more calls from nsQueryInteface::operator().  Can
> we limit the output to things that call nsQueryInteface::operator() which then
> calls nsStandardURL::QueryInterface?  That might clean up the mess a little
> bit...
> 

I obtained the data by dumping stack at every call to nsID::Equals(). Stackwalking code was crashing in release build so I had to do it in debug build :-/

There were 45432 calls to nsID::Equals() from nsStandardURL::QueryInterface from nsQueryInteface::operator(). QueryInterface() was called 12409x (10826x interface was found and 1583x wasn't). Callgraph for successful QIs is at http://www.allpeers.com/download/michal/nsStandardURL+QueryInterface_20levels_min100calls.png
It is quite large so to keep it as small as possible only calls with count 100 and more are there.

I can't easily find all QIs that were unsuccessful but I can see 1573 of them. They triggered 12850 calls to nsID::Equals().

Callgraph for those is at:
http://www.allpeers.com/download/michal/nsStandardURL+QueryInterface_failed_20levels_min10calls.png
Minimum count for this graph was 10.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Michal, thanks for those!

On the QI hit side, it looks like a lot of QIs to nsIURL and nsIMutable.  Perhaps we should move nsIMutable up in the QI impl?  And for moz2 think about making all URIs immutable, pretty please?

On the QI miss side, nsINestedURI is very prominent, which makes sense: it's used in various jar: stuff, security checks, etc.  Perhaps we should consider putting in a clause that bails out early saying we don't implement that interface?

On the other hand, maybe Benjamin's code-gen thing will help here?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> And for moz2 think about
> making all URIs immutable, pretty please?

Sure -- just need to get to it by hand, unless you think static analysis would be helpful.

> On the QI miss side, nsINestedURI is very prominent, which makes sense: it's
> used in various jar: stuff, security checks, etc.  Perhaps we should consider
> putting in a clause that bails out early saying we don't implement that
> interface?

We should do something short-term if we can here.

> On the other hand, maybe Benjamin's code-gen thing will help here?

Bug on that? Not 1.9/fx3 fodder I presume.

/be

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #17)
> Sure -- just need to get to it by hand, unless you think static analysis would
> be helpful.

That's immutability as part of the interface, not immutability in how we use them -- a design problem, not a static analysis one.

> Bug on that? Not 1.9/fx3 fodder I presume.

Bug 412320, and I think the plan *was* 1.9-fodder.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> That's immutability as part of the interface,

Correct.  Change nsIURI so that the setters would return a new URI object, identical to the original one, except for the part the setter was changing.  Fixing C++ code to deal should not be too bad, especially since it'll just fail to compile if it needs fixing.  We don't actually have all that many URI-setter-calling things in our code, thank God (since in most cases, such a caller is buggy unless it's cloned the URI first).  JS will be a harder issue, especially extension JS.  And static analysis wouldn't help there, of course.

Of course I'm not sure I've sold everyone on this approach yet... ;)

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> > That's immutability as part of the interface,

Cool, I'm in favor. Get on it ;-).

> JS will be a harder issue,
> especially extension JS.  And static analysis wouldn't help there, of course.

Conservative static analysis can help, but we'll have to see how the tools evolve (David Teller is working on one, Jstify IIRC.

/be

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Unassigning Michal to move bugs back to triage pool.

Comment 22

[Andrew McCreight [:mccr8]]

There's been a bit of work on profiling and improving QIs since 2008, so I think we can close this.