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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Benjamin Smedberg, Assigned: bz)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
cc'ing sayrer since he implemented .getElementsByClassName intially...

Comment 2

10 years ago
Is it in a loop? I think they might be caching stuff.
(Reporter)

Comment 3

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

http://github.com/jeresig/dromaeo/tree/master/tests/dom-query.html#L129
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.
Created attachment 415889 [details] [diff] [review]
Add a cache

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

Comment 11

9 years ago
(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
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Duplicate of this bug: 545302
You need to log in before you can comment on or make changes to this bug.