Closed Bug 453929 Opened 13 years ago Closed 12 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: bzbarsky)

References

Details

(Keywords: perf)

Attachments

(1 file)

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
cc'ing sayrer since he implemented .getElementsByClassName intially...
Is it in a loop? I think they might be caching stuff.
Well kinda, but it's asking for a different class each time:

http://github.com/jeresig/dromaeo/tree/master/tests/dom-query.html#L129
Depends on: 454280
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.
Depends on: 454317
Attached patch Add a cacheSplinter Review
This takes the Dromaeo dom-query test from ~800 to ~3000.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #415889 - Flags: review?(jst)
That gain is ... acceptable.  Can we use this cache in other places too?
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.  :(
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
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.
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
(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.
Yeah, nsTHashTable does.  I hadn't realized that at the time I made the comment above.
Summary: Dromaeo profile: "DOM Query: getElementsByClassName" → [FIX]Dromaeo profile: "DOM Query: getElementsByClassName"
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.
Attachment #415889 - Flags: review?(jst) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/bb7f5fa01db7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 545302
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.