Closed
Bug 1221371
Opened 8 years ago
Closed 8 years ago
Switch chromium IPC code to use mozilla::Tuple
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(6 files, 2 obsolete files)
1.81 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
6.06 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
7.51 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
16.33 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
3.46 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•8 years ago
|
||
I had to make a few changes where we use NewRunnableFunction/Method. Here are the GMP changes.
Attachment #8682857 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
This patch removes some chromium code that we never use.
Attachment #8682868 -
Flags: review?(jld)
Assignee | ||
Comment 5•8 years ago
|
||
Still waiting on a try push here, but I think we can completely remove chromium's tuple.h.
Attachment #8682869 -
Flags: review?(jld)
Assignee | ||
Updated•8 years ago
|
Attachment #8682868 -
Attachment description: patch → remove chromium callback stuff
Comment 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
(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?
Updated•8 years ago
|
Attachment #8682857 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(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
Assignee | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
(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•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
(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.
Comment 15•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8682868 -
Flags: review?(jld) → review+
Updated•8 years ago
|
Attachment #8682860 -
Flags: review?(jld) → review+
Comment 18•8 years ago
|
||
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
Assignee | ||
Comment 19•8 years ago
|
||
Here's an updated patch in case anyone is interested in commenting.
Attachment #8682855 -
Attachment is obsolete: true
Attachment #8683403 -
Flags: review+
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd99e5060e1e https://hg.mozilla.org/integration/mozilla-inbound/rev/a50c676caf7f
![]() |
||
Comment 21•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9567dbd464b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/7686821fd178
Comment 23•8 years ago
|
||
(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.
Assignee | ||
Comment 24•8 years ago
|
||
The new landing fixed that problem and it seems to be building fine on JB.
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9567dbd464b8 https://hg.mozilla.org/mozilla-central/rev/7686821fd178
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
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+
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5d9e71f9738
You need to log in
before you can comment on or make changes to this bug.
Description
•