Closed Bug 1547116 Opened 7 months ago Closed 6 months ago

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

Categories

(Firefox :: General, defect, P2)

66 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 + fixed
firefox69 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug, Regression)

Details

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

Attachments

(1 file)

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?

Flags: needinfo?(spohl.mozilla.bugs)

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.

Summary: Refreshing the touch bar makes up 20-30% of tab switch time → Refreshing the touch bar makes up a big chunk of tab switch time

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

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.

(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.

Flags: needinfo?(spohl.mozilla.bugs)

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?
Whiteboard: [fxperf]
Whiteboard: [fxperf] → [fxperf:p3]

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(spohl.mozilla.bugs)
Priority: -- → P3

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

Priority: P3 → P2

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...

Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

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.

Attachment #9060837 - Attachment description: Bug 1547116 - WIP patch to avoid updating unnecessary touch bar inputs. → Bug 1547116 - Avoid updating unnecessary touch bar inputs. r=mikedeboer
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05ca9ecd862d
Avoid updating unnecessary touch bar inputs. r=mikedeboer
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Component: Widget: Cocoa → General
Product: Core → Firefox
Target Milestone: mozilla69 → Firefox 69

Is this worth uplifting to beta?

Flags: needinfo?(jhofmann)

Yeah, absolutely, thanks for the reminder.

Flags: needinfo?(jhofmann)

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
Attachment #9060837 - Flags: approval-mozilla-beta?

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

Attachment #9060837 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.