Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Description • 19 years ago

Towards the end of the security meeting we ended up discussing QI performance.
Two ideas were brought up that both need statistical data for which interfaces
we QI to on which classes. Using such data we can

1. Add explicit early-returns for interfaces that we QI to often but don't
implement. This way we won't have to run down a long list of iid-checks but can
rather bail right away.

2. Rearrange order of iid-checks so that the interfaces we QI to most often are
on top of the list.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 19 years ago

So... doing this in general would take work, but I did it for HTML elements by
applying a patch (coming up), then running bc's spider at depth 1 on
http://bclary.com/2004/07/10/tests/test-sites.html and post-processing the
result (with a script that's also coming up).  I really wish we had a better way
to get interface names for non-idl stuff.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 19 years ago

I put in some common IIDs by hand, but that really doesn't scale, esp if people
change IIDs....

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 19 years ago

First thing that jumps out at me is nsIXPConnectWrappedJS.  I believe we do a
QI to that in XPConnect in various cases, but in particular when first creating
an XPCWrappedNative...

That said, the 25000-some QIs to that put it at about 10th place in the total
QI calls list.

Also note that this build doesn't have my caps/xpconnect patches that should
eliminate most of the QIs to nsIScriptObjectPrincipal.

I guess I'm wondering where to go from here.  Should I try for a bigger
dataset?  Or instrumenting a wider variety of classes?	Or both?

Also note that I'm not outputting the concrete class anywhere here.  Should I
be, perhaps?  Do we care?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 5 • 19 years ago

nsIDOMEventReceiver is pretty noteworthy. Mostly because it is implemented as a
tearoff and so it will often result in a malloc in addition to the normal QI
cost. We should probably consider making that a normal interface or caching a
pointer in nsDOMSlots.

I think it might be interesting to get these stats on a per element bases. And
for xul elements too. I'm wondering if we're QIing to nsIImageLoadingContent and
nsILink on all elements, or if we just do it a lot on the elements where it
makes sense.

Other then that, i think we might be able to rearrange code to get nsIDOMNode
and nsIDOMElement be tested earlier.

	Benjamin Smedberg
	Comment 6 • 19 years ago

Perhaps we should resurrect the possibility of table-driven-QI? According to Don
Box table-driven QI can significantly improve performance due to locality of CPU
cache and reduced codesize (single shared QI impl). It might allow allow us to
instrument QI to do the other optimizations here more easily.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 19 years ago

> We should probably consider making that a normal interface or caching a
> pointer in nsDOMSlots.

I think generally caching our tearoffs in domslots and using an aggregated
refcount with the content node would be a great idea....

I'll try to change the instrumentation to note the element class.

	Darin Fisher
	Comment 8 • 19 years ago

> Perhaps we should resurrect the possibility of table-driven-QI?

How would you support NS_INTERFACE_MAP_ENTRY_CONDITIONAL (and some of the other
nasty QI macros that we have)?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 9 • 19 years ago

We also have tearoffs that can be tricky. Is there a writeup somewhere about how
tabledriven QI works?

	Darin Fisher
	Comment 10 • 19 years ago

> We also have tearoffs that can be tricky. Is there a writeup somewhere about 
> how tabledriven QI works?

Don Box's book is the best reference I know.  I have a copy sitting in my book
shelf, so I've never bothered to search the web looking for similar info.  In a
nut-shell, you replace the QI implementation with a simple table of IIDs and
offsets.  The offsets are used to increment the |this| pointer to get at the
correct vtable for the corresponding IID.  Then, you just loop over the table,
and when the IIDs match, you adjust the |this| pointer according to the offset
and return that value.  So, it's pretty trivial to implement NS_IMPL_ISUPPORTS.
 The challenge is in supporting all of the variations allowed by QueryInterface.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 11 • 19 years ago

You have to supply the class that is queried too, no? And that along with the
IID is the key into the table. Can you use the vtablepointer as class? Or would
we just a giant enum?

	Benjamin Smedberg
	Comment 12 • 19 years ago

You don't have to do the variations table-driven: you can do something like this:

** Simple (no-tearoff, no-conditional) case **

nsresult Class::QueryInterface(aIID, aResult)
{
  return Table_driven_QI(NS_STATIC_CAST(void*, this), static_lookup_table, aIID,
aResult);
}

** Complex case **

nsresult Class::QueryInterface(aIID, aResult)
{
  nsresult rv = Table_driven_QI(as above);
  if (NS_SUCCEEDED(rv))
    return rv;

  do conditional_and_tearoff_QI_manually_here
}

	Darin Fisher
	Comment 13 • 19 years ago

bsmedberg: exactly, but it's not clear how to do that with our existing macros :-/
i think we'd probably have to touch a lot of code to fix up the macro usage. 
any clever ways to avoid that?

	Benjamin Smedberg
	Comment 14 • 19 years ago

I'm willing to fixup the NS_IMPL_ISUPPORTSx macros first, and then do some
reordering and fixup of the INTERFACE_MAP macros which are harder... I don't
think it's going to be possible to automatically fixup the current INTERFACE_MAP
usages which have _CONDITIONAL or _TEAROFF usages.

	Darin Fisher
	Comment 15 • 19 years ago

Sounds like a good plan to me.

	Benjamin Smedberg
	Comment 16 • 19 years ago

Table-driven QI forked off to bug 313309.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 19 years ago

This is a log from bc's spider on the top 70 sites again.  Note that this was
using the spider chrome, not the usual Firefox chrome and that I did not
interact with the browser much while it was running.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 19 years ago

Interesting things to note here:

1)  Whence QIs to nsPIDOMWindow?  Someone QIing event targets?
2)  Lots of QIs on XUL to nsIDOMEventTarget.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 19 years ago

Ah, the nsPIDOMWindow QI is a debug-only QI in
nsDOMEvent::SetTarget/SetCurrentTarget.

Come to think of it, doing such stats in a debug build is silly.  I'll try to
patch one of my opt builds and retest.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 22 • 19 years ago

I'm very curious who is QIing to nsIDOMHTMLSelectElement and
nsIDOMHTMLOptGroupElement. My money is on someone is searching for
select/optgroup elements and QIs rather then checking the nodename.

	Darin Fisher
	Comment 23 • 19 years ago

Jonas: the results are certainly bogus in light of Assert_NoQueryNeeded :-)

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 19 years ago

I filed bug 313343 on the nsIDocumentObserver change.  I'll build an opt build
with this instrumentation and have some data on Sunday, hopefully.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 19 years ago

As expected, a lot less QI to nsIStyledContent.  But the QIs to nsIDOMHTMLSelectElement and nsIDOMHTMLOptGroupElement are still there.

The nsIDOMHTMLSelectElement QIs come from frame construction -- ContentAppended and ContentInserted always check whether the parent content node is an <html:select> before doing a WipeContainingBlock check.  That's silly.  What really matters here, imo, is not the type of the parent node but the type of the parent _frame_, since that's what WipeContainingBlock cares about.  We should fix this.

The nsIDOMHTMLOptGroupElement QIs come from <select> layout.  In particular, GetMaxOptionHeight() (in nsListControlFrame.cpp) QIs all options to this interface because it would need to recurse in that case.  We could try to move to a tag test here, I suppose, or just go with a QI to nsIDOMHTMLOptionElement instead (would be hit more often) and reverse the two branches of the if.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 19 years ago

No more nsIDOMHTMLSelectElement.  We still have misses for nsIXPConnectWrappedJS, nsIScriptObjectPrincipal (which is on its way out), nsIDOMHTMLOptGroupElement (should probably file a bug), nsIDocument, nsIScriptGlobalObject, nsIDOMCharacterData.  And small fry after that.  Note that the perl script needs new IIDs for nsIContent and nsIDocument to generate this output; I won't bother attaching the update, since those IIDs keep changing...

	Simon Fraser [no longer active]
	Comment 27 • 19 years ago

See also bug 71528.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 19 years ago

I'm removing this from my tree for now...

	Doug Turner (:dougt)
	Comment 29 • 17 years ago

mass reassigning to nobody.