Closed Bug 565742 Opened 10 years ago Closed 10 years ago

too much QIing/Addrefing/Releasing under NativeInterface2JSObject

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: smaug, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When for example calling createElement in JS we seem to spend 20% of 
NativeInterface2JSObject in QIing/Addrefing/Releasing.
And NativeInterface2JSObject (or things under it) shows up pretty high up in the profiles.
(1) So in NativeInterface2JSObject we occasionally do CallQueryInterface(src, &cache)
where cache is nsWrapperCache. We could at least in some more cases get cache
from caller. nsINodes are nsWrapperCache objects, so we could pass cache object.

(2) ConstructSlimWrapper has nsCOMPtr<nsISupports> identityObj = do_QueryInterface(p);
but it I read the code right, p is often the same as what identityObj
will be. We should be able to know that somewhere in
quickstub level at compile time, and pass that information to 
ConstructSlimWrapper.

(3) ConstructSlimWrapper also
nsRefPtr<nsXPCClassInfo> classInfoHelper;
CallQueryInterface(p, getter_AddRefs(classInfoHelper))
Could we get nsXPCClassInfo somehow from quickstub without calling
QI?

There might be still some other cases to optimize.

I think (2) would be pretty easy to do, and (1) depends on quickstub code.
I'm not sure about (3).
Bug 564266 is similar, though not for quickstubs.
Assignee: nobody → Olli.Pettay
Depends on: 566466
(In reply to comment #1)
> (1) So in NativeInterface2JSObject we occasionally do CallQueryInterface(src,
> &cache)
> where cache is nsWrapperCache. We could at least in some more cases get cache
> from caller. nsINodes are nsWrapperCache objects, so we could pass cache
> object.

We do that already (see xpc_qsGetWrapperCache), or are you saying that's not working?
I may have overlooked that part.
CreateElement patch has a fix for (2) and (3)
Attached patch v1 (obsolete) — Splinter Review
Assignee: Olli.Pettay → peterv
Status: NEW → ASSIGNED
Attachment #460057 - Flags: review?(Olli.Pettay)
Attachment #460057 - Flags: review?(Olli.Pettay)
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #460057 - Attachment is obsolete: true
Attachment #460569 - Flags: review?(Olli.Pettay)
Comment on attachment 460569 [details] [diff] [review]
v1.1


> class nsXTFElementWrapper : public nsXTFElementWrapperBase,
>                             public nsIXTFElementWrapper,
>-                            public nsIClassInfo
>+                            public nsXPCClassInfo
> {
...
>+  virtual PRUint32 GetInterfacesBitmap()
>+  {
>+    nsXPCClassInfo *ci = GetBaseXPCClassInfo();
>+    return ci ? ci->GetInterfacesBitmap() :  0;
>+  }
So this makes XTF to use the same fast path as other elements, but doesn't limit which interfaces are exposed to JS, right?


>+++ b/js/src/xpconnect/src/xpcprivate.h

...> 
>+class xpcObjectHelper
>+{
>+public:
>+  xpcObjectHelper(nsISupports *aObject, nsWrapperCache *aCache = nsnull)
>+  : mCanonical(nsnull),
>+    mObject(aObject),
>+    mCache(aCache),
>+    mIsNode(PR_FALSE)
>+  {
>+    if(!mCache)
>+    {
>+      if(aObject)
>+        CallQueryInterface(aObject, &mCache);
>+      else
>+        mCache = nsnull;
>+    }
>+  }
>+
>+  nsISupports* Object()
>+  {
>+    return mObject;
>+  }
>+
>+  nsISupports* GetCanonical()
>+  {
>+    if (!mCanonical) {
>+      mCanonicalStrong = do_QueryInterface(mObject);
>+      mCanonical = mCanonicalStrong;
>+    }
>+    return mCanonical;
>+  }
>+
>+  already_AddRefed<nsISupports> forgetCanonical()
>+  {
>+    NS_ASSERTION(mCanonical, "Huh, no canonical to forget?");
>+
>+    if (!mCanonicalStrong)
>+      mCanonicalStrong = mCanonical;
>+    mCanonical = nsnull;
>+    return mCanonicalStrong.forget();
>+  }
>+
>+  nsIClassInfo *GetClassInfo()
>+  {
>+    if(mXPCClassInfo)
>+      return mXPCClassInfo;
>+    if(!mClassInfo)
>+      mClassInfo = do_QueryInterface(mObject);
>+    return mClassInfo;
>+  }
>+  nsXPCClassInfo *GetXPCClassInfo()
>+  {
>+    if(!mXPCClassInfo)
>+    {
>+      if(mIsNode)
>+        mXPCClassInfo = static_cast<nsINode*>(GetCanonical())->GetClassInfo();
>+      else
>+        CallQueryInterface(mObject, getter_AddRefs(mXPCClassInfo));
>+    }
>+    return mXPCClassInfo;
>+  }
>+
>+  already_AddRefed<nsXPCClassInfo> forgetXPCClassInfo()
>+  {
>+    GetXPCClassInfo();
>+
>+    return mXPCClassInfo.forget();
>+  }
>+
>+  nsWrapperCache *GetWrapperCache()
>+  {
>+    return mCache;
>+  }
>+
>+protected:
>+  xpcObjectHelper(nsISupports *aObject, nsISupports *aCanonical,
>+                  nsWrapperCache *aCache, PRBool aIsNode)
>+  : mCanonical(aCanonical),
>+    mObject(aObject),
>+    mCache(aCache),
>+    mIsNode(aIsNode)
>+  {
>+    if(!mCache && aObject)
>+      CallQueryInterface(aObject, &mCache);
>+  }
>+
>+  nsCOMPtr<nsISupports>    mCanonicalStrong;
>+  nsISupports*             mCanonical;
>+
>+private:
>+  xpcObjectHelper(xpcObjectHelper& aOther);
>+
>+  nsISupports*             mObject;
>+  nsWrapperCache*          mCache;
>+  nsCOMPtr<nsIClassInfo>   mClassInfo;
>+  nsRefPtr<nsXPCClassInfo> mXPCClassInfo;
>+  PRBool                   mIsNode;
>+};
>+
The file uses 4 space indentation elsewhere.


>+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 2) || \
>+    _MSC_FULL_VER >= 140050215
I assume this check is right.
Attachment #460569 - Flags: review?(Olli.Pettay) → review+
> So this makes XTF to use the same fast path as other elements, but doesn't
> limit which interfaces are exposed to JS, right?

Doesn't, right. The interfaces in the bitmap are all internal interfaces or classes. If we ever did add scriptable DOM interfaces this could limit it, but that already doesn't work right know afaict.

> >+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 2) || \
> >+    _MSC_FULL_VER >= 140050215
> I assume this check is right.

I copied this from nsDOMClassInfoID.h. I'll file a bug to have a common NS_CASTABLE_TO macro that both DOM and XPConnect can use.
Attached patch v1.1Splinter Review
This cleans up some code that recently landed, and seems to give a slight performance win (bug 585090 might bring us some more on top of this).
Attachment #460569 - Attachment is obsolete: true
Attachment #463834 - Flags: review+
Attachment #463834 - Flags: approval2.0?
Attachment #463834 - Flags: approval2.0? → approval2.0+
blocking2.0: --- → beta5+
Filed bug 590937 to share the *_CASTABLE_TO macros.

http://hg.mozilla.org/mozilla-central/rev/bb8020341d71
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.