1556688 - Reduce TouchBar NSImage creation

Status: NEW

(Core :: Widget: Cocoa, defect, P2)

Description

[Johann Hofmann [:johannh]]

We fixed one aspect of this (the JS part) in bug 1547116, but we still have to optimize the widget code to avoid creating new NSImages every time we update the TouchBar.

See original report below.

+++ This bug was initially created as a clone of Bug #1547116 +++

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

[Mike de Boer [:mikedeboer]]

I think I've mentioned this elsewhere, but there's no real reason for the touchbar to be updating immediately at tab switch. I think that notifications can be caught with a timeout and do a single update after a couple of milliseconds. That would also make sure that it's not affecting tab-switch-time directly, no?
Harry's back at Mozilla and he'll be spending his re-onboarding cycles on touchbar followups! I needinfo'd him for posterity.

Comment 2

[Johann Hofmann [:johannh]]

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

I think I've mentioned this elsewhere, but there's no real reason for the touchbar to be updating immediately at tab switch. I think that notifications can be caught with a timeout and do a single update after a couple of milliseconds. That would also make sure that it's not affecting tab-switch-time directly, no?

This might be a further optimization but I'm not sure it's even necessary. We mostly need to re-use NSImages, which I'm sure is possible if not easy, but I don't have the time to look into it. If we just batch the updates we're still doing a lot of unnecessary work. The profiler link (IIRC) tells us that NSImage creation is virtually all that is slow about this.

Harry's back at Mozilla and he'll be spending his re-onboarding cycles on touchbar followups! I needinfo'd him for posterity.

Great!

Comment 3

[Harry Twyford]

Clearing needinfo since the first part of the work needed to accomplish this has been posted for review over at bug 1521893. I'm actively at work on this problem.