1131445 - Add variadic NS_NewRunnableMethodWithArgs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

NS_NewRunnableMethod and NS_NewRunnableMethodWithArg only allow calls to methods with 0 or 1 arguments.
Some users of NewRunnableMethodX could use it with more arguments, as they currently need to resort to tricks so that only 1 argument is passed around, e.g. bug #1130839.

To do: Implement variadic NS_NewRunnableMethodWithArgs that can accept an arbitrary number of arguments.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Semi-variadic implementation for 0-4 arguments

Proposed implementation, NS_NewRunnableMethodWithArgs(nsRefPtr<C>, &C::method, args...) is variadic, but relies on a small templated struct nsRunnableMethodArguments that is currently specialized for 0-4 arguments, it should be enough for most uses but is easy to extend as necessary. A fully variadic template should be possible but would probably require a lot of effort -- unless there's an easy way to store lambdas, or a tuple template I don't know about?
Also added NS_NewNonOwningRunnableMethodWithArgs().
Added gtest, to verify non-variadic functions still work as expected.

Comment 2

[Jean-Yves Avenard [:jya]]

I had worked on a non-variadic version to take two arguments, I asked Gerald if he could improve on it.

There's at least three custom nsRunnable in the mediasource code we could replace with this

Comment 3

[Benjamin Smedberg]

Please see bug 1098109 and https://bugzilla.mozilla.org/show_bug.cgi?id=622728#c21

Nathan is the module owner now, so he could decide differently, but I am strongly opposed to having a variadic method like this unless the storage type is explicit:

e.g. NS_NewRunnableMethod<nsCOMPtr<nsIFoo>, uint32_t, nsSomething*>(foo, val, something);

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Please see bug 1098109 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=622728#c21
> 
> Nathan is the module owner now, so he could decide differently, but I am
> strongly opposed to having a variadic method like this unless the storage
> type is explicit:
> 
> e.g. NS_NewRunnableMethod<nsCOMPtr<nsIFoo>, uint32_t, nsSomething*>(foo,
> val, something);

That's exactly how you call them, see the unit tests:
NS_NewRunnableMethodWithArgs<int, int, int>(rpt, &ThreadUtilsObject::foo3, 1111, 2222, 3333);

I had already indicated to Gerald that it needed to have explicit types, as those concerns had already been raised in bug 622728.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8561898 [details] [diff] [review]
Semi-variadic implementation for 0-4 arguments

cool. I'm going to delegate this to Waldo since he's our c++-template guru.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8561898 [details] [diff] [review]
Semi-variadic implementation for 0-4 arguments

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

::: xpcom/glue/nsThreadUtils.h
@@ +313,5 @@
>  };
>  #endif
>  
> +// struct used to store arguments and later apply them to a method.
> +template <typename... Ts> struct nsRunnableMethodArguments {};

Leave off the {} here so that attempts will fail on nsRunnableMethods<...> not being defined -- a slightly earlier error message, versus "has no apply method" or whatever.

@@ +340,5 @@
> +  nsRunnableMethodArguments(T0&& a0, T1&& a1)
> +    : m0(mozilla::Forward<T0>(a0))
> +    , m1(mozilla::Forward<T1>(a1))
> +  {}
> +  template<class C, typename M> void apply(C o, M m) { ((*o).*m)(m0, m1); }

I think m0/m1/mN need to be passed using mozilla::Move.  Consider what happens if you have this class/method used with this struct:

class C
{
  public:
    void foo(mozilla::UniquePtr<int> i) {}
};

The user must pass a UniquePtr temporary into the method.  mozilla::Forward ensures that temporariness continues into the initialization of a UniquePtr<int> member.  But then when it comes time to pass it to the method, just passing by value would try to copy-construct a UniquePtr: not permitted.  Ergo, pass mozilla::Move(m0) and so on instead, unless I'm still missing something (which is possible, but I don't think so just right now).

::: xpcom/glue/tests/gtest/TestThreadUtils.cpp
@@ +47,5 @@
> +  void foo0() { ++mCount0; }
> +  void foo1(int a0) { ++mCount1; mA0 = a0; }
> +  void foo2(int a0, int a1) { ++mCount2; mA0 = a0; mA1 = a1; }
> +  void foo3(int a0, int a1, int a2) { ++mCount3; mA0 = a0; mA1 = a1; mA2 = a2; }
> +  void foo4(int a0, int a1, int a2, int a3)

Convert argument a1 to a UniquePtr<int>, then pass in MakeUnique<int>(5) for the various corresponding arguments.

We probably should have more different things for these -- lvalue references, rvalue references, etc. -- but I'm somewhat loathe to spend much more time suggesting more things, when people should see mistakes as compile errors when they actually try to do stuff wrong.

Also: for one of these arguments, pass in a short instead of an int, to be sure that forwarding works even when the provided argument doesn't exactly match the type of the stored argument.  Or make it an int*, then pass in the address of an int for some, nullptr for others.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1131445-NewRunnableMethodVariadic.patch

Thanks for the previous r+, Jeff.
Removed the {} after nsRunnableMethodArguments as advised.
Now, your comments about Move and UniquePtr led me down the rabbit hole.

Trying to summarize my thoughts:
Moving arguments would indeed be useful to allow UniquePtr and friends.
The main risk is that it would prevent multiple calls to the same Runnable; However from a quick survey it seems Runnables are only ever run once, but I would prefer to conduct a thorough study before changing this behavior.

Note that the original NS_NewRunnableMethodWithArg was making a copy-construction and then passing the argument by value, so all the current code was made to work with that (some are using nsAutoPtr to forward the ownership towards the method call).

So I would like to start with a safer change, that would still allow move-only arguments (if explicitly requested) as well as references and pointers (raw or nsRefPtr), with options to later improve the default behavior if possible.

This new patch introduces argument-type decorators that inform NS_NewRunnableMethodWithArgs how to store and then pass arguments.
E.g.: NS_NewRunnableMethodWithArgs<StoreCopyPassByRRef<UniquePtr<int>>>(ptr, &C::Foo, MakeUnique<int>(5))
Default behavior for types without decorators is what seemed most appropriate to me, see doc above nsParameterStorage for details.
Special cases:
- 'T&&' is a shortcut for 'StoreCopyPassByRRef<T>' for easier access, so the above example just becomes:
NS_NewRunnableMethodWithArgs<UniquePtr<int>&&>(ptr, &C::Foo, MakeUnique<int>(5))
- 'T*', where T derives from nsISupports, is a shortcut for 'StorensRefPtrPassByPtr<T>', which stores the pointer in an nsRefPtr (thereby keeping the pointee alive while the Runnable exists), and then passes the pointer to the method.

Finally I've added lots of tests to try all these options.

If the above is acceptable, I can think of the following future tasks:
- Survey all uses of NS_NewRunnableMethodWithArg, to see if changing the default behavior from StoreCopyPassByValue to StoreCopyPassByRRef is possible; and/or modify these uses to specify more context-appropriate storage&passing strategies.
- Add NS_NewRunnableLambda, to store a lambda in a Runnable; It might bring some advantages compared to being forced to call a method? Especially in anticipation of generalized lambdas, which will allow efficient move-constructions of captured variables.

Thoughts?

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8566946 [details] [diff] [review]
1131445-NewRunnableMethodVariadic.patch

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

In the slightly longer run, I don't think there's much, if any, value in allowing this to support multiple calls.  The usual use case for a runnable is to dispatch it and run it once.  Not multiple times.

In the short run, this explicitness may be kinda nice, tho.  So okay.  But that thirty-line template conditional has to go.  See my comments for how I'd try to break this down to be more readable.

::: xpcom/glue/nsThreadUtils.h
@@ +272,4 @@
>  struct nsRunnableMethodReceiver
>  {
>    nsRefPtr<ClassType> mObj;
> +  nsRunnableMethodReceiver(ClassType* aObj) : mObj(aObj) {}

Incidentally, seems likely this wants to be explicit.

@@ +320,5 @@
> +// see nsParameterStorage).
> +// When creating a new storage class, add a specialization for it to be
> +// recognized.
> +template<typename T>
> +struct nsIsParameterStorageClass : public mozilla::FalseType {};

Leave off the ns -- just visual noise.

@@ +496,5 @@
> +// -              StoreCopyPassByPtr<T>          : Store T, pass T* (of copy!)
> +// Or create your own class with PassAsParameter() method, optional
> +// clean-up in destructor, and with associated nsIsParameterStorageClass<>.
> +template<typename T>
> +struct nsParameterStorage

Again, leave off the ns prefix.

@@ +499,5 @@
> +template<typename T>
> +struct nsParameterStorage
> +{
> +  template<typename P> struct RemovePointer { typedef P Type; };
> +  template<typename P> struct RemovePointer<P*> { typedef P Type; };

Don't add this.  Instead add mozilla::RemovePointer to TypeTraits.h, corresponding to std::remove_pointer, and use that.

@@ +501,5 @@
> +{
> +  template<typename P> struct RemovePointer { typedef P Type; };
> +  template<typename P> struct RemovePointer<P*> { typedef P Type; };
> +
> +  typedef typename mozilla::Conditional<

Egad.  This is impossible.  I'm not even going to read this.

I think you should structure this as a much longer series of template classes built from the ground up, layer by layer.  Something like:

template <typename T, typename TWithoutPointer>
struct NonSupportsPointer
  : Conditional<IsConst<TWithoutPointer>::value,
                StoreConstPtrPassByConstPtr<typename RemoveConst<TWithoutPointer>::Type>,
                StorePtrPassByPtr<TWithoutPointer>>
{};

template <typename T, typename TWithoutPointer>
struct PointerStorageType
  : Conditional<IsBaseOf<nsISupports, TWithoutPointer>::value,
                StorensRefPtrPassByPtr<TWithoutPointer>,
                NonSupportsPointer<T, TWithoutPointer>::Type>
{};

template <typename T>
struct NonParameterStorageClass
  : Conditional<IsPointer<T>::value,
                typename PointerStorageType<T, typename RemovePointer<T>::Type>::Type,
                typename NonPointerStroageType<T>::Type>
{};

template <typename T>
struct ParameterStorage
  : Conditional<IsParameterStorageClass<T>::value,
                T,
                typename NonParameterStorageClass<T>::Type>
{};

...and so on.  Put it all inside namespace detail so it's clear it's not something to be used directly.

In any event, thirty straight lines of nesting is just not approachable.  The above, fully elaborated, has a fighting chance of being readable.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1131445-NewRunnableMethodVariadic.patch

Sorry for making your eyes bleed!
All comments implemented in this new patch.

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1131445-NewRunnableMethodVariadic.patch

Apologies, previous patch didn't build with gcc. Also was missing 'explicit' in test classes.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8568998 [details] [diff] [review]
1131445-NewRunnableMethodVariadic.patch

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

My eyes are glazing over a bit looking at the test, and I'm not sure I have time to give it a 100% thorough examination.  The header code looks fine, and you at least *seem* to be exercising all the esoteric cases, so I'm going to punt a little bit on spending too much time on it.  rs=me for the test, r=me for the rest.  Hopefully I won't have cause to regret this in the future.  :-)

::: mfbt/TypeTraits.h
@@ +959,5 @@
> +
> +template<typename P>
> +struct RemovePointer<P*>
> +{
> +  typedef P Type;

This header implements the C++11/14 <type_traits> API, and in that API, the requirements for IsPointer<T> are:

"""
If T has type “(possibly cv-qualified) pointer to T1” then the member typedef type shall name T1; otherwise, it shall name T.
"""

So you want the trait as you've implemented it, in namespace detail, and you want the actually-exposed one to inherit detail::IsPointer<typename RemoveCV<T>::Type>.  (See the majority of the other traits in TypeTraits.h for examples.)

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Well-spotted. I'll fix RemovePointer to work correctly with CV-qualified pointers.


Regarding IsPointer:
Reading the standard, 20.9.4.1-2 (related to is_pointer and other primary type categories):

"""For any given type T, the result of applying one of these templates to T and to cv-qualified T shall yield the same result."""

The current implementation of mozilla:IsPointer doesn't match std::is_pointer. (At a quick glance, the other IsX templates look correct in this respect.)

Should I fix IsPointer right here, or create a separate bug?

Comment 13

[Jeff Walden [:Waldo]]

New bug works fine.  If we overlapped on IRC I'd also be totally fine reviewing a pastebin, too.  :-)

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1131445-NewRunnableMethodVariadic.patch

Depends on bug 1137583, so I didn't add RemoveCV when using IsPointer.
But I've added tests to ensure that CV-qualified pointers were recognized.

Also found an issue where the non-thread-safe nsRefPtr receiver object was copied, causing problems when that copy happened on the wrong thread.
Fixed by extracting the raw pointer when calling the method.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8570842 [details] [diff] [review]
1131445-NewRunnableMethodVariadic.patch

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

::: mfbt/TypeTraits.h
@@ +958,5 @@
>  
>  /* 20.9.7.5 Pointer modifications [meta.trans.ptr] */
>  
> +/**
> + * Converts pointer types to the underlying types.

This works, but it's better to be consistent with how the entire file does it, with a helper and using RemoveCV.  You dont' need IsPointer to be present/defined in order to do that -- and, indeed, I'm not entirely sure off the top of my head how I'd naturally write such a template that actually required IsPointer in its definition.

Rather, you should be able to do this instead:

namespace detail {

template<typename T, typename CVRemoved>
struct RemovePointerHelper
{
  typedef T Type;
};

template<typename T, typename Pointee>
struct RemovePointerHelper<T, Pointee*>
{
  typedef Pointee Type;
};

} // namespac detail

/**
 * ...the doc comment...
 */
template<typename T>
struct RemovePointer
  : detail::RemovePointerHelper<T, typename RemoveCV<T>::Type>
{};

@@ +961,5 @@
> +/**
> + * Converts pointer types to the underlying types.
> + *
> + * mozilla::RemovePointer<T>::Type is T;
> + * mozilla::RemovePointer<T*>::Type is T;

As far as the comment goes.  It's probably worth being clear that RemovePointer acts upon pointers only -- not upon pointer to member.  So I'd do it like so:

/**
 * Produces the pointed-to type if a pointer is provided, else returns the input
 * type.  Note that this does not dereference pointer-to-member pointers.
 *
 * struct S { bool m; void f(); };
 * mozilla::RemovePointer<int>::Type is int;
 * mozilla::RemovePointer<int*>::Type is int;
 * mozilla::RemovePointer<const long*>::Type is long;
 * mozilla::RemovePointer<void* const>::Type is void;
 * mozilla::RemovePointer<void (S::*)()>::Type is void (S::*)();
 * mozilla::RemovePointer<void (*)()>::Type is void();
 * mozilla::RemovePointer<bool S::*>::Type is bool S::*.
 */

And it occurs to me.  You haven't added any tests to mfbt/tests/TestTypeTraits.cpp.  Please add these all as tests in the place in that file that corresponds to the ordering of traits in TypeTraits.h -- so just after the RemoveExtent tests.  And if there are any other tests along these lines you think are worth adding there, please add them as well.

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1131445-NewRunnableMethodVariadic.patch

Changes as per comment 15: Better doc, added tests.

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee9faccedd5

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/974ca22a87c6

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/974ca22a87c6