243727 - Consider storing nsIDOMElement* in the getElementById hashtable

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See profile data in bug 243726.  On that DHTML testcase, 50% of the time spent
under GetElementById is spent under QI of the nsIContent to nsIDOMElement.

Now granted, the time spent under GetElementById is only 0.2% of the total time
taken.  So maybe this is a non-issue (and in fact, maybe this is not worth the
perf hit of QIing on addition to the table and the cleanliness hit of using a
weak ptr to an interface we got via QI -- we're assuming it's not a tearoff).

jst, what do you think?

Comment 1

[Johnny Stenback (:jst)]

Hmm, I guess I'm not entirely against it, but I don't really like the idea
either. Maybe we could reorganize the order of the IIDs in the QI
implementations to get a hit for nsIDOMElement sooner in stead? Or are we just
looking at function call overhead here and not that QI on a HTML element is slow?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't actually know how to evaluate which it is...  I suspect function call
overhead is the real problem; optimizing the QI order based on one testcase
isn't really right, imo.

Comment 3

[Johnny Stenback (:jst)]

You're absolutely right, but looking at the current QI, we check IIDs in this
order (for a generic HTML element):

  nsIHTMLContent
  nsIContent
  nsIStyledContent
  nsIDOM3Node
  nsIDOMEventReceiver
  nsIDOMEventTarget
  nsIDOM3EventTarget
  nsISupports
  nsIDOMNode
  nsIDOMElement
  nsIDOMHTMLElement
  nsIDOMNSHTMLElement
  nsIDOMElementCSSInlineStyle
  *element class interfaces*
  nsIClassInfo
  *XBL*

And while one testcase isn't enough to show much real world data, I'm fairly
sure we could improve things in *that* list :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

hmm... yeah....

Comment 5

[Mike Schroepfer]

Looking at just the getElementByID performance for HTML at the testcase in the URL:

Minefield: 36ms
Safari 3.0.0.4: 4ms
Opera 9.5b1: 6ms

So 6-9x delta.  Gotta be some low hanging fruit there :-).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Mike, one question.  How does that testcase scale if you go to 10000 nodes instead of 1000?  It might actually scale linearly for us, depending on whether all those nodes the testcase creates are ending up in the hashtable (which I suspect they are).

And frankly, I doubt this bug is the issue on that testcase, though I would welcome a profile proving me wrong.  I'll bet that particular testcase is dominated by xpconnect and js code.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I actually think we should WONTFIX this. While the QI might be a large portion of time spent in getElementById, I've never seen the getElementById time be very large. Usually we're spending way more time actually getting to the getElementById implementation, or on completely separate things. See comment 0 where we're only spending a total of .2% of the time inside getElementById. 

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't think we QI to nsIDOMElement anymore.