Last Comment Bug 614171 - Store the nodeName on the nodeinfo for DOM elements
: Store the nodeName on the nodeinfo for DOM elements
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal with 1 vote (vote)
: mozilla6
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: domperf 614164
  Show dependency treegraph
 
Reported: 2010-11-22 21:51 PST by Boris Zbarsky [:bz]
Modified: 2011-05-05 13:38 PDT (History)
10 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple performance testcase (212 bytes, text/html)
2011-05-04 09:18 PDT, Boris Zbarsky [:bz]
no flags Details
part 1. Switch away from Init() to using a constructor for nodeinfo. (6.12 KB, patch)
2011-05-04 10:28 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 2. Cache the qualified name, in normal and corrected case, on the nsINodeInfo. (12.02 KB, patch)
2011-05-04 10:30 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 3. deCOM qualified name getters a bit. (9.81 KB, patch)
2011-05-04 10:31 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 4. Eliminate nsINodeInfo::GetLocalName in favor of GetName. (8.13 KB, patch)
2011-05-04 10:32 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 5. Inline the cheap Equals() methods on nsINodeInfo. (4.84 KB, patch)
2011-05-04 10:33 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2010-11-22 21:51:50 PST
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 [:bz] 2010-11-22 22:24:58 PST
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) PTO Until July 5th 2010-11-23 00:55:04 PST
I concur!
Comment 3 Olli Pettay [:smaug] 2010-11-23 06:17:27 PST
Need to be just a bit careful when adopting element from HTML document
to non-HTML-document and vise versa.
Comment 4 Boris Zbarsky [:bz] 2010-11-23 06:57:23 PST
Adopting creates gets new nodeinfo from the new nodeinfo manager, no?
Comment 5 Olli Pettay [:smaug] 2010-11-23 06:59:26 PST
Yeah. But wanted to just remind that caches shouldn't be copied.
Comment 6 Boris Zbarsky [:bz] 2010-11-23 07:03:11 PST
Right; nodeinfo initialization should look exactly like it does now in terms of its public api.
Comment 7 Boris Zbarsky [:bz] 2011-02-08 15:18:58 PST
Fwiw, webkit's equivalent of nodeinfo has localName, localNameUpper, namespace, prefix.
Comment 8 Boris Zbarsky [:bz] 2011-05-04 09:18:48 PDT
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 Olli Pettay [:smaug] 2011-05-04 09:41:03 PDT
after your patch, where are we spending most of the time?
Comment 10 Boris Zbarsky [:bz] 2011-05-04 10:27:26 PDT
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 [:bz] 2011-05-04 10:28:21 PDT
Created attachment 530069 [details] [diff] [review]
part 1.  Switch away from Init() to using a constructor for nodeinfo.
Comment 12 Boris Zbarsky [:bz] 2011-05-04 10:30:57 PDT
Created attachment 530070 [details] [diff] [review]
part 2.  Cache the qualified name, in normal and corrected case, on the nsINodeInfo.
Comment 13 Boris Zbarsky [:bz] 2011-05-04 10:31:51 PDT
Created attachment 530071 [details] [diff] [review]
part 3.  deCOM qualified name getters a bit.
Comment 14 Boris Zbarsky [:bz] 2011-05-04 10:32:20 PDT
Created attachment 530072 [details] [diff] [review]
part 4.  Eliminate nsINodeInfo::GetLocalName in favor of GetName.
Comment 15 Boris Zbarsky [:bz] 2011-05-04 10:33:23 PDT
Created attachment 530073 [details] [diff] [review]
part 5.  Inline the cheap Equals() methods on nsINodeInfo.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-04 11:19:38 PDT
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 [:bz] 2011-05-04 11:22:01 PDT
> 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) PTO Until July 5th 2011-05-04 11:24:23 PDT
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) PTO Until July 5th 2011-05-04 11:26:31 PDT
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) PTO Until July 5th 2011-05-04 11:28:58 PDT
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 [:bz] 2011-05-04 11:32:28 PDT
> 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 [:bz] 2011-05-04 11:33:09 PDT
> 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 [:bz] 2011-05-04 11:37:19 PDT
> 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);
  }

Note You need to log in before you can comment on or make changes to this bug.