PRemoteOpenFile could be more IPC-efficient

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: cjones, Assigned: bent.mozilla)

Tracking

unspecified
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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 ;) .)
Summary: AsyncOpenFile could be more IPC-efficient → PRemoteOpenFile could be more IPC-efficient
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?

Comment 3

7 years ago
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.
Posted patch Patch, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #709111 - Flags: review?(jones.chris.g)

Comment 6

7 years ago
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.
Attachment #709111 - Flags: review?(jones.chris.g) → review+
Is this ready to land?

Comment 8

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

Comment 9

6 years ago
Posted patch v1.1: unbitrotted (obsolete) — Splinter Review
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.
Attachment #709111 - Attachment is obsolete: true
Attachment #711172 - Flags: review+
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
Attachment #711172 - Flags: approval-mozilla-b2g18?
Attachment #711172 - Flags: approval-mozilla-b2g18?
Posted patch Patch, v1.2Splinter Review
Now with less made-up identifiers.
Attachment #711172 - Attachment is obsolete: true
Attachment #711278 - Flags: review+
Comment on attachment 711278 [details] [diff] [review]
Patch, v1.2

[Approval Request Comment]
See comment 11.
Attachment #711278 - Flags: approval-mozilla-b2g18?
(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.

Comment 17

6 years ago
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
Attachment #711278 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

Comment 20

6 years ago
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.
Depends on: 835698
https://hg.mozilla.org/mozilla-central/rev/331e2dcd93d7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 23

6 years ago
Comment on attachment 711278 [details] [diff] [review]
Patch, v1.2

Clearing request for b2g18 approval based on bug 835698 comment 53 and 54.
Attachment #711278 - Flags: approval-mozilla-b2g18+
(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.
tracking-b2g18: + → ---
You need to log in before you can comment on or make changes to this bug.