1349742 - Investigate UITour's performance impact during window opening and closing

Status: RESOLVED FIXED

(Firefox :: Tours, enhancement, P1)

Description

[Florian Quèze [:florian]]

Attachment: Patch

The observer from UITour.jsm is very visible in profiles: https://perfht.ml/2noCrEC

http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/browser/components/uitour/UITour.jsm#718

This iterates over all the existing windows, and that loop seems to be O(n2) (this landed in bug 1126756).

This slow code runs when receiving the message-manager-close notification, which is fired several times as a result of the tabbrowser updateBrowserRemoteness call.

This UITour code runs because for each new browser window there are 3 UITour:onPageEvent notifications with getConfiguration calls for "sync", "appinfo" and "selectedSearchEngine".

I'm not sure what's causing these calls, I suspect it's self-support but haven't found the actual code yet.

It's very easy to make the performance impact disappear here by not initializing UITour when the page event is only a getConfiguration one (initialization doesn't matter in that case because we don't display anything in the browser, so nothing needs to be uninitialized later). This is what the attached patch does.

But I think this deserves a bit more analysis.

Comment 1

[Matthew N. [:MattN]]

Are you testing with about:home in the new window? UITour shouldn't be running code at all unless a tour page (about:home, some SUMO, or maybe some remaining www?) fires a UITour event.

Comment 2

[Florian Quèze [:florian]]

This UITour.jsm "message-manager-close" observer also shows up a lot when profiling window closing. Eg. on this profile https://perf-html.io/public/eefae3b9227ce6d027c80d687d29e6cc2293ae03/calltree/?jsOnly&thread=0 of closing 8 or so empty (eg. only about:home loaded) windows, it's where we spend 13% of the time, and the top piece of JS code visible in the profile.

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #1)
> Are you testing with about:home in the new window?

Yes, my testing was done with fresh profiles, so about:home was what loaded by default when opening a window.

It seems this only calls getConfiguration, so the trivial patch I attached may be a straightforward way to get a quick win.


But I still think it would be worth looking more at this code to find an uninitialization solution that doesn't require going through all windows each time one is closed.

Comment 3

[Matthew N. [:MattN]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> This UITour.jsm "message-manager-close" observer also shows up a lot when
> profiling window closing. Eg. on this profile
> https://perf-html.io/public/eefae3b9227ce6d027c80d687d29e6cc2293ae03/
> calltree/?jsOnly&thread=0 of closing 8 or so empty (eg. only about:home
> loaded) windows, it's where we spend 13% of the time, and the top piece of
> JS code visible in the profile.

That doesn't sound like a real world use case… IMO it would be much more useful to have different contents in the windows.

> (In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are
> blocking you) from comment #1)
> > Are you testing with about:home in the new window?
> 
> Yes, my testing was done with fresh profiles, so about:home was what loaded
> by default when opening a window.
> 
> It seems this only calls getConfiguration, so the trivial patch I attached
> may be a straightforward way to get a quick win.

Sure, request review/feedback if you want me to consider it more.

> But I still think it would be worth looking more at this code to find an
> uninitialization solution that doesn't require going through all windows
> each time one is closed.

Well don't most users have 1 window? And what is the distribution of # of windows for real users?

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8850211 [details] [diff] [review]
Patch

If this doesn't have negative consequences, I think it'll be worth taking, so requesting review.

(In reply to Matthew N. [:MattN] from comment #3)

> > But I still think it would be worth looking more at this code to find an
> > uninitialization solution that doesn't require going through all windows
> > each time one is closed.
> 
> Well don't most users have 1 window? And what is the distribution of # of
> windows for real users?

"message-manager-close" fires very often though. If I remember correctly, when I initially filed this bug, it was fired 8 times when opening a new window (in large part due to the browser initially being non-remote and then being changed to a remote one; I think this has been fixed since I filed this bug by making the initial about:blank load in a remote browser). I also expect this notification to fire whenever a tab is closed.

Comment 5

[Florian Quèze [:florian]]

Another profile showing the problem better: https://perfht.ml/2pnpcVR

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8850211 [details] [diff] [review]
Patch

Review of attachment 8850211 [details] [diff] [review]:
-----------------------------------------------------------------

The reason for the inefficient loop is to find the <browser> associated with the message-manager-close. If there is a way to QI to something to get that <browser> reference the multiple loops could be replaced. That would help for cases where there are real UI tours going on too, not just for cases like about:home which only use getConfiguration.

Your approach would have also addressed bug 1293181 which is still relevant (bug 1304635) so I guess that's a nice side benefit. I can't think of any problems with this approach off-hand but it is kinda hacky (the comment will help address that). Sorry for the delay, I've been digesting this change for a while to see if any issue would jump out at me.

::: browser/components/uitour/UITour.jsm
@@ +662,5 @@
>        }
>      }
>  
> +    if (action != "getConfiguration")
> +      this.initForBrowser(browser, window);

Can you add a comment stating that this is primary/solely for performance reasons as I think it wouldn't be obvious by just looking at this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea35685faf0d
avoid initializing UITour whenever about:home is loaded, r=MattN.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea35685faf0d