453929 - [FIX]Dromaeo profile: "DOM Query: getElementsByClassName"

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

In Dromaeo v2, the "DOM Query" section has a test for getElementsByClassName.

Safari takes 0-1ms to complete this test.
m-c takes 60ms

I'm not quite sure of the architectural considerations here, so it's possible we should just live with the difference, but here's the relevant profile:

I've flattened recursion, since we recurse deeply in nsContentList::PopulateWith

+ 99.1%, js_Interpret, libmozjs.dylib
| + 91.9%, js_NativeGet, libmozjs.dylib
| | + 91.7%, _ZL30nsIDOMHTMLCollection_GetLengthP9JSContextP8JSObjectlPl, XUL
| | | + 91.6%, nsContentList::GetLength(unsigned int*), XUL
| | | | + 91.6%, nsContentList::Length(int), XUL
| | | | | + 91.6%, nsContentList::PopulateSelf(unsigned int), XUL
| | | | | | + 91.6%, nsContentList::PopulateWithStartingAfter(nsINode*, nsINode*, unsigned int&), XUL
| | | | | | | + 91.6%, nsContentList::PopulateWith(nsIContent*, unsigned int&), XUL
| | | | | | | | + 40.6%, nsContentList::Match(nsIContent*), XUL
| | | | | | | | | + 25.9%, nsDocument::MatchClassNames(nsIContent*, int, nsIAtom*, void*), XUL
| | | | | | | | | | + 7.7%, nsStyledElement::GetClasses() const, XUL
| | | | | | | | | | |   1.7%, nsAttrAndChildArray::GetAttr(nsIAtom*, int) const, XUL
| | | | | | | | | |   2.1%, nsGenericDOMDataNode::GetClasses() const, XUL
| | | | | | | | | | - 0.7%, nsAttrValue::Contains(nsIAtom*, nsCaseTreatment) const, XUL
| | | | | | | | |   1.5%, nsGenericDOMDataNode::GetClasses() const, XUL
| | | | | | | | |   1.2%, nsStyledElement::GetClasses() const, XUL
| | | | | | | |   10.1%, nsAttrAndChildArray::GetSafeChildAt(unsigned int) const, XUL
| | | | | | | |   8.9%, nsGenericElement::GetChildCount() const, XUL
| | | | | | | |   3.9%, nsGenericElement::GetChildAt(unsigned int) const, XUL
| | | | | | | |   2.8%, nsGenericDOMDataNode::GetChildCount() const, XUL
| | | | | | | |   0.8%, nsDocument::MatchClassNames(nsIContent*, int, nsIAtom*, void*), XUL
| - 2.7%, js_GetPropertyHelper, libmozjs.dylib
| - 1.6%, js_GetProperty, libmozjs.dylib
| - 1.2%, _ZL39nsIDOMNSDocument_GetElementsByClassNameP9JSContextjPl, XUL
- 0.6%, _ZL39nsIDOMNSDocument_GetElementsByClassNameP9JSContextjPl, XUL

Comment 1

[Benjamin Smedberg]

cc'ing sayrer since he implemented .getElementsByClassName intially...

Comment 2

[Robert Sayre]

Is it in a loop? I think they might be caching stuff.

Comment 3

[Benjamin Smedberg]

Well kinda, but it's asking for a different class each time:

http://github.com/jeresig/dromaeo/tree/master/tests/dom-query.html#L129

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'll be filing some bugs blocking this based on profiles, but the heart of the matter is that Safari caches the lists and we don't.  And this test does in fact hit this cache.  A lot.  In fact, it probably throws out the one time that any real work is done as an outlier.

We _can_ speed up said real work by a good bit, though: that's what the dependant bugs are about.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a cache

This takes the Dromaeo dom-query test from ~800 to ~3000.

Comment 6

[Mike Shaver (:shaver emeritus)]

That gain is ... acceptable.  Can we use this cache in other places too?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Not really.  At this point the only get* content lists that we _don't_ effectively cache (either as members of the relevant root nodes or in the global cache here) are the XUL getElementsByAttribute(NS) ones.  And those would need to cache based on two strings (attr name and attr value), so can't go in this cache.

I should note that the 800->3000 change there corresponds to a 2% change in the overall dromaeo number.  :(

Comment 8

[Brendan Eich [:brendan]]

Far be it from me to object to raw PLDHashTable usage but is there a reason you can't use the spanky XPCOM template veneer?

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

It wouldn't be much better if I did, since I'd have to implement a whole new key class.  For the case of getting an uncached list I'd also have to have two hashtable ops instead of 1.

Comment 10

[Brendan Eich [:brendan]]

Cc'ing Luke, who is doing a C++ version of JSDHashTable in bug 515812 and may be able to test it by inspection against your use of PLDHashTable.

/be

Comment 11

[Luke Wagner [:luke]]

(In reply to comment #10)
> Cc'ing Luke, who is doing a C++ version of JSDHashTable in bug 515812 and may
> be able to test it by inspection against your use of PLDHashTable.

  > For the case of getting an uncached list I'd also have to have two
  > hashtable ops instead of 1.

From a quick look, I'm not sure why nsTHashTable::PutEntry/RawRemoveEntry don't allow you to achieve the same result with the same number of hash table operations.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, nsTHashTable does.  I hadn't realized that at the time I made the comment above.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 415889 [details] [diff] [review]
Add a cache

r=jst for this as is to get this optimization into the tree. I'm happy to tweak the hash usage here if we feel it's necessary, but I'm happy to take this as is.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/bb7f5fa01db7