Closed Bug 614171 Opened 9 years ago Closed 9 years ago

Store the nodeName on the nodeinfo for DOM elements

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(6 files)

nodeName for elements is the qualified name except in an HTML document for HTML elements, for which it's the uppercased qualified name.

There's no reason I see to uppercase (and copy, etc) on every single nodeName get (of which jquery seems to do lots).  Why not just store the relevant string in the nodeinfo?
In general, it seems like we should move things like tagName, nodeName, etc to nodeinfo, have it cache whatever info it needs, and make those all non-virtual.  The memory increase from adding some members to nodeinfos should be pretty minor, I'd hope.....
OS: Mac OS X → All
Hardware: x86 → All
Need to be just a bit careful when adopting element from HTML document
to non-HTML-document and vise versa.
Adopting creates gets new nodeinfo from the new nodeinfo manager, no?
Yeah. But wanted to just remind that caches shouldn't be copied.
Right; nodeinfo initialization should look exactly like it does now in terms of its public api.
Assignee: nobody → bzbarsky
Priority: -- → P2
Fwiw, webkit's equivalent of nodeinfo has localName, localNameUpper, namespace, prefix.
Time on this goes from 230ms to 100ms.  Of course Chrome and Safari are at 40ms... ;)
after your patch, where are we spending most of the time?
11% mjit-generated code.
24% the slow path mjit takes for getter-backed property gets due to bug 557358
 9% self time in the nsIDOMNode_GetNodeName quickstub
18% converting the XPCOM string return value to a jsval
24% getting the nsIDOMNode* out of the given JSObject
 5% Atomic increment on the return value string in nsGenericElement::GetNodeName
 3% nsAString_internal::Assign in GetNodeName

plus some long-tail bits.  So if we had a custom quickstub for this that just took the return value and jsvalified it, this could be a lot faster.  But doing that involves putting GetNodeName on the nodeinfo, and in particular knowing what the nodeinfo is for (because the algorithm is different for elements, cdata sections, documents, etc).

Or we could just push this up to nsINode, I guess....
Flags: in-testsuite?
Priority: P2 → P1
Whiteboard: [need review]
Comment on attachment 530070 [details] [diff] [review]
part 2.  Cache the qualified name, in normal and corrected case, on the nsINodeInfo.

Review of attachment 530070 [details] [diff] [review]:

::: content/base/src/nsNodeInfo.cpp
@@ +125,5 @@
+  if (aPrefix) {
+    aPrefix->ToString(mQualifiedName);
+    mQualifiedName.Append(PRUnichar(':'));
+  }
+  mQualifiedName.Append(nsDependentAtomString(mInner.mName));

Actually, it might be worth having a separate codepath for the no-prefix case which uses an nsAtomString. That way the string at the atom can share buffer and no allocation will happen.

@@ +127,5 @@
+    mQualifiedName.Append(PRUnichar(':'));
+  }
+  mQualifiedName.Append(nsDependentAtomString(mInner.mName));
+
+  // Qualified name in corrected cas

case
Attachment #530070 - Flags: review?(jonas) → review+
> Actually, it might be worth having a separate codepath for the no-prefix case
> which uses an nsAtomString. That way the string at the atom can share buffer
> and no allocation will happen.

But that trades off time in nodeinfo creation (rare, I hope) for time on every qualified name get, right?  And the space savings would be order of the atomized string, which is pretty small, since we don't have _that_ many nodeinfos.  I can do that if you want, but I'm not sure it's really desirable.

> case

Fixed locally.
Comment on attachment 530071 [details] [diff] [review]
part 3.  deCOM qualified name getters a bit.

Review of attachment 530071 [details] [diff] [review]:

::: content/base/src/nsGenericElement.cpp
@@ +5202,5 @@
 {
   PRInt32 indent;
   for (indent = aIndent; --indent >= 0; ) fputs("  ", out);
 
+  nsAutoString buf(mNodeInfo->QualifiedName());

Use a nsString& instead?
Attachment #530071 - Flags: review?(jonas) → review+
Comment on attachment 530072 [details] [diff] [review]
part 4.  Eliminate nsINodeInfo::GetLocalName in favor of GetName.

Review of attachment 530072 [details] [diff] [review]:

fahrvergnügen
Attachment #530072 - Flags: review?(jonas) → review+
Comment on attachment 530073 [details] [diff] [review]
part 5.  Inline the cheap Equals() methods on nsINodeInfo.

Review of attachment 530073 [details] [diff] [review]:

::: content/base/public/nsINodeInfo.h
@@ +274,5 @@
+  {
+    return mInner.mName->Equals(aName) && mInner.mNamespaceID == aNamespaceID &&
+      (mInner.mPrefix ? mInner.mPrefix->Equals(aPrefix) : aPrefix.IsEmpty());
+  }
+

Are all of these really used?
Attachment #530073 - Flags: review?(jonas) → review+
> Use a nsString& instead?

Done.

> fahrvergnügen

Google translate fail (though I get the general idea, given r+ and no other comments!)
> Are all of these really used?

I didn't check.  dxr is broken as usual, searching for it via mxr is a pain, and compiling to test takes a while.  I can look into it if you really want.
> Actually, it might be worth having a separate codepath for the no-prefix case

Jonas explained this on irc.  I'll do this:

  if (aPrefix) {
    aPrefix->ToString(mQualifiedName);
    mQualifiedName.Append(PRUnichar(':'));
    mQualifiedName.Append(nsDependentAtomString(mInner.mName));
  } else {
    mInner.mName->ToString(mQualifiedName);
  }
Whiteboard: [need review] → [need landing]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.