Closed Bug 864035 Opened 7 years ago Closed 7 years ago

Add a thread-safe RefCounted and WeakPtr implementation

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ehsan, Assigned: glandium)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Depends on: 863884
Blocks: 865583
Assignee: ehsan → mh+mozilla
Attachment #741717 - Flags: feedback?(ehsan)
Attachment #741773 - Flags: review?(bugs) → review+
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.
Attachment #741773 - Attachment is obsolete: true
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 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.
Attachment #741905 - Flags: review?(bugs) → review+
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 .
Attachment #741717 - Flags: feedback?(ehsan) → feedback+
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.
Attachment #741717 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #748901 - Flags: review?(jwalden+bmo)
Attachment #741717 - Attachment is obsolete: true
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.
Attachment #748901 - Flags: review?(jwalden+bmo) → review+
Backed out the first part because of windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/013863871e61
(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.
Guessing the backout amounts to a [leave open]?
Whiteboard: [leave open]
(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
PS: I am using VS 2012 Update 2.
(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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I filed bug 923554 as I do not understand how a WeakPtr to an AtomicSupportsWeakPtr can currently be used atomically.
(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.
You need to log in before you can comment on or make changes to this bug.