614171 - Store the nodeName on the nodeinfo for DOM elements

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.....

Comment 2

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

I concur!

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Need to be just a bit careful when adopting element from HTML document
to non-HTML-document and vise versa.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Adopting creates gets new nodeinfo from the new nodeinfo manager, no?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Yeah. But wanted to just remind that caches shouldn't be copied.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right; nodeinfo initialization should look exactly like it does now in terms of its public api.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fwiw, webkit's equivalent of nodeinfo has localName, localNameUpper, namespace, prefix.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple performance testcase

Time on this goes from 230ms to 100ms.  Of course Chrome and Safari are at 40ms... ;)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

after your patch, where are we spending most of the time?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch away from Init() to using a constructor for nodeinfo.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Cache the qualified name, in normal and corrected case, on the nsINodeInfo.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. deCOM qualified name getters a bit.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Eliminate nsINodeInfo::GetLocalName in favor of GetName.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Inline the cheap Equals() methods on nsINodeInfo.

Comment 16

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

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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 18

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

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?

Comment 19

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

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

Comment 20

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

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?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Use a nsString& instead?

Done.

> fahrvergnügen

Google translate fail (though I get the general idea, given r+ and no other comments!)

Comment 22

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> 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);
  }

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed:

  http://hg.mozilla.org/mozilla-central/rev/3d82b6d2ec58
  http://hg.mozilla.org/mozilla-central/rev/0226c56cdd58
  http://hg.mozilla.org/mozilla-central/rev/65844f8be46f
  http://hg.mozilla.org/mozilla-central/rev/eb59af71bd59
  http://hg.mozilla.org/mozilla-central/rev/0591c3a2b297