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)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.3

People

(Reporter: billm, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
(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
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
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)
I'll push to try as soon as the trees reopen.
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+
Iteration: --- → 36.3
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
(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.
Depends on: 1010411
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 :)
Blocks: 1010411
No longer depends on: 1010411
https://hg.mozilla.org/mozilla-central/rev/ad799d59abdc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1103068
Depends on: 1106936
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
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.