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)
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)
Assignee | ||
Updated•14 years ago
|
tracking-fennec: --- → ?
Comment 1•14 years ago
|
||
jduell, honza, any suggestions here?
Comment 2•14 years ago
|
||
mfinkle, is the cost mostly with the script evaluation, or with the IPC overhead.
Assignee | ||
Comment 3•14 years ago
|
||
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
Comment 4•14 years ago
|
||
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?
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
(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.
Comment 7•14 years ago
|
||
could you verify this by a debugger statement? Maybe i mess something up in gdb.
Assignee | ||
Comment 8•14 years ago
|
||
(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.
Comment 9•14 years ago
|
||
that is wierd... it is a registered listener, and we do call that script according to gdb.
Comment 10•14 years ago
|
||
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.
Comment 11•14 years ago
|
||
2.0-, but we'd take a patch to improve this situation
tracking-fennec: ? → 2.0-
Comment 12•14 years ago
|
||
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
Updated•14 years ago
|
Whiteboard: [fennec-4.1?]
Updated•14 years ago
|
Assignee: nobody → mark.finkle
Assignee | ||
Comment 13•14 years ago
|
||
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.
Description
•