1179787 - make nsRunnableMethodArguments deal with ns{Ref,COM}Ptr arguments as one would expect

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

Based on the handy chart here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#582

passing a nsCOMPtr<T> template arg to NS_NewRunnableMethodWithArg{,s} when the method to be run takes a T* in the corresponding position is inefficient, because we'll copy the nsCOMPtr<T> member for argument passing, only to destroy it after the method has been called.  This extra addref/release cycle is completely unnecessary.

The same argument applies to nsRefPtr<T> template arguments.

The right thing to do is default to something like StorensRefPtrPassByPtr for such arguments, possibly with a few tweaks to deal with arguments that can be safely Move()'d.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - forward StorensRefPtrPassByPtr's constructor argument

StorensRefPtrPassByPtr is currently used for storing reference-counted
types passed as T* template arguments to NS_NewRunnableMethodWith*.
We'd also like to use it to store nsRefPtr<T> template arguments.  While
it could be used in its current form, it'd be better for its constructor
to support forwarding, so that something like:

  NS_NewRunnableMethodWithArg<nsRefPtr<T>>(..., nsRefPtr<T>(local));

doesn't cause unnecessary reference counting.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add StorensCOMPtrPassByPtr

This class is an exact copy of StorensRefPtrPassByPtr, except for nsCOMPtr.

I tried making a superclass, but it didn't seem to make the code much clearer
once you had to add a forwarding constructor.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add specializations of ParameterStorage for ns{Ref,COM}Ptr

I suppose it would it some sense be better to add this to the already-existing
ParameterStorage test cascade, but this way is much easier and much clearer, IMHO.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8629044 [details] [diff] [review]
part 2 - add StorensCOMPtrPassByPtr

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

::: xpcom/glue/nsThreadUtils.h
@@ +455,5 @@
>  struct IsParameterStorageClass<StorensRefPtrPassByPtr<S>>
>    : public mozilla::TrueType {};
>  
>  template<typename T>
> +struct StorensCOMPtrPassByPtr

If we're going to pass it by pointer, does storing it by nsCOMPtr give us anything over storing it by nsRefPtr?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #4)
> ::: xpcom/glue/nsThreadUtils.h
> @@ +455,5 @@
> >  struct IsParameterStorageClass<StorensRefPtrPassByPtr<S>>
> >    : public mozilla::TrueType {};
> >  
> >  template<typename T>
> > +struct StorensCOMPtrPassByPtr
> 
> If we're going to pass it by pointer, does storing it by nsCOMPtr give us
> anything over storing it by nsRefPtr?

You are Not Supposed to use interface classes with nsRefPtr, just as you're Not Supposed to use concrete classes with nsCOMPtr.  We might be able to get away with it here, though?

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8629045 [details] [diff] [review]
part 3 - add specializations of ParameterStorage for ns{Ref,COM}Ptr

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

::: xpcom/glue/nsThreadUtils.h
@@ +636,5 @@
> +template<typename T>
> +struct ParameterStorage<nsRefPtr<T>>
> +{
> +  typedef StorensRefPtrPassByPtr<T> Type;
> +};

ParameterStorage currently only has a primary template, and uses a cascade of mozilla::Conditionals to choose the storage type. Is there a reason we're breaking from that pattern and using specializations here? If not, I would prefer sticking to the existing technique for uniformity.

Otherwise, looks good!

Comment 7

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> (In reply to Botond Ballo [:botond] from comment #4)
> > ::: xpcom/glue/nsThreadUtils.h
> > @@ +455,5 @@
> > >  struct IsParameterStorageClass<StorensRefPtrPassByPtr<S>>
> > >    : public mozilla::TrueType {};
> > >  
> > >  template<typename T>
> > > +struct StorensCOMPtrPassByPtr
> > 
> > If we're going to pass it by pointer, does storing it by nsCOMPtr give us
> > anything over storing it by nsRefPtr?
> 
> You are Not Supposed to use interface classes with nsRefPtr, just as you're
> Not Supposed to use concrete classes with nsCOMPtr.  We might be able to get
> away with it here, though?

It looks like we currently use nsRefPtr<T> with an interface class if it's passed by raw pointer, as we choose StorensRefPtrPassByPtr for anything that HasRefCountMethods here [1]. I feel like we should care or not care about the distinction consistently :)

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=470bb48707f8#542

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8629044 [details] [diff] [review]
part 2 - add StorensCOMPtrPassByPtr

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

Dropping review flag until we come to a resolution on the nsRefPtr/nsCOMPtr issue.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8629045 [details] [diff] [review]
part 3 - add specializations of ParameterStorage for ns{Ref,COM}Ptr

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

Dropping review flag until we come to a resolution about comment 6.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add template logic for smart pointer template arguments in NS_NewRunnableMethod*

OK, this (significantly longer) patch implements things as an extra case in the
ParameterStorage cascade.  We also don't bother trying to separate out nsCOMPtr
storage vs. nsRefPtr storage anymore.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8639406 [details] [diff] [review]
part 2 - add template logic for smart pointer template arguments in NS_NewRunnableMethod*

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

Thanks! r+ with comments.

::: xpcom/glue/nsThreadUtils.h
@@ +545,5 @@
> +
> +template<typename T>
> +struct IsRefcountedSmartPointer<nsRefPtr<T>> : public mozilla::TrueType
> +{
> +};

For consistency with surrounding code, please place the braces on the same line.

@@ +550,5 @@
> +
> +template<typename T>
> +struct IsRefcountedSmartPointer<nsCOMPtr<T>> : public mozilla::TrueType
> +{
> +};

Ditto.

@@ +591,5 @@
>  template<typename T>
> +struct SmartPointerStorageClass
> +  : mozilla::Conditional<IsRefcountedSmartPointer<T>::value,
> +                         StorensRefPtrPassByPtr<
> +                           typename StripSmartPointer<T>::Type>,

Can we use |typename T::element_type| instead of introducing |StripSmartPointer|?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #11)
> @@ +591,5 @@
> >  template<typename T>
> > +struct SmartPointerStorageClass
> > +  : mozilla::Conditional<IsRefcountedSmartPointer<T>::value,
> > +                         StorensRefPtrPassByPtr<
> > +                           typename StripSmartPointer<T>::Type>,
> 
> Can we use |typename T::element_type| instead of introducing
> |StripSmartPointer|?

I didn't know about T::element_type here!

...but I don't think we can use it, because the arguments to the Conditional template are evaluated non-lazily (apparently?), so trying to evaluate |typename T::element_type| when T doesn't have an element_type member results in a mess of unhelpful compilation errors (at least with GCC).  I initially tried to avoid giving StripSmartPointer a default implementation, but found that giving it one made all these strange errors go away... =/

Comment 13

[Botond Ballo [:botond]]

Ah, good point. Lazy evaluation can be introduced by turning something of this form:

  Conditional<condition,
              typename Metafunction1<Args1>::Type,
              typename Metafunction2<Args2>::Type>

into something of this form:

  Conditional<condition,
              Metafunction1<Args1>,
              Metafunction2<Args2>>::Type

but it's not clear that doing that here is worth it (especially as we'd have to get it to be in the first form to begin with), so I'm OK with going with StripSmartPointer.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f6ded61005
https://hg.mozilla.org/integration/mozilla-inbound/rev/63ad8e412a0f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a8f6ded61005
https://hg.mozilla.org/mozilla-central/rev/63ad8e412a0f