1152753 - Add support for posting C++11 lambdas to messageloop/threads

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Language features make things easier. I want this for bug 1146349 (specifically the Task version, so I can post lambdas to a MessageLoop). I'm adding a nsThreadUtils version too even though I'm not using that myself (I have to use the MessageLoop - see bug 1146349 comment 11).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Oh, and if you want to see it in action, search for "NewRunnableLambda" in https://bug1146349.bugzilla.mozilla.org/attachment.cgi?id=8587309

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8590229 [details] [diff] [review]
Patch

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

Just granting f+ here, but assuming we can call this with function pointers, then r+ with the renaming below.

Everything for the nsRunnable version applies to the task version, I think.

::: xpcom/glue/nsThreadUtils.h
@@ +237,5 @@
>  protected:
>    virtual ~nsCancelableRunnable() {}
>  };
>  
> +// An event that can be used to call a C++11 lambda/closure. The lambda

I'm not up to speed on all the nifty use of lambda, so apologies for the dumb question here, but are lambdas and pointers-to-functions equivalent here?  So we could call:

NS_NewRunnableLambda(&MyStaticFunction);

?  Might not be as convenient, because you want to capture variables and the like, I guess.

If you can call this with static functions, then I think we ought to call it NS_NewRunnableFunction and let the "lambda is-a function" equivalence let people know they can call it with lambdas.  If not...is there a way to make it so lambdas and functions work equally well with this?

@@ +238,5 @@
>    virtual ~nsCancelableRunnable() {}
>  };
>  
> +// An event that can be used to call a C++11 lambda/closure. The lambda
> +// must have no required arguments.

This requirement will be compiler-checked by the call to mLambda...can we also do something like:

static_assert(mozilla::IsSame<void, decltype(Lambda)>::value, "lambda must return void!");

?  (Or whatever the magic is to extract the return type, if not decltype.)

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> I'm not up to speed on all the nifty use of lambda, so apologies for the
> dumb question here, but are lambdas and pointers-to-functions equivalent
> here?  So we could call:
> 
> NS_NewRunnableLambda(&MyStaticFunction);
> 
> ?  Might not be as convenient, because you want to capture variables and the
> like, I guess.

No, I don't think lambdas and pointers-to-function are equivalent, precisely because of the variable-capturing features. So I think we will need to keep separate versions for lambdas and function pointers. However I'll defer to Botond on this.

> static_assert(mozilla::IsSame<void, decltype(Lambda)>::value, "lambda must
> return void!");
> 
> ?  (Or whatever the magic is to extract the return type, if not decltype.)

Hm, good point. I'll try to throw this in as well.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Now with 200% more decltype. Compilation verified locally on linux+gcc and mac+clang.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8590335 [details] [diff] [review]
Patch v2

Ugh, can we please not add anything to make chromium tasks easier to use? We don't test that code very well and uses of Task tend to cause bugs. Gecko code should stick to runnables whenever possible.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

In this case I have no choice (see comment 0 or bug 1146349 comment 11). :smaug also doesn't like having to use MessageLoop/Task and is trying to come up with something that doesn't require this.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Well, but maybe just add your template magic to the file you're needing this in? Don't add something to task.h please!

Comment 8

[Kartikaya Gupta (email:[e-mail])]

^ If I do this I'd put the stuff from task.h in nsDOMWindowUtils.cpp (for bug 1146349), or maybe some smaller header that I include into nsDOMWindowUtils.cpp.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8590335 [details] [diff] [review]
Patch v2

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> > I'm not up to speed on all the nifty use of lambda, so apologies for the
> > dumb question here, but are lambdas and pointers-to-functions equivalent
> > here?  So we could call:
> > 
> > NS_NewRunnableLambda(&MyStaticFunction);
> > 
> > ?  Might not be as convenient, because you want to capture variables and the
> > like, I guess.
> 
> No, I don't think lambdas and pointers-to-function are equivalent, precisely
> because of the variable-capturing features. So I think we will need to keep
> separate versions for lambdas and function pointers. However I'll defer to
> Botond on this.

This same code should indeed be usable with:

  - lambdas
  - function objects
  - pointers to non-member functions
  - pointers to static member functions

(In the last two cases, the template parameter currently named 'Lambda' would be a function pointer type. In the first two cases, it would be the function object type. Note that lambdas are just function objects that the compiler generates for you, with a field for each captured variable.)

So, I'd be in favour of renaming "lambda" to a more generic name like "function". Otherwise, looks great!

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

The main question I have is that why would we want to start using MessageLoop more vs. using xpcom event loop?

Bent might have an opinion on that.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Bent might have an opinion on that.

We should avoid MesssageLoop/Task as much as possible. The only reason we should use either is when we're running in a process that doesn't load XPCOM (I think GMP and plugins are the only cases?) or if we're in some library that is trying to be buildable without XPCOM (iirc GFX tried to do this at one point).

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8590335 [details] [diff] [review]
Patch v2

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

Thanks for the explanation, Botond!

r=me with the s/Lambda/Function/g name change, minor nits, and putting the task.h bits somewhere less convenient for everybody else's purposes.

::: xpcom/glue/nsThreadUtils.h
@@ +237,5 @@
>  protected:
>    virtual ~nsCancelableRunnable() {}
>  };
>  
> +// An event that can be used to call a C++11 lambda/closure. The lambda

We should #include "mozilla/TypeTraits.h" up above this for...

@@ +248,5 @@
> +    : mLambda(aLambda)
> +  { }
> +
> +  NS_IMETHOD Run() {
> +    static_assert(mozilla::IsSame<void, decltype(mLambda())>::value,

mozilla::IsVoid<decltype(mLambda())>::value is shorter and neater, please use that instead.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #9)
>
> This same code should indeed be usable with:
> 
>   - lambdas
>   - function objects
>   - pointers to non-member functions
>   - pointers to static member functions

Ah, that makes sense. I was thinking of somehow "coercing" a function pointer into a lambda or vice-versa, and failed to realize that it's the template class that will just handle all of these cases.

I'll fix up the patch as per comments above. I'm also checking again to see if I can get rid of the MessageLoop version and just use the XPCOM version, which would make everybody happy. I think it might be doable with a bit of work and disabling one test on OS X, which isn't too bad.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Ok, turns out I don't need the MessageLoop version at all, the robustification of tests that I did in bug 1150452 means that I can use the XPCOM version and it works fine. Green try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6cf1852fff

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35cae02f1552

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7de210d3a493

for introducing a footgun with respect to refcounted pointers. See IRC discussion starting at around [1] (the actual footgun appears at 15:17 in the log, with more discussion after that). See also bug 1146349 comment 30 and onwards. We might be able to remove the footgun by special-casing things that extend nsISupports, or something. Either way, backed out until we figure it out.

[1] http://logs.glob.uno/?c=mozilla%23developers&s=10%20Apr%202015&e=10%20Apr%202015#c1199276

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Backed out:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7de210d3a493
> 
> for introducing a footgun with respect to refcounted pointers. See IRC
> discussion starting at around [1] (the actual footgun appears at 15:17 in
> the log, with more discussion after that). See also bug 1146349 comment 30
> and onwards. We might be able to remove the footgun by special-casing things
> that extend nsISupports, or something. Either way, backed out until we
> figure it out.
> 
> [1]
> http://logs.glob.uno/
> ?c=mozilla%23developers&s=10%20Apr%202015&e=10%20Apr%202015#c1199276

My reading of this discussion is that the takeaway is "don't schedule lambdas that capture refcounted pointers" (at least not until we can enforce that the lambdas capture them by nsRefPtr rather than by raw pointer), not "don't schedule lambdas at all". Wouldn't this utility still be useful for scheduling lambdas that don't capture refcounted pointers?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #16)
> My reading of this discussion is that the takeaway is "don't schedule
> lambdas that capture refcounted pointers" (at least not until we can enforce
> that the lambdas capture them by nsRefPtr rather than by raw pointer), not
> "don't schedule lambdas at all". Wouldn't this utility still be useful for
> scheduling lambdas that don't capture refcounted pointers?

It certainly would be useful.  The question is whether you can count on users of the function to write code that uses lambdas in the correct fashion.  Opinions differ on the reliability of users (and/or code reviewers) in this case. :)

Comment 18

[Botond Ballo [:botond]]

Fair enough - I guess if a static analysis to catch capturing refcounting pointers as raw pointers is forthcoming in bug 1153304, it makes sense to wait for that and then add this utility back.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Now that bug 1153304 has landed, I should be ok to reland this, right?

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v3

This is the version that I actually landed last time, carrying r+

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Went ahead and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/453564fa5606

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/453564fa5606