566466 - speed up createElement()

Status: RESOLVED FIXED

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

Description

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

I have some code for this. Will upload the patch once I've cleaned up the code.

Comment 1

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

Attachment: patch

Uploaded this to tryserver.
I could split this to parts, though most of the patch is just mechanical
simple changes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

It might be better to take already_AddRefed<nsINodeInfo> instead of nsCOMPtr<nsINodeInfo>; that would make it clear what's going on at the callsite, where a forget() call would be needed...

Why do we need to out-of-line the nsINode ctor?

Comment 3

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

well, if we care about OOM situation (which we may not anymore)
already_AddRefed<nsINodeInfo> can't really handle that.

Comment 4

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

Attachment: already_addrefed

Comment 5

[Boris Zbarsky [:bzbarsky]]

> already_AddRefed<nsINodeInfo> can't really handle that.

How was nsCOMPtr handling it any better?

Do you really need the ctor body with the already_AddRefed?  Can you not just use initializer syntax?  For that matter, is there a need for the separate ctor with no nodeinfo arg?  Seems like you could just use the already_AddRefed ctor and pass null...

You should probably make the ctor take a non-const reference, not an object.  Otherwise code like this:

  nsCOMPtr<nsINodeInfo> ni = aNodeInfo;
  nsCommentNode *it = new nsCommentNode(ni); // forgot forget()

will compile, right?  But with a non-const reference, it shouldn't...  I'd hope the forget() variant would, though.

Comment 6

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

(In reply to comment #5)
> How was nsCOMPtr handling it any better?
nsCOMPtr releases nsINodeInfo there is OOM when creating some node.


> 
> Do you really need the ctor body with the already_AddRefed?  Can you not just
> use initializer syntax?
I don't understand this.


  For that matter, is there a need for the separate ctor
> with no nodeinfo arg?  Seems like you could just use the already_AddRefed ctor
> and pass null...
Left over from the nsCOMPtr patch

> You should probably make the ctor take a non-const reference, not an object. 
What you mean here? The nsCOMPtr patch takes reference.
And already_AddRefed patch takes object, but new nsCommentNode(ni) doesn't compile.

Comment 7

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

But since |new| is infallible, do I need to care about OOM?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> nsCOMPtr releases nsINodeInfo there is OOM when creating some node.

Ah, ok.  Indeed.  I think we can ignore this given infallible new.

> I don't understand this.

  mNodeInfo(aNodeInfo)

before the ctor body instead of:

  mNodeInfo = aNodeInfo;

in body.

> And already_AddRefed patch takes object, but new nsCommentNode(ni) doesn't
> compile.

Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?

Comment 9

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

(In reply to comment #8)
 
>   mNodeInfo(aNodeInfo)
> 
> before the ctor body instead of:
> 
>   mNodeInfo = aNodeInfo;
> 
> in body.
Uh, right. I read the comment somehow very differently.

> Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?
I could re-check, but it was something about using reference to a temporary object.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> it was something about using reference to a temporary object.

Perfect.  If the .get() version also doesn't compile, we're good!

Comment 11

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

(In reply to comment #9)
> > Oh?  What's the error message?  Does nsCommentNode(ni.get()) compile?
> I could re-check, but it was something about using reference to a temporary
> object.
Oh, wait, that was for something else, I think.
I was using reference to already_AddRefed.

But I'll test few different syntaxes.

Comment 12

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

(1) So, when using already_AddRefed<nsINodeInfo>&
NS_NewElement(getter_AddRefs(content), ns, nodeInfo.forget(), PR_FALSE);
doesn't compile. 
error: invalid initialization of non-const reference of type ‘already_AddRefed<nsINodeInfo>&’ from a temporary of type ‘already_AddRefed<nsINodeInfo>’

(2) When using already_AddRefed<nsINodeInfo>, NS_NewElement(getter_AddRefs(content), ns, nodeInfo, PR_FALSE);
does not compile.
error: conversion from ‘nsCOMPtr<nsINodeInfo>’ to non-scalar type ‘already_AddRefed<nsINodeInfo>’ requested 
.get() does compile.

I think (2) is still better, and should be good enough.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  It makes it easy to underflow a refcount due to someone accidentally passing in a raw pointer without addreffing.  Maybe that's not a problem....

Comment 14

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

Attachment: still few fixes

I uploaded this to tryserver

Comment 15

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

Attachment: up-to-date

There were some SVG/SMIL change

Comment 16

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

Tryserver found still some problem.
I'll try to fix that and request a review tomorrow.

(On 64bit linux this drops my createElement testcase from 17 to 10ms)

Comment 17

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

Attachment: patch

I think it wouldn't make things any easier if I'd split the patch.
But I could do it if needed.

The changes are:
- pass already_AddRefed<nsINodeInfo> to nsINode ctors
- virtual nsXPCClassInfo* GetClassInfo() in each nsINode implementation.
  That greatly reduces QIing/Addref/release in ConstructSlimWrapper
- Change nodeinfo hash to hash using string not atoms. This way we don't
  have to find first atom (using string key) and then nodeinfo using
  atom key. Only string key is enough.
- non-virtual ::createElement in nsDocument.cpp
- non-virtual createTextNode in nsDocument.cpp
- qsDOMObjectHelper to quickstubs. With that one addred/release can be reduced, and
  it gives a fast way to access classinfo.
  The usage of it isn't perhaps the prettiest in quickstub code, but should work.
- custom quickstubs for createElement and createTextNode

Comment 18

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

Boris, feel free to move the review to someone else if you don't have time for
it.
Peter should perhaps review the quickstub parts. He might have suggestions
for it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 447305 [details] [diff] [review]
patch

jst, do you have time to review this?  If not, just punt back to me...

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 447305 [details] [diff] [review]
patch

 #ifdef MOZILLA_INTERNAL_API
-  nsINode(nsINodeInfo* aNodeInfo)
-    : mNodeInfo(aNodeInfo),
-      mParentPtrBits(0),
-      mFlagsOrSlots(NODE_DOESNT_HAVE_SLOTS),
-      mNextSibling(nsnull),
-      mPreviousSibling(nsnull),
-      mFirstChild(nsnull)
+  nsINode(already_AddRefed<nsINodeInfo> aNodeInfo)
+  : mNodeInfo(aNodeInfo),
+    mParentPtrBits(0),
+    mFlagsOrSlots(NODE_DOESNT_HAVE_SLOTS),
+    mNextSibling(nsnull),
+    mPreviousSibling(nsnull),
+    mFirstChild(nsnull)

I personally prefer the old indentation style here, but I can deal either way, until I see you use the above old style later on in this patch :) So maybe change the indentation back to what it was here? :)

- In content/base/src/nsDocument.cpp:

+#include "nsXMLEventsManager.h"
+

Looks like an unrelated change. nsDocument.h also has a forward declaration of nsXMLEventsManager which looks unrelated.

- In nsNodeInfoManager::GetNodeInfo():

+  nsRefPtr<nsNodeInfo> newNodeInfo = nsNodeInfo::Create();
+  NS_ENSURE_TRUE(newNodeInfo, nsnull);

This method returns an nsresult, so nsnull is wrong there. NS_ERROR_OUT_OF_MEMORY is probably what you want here.

+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);

This can't fail for any other reason than OOM, so use NS_ERROR_OUT_OF_MEMORY here too.

- In nsGenericHTMLElement::SetInnerHTML():

-  if (doc->IsHTML() && nsHtml5Module::sEnabled) {
+  if (doc->IsHTML() && PR_FALSE && nsHtml5Module::sEnabled) {

Might not want to check this in :)

- In js/src/xpconnect/src/xpcprivate.h:

@@ -3034,45 +3037,49 @@ public:
      * @param Interface the interface of src that we want
      * @param cache the wrapper cache for src (may be null, in which case src
      *              will be QI'ed to get the cache)
      * @param scope the default scope to put on the new JSObject's parent chain
      * @param allowNativeWrapper if true, this method may wrap the resulting
      *        JSObject in an XPCNativeWrapper and return that, as needed.
      * @param isGlobal
      * @param pErr [out] relevant error code, if any.
+     * @param src_is_identity optional performance hint. Set to PR_TRUE only
+     *                        if src is the identity pointer.
      */

That comment change doesn't match the code change.

- In xpc_qsGetWrapperCache():

+/** Convert an XPCOM pointer to jsval. Return JS_TRUE on success. 
+ * aIdentity is a performance optimization. Set it to PR_TRUE,
+ * only if p is the identity pointer.
+ */

This comment change doesn't match the code change.

- In  ConstructSlimWrapper():

     // Transfer ownership to the wrapper's private.
-    identityObj.forget();
+    if (strongIdentity) {
+        strongIdentity.forget();
+    } else {
+      aIdentity->forgetOwnership();
+    }

Stick to 2-space indentation :)

- In layout/forms/nsIsIndexFrame.cpp:

  * The Initial Developer of the Original Code is
  * Netscape Communications Corporation.
  * Portions created by the Initial Developer are Copyright (C) 1998
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
- *
+ *                               

Just adding whitespace... Undo this?

r=jst with that, but I would like to see peterv look over the (small) changes in xpconnect/src.

Comment 21

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

Yeah, Peter, would be great if you had time to look at the xpconnect changes.
Perhaps you even have suggestions how to achieve the same thing with
less ugly way. Note, all the Addref/Release and QIing shows up badly in the
profiles.

Comment 22

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

(In reply to comment #20)
> - In nsGenericHTMLElement::SetInnerHTML():
> 
> -  if (doc->IsHTML() && nsHtml5Module::sEnabled) {
> +  if (doc->IsHTML() && PR_FALSE && nsHtml5Module::sEnabled) {
> 
Oops :)
Left-over from some testing.

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 447305 [details] [diff] [review]
patch

I think split patches would have been better (especially with the fixes for bug 565742 in that bug).

There's a bunch of long lines you can easily resolve.

I think you should make nsINode::Clone() take an already_AddRefed<nsINodeInfo> too. The only tricky part is if Clone is not implemented (returns NS_ERROR_NOT_IMPLEMENTED) then you need to make sure to release that pointer.

>diff --git a/content/base/public/nsINodeInfo.h b/content/base/public/nsINodeInfo.h

>     nsIAtom*            mName;
>     nsIAtom*            mPrefix;
>     PRInt32             mNamespaceID;
>+    const nsAString*    mTmpName;

mNameString?

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp

>+nsresult
>+nsDocument::CreateElement(const nsAString& aTagName,
>+                          nsIContent** aReturn)
>+{
>+  *aReturn = nsnull;
>+  nsresult rv;
>+
>+  // if we are in quirks, allow surrounding '<' '>' for IE compat
>+  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
>+                     aTagName.Length() > 2 &&
>+                     aTagName.First() == '<' &&
>+                     aTagName.Last() == '>');
>+  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(aTagName);
>+  nsAutoString tagName;
>+  PRBool ieCompatOrLowerCase = ieCompat || needsLowercase;
>+  if (ieCompat) {
>+    tagName = Substring(aTagName, 1, aTagName.Length() - 2);
>+  }
>+
>+  rv = nsContentUtils::CheckQName(ieCompatOrLowerCase ? 
>+                                  tagName : aTagName, PR_FALSE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  return CallQueryInterface(content, aReturn);
>+  if (needsLowercase) {
>+    ToLowerCase(tagName);
>+  }
>+
>+  rv = CreateElem(ieCompatOrLowerCase ? tagName : aTagName, nsnull,
>+                  IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
>+                  PR_TRUE, aReturn);
>+  return rv;
> }

I don't understand how this can work for a non-lowercase tagname without surrounding '<' '>', tagName is never set? Please add a test for that if there isn't one already.
This looks cleaner to me (if it works):

  // if we are in quirks, allow surrounding '<' '>' for IE compat
  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
                     aTagName.Length() > 2 &&
                     aTagName.First() == '<' &&
                     aTagName.Last() == '>');
  const nsAString& tagName = ieCompat ?
                             Substring(aTagName, 1, aTagName.Length() - 2) :
                             aTagName;

  rv = nsContentUtils::CheckQName(tagName, PR_FALSE);
  NS_ENSURE_SUCCESS(rv, rv);

  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(tagName);
  nsAutoString lcTagName;
  if (needsLowercase) {
    ToLowerCase(tagName, lcTagName);
  }

  return CreateElem(needsLowercase ? lcTagName : tagName, nsnull,
                    IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
                    PR_TRUE, aReturn);

>diff --git a/content/base/src/nsGenericElement.h b/content/base/src/nsGenericElement.h

>+#define DOMCI_NODE_DATA(_interface, _class)                             \
>+  DOMCI_DATA(_interface, _class)                                        \
>+  nsXPCClassInfo* _class::GetClassInfo()                                \
>+  {                                                                     \
>+    return static_cast<nsXPCClassInfo*>(                                \
>+      NS_GetDOMClassInfoInstance(eDOMClassInfo_##_interface##_id));     \

Can we make NS_GetDOMClassInfoInstance return nsXPCClassInfo*?

>diff --git a/content/base/src/nsNodeInfoManager.cpp b/content/base/src/nsNodeInfoManager.cpp

> PRIntn
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>   NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> 
>   const nsINodeInfo::nsNodeInfoInner *node1 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
>   const nsINodeInfo::nsNodeInfoInner *node2 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
> 
>-  return (node1->mName == node2->mName &&
>+  if (node1->mName) {
>+    if (node2->mName) {
>+      return (node1->mName == node2->mName &&
>+              node1->mPrefix == node2->mPrefix &&
>+              node1->mNamespaceID == node2->mNamespaceID);
>+    }
>+    return (node1->mName->Equals(*(node2->mTmpName)) &&
>+            node1->mPrefix == node2->mPrefix &&
>+            node1->mNamespaceID == node2->mNamespaceID);
>+  } else if (node2->mName) {
>+    return (node2->mName->Equals(*(node1->mTmpName)) &&
>+            node1->mPrefix == node2->mPrefix &&
>+            node1->mNamespaceID == node2->mNamespaceID);
>+  }
>+  return (node1->mTmpName->Equals(*(node2->mTmpName)) &&
>           node1->mPrefix == node2->mPrefix &&
>           node1->mNamespaceID == node2->mNamespaceID);

I think this would be more readable if you returned early if node1->mPrefix != node2->mPrefix or node1->mNamespaceID != node2->mNamespaceID.

> nsresult
> nsNodeInfoManager::GetNodeInfo(const nsAString& aName, nsIAtom *aPrefix,
>                                PRInt32 aNamespaceID, nsINodeInfo** aNodeInfo)
> {
>-  nsCOMPtr<nsIAtom> name = do_GetAtom(aName);
>-  *aNodeInfo = nsNodeInfoManager::GetNodeInfo(name, aPrefix, aNamespaceID).get();
>-  return *aNodeInfo ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+  NS_ASSERTION(!aName.Equals(EmptyString()),

Why not IsEmpty()?

>+  PLHashEntry *he;
>+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
>+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);
>+
>+  *aNodeInfo = newNodeInfo.forget().get();

I think |newNodeInfo.forget(aNodeInfo);| would work here.

>+
>+  return NS_OK;;

Stray semicolon

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h

> #define NS_IMPL_NS_NEW_HTML_ELEMENT(_elementName)                            \
> nsGenericHTMLElement*                                                        \
>-NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo, PRBool aFromParser)\
>+NS_NewHTML##_elementName##Element(already_AddRefed<nsINodeInfo> aNodeInfo,  \

Realign the \ here and in other places.

>diff --git a/content/html/content/src/nsHTMLSharedElement.cpp b/content/html/content/src/nsHTMLSharedElement.cpp

>+nsXPCClassInfo* 
>+nsHTMLSharedElement::GetClassInfo()
>+{
>+  nsIClassInfo* info = nsnull;
>+  if (mNodeInfo->Equals(nsGkAtoms::param)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLParamElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::wbr)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLWBRElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::isindex)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLIsIndexElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::base)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLBaseElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::spacer)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLSpacerElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dir)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDirectoryElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::menu)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLMenuElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::q)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::blockquote)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::head)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHeadElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::html)) {
>+    info = NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHtmlElement_id);
>+  }
>+  return static_cast<nsXPCClassInfo*>(info);
>+}

It'd be nice if we could somehow share these with the QI implementations in classes that can represent multiple types of elements. Maybe have a static GetClassInfo that the virtual and QI can call?

>diff --git a/content/html/document/src/nsHTMLContentSink.cpp b/content/html/document/src/nsHTMLContentSink.cpp

> nsresult
>-NS_NewHTMLElement(nsIContent** aResult, nsINodeInfo *aNodeInfo,
>+NS_NewHTMLElement(nsIContent** aResult, already_AddRefed<nsINodeInfo> aNodeInfo,
>                   PRBool aFromParser)
> {
>   *aResult = nsnull;
> 
>   nsIParserService* parserService = nsContentUtils::GetParserService();
>   if (!parserService)
>     return NS_ERROR_OUT_OF_MEMORY;

Maybe put the nodeinfo in a local comptr so it won't leak? This shouldn't happen, but still.
>diff --git a/js/src/xpconnect/src/dom_quickstubs.qsconf b/js/src/xpconnect/src/dom_quickstubs.qsconf

>+    'nsIDOMDocument_CreateElement': {
>+        'thisType': 'nsIDocument',
>+        'code': '    nsIContent* result = nsnull;\n'
>+                '    rv = static_cast<nsDocument*>(self)->CreateElement(arg0, &result);\n'

thisType should just be nsDocument.

>+    'nsIDOMDocument_CreateTextNode': {
>+        'thisType': 'nsIDocument',
>+        'code': '    nsIContent* result = nsnull;\n'
>+                '    rv = static_cast<nsDocument*>(self)->CreateTextNode(arg0, &result);\n'

Same here.

>diff --git a/js/src/xpconnect/src/qsgen.py b/js/src/xpconnect/src/qsgen.py

>@@ -675,19 +675,21 @@ def writeResultConv(f, type, jsvalPtr, j
>             return
>         # else fall through; this type isn't supported yet
>     elif isInterfaceType(type):
>         if isVariantType(type):
>             f.write("    return xpc_qsVariantToJsval(lccx, result, %s);\n"
>                     % jsvalPtr)
>             return
>         else:
>-            f.write("    return xpc_qsXPCOMObjectToJsval(lccx, "
>+            f.write("    qsDOMObjectHelper doh(identity);\n"

I really dislike this identity variable, because it means that the generated quickstubs code has to opt-in. There are already a number of quickstubs that have a nsINode result and afaict they're not getting the benefits of this patch. We should have a constructor for qsDOMObjectHelper that just sets mRef to nsnull for objects for which we don't have the canonical nsISupports. See below for more.

>+                    "    return xpc_qsXPCOMObjectToJsval(lccx, "
>                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
>-                    "&NS_GET_IID(%s), &interfaces[k_%s], %s);\n"
>+                    "&NS_GET_IID(%s), &interfaces[k_%s], %s,"

Space after comma.

>@@ -1196,19 +1198,21 @@ def writeTraceableResultConv(f, type):

>+            f.write("    qsDOMObjectHelper doh(identity);\n"
>+                    "    JSBool ok = xpc_qsXPCOMObjectToJsval(lccx, "
>                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
>-                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0]);\n"
>+                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0],"
>+                    " doh.get() ? &doh : nsnull);\n"

Space after comma, not before.

>diff --git a/js/src/xpconnect/src/xpcquickstubs.h b/js/src/xpconnect/src/xpcquickstubs.h

>@@ -71,16 +73,51 @@ struct xpc_qsHashEntry {
>     const xpc_qsFunctionSpec *functions;
>     const xpc_qsTraceableSpec *traceables;
>     // These last two fields index to other entries in the same table.
>     // XPC_QS_NULL_ENTRY indicates there are no more entries in the chain.
>     size_t parentInterface;
>     size_t chain;
> };
> 
>+// This is for nsINode objects for now.

I think we should generalize this a bit (but keep some special code for nsINode). The problem we're running into is that we lazily get a number of things in NativeInterface2JSObject, and sometimes we already have them before calling NativeInterface2JSObject. We should also be caching these things sometimes, for example if ConstructSlimWrapper fails we call XPCWrappedNative::GetNewOrUsed which gets the canonical nsISupports and the classinfo again. So I think that we should actually replace |nsISupports* src| with |qsDOMObjectHelper& helper| (probably renaming qsDOMObjectHelper), and have that store:

- the object to be wrapped
- the canonical nsISupports pointer for the object to be wrapped
- the nsWrapperCache for the object to be wrapped
- the classinfo for the object to be wrapped

It should also have getters that either return the existing value or get it and cache it. Because sometimes we have a weak pointer to some of these and sometimes a strong we might need to have both a weak and a nsCOMPtr. I think we need that for the canonical nsISupports and for the classinfo (probably weak nsXPCClassInfo and nsCOMPtr<nsIClassInfo>).
For now we also probably need to store nsINode (for GetClassInfo), ideally we'd make that more general too but I don't have any good ideas there.

We'd pass in ToSupports(result), ToCanonicalSupports(result) and xpc_qsGetWrapperCache(result) to the constructor of the helper. We'll need a ToCanonicalSupports that takes a already_AddRefed<nsINode> (returning already_AddRefed<nsISupports>) and one that takes a nsINode* (returning nsISupports*), and two constructors. Might also add ToCanonicalSupports for nsINodeList. And one that takes void and returns null.

We should then pass that helper to ConstructSlimWrapper and XPCWrappedNative::GetNewOrUsed.

What do you think? It's a bit complicated, but there doesn't seem to be an easy solution to cover all cases (don't have canonical, have weak canonical, have strong canonical, with or withour wrappercache and with or without fast access to CI).

> JSBool
> xpc_qsXPCOMObjectToJsval(XPCLazyCallContext &lccx,
>-                         nsISupports *p,
>+                         nsISupports* p,

Don't.

Comment 24

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

(In reply to comment #23)
> I think you should make nsINode::Clone() take an already_AddRefed<nsINodeInfo>
> too. The only tricky part is if Clone is not implemented (returns
> NS_ERROR_NOT_IMPLEMENTED) then you need to make sure to release that pointer.
That would be a followup bug.


> 
> >     nsIAtom*            mName;
> >     nsIAtom*            mPrefix;
> >     PRInt32             mNamespaceID;
> >+    const nsAString*    mTmpName;
> 
> mNameString?
Ok


> 
> >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
> 
> >+nsresult
> >+nsDocument::CreateElement(const nsAString& aTagName,
> >+                          nsIContent** aReturn)
> >+{
> >+  *aReturn = nsnull;
> >+  nsresult rv;
> >+
> >+  // if we are in quirks, allow surrounding '<' '>' for IE compat
> >+  PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
> >+                     aTagName.Length() > 2 &&
> >+                     aTagName.First() == '<' &&
> >+                     aTagName.Last() == '>');
> >+  PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(aTagName);
> >+  nsAutoString tagName;
> >+  PRBool ieCompatOrLowerCase = ieCompat || needsLowercase;
> >+  if (ieCompat) {
> >+    tagName = Substring(aTagName, 1, aTagName.Length() - 2);
> >+  }
> >+
> >+  rv = nsContentUtils::CheckQName(ieCompatOrLowerCase ? 
> >+                                  tagName : aTagName, PR_FALSE);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  return CallQueryInterface(content, aReturn);
> >+  if (needsLowercase) {
> >+    ToLowerCase(tagName);
> >+  }
> >+
> >+  rv = CreateElem(ieCompatOrLowerCase ? tagName : aTagName, nsnull,
> >+                  IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
> >+                  PR_TRUE, aReturn);
> >+  return rv;
> > }
> 
> I don't understand how this can work for a non-lowercase tagname without
> surrounding '<' '>', tagName is never set? Please add a test for that if there
> isn't one already.
Argh, there is a bug indeed. It used to be ok when I had nsString* tagName

> This looks cleaner to me (if it works):
> 
>   // if we are in quirks, allow surrounding '<' '>' for IE compat
>   PRBool ieCompat = (mCompatMode == eCompatibility_NavQuirks &&
>                      aTagName.Length() > 2 &&
>                      aTagName.First() == '<' &&
>                      aTagName.Last() == '>');
>   const nsAString& tagName = ieCompat ?
>                              Substring(aTagName, 1, aTagName.Length() - 2) :
>                              aTagName;
> 
>   rv = nsContentUtils::CheckQName(tagName, PR_FALSE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRBool needsLowercase = IsHTML() && !IsLowercaseASCII(tagName);
>   nsAutoString lcTagName;
>   if (needsLowercase) {
>     ToLowerCase(tagName, lcTagName);
>   }
> 
>   return CreateElem(needsLowercase ? lcTagName : tagName, nsnull,
>                     IsHTML() ? kNameSpaceID_XHTML : GetDefaultNamespaceID(),
>                     PR_TRUE, aReturn);
OK, will use this.


> >+#define DOMCI_NODE_DATA(_interface, _class)                             \
> >+  DOMCI_DATA(_interface, _class)                                        \
> >+  nsXPCClassInfo* _class::GetClassInfo()                                \
> >+  {                                                                     \
> >+    return static_cast<nsXPCClassInfo*>(                                \
> >+      NS_GetDOMClassInfoInstance(eDOMClassInfo_##_interface##_id));     \
> 
> Can we make NS_GetDOMClassInfoInstance return nsXPCClassInfo*?
Perhaps we can. I don't care.

 
> I think this would be more readable if you returned early if node1->mPrefix !=
> node2->mPrefix or node1->mNamespaceID != node2->mNamespaceID.
ok


> > nsNodeInfoManager::GetNodeInfo(const nsAString& aName, nsIAtom *aPrefix,
> >                                PRInt32 aNamespaceID, nsINodeInfo** aNodeInfo)
> > {
> >-  nsCOMPtr<nsIAtom> name = do_GetAtom(aName);
> >-  *aNodeInfo = nsNodeInfoManager::GetNodeInfo(name, aPrefix, aNamespaceID).get();
> >-  return *aNodeInfo ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> >+  NS_ASSERTION(!aName.Equals(EmptyString()),
> 
> Why not IsEmpty()?
DEBUG code, I don't care. But will change.

> >+  PLHashEntry *he;
> >+  he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo);
> >+  NS_ENSURE_TRUE(he, NS_ERROR_FAILURE);
> >+
> >+  *aNodeInfo = newNodeInfo.forget().get();
> 
> I think |newNodeInfo.forget(aNodeInfo);| would work here.
Sure, it does the same thing. I can change.


> It'd be nice if we could somehow share these with the QI implementations in
> classes that can represent multiple types of elements. Maybe have a static
> GetClassInfo that the virtual and QI can call?
Sounds good.

> 
> Maybe put the nodeinfo in a local comptr so it won't leak? This shouldn't
> happen, but still.
It really shouldn't happen, but I could do that.

> I really dislike this identity variable, because it means that the generated
> quickstubs code has to opt-in. There are already a number of quickstubs that
> have a nsINode result and afaict they're not getting the benefits of this
> patch. We should have a constructor for qsDOMObjectHelper that just sets mRef
> to nsnull for objects for which we don't have the canonical nsISupports. See
> below for more.
This is the kind of review I was hoping to get :)


> 
> >+                    "    return xpc_qsXPCOMObjectToJsval(lccx, "
> >                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
> >-                    "&NS_GET_IID(%s), &interfaces[k_%s], %s);\n"
> >+                    "&NS_GET_IID(%s), &interfaces[k_%s], %s,"
> 
> Space after comma.
I don't know which comma, but perhaps I'll figure out.


> 
> >@@ -1196,19 +1198,21 @@ def writeTraceableResultConv(f, type):
> 
> >+            f.write("    qsDOMObjectHelper doh(identity);\n"
> >+                    "    JSBool ok = xpc_qsXPCOMObjectToJsval(lccx, "
> >                     "ToSupports(result), xpc_qsGetWrapperCache(result), "
> >-                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0]);\n"
> >+                    "&NS_GET_IID(%s), &interfaces[k_%s], &vp.array[0],"
> >+                    " doh.get() ? &doh : nsnull);\n"
> 
> Space after comma, not before.
Same here.


> I think we should generalize this a bit (but keep some special code for
> nsINode). The problem we're running into is that we lazily get a number of
> things in NativeInterface2JSObject, and sometimes we already have them before
> calling NativeInterface2JSObject. We should also be caching these things
> sometimes, for example if ConstructSlimWrapper fails we call
> XPCWrappedNative::GetNewOrUsed which gets the canonical nsISupports and the
> classinfo again. So I think that we should actually replace |nsISupports* src|
> with |qsDOMObjectHelper& helper| (probably renaming qsDOMObjectHelper), and
> have that store:
> 
> - the object to be wrapped
> - the canonical nsISupports pointer for the object to be wrapped
> - the nsWrapperCache for the object to be wrapped
> - the classinfo for the object to be wrapped
> 
> It should also have getters that either return the existing value or get it and
> cache it. Because sometimes we have a weak pointer to some of these and
> sometimes a strong we might need to have both a weak and a nsCOMPtr. I think we
> need that for the canonical nsISupports and for the classinfo (probably weak
> nsXPCClassInfo and nsCOMPtr<nsIClassInfo>).
> For now we also probably need to store nsINode (for GetClassInfo), ideally we'd
> make that more general too but I don't have any good ideas there.
> 
> We'd pass in ToSupports(result), ToCanonicalSupports(result) and
> xpc_qsGetWrapperCache(result) to the constructor of the helper. We'll need a
> ToCanonicalSupports that takes a already_AddRefed<nsINode> (returning
> already_AddRefed<nsISupports>) and one that takes a nsINode* (returning
> nsISupports*), and two constructors. Might also add ToCanonicalSupports for
> nsINodeList. And one that takes void and returns null.
> 
> We should then pass that helper to ConstructSlimWrapper and
> XPCWrappedNative::GetNewOrUsed.
> 
> What do you think? It's a bit complicated, but there doesn't seem to be an easy
> solution to cover all cases (don't have canonical, have weak canonical, have
> strong canonical, with or withour wrappercache and with or without fast access
> to CI).
I think this sounds good. A bit complicated but way better what I did.
All the qs code is just very new to me.

Comment 25

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

Attachment: some tests

Comment 26

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

Attachment: up-to-date

Backing up the patch here. Misses still some xpconnect changes.

Comment 27

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

Attachment: patch

This has a bit less-ugly qs implementation.
Couldn't use qsObjectHelper ctor for everything because of 
C++'s idiotic parameter evaluation order.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: patch

I think this is the unbitrotted version of attachment 451269 [details] [diff] [review] (sorry for the delays). I'll put an additional patch for qsObjectHelper in bug 565742, so that this can just land.

Please don't forget to file the bug for making Clone take an already_AddRefed<nsINodeInfo>.

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>@@ -191,16 +191,18 @@ static NS_DEFINE_CID(kDOMEventGroupCID, 
> #include "nsSVGUtils.h"
> #endif // MOZ_SMIL
> 
> // FOR CSP (autogenerated by xpidl)
> #include "nsIContentSecurityPolicy.h"
> #include "nsHTMLStyleSheet.h"
> #include "nsHTMLCSSStyleSheet.h"
> 
>+#include "nsXMLEventsManager.h"

Not needed.

>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>--- a/content/base/src/nsDocument.h
>+++ b/content/base/src/nsDocument.h
>@@ -125,16 +125,17 @@ class nsDocument;
> class nsIDTD;
> class nsIRadioVisitor;
> class nsIFormControl;
> struct nsRadioGroupStruct;
> class nsOnloadBlocker;
> class nsUnblockOnloadEvent;
> class nsChildContentList;
> class nsXMLEventsManager;
>+class nsXMLEventsManager;

Not needed.

>diff --git a/content/base/src/nsNodeInfoManager.cpp b/content/base/src/nsNodeInfoManager.cpp

> PRIntn
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>   NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> 
>   const nsINodeInfo::nsNodeInfoInner *node1 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
>   const nsINodeInfo::nsNodeInfoInner *node2 =
>     reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
> 
>-  return (node1->mName == node2->mName &&
>-          node1->mPrefix == node2->mPrefix &&
>-          node1->mNamespaceID == node2->mNamespaceID);
>+  if (node1->mPrefix != node2->mPrefix ||
>+      node1->mNamespaceID != node2->mNamespaceID) {
>+    return 0;
>+  }
>+
>+  if (node1->mName) {
>+    if (node2->mName) {
>+      return (node1->mName == node2->mName);
>+    }
>+    return (node1->mName->Equals(*(node2->mNameString)));
>+  } else if (node2->mName) {
>+    return (node2->mName->Equals(*(node1->mNameString)));
>+  }
>+  return (node1->mNameString->Equals(*(node2->mNameString)));

Much better, now remove the else-after-returns :-).

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h

>+#define NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_IF_TAG \
>+  if (aIID.Equals(NS_GET_IID(nsIClassInfo)) ||      \
>+      aIID.Equals(NS_GET_IID(nsXPCClassInfo))) {    \
>+    foundInterface = GetClassInfoInternal();        \
>+    if (!foundInterface) {                          \
>+      *aInstancePtr = nsnull;                       \
>+      return NS_ERROR_OUT_OF_MEMORY;                \
>+    }                                               \
>   } else

I'd make this NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_GETTER(_getter) and use NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_GETTER(GetClassInfoInternal) where needed. There isn't really anything related to a tag in the implementation of the macro.

>diff --git a/content/html/content/src/nsHTMLDelElement.cpp b/content/html/content/src/nsHTMLDelElement.cpp

>+nsIClassInfo* 
>+nsHTMLModElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::del)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDelElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::ins)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLInsElement_id);
>+  }

else-after-return.

>diff --git a/content/html/content/src/nsHTMLOListElement.cpp b/content/html/content/src/nsHTMLOListElement.cpp

>+nsIClassInfo* 
>+nsHTMLSharedListElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::ol)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLOListElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dl)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDListElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::ul)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLUListElement_id);
>+  }

else-after-return.

>diff --git a/content/html/content/src/nsHTMLSharedElement.cpp b/content/html/content/src/nsHTMLSharedElement.cpp

>+nsIClassInfo*
>+nsHTMLSharedElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::param)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLParamElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::wbr)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLWBRElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::isindex)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLIsIndexElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::base)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLBaseElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::spacer)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLSpacerElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::dir)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLDirectoryElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::menu)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLMenuElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::q)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::blockquote)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLQuoteElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::head)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHeadElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::html)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLHtmlElement_id);
>+  }

else-after-return

>diff --git a/content/html/content/src/nsHTMLSharedObjectElement.cpp b/content/html/content/src/nsHTMLSharedObjectElement.cpp

>+nsIClassInfo*
>+nsHTMLSharedObjectElement::GetClassInfoInternal()
>+{
>+  if (mNodeInfo->Equals(nsGkAtoms::applet)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLAppletElement_id);
>+  } else if (mNodeInfo->Equals(nsGkAtoms::embed)) {
>+    return NS_GetDOMClassInfoInstance(eDOMClassInfo_HTMLEmbedElement_id);
>+  }

else-after-return

>diff --git a/content/svg/content/src/nsSVGElement.h b/content/svg/content/src/nsSVGElement.h

> #define NS_IMPL_NS_NEW_SVG_ELEMENT(_elementName)                             \
> nsresult                                                                     \
> NS_NewSVG##_elementName##Element(nsIContent **aResult,                       \
>-                                 nsINodeInfo *aNodeInfo)                     \
>+                                 already_AddRefed<nsINodeInfo> aNodeInfo)           \

> #define NS_IMPL_NS_NEW_SVG_ELEMENT_CHECK_PARSER(_elementName)                \
> nsresult                                                                     \
> NS_NewSVG##_elementName##Element(nsIContent **aResult,                       \
>-                                 nsINodeInfo *aNodeInfo,                     \
>+                                 already_AddRefed<nsINodeInfo> aNodeInfo,           \

Realign the backslashes.

>diff --git a/js/src/xpconnect/src/Makefile.in b/js/src/xpconnect/src/Makefile.in

>diff --git a/layout/forms/nsIsIndexFrame.cpp b/layout/forms/nsIsIndexFrame.cpp
>--- a/layout/forms/nsIsIndexFrame.cpp
>+++ b/layout/forms/nsIsIndexFrame.cpp
>@@ -15,17 +15,17 @@
>  * The Original Code is mozilla.org code.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- *
>+ *                               

Not needed.

Comment 29

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

Attachment: patch

Comment 30

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

http://hg.mozilla.org/mozilla-central/rev/58101a16aff7

Comment 31

[Paul Rouget [:paul]]

How "faster" is the new implementation?

Comment 32

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

Comparing to what?

~45% faster than 3.6.

Comment 33

[Paul Rouget [:paul]]

to 3.6. Thank you!