Right now the protocol looks like (from the child side) SendAsyncOpenFileCtor -> <- (RecvFileOpened | RecvFileDidNotOpen) Send__delete__() -> instead, this should be union Result { null_t; FileDescriptor; }; child: __delete__(Result res); which saves a round trip. This isn't a significant part of startup profiles, but it's showing up and would be a simple fix. (And kind of bugs me ;) .)

Oh, actually it's a bit worse SendPRemoteOpenFileCtor -> SendAsyncOpenFile -> <- (RecvFileOpened | RecvFileDidNotOpen) Send__delete__() -> instead, this should be SendPRemoteOpenFileCtor(path) -> <- Recv__delete__(Result res) union Result { null_t; FileDescriptor; }; child: __delete__(Result res);

I think this is jduell's maybe?

Sure, I'll fix this, but just to be clear, where's the round trip we're saving? All IPDL messages here are async IIUC. So the fact that we send delete from the child instead of the parent just means we send one or two extra IPDL msgs, and the channel is kept alive for a teeny bit longer, but I don't see how anything we care about is delayed by it. Question: delete on the parent triggers closing the file descriptor. If we move sending the fd to the child to __delete__, hopefully IPDL will have already done whatever it takes to dupe the fd to the child (or otherwise make sure the close() in the parent destructor doesn't close the fd before it's shared w/child)? I'll give it a go and see, unless cjones/bent know already that it won't work.

This is saving unnecessary IPC traffic. Passing a FileDescriptor to a Send__delete__() call will do the right thing and deliver the fd to the subprocess intact.

This is the most minimal change I can make to clean up the protocol I think. I didn't take care of the linux-only or main-thread-io stuff, we'll need followups for that.

Comment on attachment 709111 [details] [diff] [review] Patch, v1 Review of attachment 709111 [details] [diff] [review]: ----------------------------------------------------------------- stealing review. Looks good. I'll file followup for the NSPR-ization and off-main-thread issues.

Is this ready to land?

Looks like it's based on top of patch bug 835698 (though with a little bitrot there, too)

Here's the patch, unbitrotted after bug 835698 landed. This is for the b2g18 branch. I'm holding off on landing until we sort out the bug 835698 oranges on windows.

Comment on attachment 711172 [details] [diff] [review] v1.1: unbitrotted [Approval Request Comment] Bug caused by (feature/regressing bug #): 815523 User impact if declined: Slower performance Testing completed: m-i Risk to taking this patch (and alternatives if risky): None, really. String or UUID changes made by this patch: None

Backed out for build bustage: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=84db7473edfb https://hg.mozilla.org/integration/mozilla-inbound/rev/84db7473edfb

Now with less made-up identifiers.

Comment on attachment 711278 [details] [diff] [review] Patch, v1.2 [Approval Request Comment] See comment 11.

(In reply to ben turner [:bent] from comment #11) > User impact if declined: Slower performance Can you quantify which apps will perform better, when they'll perform better, and a rough estimate of how much improvement we can expect? Although this is very low risk, we need to make sure the change has noticeable user impact.

All apps will have slightly better startup time from this patch. It reduces the number of IPDL msgs sent at startup, and thus the number of context switches on the device if nothing else. But I don't have numbers for how much it helps. It's probably not a lot. It is very low risk--pretty much the same codepath always, and it happens at every app startup, so we'd see bugs quickly. My sense is that it's worth uplifting as an unquantified but very low risk perf win.

Comment on attachment 711278 [details] [diff] [review] Patch, v1.2 Given that it doesn't sound like a noticeable improvement to the user's experience let's not take on any unnecessary risk here, we can take this for uplift to v1.1 after Wednesday's branching - triage will come around again to approve later this week.

Comment on attachment 711278 [details] [diff] [review] Patch, v1.2 Please go ahead with landing to mozilla-b2g18 tip as per https://wiki.mozilla.org/Release_Management/B2G_Landing#More_Details so it will be on v1.1.0

I'm holding off on landing this on b2g18 until we sort out why bug 835698 was failing on that branch. This patch touches similar code so it might muddy the waters.

Comment on attachment 711278 [details] [diff] [review] Patch, v1.2 Clearing request for b2g18 approval based on bug 835698 comment 53 and 54.

(In reply to Jason Duell (:jduell) from comment #23) > Comment on attachment 711278 [details] [diff] [review] > Patch, v1.2 > > Clearing request for b2g18 approval based on bug 835698 comment 53 and 54. Removing tracking+ too then.