Closed
Bug 1099490
Opened 10 years ago
Closed 10 years ago
[e10s] Lots of onStateChange messages sent to parent; nsBrowserStatusFilter should be used in the content process rather than in the chrome process
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
People
(Reporter: billm, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.82 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
I'm not sure how serious this is, but I was surprised by it. Just loading nytimes.com sends about 1000 onStateChange messages to the parent process. That seems pretty excessive. I remember we have an object that filters these messages so there aren't so many of them. I think we're using it in the parent process but not in the child. It would make more sense to do it in the child. This probably isn't a huge performance problem, but it might save a little time and it might be easy to fix.
Comment 1•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #0) > I remember we have an object that filters these > messages so there aren't so many of them. http://hg.mozilla.org/mozilla-central/annotate/acbd7b68fa8c/browser/base/content/tabbrowser.xml#l1664
Summary: [e10s] Lots of onStateChange calls sent to parent → [e10s] Lots of onStateChange messages sent to parent; nsBrowserStatusFilter should be used in the content process rather than in the chrome process
Version: 34 Branch → Trunk
Assignee | ||
Comment 2•10 years ago
|
||
We should do this, with the status filter applied we only have about 24 messages for the test page I used where as currently we send 900+ just to load the page.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Rewrote RemoteWebProgress a little to handle aWebProgress=null better. nsBrowserStatusFilter sends OnStatusChange() only with aWebProgress=null and aRequest=null. From some local smoke testing and running a few tests it seems that the double-filtering doesn't do any harm. It would be great to only filter once of course but we could probably only do that by making non-e10s use RemoteWebProgress as well.
Attachment #8523421 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 4•10 years ago
|
||
I'll push to try as soon as the trees reopen.
Assignee | ||
Comment 5•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ae5677ac10ba
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8523421 [details] [diff] [review] 0001-Bug-1099490-e10s-Use-nsBrowserStatusFilter-in-the-co.patch Review of attachment 8523421 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: toolkit/modules/RemoteWebProgress.jsm @@ +156,5 @@ > json.originalRequestURI); > } > > // Update the actual WebProgress fields. > + if (webProgress) { Maybe move this code up to the previous |if (json.webProgress)| check?
Attachment #8523421 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Updated•10 years ago
|
Iteration: --- → 36.3
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #6) > > // Update the actual WebProgress fields. > > + if (webProgress) { > > Maybe move this code up to the previous |if (json.webProgress)| check? Thanks, will do.
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ad799d59abdc
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d8287013afb3 Pushed a small follow-up to fix the recent spike of intermittent failures (bug 1010411) that started with the above push. Guess we really are faster now :)
Assignee | ||
Updated•10 years ago
|
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ad799d59abdc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 12•9 years ago
|
||
I stumbled across another use of nsIWebProgressListener in the SessionStore via FrameTree.jsm and was wondering if that code should be using BrowserFilter too: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/FrameTree.jsm?force=1#66
Assignee | ||
Comment 13•9 years ago
|
||
We could indeed use the filter there too but OTOH this piece of code shouldn't be too expensive to run. At least not more expensive than the filter itself, excluding any XPCOM overhead. We don't send messages for every state change notification from the FrameTree like we did here. Using the filter for the FrameTree as well should "just work" but I don't know whether there is any advantage from doing so?
You need to log in
before you can comment on or make changes to this bug.
Description
•