Closed Bug 633442 Opened 14 years ago Closed 14 years ago

Optimize onProgress and onStatus callbacks in child

Categories

(Firefox for Android Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(fennec-)

RESOLVED WORKSFORME
Tracking Status
fennec - ---

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Whiteboard: [fennec-4.1?])

HttpChannelParent will send onProgress and onStatus messages (to IPC-ify the nsIProgressSink interface): http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#486 http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#497 If the child doesn't have any consumers for the onProgress or onStatus callbacks, we shouldn't waste the time sending them. I'm assuming nsIWebProgress might use it to send to listeners. In Fennec for example, we do not listen for nsIWebProgress progress and status callabcks in the child. Just to see if it makes a difference, I commented out the code in HttpChannelParent that sends the messages. My Tp tests should a 8.7% improvement. w/ Messages: [2415, 2252, 2557] w/o Messages: [2211, 2191, 2189] (Tp is run on localhosted and static pageset with 21 pages. I ran the test 3 separate times. You see the average result from each run. Times are in milliseconds)
Blocks: 629513
tracking-fennec: --- → ?
jduell, honza, any suggestions here?
mfinkle, is the cost mostly with the script evaluation, or with the IPC overhead.
I reverted the HttpChannelParent changes and commented out the HttpChannelChild::RecvOnProgress and HttpChannelChild::RecvOnStatus impls. Tp: [2255, 2175, 2288] Still 7% better than a normal run. Looks like it could be an issue in the child after all and not the IPC overhead. Updating summary.
Summary: Stop sending onProgress and onStatus messages in HttpCHannelParent if the Child doesn't need them → Filter onProgress and onStatus callbacks in HttpCHannelChild if the Child doesn't use them
A couple of ideas here. First, I'm guessing bug 561694 (which will eliminate separate IPC traffic for all OnProgress/Status calls that is associated with an OnDataAvailable by piggybacking them on the OnData IPDL msg) is probably the main answer here, and may make any perf issue disappear. So we should bump that up in priority (note: it doesn't take much knowledge of necko, in case you've got warm bodies to throw at it. I can also probably get to to it in the next week). I'm a little puzzled though by the results in comment 3. Are you sure that the necko client is not using the OnProgress/Status calls? If they aren't, then the only thing different under your comment 3 change is that we'd no longer be no queuing + dequeuing IPDL msgs for OnStatus/Progress, only to find that there's no progressListener to deliver to. Queuing is a little wasteful, but I wouldn't expect it to cause 7% overall perf hit! Are you sure your code isn't doing something with the notifications? Do you have some sort of load progress indicator that may be behaving badly?
Depends on: 561694
during page load, there are at least 6 listeners to onProgressChange: two javascript implementations: "chrome://browser/content/LoginManagerChild.js":762 "chrome://browser/content/bindings/browser.js":19 4 native implementations: nsSecureBrowserUIImpl::OnProgressChange nsDocShellTreeOwner::OnStateChange <--- just returns ns_ok nsDocShell::OnStateChange nsHTMLDNSPrefetch::nsDeferrals::OnStateChange I suspect LoginManagerChild, but also wonder why nsHTMLDNSPrefetch is in the list.
(In reply to comment #5) > I suspect LoginManagerChild, but also wonder why nsHTMLDNSPrefetch is in the > list. If this is true, then the webprogresslistener impl is broken. When the progress listener is added, we only ask for NOTIFY_STATE_DOCUMENT (a subset of STATE) callbacks: http://mxr.mozilla.org/mobile-browser/source/chrome/content/LoginManagerChild.js#95 Nothing else should invoke a callback. Certainly not onProgressChange.
could you verify this by a debugger statement? Maybe i mess something up in gdb.
(In reply to comment #7) > could you verify this by a debugger statement? Maybe i mess something up in > gdb. Verified. No progress or status callbacks are fired in LoginManagerChild.js However, now that I am here, I will try removing the WebProgressListener from LoginManagerChild.js altogether. We do get several onStateChange callbacks fired, even on pages without logins. And we know any web progress listener JS callbacks can be expensive.
that is wierd... it is a registered listener, and we do call that script according to gdb.
Just a note: there will always be consumers on the child process to both progress and status notifications. So, fixing this bug according to the summary would probably have no larger affect. If there is a slowdown, that disappears when we remove forward of notifications from HttpChannelChild, then there is one or more slow observers that we should catch and then remove if unnecessary or fix if possible.
2.0-, but we'd take a patch to improve this situation
tracking-fennec: ? → 2.0-
Changing summary. As comment 10 points out, we're not going to eliminate the IPDL traffic here entirely (bug 561694 is dealing with reducing it), and the bulk of the perf hit appears to be from the callbacks on the child, not the IPDL traffic itself.
Summary: Filter onProgress and onStatus callbacks in HttpCHannelChild if the Child doesn't use them → Optimize onProgress and onStatus callbacks in child
Whiteboard: [fennec-4.1?]
Assignee: nobody → mark.finkle
bug 561694 fixed enough of this. There are no other spots that jump out as problems.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.