368774 - Make cycle collector work with refcounted non-XPCOM objects

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter Van der Beken [:peterv]]

We have a bunch of objects that are refcounted but are not XPCOM. If we want the cycle collector to traverse them it needs to be aware of their refcount and children, but it can't QI them to nsCycleCollectionParticipant. I have an initial approach to do this, but graydon might take this on himself. I'll attach a rough patch (applies on top of 368773, sorry, need to untangle trees).

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Rough draft

Comment 2

[Graydon Hoare :graydon]

Attachment: refresh against trunk + various minor fixes

I like the look of this, and it seems to work. It does not produce any difference in the reported leaks, but it seems like a step in the right direction.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, the nsFocusController.cpp diff in the above patch really belongs as an edit to peterv's patch in bug 368773, already fixed in v1.3 of that patch.

The double definition of NS_IMPL_CYCLE_COLLECTION_UNLINK_0 removed in nsCycleCollectionParticipant.h also seems to be patching something not in the tree.  Likewise, the context for the last change in that file (the comment after it) doesn't seem to match anything in the tree.  (And the trailing slashes in that chunk don't line up.)

Comment 4

[Peter Van der Beken [:peterv]]

My only worry with the current approach is that it is a bit fragile that you have to explicitly pass the right helper to NoteNativeChild, but I don't really have a better idea. Using templates to get the right helper would probably fail with inheritance.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 256686 [details] [diff] [review]
refresh against trunk + various minor fixes

>diff -r 1aeacb869694 xpcom/base/nsCycleCollector.cpp

>+struct nsCycleCollectionNativeRuntime : 
>+    public nsCycleCollectionLanguageRuntime 
>+{

>+    nsresult Root(const nsDeque &nodes)
>+    {
>+        NS_ERROR("Can't root!");
>+        return NS_OK;
>+    }
>+
>+    nsresult Unlink(const nsDeque &nodes)
>+    {
>+        // XXX Currently do nothing.
>+        return NS_OK;
>+    }
>+
>+    nsresult Unroot(const nsDeque &nodes)
>+    {
>+        NS_ERROR("Can't unroot!");
>+        return NS_OK;
>+    }

These NS_ERRORs currently fire.  You should either change callers or (preferably, I think), remove the NS_ERRORs.

The "XXX" comment in Unlink should probably mention that doing something requires actually doing something for Root/Unroot that make it safe to do some Unlink operation.  But I'd hope it's not a problem, since as long as there's something in the cycle that supports Unlike, things should be OK, right?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 256686 [details] [diff] [review]
refresh against trunk + various minor fixes

>diff -r 1aeacb869694 xpcom/glue/nsISupportsImpl.h
>-  mRefCnt.decr(NS_STATIC_CAST(_basetype *, this));		              \
>+  mRefCnt.decr(NS_STATIC_CAST(_basetype *, this));		                        \

Both of these lines have trailing tabs, so it's just changing one incorrectly indented version to another.  But I fixed the trailing tabs in bug 370702, which conflicts with this, so probably better to drop this change.

(I might have more comments on this later; I need to see if this patch is responsible for the Fault()s that I've been seeing.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #6)
> (I might have more comments on this later; I need to see if this patch is
> responsible for the Fault()s that I've been seeing.)

I suspect it was, but I'm not sure, since I had one other patch that I removed from my tree at the same time.  But now I've updated to the trunk, and this patch needs a good bit of merging (I could do the first bit, but didn't immediately see how to do the second).

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Updated to trunk

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Updated to trunk

Grrr.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Updated to trunk

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Updated to trunk

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

This one allows unlinking of refcounted non-XPCOM objects too. It adds a word to the size of PtrInfo, and sacrifices one more bit in mInternalRefs. The numbers I'm getting for cycle collection times aren't very stable, but it looks like this doesn't regress cycle collection times a lot.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1

This also contains the patch for bug 379593.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1

Up-to-date after landing fix for bug 379593.

Comment 15

[Peter Van der Beken [:peterv]]

I'll give a little explanation of what the patch changes in the cycle collector. It adds a nsCycleCollectionParticipant that has the methods to traverse and unlink an object (which used to be on the language runtimes). It adds a method on the language runtimes to go from object to nsCycleCollectionParticipant. The XPCOM runtime uses QI to get the nsCycleCollectionParticipant, the XPConnect runtime is the nsCycleCollectionParticipant for JS objects and returns itself. For non-XPCOM refcounted C++ objects the nsCycleCollectionParticipant is passed in by the caller of NoteNativeChild. I cached the nsCycleCollectionParticipant in the PtrInfo, replacing the language. Because we only put XPCOM objects in the purple buffer and we can't check the language anymore I also needed another bit in PtrInfo to signify 'marked purple'.

Comment 16

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

You need to traverse mAttachedStack in the nsBindingManager traverse.

Also, I think it might be worth adding something like NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY but that traverses an nsTArray. There are at least two of those in nsBindingManager and over time we'll hopefully get more.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263701 [details] [diff] [review]
v1

nsXPCOMCycleCollectionParticipant::AddRef should probably return 2
rather than 1.

+class NS_COM nsXPCOMCycleCollectionParticipant
+    : public nsISupports,
+      public nsCycleCollectionParticipant

Why does nsXPCOMCycleCollectionParticipant need to inherit from
nsISupports but the others not?  It seems like either
nsCycleCollectionParticipant should, or none of them should.
(I admit it's a little ugly to return a non-nsISupports from QI, but
we're breaking the rules of QI already.)  I don't see any reason it has
to inherit from nsISupports.

-    NS_EXTERNAL_VIS_(PRBool) CheckForRightISupports(nsISupports *s);
+    PRBool CheckForRightISupports(nsISupports *s);
I'd think you still need this, since it needs to be called from outside
XPCOM.  Although maybe the NS_COM in the class is sufficent...


     PRUint32 mColor : 2;
-    PRUint32 mInternalRefs : 30;
-    // FIXME: mLang expands back to a full word when bug 368774 lands.
-    PRUint32 mLang : 2;
-    PRUint32 mRefCount : 30;
+    PRUint32 mWasPurple : 1;
+    PRUint32 mInternalRefs : 29;
+    PRUint32 mRefCount;

Seems like you should reorder, give mInternalRefs 30, and mRefCount 31.


In NoteScriptChild:
+    nsCycleCollectionParticipant *cp = mRuntimes[langID]->ToParticipant(child);
+    if (cp) {

If you need to check this at all, it should probably be an early return,
but why do you need to check it?

Seems like mBufs should be renamed to mBuf.  And the comment before its
declaration should be fixed.

+    if ((void*)mCurrPi->mParticipant ==
+        (void*)mRuntimes[nsIProgrammingLanguage::JAVASCRIPT])

You should be able to do this without casts.  If you can't, it might
fail.

nsCycleCollector_isScanSafe seems like it could become #ifdef DEBUG and
file-static.


The nsVariant.h change doesn't look like it's needed; if it is, it seems
like it belongs elsewhere.



nsXULPrototypeElement is reference counted, so it seems like
nsXULDocument's traversal of mRoot shouldn't just traverse through them
-- nsXULElement should traverse mPrototype, and the cycle collector
should know about the prototype node objects rather than just giving
them a traverse method that steps all the way through them.  That can be
a followup bug, but I suspect one worth filing (or doing here).

... but I think that means you should leave an XXX comment in
nsXULPrototypeDocument where you're removing the current one.

r=dbaron (and sorry for the long delay here)

Comment 18

[Peter Van der Beken [:peterv]]

(In reply to comment #17)
> Why does nsXPCOMCycleCollectionParticipant need to inherit from
> nsISupports but the others not?  It seems like either
> nsCycleCollectionParticipant should, or none of them should.
> (I admit it's a little ugly to return a non-nsISupports from QI, but
> we're breaking the rules of QI already.)  I don't see any reason it has
> to inherit from nsISupports.

I removed nsISupports.

> +    PRUint32 mWasPurple : 1;
> +    PRUint32 mInternalRefs : 29;
> +    PRUint32 mRefCount;
> 
> Seems like you should reorder, give mInternalRefs 30, and mRefCount 31.

I originally didn't change mRefCount because if mInternalRefs overflows we simply won't Unlink, but if mRefCount overflows we could Unlink too soon. Though I guess overflowing mRefCount would mean we're failing somewhere anyway.

> In NoteScriptChild:
> +    nsCycleCollectionParticipant *cp =
> mRuntimes[langID]->ToParticipant(child);
> +    if (cp) {
> 
> If you need to check this at all, it should probably be an early return,
> but why do you need to check it?

Currently we don't need to, but I don't know how other language runtimes will use ToParticipant, they might not have a participant for every object?

> The nsVariant.h change doesn't look like it's needed; if it is, it seems
> like it belongs elsewhere.

It does belong here, nsVariant has a Traverse method that needs it. It probably always got the definition from nsCycleCollector.h somehow, but I removed that one.

> nsXULPrototypeElement is reference counted, so it seems like
> nsXULDocument's traversal of mRoot shouldn't just traverse through them
> -- nsXULElement should traverse mPrototype, and the cycle collector
> should know about the prototype node objects rather than just giving
> them a traverse method that steps all the way through them.  That can be
> a followup bug, but I suspect one worth filing (or doing here).

Did it here. I thought I'd done this already but apparently not :-/.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> > nsXULPrototypeElement is reference counted, so it seems like
> > nsXULDocument's traversal of mRoot shouldn't just traverse through them
> > -- nsXULElement should traverse mPrototype, and the cycle collector
> > should know about the prototype node objects rather than just giving
> > them a traverse method that steps all the way through them.  That can be
> > a followup bug, but I suspect one worth filing (or doing here).
> 
> Did it here. I thought I'd done this already but apparently not :-/.

Looks good, but you should also add the traversal of mPrototype to nsXULElement.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The other thing that worries me a little is how many unlink methods you're not modifying.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 264998 [details] [diff] [review]
v1.1

>Index: js/src/xpconnect/src/nsXPConnect.cpp

>-nsresult 
>-nsXPConnect::Traverse(void *p, nsCycleCollectionTraversalCallback &cb)
>+NS_IMETHODIMP 
>+nsXPConnect::Traverse(void *p,
>+                      nsCycleCollectionTraversalCallback &cb)

You have a trailing space after NS_IMETHODIMP.

Comment 23

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> The other thing that worries me a little is how many unlink methods you're not
> modifying.

Modifying in what way?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Modifying to unlink the nsRefPtrs that you're adding traversal for.

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Not unlinking often doesn't change much, because the cycle will be broken by unlinking of another edge. Figuring out which edges can be safely unlinked isn't trivial though.

Comment 26

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

Comment on attachment 265671 [details] [diff] [review]
v1.2

>Index: content/xbl/src/nsBindingManager.cpp
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_0(nsAnonymousContentList)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsAnonymousContentList)
>+  {
>+    PRInt32 i, count = tmp->mElements->Length();
>+    for (i = 0; i < count; ++i) {
>+      NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mElements->ElementAt(i),
>+                                                      nsXBLInsertionPoint);
>+    }

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY

>Index: content/xbl/src/nsXBLBinding.cpp
>+TraverseKey(nsISupports* aKey, nsInsertionPointList* aData, void* aClosure)
>+{
>+  nsCycleCollectionTraversalCallback &cb = 
>+    *NS_STATIC_CAST(nsCycleCollectionTraversalCallback*, aClosure);
>+
>+  cb.NoteXPCOMChild(aKey);
>+  if (aData) {
>+    PRInt32 i, count = aData->Length();
>+    for (i = 0; i < count; ++i) {
>+      NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(aData->ElementAt(i),
>+                                                   nsXBLInsertionPoint);
>+    }
>+  }


Hmm.. I guess you can't use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY here since that only works on direct members :( Might be good letting that macro have one more level of indirection so that it can be used here?

sr=me with those.

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

(In reply to comment #26)
> (From update of attachment 265671 [details] [diff] [review])
> >+    PRInt32 i, count = tmp->mElements->Length();

> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY

Can't do that because mElements is a pointer to an nsTArray. I didn't add yet another macro for this one case.

> Hmm.. I guess you can't use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY here
> since that only works on direct members :( Might be good letting that macro
> have one more level of indirection so that it can be used here?

Did that.

Comment 28

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

If you added the indirection, couldn't you use it in the first case then?

Comment 29

[Steve England [:stevee]]

'Linux fxdbug-linux-tbox Dep' is reporting an RLk jump from 5.23KB to 6.09KB after this landing.

Comment 30

[Robert Kaiser]

SeaMonkey nye tinderbox also has a RLk of about the same size: 15.4KB -> 16.1KB

Comment 31

[Peter Van der Beken [:peterv]]

The leak regression was tracked in bug 381897.