94456 - wrappers need performance increase, support conformance

Status: VERIFIED FIXED

(Core :: XSLT, defect, P4)

Description

[Axel Hecht]

There are two issues with the wrappers of the mozilla dom (all the stuff beneath
extensions/transformiix/source/xml/dom/mozImpl):

They're slow.
They suck for nodes that aren't nodes.

Huh? ok, mozilla doesn't have attribute nodes, and it doesn't keep attribute
nodes alive, so getting the same attribute node twice generates two different 
nodes.
sux.
We should do something clever, like to make MozillaAttr not wrap a mozilla
attribute node (that almost non existent thing), but perhaps just the attribute,
ownerElement, plus localName, prefix, namespace id. So that we don't generate
mozilla attribute nodes for transformiix attribute node wrappers.

Oh, yeah, same (and more) goes for namespace nodes, if we only had some.

SPEED. Peter used nsObjectHashtable for the wrappers, hoping they wouldn't suck.
Pfff. It's still linear search per bucket, AFAICT. How about this:
Count all mozilla element, text, comment, PI, CDATA nodes in the doc. (are 
all those nsIContents?)
Generate two arrays of that size, one for the nsObjects, one for txObjects.
Put all nsIContents into the nsObjectArray, and sort them binary.
To get a wrapper, do a binary search for the index, lookup the value at that
index in the txObjectArray, if it's non-null, return, otherwise, generate the 
wrapper, and store the wrapper, return.

I just tried increasing the bucket number for the hashtable, but order info
is still worse. Increasing from 16 to 256 just gave 158 to 155 secs. (match13)

Axel

Comment 1

[Axel Hecht]

OK, I should really test the build I'm changing, here goes for an opt build:
16 buckets: 22.99, 24.343, 24.543, 21.592
256 buckets: 23.021, 21.089, 24.951, 24.559, 24.8
seconds for xalan test match13. Note, this is a huge union test, so it may be
not really the best test.

Comment 2

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

I suggest we keep pointers the wrapper objects for nextSibling and perhaps 
firstChild. Those two is by far the most common steps we take in the DOM I 
think. (well, our current namespace support is probobly causing a lot of 
parentNode walking, but that shouldn't be the case for very long)

Comment 3

[Axel Hecht]

putting this a bit out of radar.
I just came up with this:
Any clue how creating the wrappers newly for each getWrapper instead of hashing
them would affect performance? I wonder if one can test this somehow, or measure.
(Or just cache attribute nodes, once we create them only for attribute axes)

Comment 4

[Peter Van der Beken [:peterv]]

While we're sharing nightmares:
How about implementing ref-counting on standalone. How about making refcounting
on standalone a no-op and use it. Oh, and don't forget the strings, the wrappers
now do a getNSString before jumping into Mozilla DOM land, would you provide
automatic conversion from String to nsAString? We should link in the standalone
XPCOM lib into standalone, ref-counting and strings and PL_Hash. No, seriously.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1 (diff -uwN)

Comment 7

[Peter Van der Beken [:peterv]]

attsets  5136    4519     12%
avts     29967   25685    14%
axis     1211    1036     14%
bottles  24188   20967    13%
breadth
 890940  340891   62%
chart    9499    8648      9%
creation 31559   26995    14%
current  1915    1697     11%
dbtail   29469   24693    16%
decoy    2252815 1801141  20%
depth    17057   16214     5%
encrypt  11243   8789     22%
         3304999 2281275  31%

Sorry.

Comment 8

[Peter Van der Beken [:peterv]]

alphabetize  36877   36067    2%
attsets       4271    3921    8%
avts         19730   17855   10%
axis          1007    1039   -3%
backwards     5824    5254   10%
bottles      25241   23070    9%
breadth      30375   28476    6%
brutal        8880    7847   12%
chart         9209    8772    5%
creation     27682   24898   10%
current       1635    1456   11%
dbtail       16994   15109   11%
decoy       316364  277884   12%
depth         8716    7977    8%
encrypt      10882    9896    9%
functions    77010   65524   15%
game          2829    2320   18%
html          1424    1349    5%
identity     33539   29594   12%
inventory     4533    3970   12%
metric        3055    2740   10%
number        2362    2206    7%
oddtemplate    973     866   11%
patterns    183022  155727   15%
prettyprint  45573   41571    9%
priority      2219    2473  -11%
products      9798    8932    9%
queens       28955   25673   11%
reverser     25250   23719    6%
stringsort   36342   33033    9%
summarize     7327    6710    8%
total         1554    1422    8%
tower        84672   76626   10%
trend        72235   65988    9%
union          793     782    1%
xpath          729     745   -2%
xslbench1     5440    5274    3%
xslbench2    22563   19584   13%
xslbench3    38659   31430   19%
          1214543  1077779   11%

Axel ate some of my improvements. Will attach a new patch.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

Reviews anyone?

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Still waiting for reviews.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 12

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

Comment on attachment 89554 [details] [diff] [review]
v2.1

>Index: source/xml/dom/mozImpl/MozillaAttr.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xml/dom/mozImpl/MozillaAttr.cpp,v
>retrieving revision 1.10
>diff -u -3 -r1.10 MozillaAttr.cpp
>--- source/xml/dom/mozImpl/MozillaAttr.cpp	30 Oct 2001 23:45:57 -0000	1.10
>+++ source/xml/dom/mozImpl/MozillaAttr.cpp	28 Jun 2002 14:41:39 -0000
>@@ -38,21 +38,25 @@
>  */
> Attr::Attr(nsIDOMAttr* aAttr, Document* aOwner) : Node(aAttr, aOwner)
> {
>-    NS_ASSERTION(aAttr, "Wrapper needs nsObject");
>-    NS_ASSERTION(aOwner, "Wrapper needs owner document");
>-    if (!aAttr || !aOwner)
>-        return;
>+    nsCOMPtr<nsIDOMElement> parent;
>+    aAttr->GetOwnerElement(getter_AddRefs(parent));

You might wanna nullcheck the parent.

>+    CallQueryInterface(parent, &mParent);
>+
>+    nsAutoString nameString;
>+    aAttr->GetLocalName(nameString);
>+    mLocalName = NS_NewAtom(nameString);
>+    NS_ADDREF(mLocalName);

NS_NewAtom returns an addreffed atom, so this will leak.

>+
>     nsAutoString ns;
>     aAttr->GetNamespaceURI(ns);
>-    if (ns.IsEmpty()) {
>-        namespaceID = kNameSpaceID_None;
>-        return;
>+    mNamespaceID = kNameSpaceID_None;
>+    if (!ns.IsEmpty()) {
>+        NS_ASSERTION(aOwner->nsNSManager,
>+                     "owner document lacks namespace manager");
>+        if (aOwner->nsNSManager)
>+            aOwner->nsNSManager->GetNameSpaceID(ns, mNamespaceID);
>     }
>-    NS_ASSERTION(aOwner->nsNSManager,
>-                 "owner document lacks namespace manager");
>-    if (!aOwner->nsNSManager)
>-        return;
>-    aOwner->nsNSManager->GetNameSpaceID(ns, namespaceID);
>+    mNSId = mNamespaceID;
> }
> 
> /*
>@@ -69,12 +73,9 @@
>  */
> const String& Attr::getName()
> {
>-    NSI_FROM_TX(Attr)
>-
>-    nodeName.clear();
>-    if (nsAttr)
>-        nsAttr->GetName(nodeName.getNSString());
>-    return nodeName;
>+    NSI_FROM_TX(Attr);
>+    nsAttr->GetName(mNodeName.getNSString());
>+    return mNodeName;
> }
> 
> /*
>@@ -85,11 +86,9 @@
>  */
> MBool Attr::getSpecified() const
> {
>-    NSI_FROM_TX(Attr)
>+    NSI_FROM_TX(Attr);
>     MBool specified = MB_FALSE;
>-
>-    if (nsAttr)
>-        nsAttr->GetSpecified(&specified);
>+    nsAttr->GetSpecified(&specified);
>     return specified;
> }
> 
>@@ -100,25 +99,9 @@
>  */
> const String& Attr::getValue()
> {
>-    NSI_FROM_TX(Attr)
>-
>-    nodeValue.clear();
>-    if (nsAttr)
>-        nsAttr->GetValue(nodeValue.getNSString());
>-    return nodeValue;
>-}
>-
>-/*
>- * Call nsIDOMAttr::SetValue to set the value of this attribute.
>- *
>- * @return the attribute's value
>- */
>-void Attr::setValue(const String& aNewValue)
>-{
>-    NSI_FROM_TX(Attr)
>-
>-    if (nsAttr)
>-        nsAttr->SetValue(aNewValue.getConstNSString());
>+    NSI_FROM_TX(Attr);
>+    nsAttr->GetValue(mNodeValue.getNSString());
>+    return mNodeValue;
> }
> 
> /*
>@@ -128,12 +111,13 @@
>  */
> Node* Attr::getXPathParent()
> {
>-    NSI_FROM_TX_NULL_CHECK(Attr)
>+    NSI_FROM_TX(Attr);
>     nsCOMPtr<nsIDOMElement> ownerElem;
>-
>-    if (NS_SUCCEEDED(nsAttr->GetOwnerElement(getter_AddRefs(ownerElem))))
>-        return ownerDocument->createWrapper(ownerElem);
>-    return NULL;
>+    nsAttr->GetOwnerElement(getter_AddRefs(ownerElem));
>+    if (!ownerElem) {
>+        return nsnull;
>+    }
>+    return mOwnerDocument->createElement(ownerElem);
> }

You could use mParent here instead, no?


>+    PRBool success = PL_DHashTableInit(&mWrapperHashTable,
>+                                       &wrapper_hash_table_ops,
>+                                       this,
>+                                       sizeof(txWrapperHashEntry),
>+                                       PL_DHASH_MIN_SIZE);
>+    if (!success) {
>+        mWrapperHashTable.ops = nsnull;

you need to end the ctor here if this fails, otherwise you'll crash at the next
line.

>+    }
>+
>+    txWrapperHashEntry* entry =
>+        NS_STATIC_CAST(txWrapperHashEntry *,
>+                       PL_DHashTableOperate(&mWrapperHashTable,
>+                                            aDocument,
>+                                            PL_DHASH_ADD));
>+
>+    if (!entry->mWrapper) {
>+        entry->mWrapper = this;
>+    }

null-check entry for OOM.

>+    static PLDHashTableOps attribute_hash_table_ops =
>+    {
>+        PL_DHashAllocTable,
>+        PL_DHashFreeTable,
>+        txAttributeHashGetKey,
>+        txAttributeHashHashKey,
>+        txAttributeHashMatchEntry,
>+        PL_DHashMoveEntryStub,
>+        txAttributeHashClearEntry,
>+        PL_DHashFinalizeStub,
>+    };
>+    success = PL_DHashTableInit(&mAttributeNodes,
>+                                &attribute_hash_table_ops,
>+                                nsnull,
>+                                sizeof(txAttributeHashEntry),
>+                                PL_DHASH_MIN_SIZE);
>+    if (!success) {
>+        mAttributeNodes.ops = nsnull;

Same here, abort if this fails.


>+#define IMPL_CREATE_WRAPPER2(_txClass, _function)               \
>+_txClass* Document::_function(nsIDOM##_txClass* aNsObject)      \
>+{                                                               \
>+    NS_ASSERTION(aNsObject,                                     \
>+                 "Need a Mozilla object to create a wrapper."); \
>+                                                                \
>+    txWrapperHashEntry* entry =                                 \
>+        NS_STATIC_CAST(txWrapperHashEntry *,                    \
>+                       PL_DHashTableOperate(&mWrapperHashTable, \
>+                                            aNsObject,          \
>+                                            PL_DHASH_ADD));     \
>+                                                                \
>+    if (!entry->mWrapper) {                                     \

null-check entry.


>+    // Look up wrapper in the hash, except for attribute nodes since
>+    // they're in a separate attribute hash. Let them be handled in
>+    // createAttribute by falling through to the switch below.
>+    if (nodeType != nsIDOMNode::ATTRIBUTE_NODE) {
>+        txWrapperHashEntry* entry =
>+            NS_STATIC_CAST(txWrapperHashEntry *,
>+                           PL_DHashTableOperate(&mWrapperHashTable,
>+                                                aNode,
>+                                                PL_DHASH_ADD));

Shouldn't this just lookup in the hash? rather then lookup-and-add?

>         case nsIDOMNode::ELEMENT_NODE:
>-            return createElement((nsIDOMElement*)aNode);
>+        {
>+            nsIDOMElement* element;
>+            aNode->QueryInterface(NS_GET_IID(nsIDOMElement),
>+                                  (void**)&element);

use CallQueryInterface, or even better, use an nsCOMPtr. Same for the cases
below.

>+void Document::addWrapper(MozillaObjectWrapper* aWrapper)

>+MozillaObjectWrapper* Document::removeWrapper(nsISupports* aMozillaObject)

are these still used?

>-    nsISupportsKey key(aObject->getNSObj());
>-    return (TxObject*)wrapperHashTable->Remove(&key);
>+    txWrapperHashEntry* entry =
>+        NS_STATIC_CAST(txWrapperHashEntry *,
>+                       PL_DHashTableOperate(&mWrapperHashTable,
>+                                            aMozillaObject,
>+                                            PL_DHASH_REMOVE));
>+    return entry->mWrapper;

where does entry get deleted? Doesn't the PLD hash delete it before it's
returned?



more to come...

Comment 13

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

 Node* Node::appendChild(Node* aNewChild)
 {
-    NSI_FROM_TX_NULL_CHECK(Node)
-    nsCOMPtr<nsIDOMNode> newChild(do_QueryInterface(GET_NSOBJ(aNewChild)));
+    NSI_FROM_TX(Node);
+    nsCOMPtr<nsIDOMNode> newChild(do_QueryInterface(aNewChild->getNSObj()));

are you sure we never call this function with null?


+typedef CharacterData Comment;

You remove the MozillaComment.cpp file but you still have a
Document::createComment function?

Comment 14

[Peter Van der Beken [:peterv]]

>>         case nsIDOMNode::ELEMENT_NODE:
>>-            return createElement((nsIDOMElement*)aNode);
>>+        {
>>+            nsIDOMElement* element;
>>+            aNode->QueryInterface(NS_GET_IID(nsIDOMElement),
>>+                                  (void**)&element);
>
>use CallQueryInterface, or even better, use an nsCOMPtr. Same for the cases
>below.

I'll CallQueryInterface, I avoided nsCOMPtr's since this is a performance-
critical section.

> Node* Node::appendChild(Node* aNewChild)
> {
>-    NSI_FROM_TX_NULL_CHECK(Node)
>-    nsCOMPtr<nsIDOMNode> newChild(do_QueryInterface(GET_NSOBJ(aNewChild)));
>+    NSI_FROM_TX(Node);
>+    nsCOMPtr<nsIDOMNode> newChild(do_QueryInterface(aNewChild->getNSObj()));
>
>are you sure we never call this function with null?

I could add a null-check on aNewChild. I'd prefer to null-check in the callers
but I'll see, that might be too many changes.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v3

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v3 (-w)

Comment 17

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

Comment on attachment 92458 [details] [diff] [review]
v3

(in txAttributeNodeKey)
+    nsIContent* mParent;
+    nsIAtom* mLocalName;

Why not make these nsCOMPtrs? raw pointers always makes me a bit wary (watch
out for NS_NewAtom though)

Other then that, r=sicking

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v3.1 (-w)

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 94063 [details] [diff] [review]
v3.1

Switched to nsCOMPtr. Marking r=sicking.

Comment 21

[Peter Van der Beken [:peterv]]

I just saw I forgot to switch the NS_NewAtom in the Attr constructor to
do_GetAtom, consider that done (did it in my local tree).

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 94063 [details] [diff] [review]
v3.1

- In Attr::getXPathParent():

+    nsCOMPtr<nsIDOMElement> ownerElem =
+	 do_QueryInterface(mParent);
+    if (!ownerElem) {
+	 return nsnull;
+    }
+    return mOwnerDocument->createElement(ownerElem);

Are we guaranteed by this code that mOwnerDocument is non-null?

- In Document::Document():

+    PRBool success = PL_DHashTableInit(&mWrapperHashTable,
+					&wrapper_hash_table_ops,
+					this,
+					sizeof(txWrapperHashEntry),
+					PL_DHASH_MIN_SIZE);

Is it not fairly likely that there most of the time will be more than
PL_DHASH_MIN_SIZE entries in this table? If so, maybe you could make this
number bigger, maybe 512, 1024, or something like that to cut down on the
number of allocations done in this table.

- In the macro IMPL_CREATE_WRAPPER2():

+    if (!entry->mWrapper) {					 \
+	 entry->mWrapper = new _txClass(aNsObject, this);	 \
+    }								 \

What if we fail to create a new _tx_Class (i.e. we're OOM), don't you want to
remove the entry from the hash then, or does it not matter (there are a few
instances of similar code, this comment applies to all of them)?

- In Document::createAttribute():

+    if (mAttributeNodes.ops) {
+	 // This will leak.
+	 return new Attr(aAttr, this);
+    }

Did you not mean if (_!_mAttributeNodes.ops) here? And if the above code will
leak if we fail to initialize the hash table, then maybe we simply shouldn't
let the code proceed if we fail to initialize the table(s) in stead of
proceeding and leaking the world?

- In the switch statement in Document::createWrapper():

	default:
	{
	    NS_ASSERTION(0, "Don't know nodetype. This could be a ailure.");
	    return createNode(aNode);
	}

First, use NS_ERROR(...), not NS_ASSERTION(0, ...), and second of all, if this
is an error case that's worth asserting for, then why not just bail here after
asserting in stead of trying to recover from an unknown situation?

- Last change in MozillaDocument.cpp
+#ifdef DEBUG
+inline MozillaObjectWrapper::inHashTableDeletion()
+{
+    return mOwnerDocument->mInHashTableDeletion;
+}
+#endif

You'll need to define a return type for this inline method, if you don't,
you'll get compiler warnings at least.

Other than that, sr=jst

Comment 23

[Peter Van der Beken [:peterv]]

Attachment: v3.2

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: v3.2 (diff -w without removals)

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 96088 [details] [diff] [review]
v3.2

Carrying forward r= and sr=. Planning to check this in tomorrow.

Comment 26

[Peter Van der Beken [:peterv]]

Fixed. More could be done but we're going to rely less on wrappers in the future
anyway.

Comment 27

[Axel Hecht]

mass verifying