Lot of superfluous copies of vector<string> in GeckoChildProcessHost::Launch and helpers
Categories
(Core :: IPC, defect, P3)
Tracking
()
People
(Reporter: jduell.mcbugs, Unassigned)
Details
(Keywords: perf)
We should be able to change all the helper functions (AsyncLaunch, SyncLaunch, PerformAsyncLaunch, etc) to take a reference to the vector instead. This would mean getting rid of the default arguments, but that should be fine. And maybe make them all private while we're at it? Looks like only Launch is called by external code.
Reporter | ||
Comment 1•13 years ago
|
||
Never mind--the top-level functions are getting called with default args, and PerformAsync launch is on a different thread, so needs a copy not a ref.
Comment 2•7 years ago
|
||
:clangbot complained about this in bug 1401790 comment #2, because I happened to touch one of the lines where an unnecessary copy happens. It suggested adding a Move(), but I think we could go further and use rvalue references for (at least some of?) these parameters.
Comment 3•5 years ago
|
||
Hey, I was recommended to try this as my first bug by my mentor, so I'll work on it if that's okay!
Comment 4•5 years ago
|
||
This code has gotten more complicated since comment #2, and this bug was never very well specified, so I've done some looking around in searchfox to try to clarify it.
One recent change is that I already added some move
s inside IPC, although the arguments are still by value: where GeckoChildProcessHost::AsyncLaunch
is called internally by SyncLaunch
and LaunchAndWaitForProcessHandle
, and from ContentParent::LaunchSubprocessInternal
.
What can be done is:
-
Change
GeckoChildProcessHost::{Sync,Async}Launch
to takevector<string>&&
and change all of their callers in the process-type-specific code to usemove
(or, if they were passing an empty vector, I think they can just use{}
to create a default-constructed temporary). This is probably the most tedious of these changes. -
Use
move
in the closure inRunPerformAsyncLaunch
, which will need to be declaredmutable
to allow moving out of the captured variable. (What we really want here is Rust'sFnOnce
, but I don't think C++ has anything like that.) This allows changingPerformAsyncLaunch
to takevector<string>&&
. -
It should be possible to use
move
when constructing the runnable sent to the I/O thread. (This wouldn't have worked withconst T&
, as comment #1 pointed out.) -
Change the type parameter of that runnable to
vector<string>&&
, to avoid a copy into the argument ofRunPerformAsyncLaunch
, which should also be able to have its argument type changed tovector<string>&&
to ensure that it's being passed by move. These last two are both one-line changes and won't affect anything outside of IPC.
Keep in mind that an argument declared with type T&&
isn't a T&&
when it's used and needs to be move
d again to be passed to another function as T&&
; this is a little counterintuitive and I found it confusing when I first started working with C++11.
Comment 5•5 years ago
|
||
Thanks for the info Jed! I was pretty confused about it, and this sure helps. I also wanted to know, when can I have this bug assigned to me? What is the process?
Updated•5 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Description
•