Closed Bug 1163320 Opened 9 years ago Closed 8 years ago

Variadic implementation of nsRunnableMethodArguments

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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
Depends on: 1163328
Depends on: 1163329
(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.
Depends on: 1166583
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
Assignee: nobody → botond
Attachment #8607898 - Flags: review?(nfroyd)
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?
Attachment #8607898 - Flags: review?(nfroyd) → review+
ni? for comment 4.
Flags: needinfo?(botond)
Rebased to apply to trunk. Carrying r+.
Attachment #8607898 - Attachment is obsolete: true
Flags: needinfo?(botond)
Attachment #8609158 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c1cd7aff7c94
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1171059
Backing this out to get Hazard builds passing again per bug 1171059.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla41 → ---
> 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.
Depends on: 1162263
Meanwhile this patch has bitrotted, because it relied on the StoreXPassByY classes having implicit constructors, but those constructors were made explicit in bug 1188203.
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
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.
(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.
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.
Attachment #8609158 - Attachment is obsolete: true
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.
Attachment #8737445 - Flags: review?(nfroyd) → review+
Thank you for following up on this!
See Also: → 1263654
(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.
https://hg.mozilla.org/mozilla-central/rev/3a2a9b46026f
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: