Closed Bug 1646086 Opened 4 years ago Closed 4 years ago

UrlbarProviderSearchTips.jsm appears in Talos ts_paint profiles

Categories

(Firefox :: Address Bar, task, P3)

task
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 80
Iteration:
80.2 - July 13 - July 26
Tracking Status
firefox80 --- fixed

People

(Reporter: mak, Assigned: adw)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxperf:p3])

Attachments

(1 file)

I was running
mach talos-test -a ts_paint_webext --gecko-profile
and I noticed in the profile that UrlbarProviderSearchTips.jsm appears a couple times for lastBrowserUpdateDate and isBrowserShowingNotifications that are initialized from onLocationChange.
Can we do something to reduce the impact on Ts?

Summary: UrlbarProviderSearchTips.jsm appears in Talos ts tests → UrlbarProviderSearchTips.jsm appears in Talos ts_paint tests
Summary: UrlbarProviderSearchTips.jsm appears in Talos ts_paint tests → UrlbarProviderSearchTips.jsm appears in Talos ts_paint profiles

Here's a profile showing this: https://share.firefox.dev/2V7qovs

Whiteboard: [fxperf] → [fxperf:p3]

(In reply to Marco Bonardo [:mak] from comment #0)

Can we do something to reduce the impact on Ts?

_maybeShowTipForUrl does several checks to determine whether to show a tip. The first two checks are isBrowserShowingNotification and lastBrowserUpdateDate. We should move those to the end because the (vast?) majority of the time the checks that follow those two will be hit and we can return early before ever having to check isBrowserShowingNotification and lastBrowserUpdateDate.

I don't know whether that's enough to help Ts because it depends on the page that Ts loads. If it loads about:home/newtab, then it won't help. But it's worth doing for sure.

Beyond that, the stack in comment 1 is pretty surprising. isBrowserShowingNotification is a crazy big percentage. I don't really understand the stack below that call because it's all internal JS and compartments stuff...

(In reply to Drew Willcoxon :adw from comment #2)

We should move those to the end because the (vast?) majority of the time the checks that follow those two will be hit and we can return early before ever having to check isBrowserShowingNotification and lastBrowserUpdateDate.

I'll make a patch for this.

Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 80.2 - July 13 - July 26
Points: --- → 1

If I'm reading that profile right, a big part is loading AppMenuNotifications.jsm and ShellService.jsm (via DefaultBrowserCheck.willCheckDefaultBrowser in BrowserGlue.jsm).

At least for Ts, we could delay checking for tips until after delayed startup or whatever the condition is when Ts ends.

(In reply to Drew Willcoxon :adw from comment #5)

At least for Ts, we could delay checking for tips until after delayed startup or whatever the condition is when Ts ends.

I assume tips are a non-critical feature on startup, they will be effective even if shown later in the session. delayed startup would be a good start.

Points: 1 → 3
Attachment #9158777 - Attachment description: Bug 1646086 - Make UrlbarProviderSearchTips check the current URL before doing other more expensive checks to improve performance. → Bug 1646086 - Improve UrlbarProviderSearchTips's impact on ts_paint.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fb311e74315
Improve UrlbarProviderSearchTips's impact on ts_paint. r=mak
Blocks: 1649273
Flags: qe-verify-
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80

== Change summary for alert #26408 (as of Thu, 02 Jul 2020 03:27:46 GMT) ==

Improvements:

4% sessionrestore windows7-32-shippable opt e10s stylo 479.83 -> 458.50
3% startup_about_home_paint windows7-32-shippable opt e10s stylo 605.25 -> 586.75
3% startup_about_home_paint windows7-32-shippable opt e10s stylo 604.79 -> 586.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26408

nice!

Regressions: 1675541
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: