Switch chromium IPC code to use mozilla::Tuple

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Created attachment 8682855 [details] [diff] [review]
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.
Attachment #8682855 - Flags: review?(jld)
Created attachment 8682857 [details] [diff] [review]
gmp changes

I had to make a few changes where we use NewRunnableFunction/Method. Here are the GMP changes.
Attachment #8682857 - Flags: review?(cpearce)
Created attachment 8682860 [details] [diff] [review]
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.)
Attachment #8682860 - Flags: review?(jld)
Created attachment 8682867 [details] [diff] [review]
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.
Attachment #8682867 - Flags: review?(bugmail.mozilla)
Created attachment 8682868 [details] [diff] [review]
remove chromium callback stuff

This patch removes some chromium code that we never use.
Attachment #8682868 - Flags: review?(jld)
Created attachment 8682869 [details] [diff] [review]
remove tuple.h

Still waiting on a try push here, but I think we can completely remove chromium's tuple.h.
Attachment #8682869 - Flags: review?(jld)
Attachment #8682868 - Attachment description: patch → remove chromium callback stuff
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 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)?
(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?
Attachment #8682857 - Flags: review?(cpearce) → review+
(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 on attachment 8682869 [details] [diff] [review]
remove tuple.h

I guess this is used by base/timer.h.
Attachment #8682869 - Attachment is obsolete: true
Attachment #8682869 - Flags: review?(jld)
(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 on attachment 8682867 [details] [diff] [review]
gfx changes

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

Partly a rubber-stamp :)
Attachment #8682867 - Flags: review?(bugmail.mozilla) → review+

Updated

3 years ago
Depends on: 1221368
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.

Updated

3 years ago
Depends on: 1221680
(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 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.
Attachment #8682855 - Flags: review?(jld) → review+
Attachment #8682868 - Flags: review?(jld) → review+
Attachment #8682860 - Flags: review?(jld) → review+
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.
No longer depends on: 1221680
Created attachment 8683403 [details] [diff] [review]
use mozilla::Tuple v2

Here's an updated patch in case anyone is interested in commenting.
Attachment #8682855 - Attachment is obsolete: true
Attachment #8683403 - Flags: review+
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
(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.
The new landing fixed that problem and it seems to be building fine on JB.

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9567dbd464b8
https://hg.mozilla.org/mozilla-central/rev/7686821fd178
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Created attachment 8689142 [details] [diff] [review]
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.
Attachment #8689142 - Flags: review?(wmccloskey)
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!
Attachment #8689142 - Flags: review?(wmccloskey) → review+

Updated

3 years ago
Depends on: 1229262
You need to log in before you can comment on or make changes to this bug.