1163320 - Variadic implementation of nsRunnableMethodArguments

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Botond Ballo [:botond]]

nsRunnableMethodArguments [1] is a helper class for storing arguments by NS_NewRunnableMethod and ilk.

It is currently implemented by specializing the class for every number of arguments from 0 to 8, and implementing each one separately. This is a lot of boilerplate, and new specializations have to be added if we want more arguments than the limit (for example, recently bug 1146349 had to increase the maximum number of arguments from 4 to 8).

This implementation technique was the best we could do before variadic templates, but now that we have variadic templates, it's possible to give this class a single implementation that works for any number of arguments.

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

Comment 1

[Botond Ballo [:botond]]

To implement this, we need two pieces of supporting infrastructure:

  - A structure that is able to store a variadic number of data members.

  - A utility for expanding the data members stored in said class into
    a variadic argument list.

The C++11 standard library provides both of these, in the form of std::tuple and std::index_sequence, respectively (see [1] for an example of how the latter works). However, as we can't use C++11 standard library features yet, we would have to provide our own.

[1] http://en.cppreference.com/w/cpp/utility/integer_sequence

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [at c++ standards meeting May 4-9] from comment #1)
> To implement this, we need two pieces of supporting infrastructure:
> 
>   - A structure that is able to store a variadic number of data members.

Filed bug 1163328 for this.

>   - A utility for expanding the data members stored in said class into
>     a variadic argument list.

Filed bug 1163329 for this.

Comment 3

[Botond Ballo [:botond]]

Attachment: Variadic implementation of nsRunnableMethodArguments

I have this working now, on top of the patches in the dependent bugs.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ab9f61a833

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8607898 [details] [diff] [review]
Variadic implementation of nsRunnableMethodArguments

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

I guess we need to apply the "cleanup patches" in bug 1162029 before applying this patch?  Do you want to take care of committing those, do you want me to commit those, or do you want to rebase this patch on top of trunk?

Comment 5

[Nathan Froyd [:froydnj]]

ni? for comment 4.

Comment 6

[Botond Ballo [:botond]]

Attachment: Variadic implementation of nsRunnableMethodArguments

Rebased to apply to trunk. Carrying r+.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1cd7aff7c94

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1cd7aff7c94

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backing this out to get Hazard builds passing again per bug 1171059.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/470bb48707f8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Merge of the backout: https://hg.mozilla.org/mozilla-central/rev/470bb48707f8

Comment 12

[Botond Ballo [:botond]]

> Backing this out to get Hazard builds passing again per bug 1171059.

The cause of the hazard build failures is some sort of bad interaction between this code and the GCC plugin that does the hazard checks. :sfink said the issue is likely to go away when upgrading the GCC version used for hazard builds from 4.7 to 4.9, which bug 1162263 aims to do, so I'll wait on that bug to re-land this.

Comment 13

[Botond Ballo [:botond]]

Meanwhile this patch has bitrotted, because it relied on the StoreXPassByY classes having implicit constructors, but those constructors were made explicit in bug 1188203.

Comment 14

[Botond Ballo [:botond]]

I unbitrotted this. I'm not sure whether the hazard build failures that originally kept this from landing are still around / relevant. Let's see:

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

Comment 15

[Steve Fink [:sfink] [:s:]]

Oh! Sorry, I should have remembered this bug. At long last, I recently managed to land the gcc upgrades (and jettisoned some b2g coverage that made it hard to upgrade in the first place), so the original problem at least should be gone. And if any new problem shows up, it should be straightforward to fix at this point.

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> I unbitrotted this. I'm not sure whether the hazard build failures that
> originally kept this from landing are still around / relevant. Let's see:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c11202e52b

It's looking good. In particular, I don't see the "silent errors" from bug 1171059 in the hazard build's raw log.

Comment 17

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1163320 - Variadic implementation of nsRunnableMethodArguments. r=froydnj

Review commit: https://reviewboard.mozilla.org/r/43987/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43987/

Comment 18

[Botond Ballo [:botond]]

Note that this has been reviewed before, but I'm requesting review again because it's been a while, and because I had to change the StoreXPassByY constructors back to implicit.

My justification for the latter change is that the patch that made them explicit (bug 1188203) appears to have done so "because we can", that is, because no existing call site needed it to be implicit. Now that they're inside a tuple, though, they need to be implicit so that the tuple can be constructed from stored types.

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8737445 [details]
MozReview Request: Bug 1163320 - Variadic implementation of nsRunnableMethodArguments. r=froydnj

https://reviewboard.mozilla.org/r/43987/#review40717

::: xpcom/glue/nsThreadUtils.h:374
(Diff revision 1)
>  {
>    typedef T stored_type;
>    typedef T passed_type;
>    stored_type m;
>    template <typename A>
> -  explicit StoreCopyPassByValue(A&& a) : m(mozilla::Forward<A>(a)) {}
> +  MOZ_IMPLICIT StoreCopyPassByValue(A&& a) : m(mozilla::Forward<A>(a)) {}

Is this something we can fix in `Tuple` itself by sprinkling some constructors about?  It seems unfortunate that using things in `Tuple` would require using implicit constructors.

I don't think changing `Tuple` needs to block this, but a followup bug would be welcome.

Comment 20

[Nathan Froyd [:froydnj]]

Thank you for following up on this!

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2a9b46026f

Comment 22

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> ::: xpcom/glue/nsThreadUtils.h:374
> (Diff revision 1)
> >  {
> >    typedef T stored_type;
> >    typedef T passed_type;
> >    stored_type m;
> >    template <typename A>
> > -  explicit StoreCopyPassByValue(A&& a) : m(mozilla::Forward<A>(a)) {}
> > +  MOZ_IMPLICIT StoreCopyPassByValue(A&& a) : m(mozilla::Forward<A>(a)) {}
> 
> Is this something we can fix in `Tuple` itself by sprinkling some
> constructors about?

I believe so! Filed bug 1263645 for this.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/3a2a9b46026f