Bug 1385459 - Don't use QI to canonicalize nsISupports pointers in the purple buffer. r=smaug
authorAndrew McCreight <continuation@gmail.com>
Fri, 28 Jul 2017 15:24:17 -0700
changeset 372052 80647273e2f06e3968f4bba052be4a516a2b0f61
parent 372051 29ffe5a88b7b3b113ee9ee4db80885a1470ab0b9
child 372053 6932e938628b49d7e824ec9c3cda34187fb805c2
push id32263
push userkwierso@gmail.com
push dateMon, 31 Jul 2017 23:43:31 +0000
treeherdermozilla-central@8b19670d12fd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1385459
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1385459 - Don't use QI to canonicalize nsISupports pointers in the purple buffer. r=smaug The nsISupports objects added to the purple buffer are already canonical, so we can avoid some overhead by not QIing them to nsCycleCollectionISupports. This QIing overhead shows up in profiles. MozReview-Commit-ID: CQN6wwc7MZm
xpcom/base/nsCycleCollector.cpp
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -951,37 +951,16 @@ static nsISupports*
 CanonicalizeXPCOMParticipant(nsISupports* aIn)
 {
   nsISupports* out = nullptr;
   aIn->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
                       reinterpret_cast<void**>(&out));
   return out;
 }
 
-static inline void
-ToParticipant(nsISupports* aPtr, nsXPCOMCycleCollectionParticipant** aCp);
-
-static void
-CanonicalizeParticipant(void** aParti, nsCycleCollectionParticipant** aCp)
-{
-  // If the participant is null, this is an nsISupports participant,
-  // so we must QI to get the real participant.
-
-  if (!*aCp) {
-    nsISupports* nsparti = static_cast<nsISupports*>(*aParti);
-    nsparti = CanonicalizeXPCOMParticipant(nsparti);
-    NS_ASSERTION(nsparti,
-                 "Don't add objects that don't participate in collection!");
-    nsXPCOMCycleCollectionParticipant* xcp;
-    ToParticipant(nsparti, &xcp);
-    *aParti = nsparti;
-    *aCp = xcp;
-  }
-}
-
 struct nsPurpleBufferEntry
 {
   nsPurpleBufferEntry(void* aObject, nsCycleCollectingAutoRefCnt* aRefCnt,
                       nsCycleCollectionParticipant* aParticipant)
     : mObject(aObject)
     , mRefCnt(aRefCnt)
     , mParticipant(aParticipant)
   {
@@ -1421,16 +1400,31 @@ ToParticipant(nsISupports* aPtr, nsXPCOM
   // We use QI to move from an nsISupports to an
   // nsXPCOMCycleCollectionParticipant, which is a per-class singleton helper
   // object that implements traversal and unlinking logic for the nsISupports
   // in question.
   *aCp = nullptr;
   CallQueryInterface(aPtr, aCp);
 }
 
+static void
+ToParticipant(void* aParti, nsCycleCollectionParticipant** aCp)
+{
+  // If the participant is null, this is an nsISupports participant,
+  // so we must QI to get the real participant.
+
+  if (!*aCp) {
+    nsISupports* nsparti = static_cast<nsISupports*>(aParti);
+    MOZ_ASSERT(CanonicalizeXPCOMParticipant(nsparti) == nsparti);
+    nsXPCOMCycleCollectionParticipant* xcp;
+    ToParticipant(nsparti, &xcp);
+    *aCp = xcp;
+  }
+}
+
 template<class Visitor>
 MOZ_NEVER_INLINE void
 GraphWalker<Visitor>::Walk(PtrInfo* aPi)
 {
   nsDeque queue;
   CheckedPush(queue, aPi);
   DoWalk(queue);
 }
@@ -2229,17 +2223,17 @@ CCGraphBuilder::AddNode(void* aPtr, nsCy
                "nsCycleCollectionParticipant shouldn't change!");
   }
   return result;
 }
 
 bool
 CCGraphBuilder::AddPurpleRoot(void* aRoot, nsCycleCollectionParticipant* aParti)
 {
-  CanonicalizeParticipant(&aRoot, &aParti);
+  ToParticipant(aRoot, &aParti);
 
   if (WantAllTraces() || !aParti->CanSkipInCC(aRoot)) {
     PtrInfo* pinfo = AddNode(aRoot, aParti);
     if (!pinfo) {
       return false;
     }
   }
 
@@ -2665,17 +2659,17 @@ public:
 
   bool
   Visit(nsPurpleBuffer& aBuffer, nsPurpleBufferEntry* aEntry)
   {
     MOZ_ASSERT(aEntry->mObject, "Null object in purple buffer");
     if (!aEntry->mRefCnt->get()) {
       void* o = aEntry->mObject;
       nsCycleCollectionParticipant* cp = aEntry->mParticipant;
-      CanonicalizeParticipant(&o, &cp);
+      ToParticipant(o, &cp);
       SnowWhiteObject swo = { o, cp, aEntry->mRefCnt };
       mObjects.InfallibleAppend(swo);
       aBuffer.Remove(aEntry);
     }
     return true;
   }
 
   bool HasSnowWhiteObjects() const
@@ -2791,17 +2785,17 @@ public:
       } else if (!mDispatchedDeferredDeletion) {
         mDispatchedDeferredDeletion = true;
         nsCycleCollector_dispatchDeferredDeletion(false);
       }
       return true;
     }
     void* o = aEntry->mObject;
     nsCycleCollectionParticipant* cp = aEntry->mParticipant;
-    CanonicalizeParticipant(&o, &cp);
+    ToParticipant(o, &cp);
     if (aEntry->mRefCnt->IsPurple() && !cp->CanSkip(o, false) &&
         (!mRemoveChildlessNodes || MayHaveChild(o, cp))) {
       return true;
     }
     aBuffer.Remove(aEntry);
     return true;
   }
 
@@ -2998,20 +2992,19 @@ public:
   Visit(nsPurpleBuffer& aBuffer, nsPurpleBufferEntry* aEntry)
   {
     MOZ_ASSERT(aEntry->mObject,
                "Entries with null mObject shouldn't be in the purple buffer.");
     MOZ_ASSERT(aEntry->mRefCnt->get() != 0,
                "Snow-white objects shouldn't be in the purple buffer.");
 
     void* obj = aEntry->mObject;
-    if (!aEntry->mParticipant) {
-      obj = CanonicalizeXPCOMParticipant(static_cast<nsISupports*>(obj));
-      MOZ_ASSERT(obj, "Don't add objects that don't participate in collection!");
-    }
+
+    MOZ_ASSERT(aEntry->mParticipant || CanonicalizeXPCOMParticipant(static_cast<nsISupports*>(obj)) == obj,
+               "Suspect nsISupports pointer must be canonical");
 
     PtrInfo* pi = mGraph.FindNode(obj);
     if (!pi) {
       return true;
     }
     MOZ_ASSERT(pi->mParticipant, "No dead objects should be in the purple buffer.");
     if (MOZ_UNLIKELY(mLogger)) {
       mLogger->NoteIncrementalRoot((uint64_t)pi->mPointer);
@@ -3484,16 +3477,19 @@ nsCycleCollector::Suspect(void* aPtr, ns
     return;
   }
 
   MOZ_ASSERT(aPtr, "Don't suspect null pointers");
 
   MOZ_ASSERT(HasParticipant(aPtr, aParti),
              "Suspected nsISupports pointer must QI to nsXPCOMCycleCollectionParticipant");
 
+  MOZ_ASSERT(aParti || CanonicalizeXPCOMParticipant(static_cast<nsISupports*>(aPtr)) == aPtr,
+             "Suspect nsISupports pointer must be canonical");
+
   mPurpleBuf.Put(aPtr, aParti, aRefCnt);
 }
 
 void
 nsCycleCollector::CheckThreadSafety()
 {
 #ifdef DEBUG
   MOZ_ASSERT(mEventTarget->IsOnCurrentThread());
@@ -4004,17 +4000,17 @@ CycleCollectedJSContext::Get()
 MOZ_NEVER_INLINE static void
 SuspectAfterShutdown(void* aPtr, nsCycleCollectionParticipant* aCp,
                      nsCycleCollectingAutoRefCnt* aRefCnt,
                      bool* aShouldDelete)
 {
   if (aRefCnt->get() == 0) {
     if (!aShouldDelete) {
       // The CC is shut down, so we can't be in the middle of an ICC.
-      CanonicalizeParticipant(&aPtr, &aCp);
+      ToParticipant(aPtr, &aCp);
       aRefCnt->stabilizeForDeletion();
       aCp->DeleteCycleCollectable(aPtr);
     } else {
       *aShouldDelete = true;
     }
   } else {
     // Make sure we'll get called again.
     aRefCnt->RemoveFromPurpleBuffer();