820257 - MFBT RefPtr does not support cycle collection

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

And this is a problem, because gfx/layers/ uses it, and is a good candidate for relying on cycle collection a bit (has complex ownership schemes that few people understand; among relatively few objects so that the CC overhead would be reasonable. See bug 775469 for a place where people have been hesitating about things that would have been solved by CC).

Since bug 806279 it's fairly trivial to extend CC support to new pointer and container types. Just implement ImplCycleCollectionUnlink and ImplCycleCollectionTraverse.

The possibly bigger difficulty here is not so much with RefPtr but with RefCounted, as it provides its own AddRef and Release, and for cycle collection we need custom AddRef and Release.

Traditionally that was handled by having AddRef and Release being implemented separately in each class by macros (IMPL_CYCLE_COLLECTING_ADDREF etc). But that doesn't fit very well in MFBT. Let's try to see if there is a more C++-ish way of fixing this. A separate RefCountedSupportingCycleCollection base class?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

CC needs custom Addref/Release because it really needs that refcounting happens
via nsCycleCollectingAutoRefCnt. nsCycleCollectingAutoRefCnt needs to rely on cycle collector,
which itself needs to use various xpcom and xpconnect stuff. 
I thought the only reason for mfbt RefPtr was to not rely on xpcom stuff.

Also, don't we expect some of that layers stuff to happen off-main-thread?

So, I see no reason to make RefPtr/RefCounted to support cycle collection.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Perhaps gfx/layers should have some its own collector. It could be a lot simpler, without all the
interaction with GC.

Comment 3

[Jeff Walden [:Waldo]]

The C++ way would probably be some sort of RefPtrTraits<T> that template <class T> class RefPtr<T> : private RefPtrTraits<T>, with RefPtr methods calling RefPtrTraits methods at various sequence points in all operations.  RefPtr.h would provide a template <class T> class RefPtrTraits { }; as the default in RefPtr.h, with voidish definitions of all the requisite methods.

I've actually very vaguely been meaning to do something along those lines to try to get rid of WebGLRefPtr, which looked a lot like an exact code-copy with some extra stuff thrown in.  Unsurprisingly, such a change is way way way down my priority list of stuff to do, tho.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Olli Pettay [:smaug] from comment #1)
> CC needs custom Addref/Release because it really needs that refcounting
> happens
> via nsCycleCollectingAutoRefCnt. nsCycleCollectingAutoRefCnt needs to rely
> on cycle collector,
> which itself needs to use various xpcom and xpconnect stuff. 

I don't realize how possible it is to isolate just the required bits without dragging in too much.

> Also, don't we expect some of that layers stuff to happen off-main-thread?

Are you saying that only main-thread stuff can rely on cycle collection? Is that because the cycle collector needs to block, and doesn't know how to block other threads? Couldn't other threads have their own cycle collector?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> The C++ way would probably be some sort of RefPtrTraits<T> that template
> <class T> class RefPtr<T> : private RefPtrTraits<T>, with RefPtr methods
> calling RefPtrTraits methods at various sequence points in all operations. 
> RefPtr.h would provide a template <class T> class RefPtrTraits { }; as the
> default in RefPtr.h, with voidish definitions of all the requisite methods.

I see, that makes sense.

> 
> I've actually very vaguely been meaning to do something along those lines to
> try to get rid of WebGLRefPtr, which looked a lot like an exact code-copy
> with some extra stuff thrown in.

Indeed, WebGLRefPtr could be reimplemented as MFBT RefPtr + some traits providing a custom AddRef and Release incrementing/decrementing the WebGL refcount as well as the primary one.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

CC is mainthread only currently. If you addref/release CCable objects in other threads, CC aborts.

I'd wish to change CC to be per thread, so that workers could use it too, but other people
disagree. Also, it is far from trivial task.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benoit Jacob [:bjacob] from comment #4) 
> I don't realize how possible it is to isolate just the required bits without
> dragging in too much.
(current) CC needs to deal with all the JS and XPConnect too. And XPConnect drags in everything.

Comment 7

[Andrew McCreight [:mccr8]]

> Is that because the cycle collector needs to block, and doesn't know how to block other threads?

That, and because the purple buffer gets manipulated during addref and release, and we don't have any locks on it. Any time an object is released to a non-zero refcount, it may be the last hold we have on a garbage cycle, so the object must be added to a buffer the CC will check the next time it runs. Of course, we could have one per thread, but it isn't set up for that currently.

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> > Is that because the cycle collector needs to block, and doesn't know how to block other threads?
> 
> That, and because the purple buffer gets manipulated during addref and
> release, and we don't have any locks on it. Any time an object is released
> to a non-zero refcount, it may be the last hold we have on a garbage cycle,
> so the object must be added to a buffer the CC will check the next time it
> runs. Of course, we could have one per thread, but it isn't set up for that
> currently.

If the addition-to-the-buffer is the only problem, then couldn't you use a lockless queue as well? I'll have compare-and-swap in bug 732043, at least for sizeof(void*) datatypes.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I think we should WONTFIX this.
We don't want to use RefPtr with the core Gecko stuff, and cycle collector in very much in
XPCOM core. If someone wants to use the CC, nsRefPtr and nsCOMPtr from xpcom/ should be used.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #5)
> CC is mainthread only currently. If you addref/release CCable objects in
> other threads, CC aborts.
> 
> I'd wish to change CC to be per thread, so that workers could use it too,
> but other people
> disagree. Also, it is far from trivial task.

These days cycle collector can be used in other threads too, but it is still per-thread thing.
We have separate CC for main thread and for each Worker.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Olli Pettay [:smaug] from comment #9)
> I think we should WONTFIX this.

Fine by me; let's wait for the current dev-platform thread,
  https://groups.google.com/forum/#!topic/mozilla.dev.platform/Xsuwy4h7lNE
to settle down, to avoid having the same conversation in parallel in two places. It does seem like consensus is building up in favor of removing the redundancy between nsRefPtr and RefPtr by removing RefPtr, but details are still being worked out on dev-platform.