Investigate UITour's performance impact during window opening and closing

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tours
P1
normal
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
Created attachment 8850211 [details] [diff] [review]
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.
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.

Updated

3 months ago
Whiteboard: [photon]
(Assignee)

Comment 2

3 months ago
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.
Summary: Investigate UITour's performance impact during window opening → Investigate UITour's performance impact during window opening and closing
(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?
(Assignee)

Comment 4

3 months ago
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.
Attachment #8850211 - Flags: review?(MattN+bmo)

Updated

2 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-performance]
(Assignee)

Comment 5

2 months ago
Another profile showing the problem better: https://perfht.ml/2pnpcVR
Blocks: 1304635
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.
Attachment #8850211 - Flags: review?(MattN+bmo) → review+

Comment 7

2 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea35685faf0d
avoid initializing UITour whenever about:home is loaded, r=MattN.

Updated

2 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
(Assignee)

Updated

2 months ago
Depends on: 1357710

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea35685faf0d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

a month ago
No longer blocks: 1348289
(Assignee)

Updated

a month ago
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.