Fix for bug 481677 (Avoid hash lookups in XPCWrappedNative::GetNewOrUsed). r=bz, sr=jst.
authorPeter Van der Beken <peterv@propagandism.org>
Wed, 18 Mar 2009 19:02:48 +0100
changeset 26352 f385e435c08214acbfc6fb2389810bca0f4a99dc
parent 26351 18b1811cd1042e4b4f6f0444f865683c2683d11d
child 26353 3414fd6ab52bfc909d80d3c4b32bab7a5f27cb55
child 26355 34964396a3679fc35eff01d51723fc5b185925e2
push id6024
push userpvanderbeken@mozilla.com
push dateThu, 19 Mar 2009 10:34:32 +0000
treeherdermozilla-central@f385e435c082 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, jst
bugs481677
milestone1.9.2a1pre
Fix for bug 481677 (Avoid hash lookups in XPCWrappedNative::GetNewOrUsed). r=bz, sr=jst.
js/src/xpconnect/src/xpccomponents.cpp
js/src/xpconnect/src/xpcconvert.cpp
js/src/xpconnect/src/xpcprivate.h
js/src/xpconnect/src/xpcwrappednative.cpp
xpcom/glue/nsCOMPtr.h
--- a/js/src/xpconnect/src/xpccomponents.cpp
+++ b/js/src/xpconnect/src/xpccomponents.cpp
@@ -4045,17 +4045,17 @@ nsXPCComponents::AttachNewComponentsObje
 
     AutoMarkingNativeInterfacePtr iface(ccx);
     iface = XPCNativeInterface::GetNewOrUsed(ccx, &NS_GET_IID(nsIXPCComponents));
 
     if(!iface)
         return JS_FALSE;
 
     nsCOMPtr<XPCWrappedNative> wrapper;
-    XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface,
+    XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull,
                                    OBJ_IS_NOT_GLOBAL, getter_AddRefs(wrapper));
     if(!wrapper)
         return JS_FALSE;
 
     aScope->SetComponents(components);
 
     jsid id = ccx.GetRuntime()->GetStringID(XPCJSRuntime::IDX_COMPONENTS);
     JSObject* obj;
--- a/js/src/xpconnect/src/xpcconvert.cpp
+++ b/js/src/xpconnect/src/xpcconvert.cpp
@@ -1137,17 +1137,17 @@ XPCConvert::NativeInterface2JSObject(XPC
             if(iface)
                 wrapper->FindTearOff(ccx, iface, JS_FALSE, &rv);
             else
                 rv = NS_OK;
         }
         else
         {
             rv = XPCWrappedNative::GetNewOrUsed(ccx, src, xpcscope, iface,
-                                                isGlobal,
+                                                cache, isGlobal,
                                                 getter_AddRefs(strongWrapper));
 
             wrapper = strongWrapper;
         }
 
         if(pErr)
             *pErr = rv;
         if(NS_SUCCEEDED(rv) && wrapper)
--- a/js/src/xpconnect/src/xpcprivate.h
+++ b/js/src/xpconnect/src/xpcprivate.h
@@ -2243,21 +2243,26 @@ public:
     HasMutatedSet() const {return IsValid() &&
                                   (!HasProto() ||
                                    GetSet() != GetProto()->GetSet());}
 
     XPCJSRuntime*
     GetRuntime() const {XPCWrappedNativeScope* scope = GetScope();
                         return scope ? scope->GetRuntime() : nsnull;}
 
+    /**
+     * If Object has a nsWrapperCache it should be passed in. If a cache is
+     * passed in then cache->GetWrapper() must be null.
+     */
     static nsresult
     GetNewOrUsed(XPCCallContext& ccx,
                  nsISupports* Object,
                  XPCWrappedNativeScope* Scope,
                  XPCNativeInterface* Interface,
+                 nsWrapperCache* cache,
                  JSBool isGlobal,
                  XPCWrappedNative** wrapper);
 
 public:
     static nsresult
     GetUsedOnly(XPCCallContext& ccx,
                 nsISupports* Object,
                 XPCWrappedNativeScope* Scope,
@@ -2398,21 +2403,21 @@ public:
         return offsets;
     }
 
     // Make ctor and dtor protected (rather than private) to placate nsCOMPtr.
 protected:
     XPCWrappedNative(); // not implemented
 
     // This ctor is used if this object will have a proto.
-    XPCWrappedNative(nsISupports* aIdentity,
+    XPCWrappedNative(already_AddRefed<nsISupports> aIdentity,
                      XPCWrappedNativeProto* aProto);
 
     // This ctor is used if this object will NOT have a proto.
-    XPCWrappedNative(nsISupports* aIdentity,
+    XPCWrappedNative(already_AddRefed<nsISupports> aIdentity,
                      XPCWrappedNativeScope* aScope,
                      XPCNativeSet* aSet);
 
     virtual ~XPCWrappedNative();
 
 private:
     void TraceOtherWrapper(JSTracer* trc);
     JSBool Init(XPCCallContext& ccx, JSObject* parent, JSBool isGlobal,
--- a/js/src/xpconnect/src/xpcwrappednative.cpp
+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
@@ -282,19 +282,24 @@ static void DEBUG_TrackShutdownWrapper(X
 /***************************************************************************/
 
 // static
 nsresult
 XPCWrappedNative::GetNewOrUsed(XPCCallContext& ccx,
                                nsISupports* Object,
                                XPCWrappedNativeScope* Scope,
                                XPCNativeInterface* Interface,
+                               nsWrapperCache *cache,
                                JSBool isGlobal,
                                XPCWrappedNative** resultWrapper)
 {
+    NS_ASSERTION(!cache || !cache->GetWrapper(),
+                 "We assume the caller already checked if it could get the "
+                 "wrapper from the cache.");
+
     nsresult rv;
 
     NS_ASSERTION(!Scope->GetRuntime()->GetThreadRunningGC(), 
                  "XPCWrappedNative::GetNewOrUsed called during GC");
 
     nsCOMPtr<nsISupports> identity;
 #ifdef XPC_IDISPATCH_SUPPORT
     // XXX This is done for the benefit of some warped COM implementations
@@ -320,35 +325,47 @@ XPCWrappedNative::GetNewOrUsed(XPCCallCo
     
     // We use an AutoMarkingPtr here because it is possible for JS gc to happen
     // after we have Init'd the wrapper but *before* we add it to the hashtable.
     // This would cause the mSet to get collected and we'd later crash. I've
     // *seen* this happen.
     AutoMarkingWrappedNativePtr wrapper(ccx);
 
     Native2WrappedNativeMap* map = Scope->GetWrappedNativeMap();
+    if(!cache)
+    {
+        {   // scoped lock
+            XPCAutoLock lock(mapLock);
+            wrapper = map->Find(identity);
+            if(wrapper)
+                wrapper->AddRef();
+        }
+
+        if(wrapper)
+        {
+            if(Interface &&
+               !wrapper->FindTearOff(ccx, Interface, JS_FALSE, &rv))
+            {
+                NS_RELEASE(wrapper);
+                NS_ASSERTION(NS_FAILED(rv), "returning NS_OK on failure");
+                return rv;
+            }
+            DEBUG_CheckWrapperThreadSafety(wrapper);
+            *resultWrapper = wrapper;
+            return NS_OK;
+        }
+    }
+#ifdef DEBUG
+    else if(!cache->GetWrapper())
     {   // scoped lock
         XPCAutoLock lock(mapLock);
-        wrapper = map->Find(identity);
-        if(wrapper)
-            wrapper->AddRef();
+        NS_ASSERTION(!map->Find(identity),
+                     "There's a wrapper in the hashtable but it wasn't cached?");
     }
-
-    if(wrapper)
-    {
-        if(Interface && !wrapper->FindTearOff(ccx, Interface, JS_FALSE, &rv))
-        {
-            NS_RELEASE(wrapper);
-            NS_ASSERTION(NS_FAILED(rv), "returning NS_OK on failure");
-            return rv;
-        }
-        DEBUG_CheckWrapperThreadSafety(wrapper);
-        *resultWrapper = wrapper;
-        return NS_OK;
-    }
+#endif
 
     // There is a chance that the object wants to have the self-same JSObject
     // reflection regardless of the scope into which we are reflecting it.
     // Many DOM objects require this. The scriptable helper specifies this
     // in preCreate by indicating a 'parent' of a particular scope.
     //
     // To handle this we need to get the scriptable helper early and ask it.
     // It is possible that we will then end up forwarding this entire call
@@ -408,25 +425,30 @@ XPCWrappedNative::GetNewOrUsed(XPCCallCo
                      "Parent should never be an XPCNativeWrapper here");
 
         if(parent != plannedParent)
         {
             XPCWrappedNativeScope* betterScope =
                 XPCWrappedNativeScope::FindInJSObjectScope(ccx, parent);
             if(betterScope != Scope)
                 return GetNewOrUsed(ccx, identity, betterScope, Interface,
-                                    isGlobal, resultWrapper);
+                                    cache, isGlobal, resultWrapper);
 
             newParentVal = OBJECT_TO_JSVAL(parent);
         }
 
         // Take the performance hit of checking the hashtable again in case
         // the preCreate call caused the wrapper to get created through some
         // interesting path (the DOM code tends to make this happen sometimes).
 
+        if(cache)
+        {
+            wrapper = static_cast<XPCWrappedNative*>(cache->GetWrapper());
+        }
+        else
         {   // scoped lock
             XPCAutoLock lock(mapLock);
             wrapper = map->Find(identity);
             if(wrapper)
                 wrapper->AddRef();
         }
 
         if(wrapper)
@@ -455,35 +477,40 @@ XPCWrappedNative::GetNewOrUsed(XPCCallCo
     {
         proto = XPCWrappedNativeProto::GetNewOrUsed(ccx, Scope, info, &sciProto,
                                                     JS_FALSE, isGlobal);
         if(!proto)
             return NS_ERROR_FAILURE;
 
         proto->CacheOffsets(identity);
 
-        wrapper = new XPCWrappedNative(identity, proto);
+        wrapper = new XPCWrappedNative(identity.get(), proto);
         if(!wrapper)
             return NS_ERROR_FAILURE;
     }
     else
     {
         AutoMarkingNativeSetPtr set(ccx);
         set = XPCNativeSet::GetNewOrUsed(ccx, nsnull, Interface, 0);
 
         if(!set)
             return NS_ERROR_FAILURE;
 
-        wrapper = new XPCWrappedNative(identity, Scope, set);
+        wrapper = new XPCWrappedNative(identity.get(), Scope, set);
         if(!wrapper)
             return NS_ERROR_FAILURE;
 
         DEBUG_ReportShadowedMembers(set, wrapper, nsnull);
     }
 
+    // The strong reference was taken over by the wrapper, so make the nsCOMPtr
+    // forget about it.
+    // Note that identity is null from here on!
+    identity.forget();
+
     NS_ADDREF(wrapper);
 
     NS_ASSERTION(!XPCNativeWrapper::IsNativeWrapper(parent),
                  "XPCNativeWrapper being used to parent XPCWrappedNative?");
     
     if(!wrapper->Init(ccx, parent, isGlobal, &sciWrapper))
     {
         NS_RELEASE(wrapper);
@@ -645,44 +672,44 @@ XPCWrappedNative::GetUsedOnly(XPCCallCon
         return rv;
     }
 
     *resultWrapper = wrapper;
     return NS_OK;
 }
 
 // This ctor is used if this object will have a proto.
-XPCWrappedNative::XPCWrappedNative(nsISupports* aIdentity,
+XPCWrappedNative::XPCWrappedNative(already_AddRefed<nsISupports> aIdentity,
                                    XPCWrappedNativeProto* aProto)
     : mMaybeProto(aProto),
       mSet(aProto->GetSet()),
       mFlatJSObject((JSObject*)JSVAL_ONE), // non-null to pass IsValid() test
       mScriptableInfo(nsnull),
       mWrapper(nsnull)
 {
-    NS_ADDREF(mIdentity = aIdentity);
+    mIdentity = aIdentity.get();
 
     NS_ASSERTION(mMaybeProto, "bad ctor param");
     NS_ASSERTION(mSet, "bad ctor param");
 
     DEBUG_TrackNewWrapper(this);
 }
 
 // This ctor is used if this object will NOT have a proto.
-XPCWrappedNative::XPCWrappedNative(nsISupports* aIdentity,
+XPCWrappedNative::XPCWrappedNative(already_AddRefed<nsISupports> aIdentity,
                                    XPCWrappedNativeScope* aScope,
                                    XPCNativeSet* aSet)
 
     : mMaybeScope(TagScope(aScope)),
       mSet(aSet),
       mFlatJSObject((JSObject*)JSVAL_ONE), // non-null to pass IsValid() test
       mScriptableInfo(nsnull),
       mWrapper(nsnull)
 {
-    NS_ADDREF(mIdentity = aIdentity);
+    mIdentity = aIdentity.get();
 
     NS_ASSERTION(aScope, "bad ctor param");
     NS_ASSERTION(aSet, "bad ctor param");
 
     DEBUG_TrackNewWrapper(this);
 }
 
 XPCWrappedNative::~XPCWrappedNative()
--- a/xpcom/glue/nsCOMPtr.h
+++ b/xpcom/glue/nsCOMPtr.h
@@ -1060,16 +1060,26 @@ class nsCOMPtr<nsISupports>
         {
           nsISupports* temp = rhs;
           NSCAP_LOG_ASSIGNMENT(this, temp);
           NSCAP_LOG_RELEASE(this, mRawPtr);
           rhs = mRawPtr;
           mRawPtr = temp;
         }
 
+      already_AddRefed<nsISupports>
+      forget()
+          // return the value of mRawPtr and null out mRawPtr. Useful for
+          // already_AddRefed return values.
+        {
+          nsISupports* temp = 0;
+          swap(temp);
+          return temp;
+        }
+
       void
       forget( nsISupports** rhs NS_OUTPARAM )
           // Set the target of rhs to the value of mRawPtr and null out mRawPtr.
           // Useful to avoid unnecessary AddRef/Release pairs with "out"
           // parameters.
         {
           NS_ASSERTION(rhs, "Null pointer passed to forget!");
           *rhs = 0;