Closed Bug 466224 Opened 11 years ago Closed 11 years ago

Make quickstubs call nsINode/nsINodeList methods

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: peterv, Assigned: peterv)

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
We can avoid an AddRef/Release pair and in some cases virtual methods.

For example for .parentNode we'd have one QI call to get the thisptr offsets (no AddRef), some bit twiddling (reinterpret_cast<nsINode*>(mParentPtrBits & ~kParentBitMask)) and then wrapping, which will use the cached wrapper if the parent node has been wrapped before (no AddRef/Release of parent node or its wrapper in that case).

I've also moved a bunch of the base functions to implement nsIDOMNode from nsGenericDOMDataNode/nsGenericElement/nsDocument to nsINode. I haven't done it for all of them on nsDocument/nsDOMAttribute. For example GetNext/PreviousSibling currently directly return null, I could call through to the base implementation on nsINode, which would fail on GetNodeParent and then return null. Not sure if we want to do that instead.

Probably want review from Jason on the qsgen.py part.
Hmm, nsIDOMNode_GetOwnerDocument can't call GetOwnerDoc without nodetype checks.
Flags: blocking1.9.1?
Blocking on this one as well. QI's cost way more than you'd think, and way more than they used to. Eliminating them from the hot path is absolutely worth it, and this ticks off significant amount of time in performance tests.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #349484 - Attachment is obsolete: true
Comment on attachment 350374 [details] [diff] [review]
v1.1

>+  nsIContent* GetSibling(PRInt32 aOffset)
>+  {
>+    nsINode *parent = GetNodeParent();
>+    if (!parent) {
>+      return nsnull;
>+    }
>+
>+    return parent->GetChildAt(parent->IndexOf(this) + aOffset);
>+  }

This doesn't look right if IndexOf returns -1, i.e. if the node is an anonymous child.
Note that I didn't change that part:

-  PRInt32 pos = parent->IndexOf(this);
-  nsIContent *sibling = parent->GetChildAt(pos - 1);

-  PRInt32 pos = parent->IndexOf(this);
-  nsIContent *sibling = parent->GetChildAt(pos + 1);

Want to file a separate bug on that?
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #350374 - Attachment is obsolete: true
Attachment #354830 - Flags: superreview?(jst)
Attachment #354830 - Flags: review?(jst)
Comment on attachment 354830 [details] [diff] [review]
v1.2

- In nsINodeList.h:

+#include "nsIDOMNodeList.h";

Loose the ';'

- In xpcquickstubs.h

+PRBool
+xpc_qsSameResult(nsISupports *result1, nsISupports *result2)
+{
+    return SameCOMIdentity(result1, result2);
+}

Make that inline (doesn't link on linux w/o it due to this function being defined in two object files).

r+sr=jst
Attachment #354830 - Flags: superreview?(jst)
Attachment #354830 - Flags: superreview+
Attachment #354830 - Flags: review?(jst)
Attachment #354830 - Flags: review+
Backed out, this was failing with

*** 34577 INFO Running /tests/dom/tests/mochitest/dom-level1-core/test_hc_attrsetvalue1.html...
*** 34578 INFO TEST-PASS | attrChildNotNull
*** 34579 INFO TEST-PASS | attrValue
*** 34580 INFO TEST-PASS | attrNodeValue
*** 34581 ERROR TEST-UNEXPECTED-FAIL | firstChildValue | got "Tomorrow", expected "impl reused node"
I can run all of dom/tests/mochitest/dom-level1-core without any failure :-(.
Wonder if this is an opt vs debug problem.
Could be, it was my opt build where I was reproducing it.
Attached patch v1.3Splinter Review
This is what I checked in. I had to make nsDOMAttribute::GetChildArray update the text value of the child. It worked in debug builds because we also call the plain DOM method to assert that both methods return the same result, and one of the earlier DOM method calls updated the text value.
On branch I won't call GetChildArray (because bug 454821 didn't land on branch), so the patches are slightly different.
Attachment #354830 - Attachment is obsolete: true
Attachment #355289 - Flags: superreview+
Attachment #355289 - Flags: review+
Attached patch v1.3 branchSplinter Review
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.