1547116 - Refreshing the touch bar makes up a big chunk of tab switch time

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Johann Hofmann [:johannh]]

I was originally investigating site identity panel performance on tab switch when I noticed that _updateTouchBarInputs was taking a huge portion of tab switch times. Here is the profile I captured, showing the inverted call stack: https://perfht.ml/2W7BFu2

I think the two causes are:

• nsITouchBarUpdater.updateTouchBarInputs is called several times per tab switch, once for touchbar-location-change, according to my debugging multiple times for bookmark-icon-updated and once for reader-mode-available

  • The thing here is that neither the bookmark nor the reader mode buttons are available by default, so the majority of these updates are completely unnecessary

• It seems like the majority of time is spent on NSImage initialization, which is probably because we create a new NSImage every time we "update" any touchbar icon. This still has a big impact on perf, even when taking away the unnecessary updates.

I have attached a WIP patch that adresses the first issue, but I would happily take suggestions (and/or defer the implementation to someone else) on how to get rid of that NSImage init code.

:spohl, do you have an idea how to best avoid the NSImage overhead? I assume we can re-work this to simply re-use the existing image?

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1547116 - Avoid updating unnecessary touch bar inputs. r=mikedeboer

Comment 2

[Johann Hofmann [:johannh]]

It's late and I realize that 20-30% is probably just wrong because that was based off the JS portion of the stack list and doesn't account for the content process. Slightly amending the title to give the right impression.

Comment 3

[Harry Twyford]

Thank you for filing this! Having worked on the Touch Bar code, your patch looks great, fwiw.

Comment 4

[Mike de Boer [:mikedeboer]]

We'd like to move away from rasterizing PDFs over at bug 1521893. I'm guessing after that we can introduce a cache just as easily. Stephen may have time to take a look there as well soon-ish.

Meanwhile, I think your patch looks good, Johann. I also think that we can invoke the touchbar updater inside a requestIdleCallback, because it doesn't need to be as instant as we want the actual tab-switch to be.

Comment 5

[Stephen A Pohl [:spohl] (OOO until 4/21)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)

We'd like to move away from rasterizing PDFs over at bug 1521893. I'm guessing after that we can introduce a cache just as easily. Stephen may have time to take a look there as well soon-ish.

I agree that some kind of cache would be the way to go here.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

I've got a couple of thoughts…

1. An early return at line 272 if the inputs array is empty should help reduce the number of calls to updateTouchBarInputs.
2. Going up a level into the observe method on line 279, we could check to see if the input is actually changing before calling this._updateTouchBarInputs. (I've found a lot of the time, it's getting called without anything changing.)
3. It looks like we have a dictionary of identifiers -> TouchBarInputs available. Maybe we could look up the nsITouchBarInput's key in that dictionary to get the TouchBarInput, and not continually re-create them?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 9

[Johann Hofmann [:johannh]]

Bug 1521893 seems a bit stalled and personally I think that this is quite bad and we should really fix up both aspects of this bug, but in order to have some progress I can move this patch forward and file a follow-up for the widget pieces...

Comment 10

[Johann Hofmann [:johannh]]

Thank you for all the suggestions, but looking at it again I think this patch already does the primary optimization we can do in the front-end and re-using TouchbarInputs in the widget code should resolve the biggest perf regression without needing other optimizations, so I'll just ask for review on that patch as-is and file a new bug for the widget work.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ca9ecd862d
Avoid updating unnecessary touch bar inputs. r=mikedeboer

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/05ca9ecd862d

Comment 13

[Julien Cristau [:jcristau]]

Is this worth uplifting to beta?

Comment 14

[Johann Hofmann [:johannh]]

Yeah, absolutely, thanks for the reminder.

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 9060837 [details]
Bug 1547116 - Avoid updating unnecessary touch bar inputs. r=mikedeboer

Beta/Release Uplift Approval Request

• User impact if declined: Slower tab switching on Mac
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very simple code-restructuring to increase efficiency of our TouchBar code.
• String changes made/needed: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9060837 [details]
Bug 1547116 - Avoid updating unnecessary touch bar inputs. r=mikedeboer

perf improvement for osx touch bar, approved for 68.0b8

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/55b177d21e76