Store the nodeName on the nodeinfo for DOM elements

RESOLVED FIXED in mozilla6

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

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.....
I concur!
OS: Mac OS X → All
Hardware: x86 → All

Comment 3

7 years ago
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?

Comment 5

7 years ago
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.
Created attachment 530039 [details]
Simple performance testcase

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

Comment 9

6 years ago
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....
Created attachment 530069 [details] [diff] [review]
part 1.  Switch away from Init() to using a constructor for nodeinfo.
Attachment #530069 - Flags: review?(jonas)
Created attachment 530070 [details] [diff] [review]
part 2.  Cache the qualified name, in normal and corrected case, on the nsINodeInfo.
Attachment #530070 - Flags: review?(jonas)
Created attachment 530071 [details] [diff] [review]
part 3.  deCOM qualified name getters a bit.
Attachment #530071 - Flags: review?(jonas)
Created attachment 530072 [details] [diff] [review]
part 4.  Eliminate nsINodeInfo::GetLocalName in favor of GetName.
Attachment #530072 - Flags: review?(jonas)
Created attachment 530073 [details] [diff] [review]
part 5.  Inline the cheap Equals() methods on nsINodeInfo.
Attachment #530073 - Flags: review?(jonas)
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+
Attachment #530069 - 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]
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.