Closed Bug 368774 Opened 13 years ago Closed 13 years ago

Make cycle collector work with refcounted non-XPCOM objects

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(2 files, 10 obsolete files)

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).
Attached patch Rough draft (obsolete) — Splinter Review
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.
Attachment #253428 - Attachment is obsolete: true
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.)
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 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 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.)
(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).
Attached patch Updated to trunk (obsolete) — Splinter Review
Attachment #256686 - Attachment is obsolete: true
Attached patch Updated to trunk (obsolete) — Splinter Review
Grrr.
Attachment #258057 - Attachment is obsolete: true
Attached patch Updated to trunk (obsolete) — Splinter Review
Attachment #258058 - Attachment is obsolete: true
Attached patch Updated to trunk (obsolete) — Splinter Review
Assignee: nobody → peterv
Attachment #260021 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #262671 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This also contains the patch for bug 379593.
Attachment #263149 - Attachment is obsolete: true
Attachment #263589 - Flags: review?(dbaron)
Attached patch v1 (obsolete) — Splinter Review
Up-to-date after landing fix for bug 379593.
Attachment #263589 - Attachment is obsolete: true
Attachment #263701 - Flags: review?(dbaron)
Attachment #263589 - Flags: review?(dbaron)
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'.
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 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)
Attachment #263701 - Flags: review?(dbaron) → review+
(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 :-/.
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #263701 - Attachment is obsolete: true
Attachment #264998 - Flags: superreview?(jonas)
Attachment #264998 - Flags: review+
(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.
The other thing that worries me a little is how many unlink methods you're not modifying.
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.
(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?
Modifying to unlink the nsRefPtrs that you're adding traversal for.
Attached patch v1.2Splinter Review
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.
Attachment #264998 - Attachment is obsolete: true
Attachment #265671 - Flags: superreview?(jonas)
Attachment #265671 - Flags: review+
Attachment #264998 - Flags: superreview?(jonas)
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.
Attachment #265671 - Flags: superreview?(jonas) → superreview+
(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.
If you added the indirection, couldn't you use it in the first case then?
'Linux fxdbug-linux-tbox Dep' is reporting an RLk jump from 5.23KB to 6.09KB after this landing.
Depends on: 381897
SeaMonkey nye tinderbox also has a RLk of about the same size: 15.4KB -> 16.1KB
The leak regression was tracked in bug 381897.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.