864035 - Add a thread-safe RefCounted and WeakPtr implementation

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[(no longer active)]

Once bug 732043 is fixed, we can borrow the code that I have in attachment 739861 [details] [diff] [review] in order to provide a thread-safe RefCounted and WeakPtr implementation in MFBT.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add a thread-safe RefCounted and WeakPtr implementation

Comment 2

[Mike Hommey [:glandium]]

Attachment: Don't use a forward declaration for mozilla::dom::SpeechRecognition for use with a WeakPtr

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 741717 [details] [diff] [review]
Add a thread-safe RefCounted and WeakPtr implementation

Review of attachment 741717 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/RefPtr.h
@@ +10,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/TypeTraits.h"
> +#include "mozilla/Atomics.h"

Lemme beat Waldo to this comment... All #include lists in MFBT are sorted alphabetically.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Don't use a forward declaration for mozilla::dom::SpeechRecognition for use with a WeakPtr

This needed some headers to be exported.

Comment 5

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

Comment on attachment 741905 [details] [diff] [review]
Don't use a forward declaration for mozilla::dom::SpeechRecognition for use with a WeakPtr

Why do we need those headers to be exported?

Comment 6

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

Comment on attachment 741905 [details] [diff] [review]
Don't use a forward declaration for mozilla::dom::SpeechRecognition for use with a WeakPtr

Oh, to be eable to #include SpeechRecognition.h I guess.
Tiny bit ugly.

Comment 7

[Guilherme Gonçalves [:ggp]]

This is just as small detail, but if we're now exporting all headers under content/media/webspeech/recognition, it should be possible to remove that path from the LOCAL_INCLUDES in dom/bindings/Makefile.in .

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 741717 [details] [diff] [review]
Add a thread-safe RefCounted and WeakPtr implementation

Review of attachment 741717 [details] [diff] [review]:
-----------------------------------------------------------------

Any reason for ThreadSafe instead of something slightly shorter (and equally appropriate, seems to me) like Atomic?

Specifying the count type as atomic or non-atomic via enum, rather than exact type, is the big issue here for me, I think.

::: mfbt/RefPtr.h
@@ +15,4 @@
>  
>  namespace mozilla {
>  
> +template<typename T, typename U> class RefCounted;

The U = int default has to be on the declaration of RefCounted, i.e. here -- not on the definition.  I'm surprised this compiles.

But let's not have this default to int -- let's make it a mandatory parameter indicating whether the count should be updated atomically or not:

enum RefCountAtomicity { AtomicRefCount, NonAtomicRefCount };

and then have RefCounted pick either int or Atomic<Int>.

Per http://stackoverflow.com/questions/10268737/c11-atomics-and-intrusive-shared-pointer-reference-count we could do somewhat better here than the default SequentiallyConsistent, if Atomic were more fully-featured like std::atomic.  We might want to implement speedups along those lines at some point, but this'll do for now.

@@ +42,5 @@
>   * state distinguishes use-before-ref (refcount==0) from
>   * use-after-destroy (refcount==0xffffdead).
> + *
> + * The RefCounted template can take a second argument, but it's an
> + * implementation detail for ThreadSafeRefCounted.

We generally try to hide this stuff from the main mozilla namespace.  So let's put RefCounted in namespace detail, then have

template<typename T>
class RefCounted : public detail::RefCounted<T, detail::NonAtomicRefCount> {};

template<typename T>
class AtomicRefCounted : public detail::RefCounted<T, detail::AtomicRefCount> {};

@@ +55,1 @@
>  class RefCounted

This is slightly clearer with RefCountAtomicity not having a default, when this is in detail and the public classes both specify atomicity explicitly.

@@ +64,1 @@
>                          "T must derive from RefCounted<T>");

I *think* omitting <T, U> will have RefCounted be interpreted as that, but I could be mistaken.  In any case, the assertion reason needs updating.

::: mfbt/WeakPtr.h
@@ +63,5 @@
>  #include "mozilla/Assertions.h"
>  #include "mozilla/NullPtr.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/TypeTraits.h"
> +#include "mozilla/Atomics.h"

Alphabetical here too.

@@ +74,5 @@
>  namespace detail {
>  
>  // This can live beyond the lifetime of the class derived from SupportsWeakPtrBase.
> +template<class T, typename U=int>
> +class WeakReference : public RefCounted<WeakReference<T, U>, U>

This should use RefCountAtomicity as well.

@@ +163,5 @@
>      RefPtr<WeakReference> ref;
>  };
>  
>  template <typename T>
> +class WeakPtr : public WeakPtrBase<T, detail::WeakReference<T, typename T::refCountType> >

namespace detail {

template<typename T>
struct WeakReferenceCount
  : Conditional<IsSame<SupportsWeakPtr>::value, 
{
    static const RefCountAtomicity atomicity =
      IsBaseOf<ThreadSafeSupportsWeakPtr<T>, T>::value ? AtomicRefCount : NonAtomicRefCount;
};

}

and then passing WeakReferenceCount<T>::atomicity should let you get away without adding a public typedef to the Supports* classes, which seems a good thing.  The refcount type is an implementation detail, not something to expose.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Add a thread-safe RefCounted and WeakPtr implementation

There are some lines that are really long, but I'm not sure how to cut them.

The mozglue/linker changes are a necessary evil to override RefCounted::Release and RefCounted::~RefCounted. As there is only one usecase of this pattern in the entire tree, I dont't feel like this needs to be made easier.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 748901 [details] [diff] [review]
Add a thread-safe RefCounted and WeakPtr implementation

Review of attachment 748901 [details] [diff] [review]:
-----------------------------------------------------------------

I'll let the LibHandle bits slide for now.  But we should follow up quickly with a RefCountedTraits<T> class, that users can correctly and safely specialize, for uses like this.  Bug 820257 wants something like this, WebGLRefCountedObject wants it, you want it, etc.  It's well past time to do this now.

::: mfbt/RefPtr.h
@@ +48,3 @@
>  #endif
>  
> +enum RefCountAtomicity

Usually this stuff is file-scoped, so let's add a comment to note the out-of-file user:

// This is used by WeakPtr.h as well as this file.

@@ +96,5 @@
> +
> +}
> +
> +template<typename T>
> +struct RefCounted: public detail::RefCounted<T, detail::NonAtomicRefCount>

Make this a class, and put an extra space after the name:

class RefCounted : public ...

Here and AtomicRefCounted both.

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be729bc526a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff5c972e3af

Comment 12

[Mike Hommey [:glandium]]

Backed out the first part because of windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/013863871e61

Comment 13

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #12)
> Backed out the first part because of windows bustage.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/013863871e61

Interesting. It worked (that is, 5ff5c972e3af worked without be729bc526a3). Something must have changed in webspeech in the meanwhile.

Comment 14

[Phil Ringnalda (:philor)]

Guessing the backout amounts to a [leave open]?

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5ff5c972e3af

Comment 16

[Yuan Pengfei]

(In reply to Phil Ringnalda (:philor) from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/5ff5c972e3af

This patch breaks Windows builds.
FYI:

netwerk\base\src\BackgroundFileSaver.cpp
- #include "mozilla/RefPtr.h"
-- #include "mozilla/Atomics.h"
--- #include <atomic>
---- #include <xatomic.h>
----- #include <xutility>
which defines macro _XUTILITY_ and breaks memory\mozalloc\msvc_raise_wrappers.h on line 15

Comment 17

[Yuan Pengfei]

PS: I am using VS 2012 Update 2.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Phil Ringnalda (:philor) from comment #14)
> Guessing the backout amounts to a [leave open]?

Actually, no :)

(In reply to Yuan Pengfei from comment #16)
> (In reply to Phil Ringnalda (:philor) from comment #15)
> > https://hg.mozilla.org/mozilla-central/rev/5ff5c972e3af
> 
> This patch breaks Windows builds.
> FYI:
> 
> netwerk\base\src\BackgroundFileSaver.cpp
> - #include "mozilla/RefPtr.h"
> -- #include "mozilla/Atomics.h"
> --- #include <atomic>
> ---- #include <xatomic.h>
> ----- #include <xutility>
> which defines macro _XUTILITY_ and breaks
> memory\mozalloc\msvc_raise_wrappers.h on line 15

That would be a comment for bug 732043, not this one.

Comment 19

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

I filed bug 923554 as I do not understand how a WeakPtr to an AtomicSupportsWeakPtr can currently be used atomically.

Comment 20

[(no longer active)]

(In reply to Benoit Jacob [:bjacob] from comment #19)
> I filed bug 923554 as I do not understand how a WeakPtr to an
> AtomicSupportsWeakPtr can currently be used atomically.

It cannot.  It's buggy.