638102 - Lot of superfluous copies of vector<string> in GeckoChildProcessHost::Launch and helpers

Status: NEW

(Core :: IPC, defect, P3)

Description

[Jason Duell]

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.

Comment 1

[Jason Duell]

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

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

: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

[Vaishnavi Kaulagi]

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

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

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 moves 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 take vector<string>&& and change all of their callers in the process-type-specific code to use move (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 in RunPerformAsyncLaunch, which will need to be declared mutable to allow moving out of the captured variable. (What we really want here is Rust's FnOnce, but I don't think C++ has anything like that.) This allows changing PerformAsyncLaunch to take vector<string>&&.

• It should be possible to use move when constructing the runnable sent to the I/O thread. (This wouldn't have worked with const T&, as comment #1 pointed out.)

• Change the type parameter of that runnable to vector<string>&&, to avoid a copy into the argument of RunPerformAsyncLaunch, which should also be able to have its argument type changed to vector<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 moved 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

[Vaishnavi Kaulagi]

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?