1221371 - Switch chromium IPC code to use mozilla::Tuple

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use mozilla::Tuple

Right now our ipc/chromium import includes its own tuple implementation that doesn't move references. This means that we can't use move arguments when calling NewRunnableFunction/Method, which is causing me trouble.

So I switched us over to using the mozilla::Tuple code, which is very similar to std::tuple.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: gmp changes

I had to make a few changes where we use NewRunnableFunction/Method. Here are the GMP changes.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: plugin changes

This change is needed because lvalue arguments are inferred to have type T& to support perfect forwarding. That type gets stripped off when the data is stored in tuples. But if we specify the type, we need to keep it in mind. (I'm not sure why the type has been written out here. Maybe readability.)

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: gfx changes

There's a few things going on here. The background is that we're now moving parameters into functions that are invoked through NewRunnableFunction rather than copying them.

In the cases where I converted ImageContainer* to RefPtr<ImageContainer>&&, the problem is that the RefPtr<ImageContainer> that's stored in the runnable is now going to die right after we pass it to the function. If the ImageContainer* had a refcount of 1, it would be freed before the function got its hands on it. Luckily RefPtr's |operator T*() const &&| is deleted so this can't happen. But we need to take the argument as a RefPtr in order to keep a ref to it.

In the places where I had to explicitly construct an nsTArray, the copy constructor is explicit. I'm not sure why this was working before.

Finally, in the place where I eliminated the mozilla::Move, I don't think it was ever valid. You can't use Move on a const reference, which is what aTargets is.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove chromium callback stuff

This patch removes some chromium code that we never use.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove tuple.h

Still waiting on a try push here, but I think we can completely remove chromium's tuple.h.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8682855 [details] [diff] [review]
use mozilla::Tuple

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

::: ipc/chromium/src/base/task.h
@@ +19,5 @@
> +
> +// Represents a sequence of integers. The sequence is stored in the identity of
> +// the type: Sequence<0, 1, 2> represents the sequence 0, 1, 2.
> +template<int...>
> +struct Sequence {};

Can you use mfbt/IndexSequence.h for this?

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8682855 [details] [diff] [review]
use mozilla::Tuple

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

::: ipc/chromium/src/base/task.h
@@ +52,5 @@
> +
> +// Call a method on the given object. Arguments are passed by move semantics
> +// from the given tuple.
> +template<class ObjT, class Method, typename... Args>
> +void DispatchTupleToMethod(ObjT* obj, Method method, mozilla::Tuple<Args...> arg)

Why move the tuple into this function (and again at the CallMethod level)? Why not take it by reference (the referred-to object living inside RunnableMethod)?

Comment 8

[Botond Ballo [:botond]]

(In reply to Bill McCloskey (:billm) from comment #3)
> In the cases where I converted ImageContainer* to RefPtr<ImageContainer>&&,
> the problem is that the RefPtr<ImageContainer> that's stored in the runnable
> is now going to die right after we pass it to the function. If the
> ImageContainer* had a refcount of 1, it would be freed before the function
> got its hands on it.

I don't understand why this is the case. The RefPtr object lives inside the tuple which is a parameter of the calling function (e.g. details::CallMethod). Why would its destructor be called before the called function executes?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Botond Ballo [:botond] from comment #7)
> > +// Call a method on the given object. Arguments are passed by move semantics
> > +// from the given tuple.
> > +template<class ObjT, class Method, typename... Args>
> > +void DispatchTupleToMethod(ObjT* obj, Method method, mozilla::Tuple<Args...> arg)
> 
> Why move the tuple into this function (and again at the CallMethod level)?
> Why not take it by reference (the referred-to object living inside
> RunnableMethod)?

Yes, I guess this would work too. I was assuming move constructors are cheap, but if they're not, your way is probably better.

> I don't understand why this is the case. The RefPtr object lives inside the tuple which is
> a parameter of the calling function (e.g. details::CallMethod). Why would its destructor be
> called before the called function executes?

You're probably right that we won't actually call the RefPtr's destructor. What actually happens is that we get a compile error because the conversion operator to T* is deleted:
http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#263

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8682869 [details] [diff] [review]
remove tuple.h

I guess this is used by base/timer.h.

Comment 11

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

(In reply to Bill McCloskey (:billm) from comment #3)
> In the places where I had to explicitly construct an nsTArray, the copy
> constructor is explicit. I'm not sure why this was working before.

It was being passed by reference before, right? So the copy only happened into the tuple and that used the explicit copy constructor.

Comment 12

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

Comment on attachment 8682867 [details] [diff] [review]
gfx changes

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

Partly a rubber-stamp :)

Comment 13

[Botond Ballo [:botond]]

(In reply to Bill McCloskey (:billm) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #7)
> > > +// Call a method on the given object. Arguments are passed by move semantics
> > > +// from the given tuple.
> > > +template<class ObjT, class Method, typename... Args>
> > > +void DispatchTupleToMethod(ObjT* obj, Method method, mozilla::Tuple<Args...> arg)
> > 
> > Why move the tuple into this function (and again at the CallMethod level)?
> > Why not take it by reference (the referred-to object living inside
> > RunnableMethod)?
> 
> Yes, I guess this would work too. I was assuming move constructors are
> cheap, but if they're not, your way is probably better.

Move constructors are usually cheap, but not always; if you have a structure with 20 int fields, its move constructor still has to copy the 20 int fields. In generic code, I think it's worth avoiding unnecessary moves.

> > I don't understand why this is the case. The RefPtr object lives inside the tuple which is
> > a parameter of the calling function (e.g. details::CallMethod). Why would its destructor be
> > called before the called function executes?
> 
> You're probably right that we won't actually call the RefPtr's destructor.
> What actually happens is that we get a compile error because the conversion
> operator to T* is deleted:
> http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#263

Indeed.

Is there a downside to having the callback function take the RefPtr by value in such cases? The move constructor of RefPtr doesn't perform a refcount increment/decrement, so there shouldn't be a performance penalty.

Comment 14

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Bill McCloskey (:billm) from comment #3)
> > In the places where I had to explicitly construct an nsTArray, the copy
> > constructor is explicit. I'm not sure why this was working before.
> 
> It was being passed by reference before, right? So the copy only happened
> into the tuple and that used the explicit copy constructor.

Actually, with the new code it's still being passed by reference, and the copy still only happens when initializing the tuple.

I found it unexpected that explicitly constructing an nsTArray is required, so I applied the patch locally and tried removing the explicit constructions to see what happens. I got a compiler error, but it didn't indicate a violation of the explicitness; it indicated a bug in the implementation of mozilla::IsConvertible (which is used by mozilla::Tuple). I'll file and fix that, after which the explicit constructions can be removed.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> I found it unexpected that explicitly constructing an nsTArray is required,
> so I applied the patch locally and tried removing the explicit constructions
> to see what happens. I got a compiler error, but it didn't indicate a
> violation of the explicitness; it indicated a bug in the implementation of
> mozilla::IsConvertible (which is used by mozilla::Tuple). I'll file and fix
> that, after which the explicit constructions can be removed.

Filed bug 1221680.

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> I found it unexpected that explicitly constructing an nsTArray is required,
> so I applied the patch locally and tried removing the explicit constructions
> to see what happens. I got a compiler error, but it didn't indicate a
> violation of the explicitness; it indicated a bug in the implementation of
> mozilla::IsConvertible (which is used by mozilla::Tuple). I'll file and fix
> that, after which the explicit constructions can be removed.

I confirmed that with the patch in bug 1221680, the code compiles without adding the explicit nsTArray(...) constructions.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8682855 [details] [diff] [review]
use mozilla::Tuple

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

I agree that this should use IndexSequence if possible.  Looks good otherwise.

Comment 18

[Botond Ballo [:botond]]

Let's not block this on bug 1221680, as I'm having trouble getting that to pass the hazard builds. We can remove the explicit nsTArray(...) constructions as a follow-up after that lands.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use mozilla::Tuple v2

Here's an updated patch in case anyone is interested in commenting.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd99e5060e1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a50c676caf7f

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f620224aae8f

B2G JB Emulators both opt and debug:
19:04:43    ERROR -  ../../../../workspace/gecko/ipc/chromium/src/base/task.h:36:4: error: use of deleted function 'RefPtr<T>::operator T*() const && [with T = nsScreenGonk]'

Failed job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a50c676caf7f
Backout job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f620224aae8f

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9567dbd464b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7686821fd178

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #21)
> B2G JB Emulators both opt and debug:
> 19:04:43    ERROR - 
> ../../../../workspace/gecko/ipc/chromium/src/base/task.h:36:4: error: use of
> deleted function 'RefPtr<T>::operator T*() const && [with T = nsScreenGonk]'

Treeherder is having problems, but that looks like 

widget/gonk/nsScreenManagerGonk.cpp:471:        NewRunnableFunction(&UpdateMirroringWidgetSync,
widget/gonk/nsScreenManagerGonk.cpp-472-                            primaryScreen,
widget/gonk/nsScreenManagerGonk.cpp-473-                            window.forget().take()));
--
widget/gonk/nsScreenManagerGonk.cpp:495:        NewRunnableFunction(&UpdateMirroringWidgetSync,
widget/gonk/nsScreenManagerGonk.cpp-496-                            primaryScreen,
widget/gonk/nsScreenManagerGonk.cpp-497-                            nullptr));

Same problem as the others.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The new landing fixed that problem and it seems to be building fine on JB.

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/9567dbd464b8
https://hg.mozilla.org/mozilla-central/rev/7686821fd178

Comment 26

[Botond Ballo [:botond]]

Attachment: Follow-up to remove unnecessary explicit nsTArray constructor calls

Now that bug 1221680 has landed, I'd like to land this follow-up to remove the explicit nsTArray constructor calls that are now unnecessary.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8689142 [details] [diff] [review]
Follow-up to remove unnecessary explicit nsTArray constructor calls

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

Thanks for all the work on this!

Comment 28

[Pulsebot]

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

Comment 29

[Carsten Book [:Tomcat]]

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