561694 - e10s HTTP: Implement nsIProgressEventSink: part2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

First, replace SendOnProgress and SendOnStatus with a single SendOnTransportStatus IPDL msg.  Just have HttpChannelParent implement nsITransportEventSink, and have nsHttpChannel's OnTransportStatus check mRemoteChannel (bug 561693) and call OnTransportStatus() on the callbacks instead of OnProgress and OnStatus.  Then unpack the RecvOnTransportStatus into the OnProgress/OnStatus calls in the child.

I strongly suspect we can actually skip SendOnTransportStatus when it's triggered by OnDataAvailable(), and roll everything into the SendOnDataAvailable msg, basically by copying the logic in nsHttpChannel::OnDataAvailable into HttpChannelChild::RecvOnDataAvailable.   We will need to keep SendOnTransportStatus around, though, for the times other than OnDataAvail when it is called. 

Be careful that you don't duplicate or lose any progress notifications, and the exact same status codes get sent as in part 1.

Comment 1

[Jason Duell]

Moving to "desired" rather than "required" bug list, since once we have part 1 done (bug 561692) we'll have correctness.

Comment 2

[Jason Duell]

Attachment: Coalesces OnStatus/OnProgress/OnDataAvailable

CC-ing fennec folks in case the performance advantage from this is worth considering it for Fennec release.  I'm guessing not--from bug 633442 comment 3 and 0, sounds like there *might* be a 1.7% speedup from this (difference between
8.7% and 7% improvement mfinkle saw).

The patch coalesces OnDataAvailable IPDL messages with the corresponding OnStatus/OnProgress msgs (status/progress msgs that aren't from an ODA still are separate IPDL msgs).  So it asymptotically reduces the IPDL message count for necko by 66% as the data payload for a channel increases.  In practice the count will be reduced less, as many channels don't carry much data, and there are other IPDL msgs like CONNECTING, etc.  Also, it won't reduce the amount of *data* sent over IPDL.  On the other hand it may make it less likely that we stick OnDataAvailable IPDL msgs in necko's IPDL msg buffer.  Anyway, it seems worth a tp4 run to see how much if affects performance (I'm emailing mfinkle to see how he's doing his comparison).

The patch is fairly simple, and relies on the following assumption, which I believe is correct:

OnDataAvailable is always immediately preceded by OnStatus with either STATUS_RECEIVING_FROM or STATUS_READING, and an OnProgress corresponding to the data being ODA will deliver.  Those two OnStatus codes are never used without OnDataAvailable being called immediately afterward.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I made some Tp testruns using the patch. The results are mostly positive. Even if we assume some noise in the runs, the data shows the patch is an overall benefit, especially on 3g. Keep in mind, Fennec does not use a disk cache.

Nexus One:
  wifi without patch: 3687ms
  wifi with patch:    3538ms

  3g without patch:   5188ms
  3g with patch:      4935ms

Galaxy Tab:
  wifi without patch: 3775ms
  wifi with patch:    3776ms

  3g without patch:   5245ms
  3g with patch:      4807ms

I can attach the actual testrun reports is needed too. The reports show that the most benefit come from more complex sites. Simple lightweight web sites don't see as much improvement, if any.

Comment 4

[Doug Turner (:dougt)]

honzab, eta for the review?

Comment 5

[Jason Duell]

Glad to see the perf win.  Though I wish I had a better idea of what's going on exactly with the numbers in comment 3.

My initial thought was that the win here is from causing fewer context switches on the mobile device.  But that doesn't explain the split between the wifi and 3g results on the Galaxy (the Nexus both improve by about 4%, the Galaxy is no improvement for wifi, 9% for 3g).

In case it actually is context switch savings: cjones tells me that IPDL tries to coalesce IPDL calls, and I'm not sure why that doesn't happen in this case, since OnStatus/Progress/Data are called in quick succession (perhaps it's because OnData is a big message?).  Anyway, it might be worth looking at other places where messages could be manually coalesced (and/or a review of the the IPDL coalescing code) to see if there are other wins to be had.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Another possible culprit we discussed was the fact that each IPDL message is dispatched as a separate event, and that's not particularly cheap (especially in our current event-loop setup).  The reason only one IPDL message is dispatched per event-loop iteration is to avoid starving other event sources.

There are various heuristics we can try like dispatching k messages per iteration, or dispatching all the messages at the front of the queue that are directed at a particular protocol/actor.  But it's pretty hard to get that right without good tests.

Anyway, manual coalescing is usually a good idea anyway in cases like this when it's known that the three notifications are a group.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment 6 sounds like a good explanation. Can we try to get this landed for the Firefox 5 push to Auora?

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 517455 [details] [diff] [review]
Coalesces OnStatus/OnProgress/OnDataAvailable

>diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp
>+void
>+HttpChannelChild::OnTransportAndData(const nsresult& status,
>+  if (mProgressSink && NS_SUCCEEDED(mStatus) && mIsPending &&
>+      !(mLoadFlags & LOAD_BACKGROUND))
>+  {
>+    // OnStatus
>+    //
>+    nsCAutoString host;
>+    mURI->GetHost(host);
>+    mProgressSink->OnStatus(this, nsnull, status,
>+                            NS_ConvertUTF8toUTF16(host).get());

You should not call OnStatus when the state is still 0 (never set) or different from READ/RECEIVE as this is just a coalescetaion.

Another opt (that could be part of this patch optionally) could be to always generate the wstring aStatusArg in OnStatus the way you do it in OnTransportAndData and get rid of it transferring.

>diff --git a/netwerk/protocol/http/HttpChannelParent.h b/netwerk/protocol/http/HttpChannelParent.h
>+  nsresult mStatus;

I'd rather call this mProgressStatus or some such.  It is confusing because mStatus is used for an error/ok state on other places of the http code.

r=honzab

Comment 9

[Jason Duell]

Fixes made (including change to OnStatus, rename variables, check for invalid
status code)

Landed on cedar:

  http://hg.mozilla.org/projects/cedar/rev/cd18613f8de4

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cd18613f8de4

Comment 11

[Andreea Pod]

Is there a way for me to verify this? If not can somebody verify this?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> Is there a way for me to verify this? If not can somebody verify this?

We need to get the Maemo / Android Talos tests running so we can look for an improvement.