1562082 - Always enable profiler popup icon in local builds

Status: REOPENED

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Most devtools have shortcuts.

I propose we have one for the profiler popup icon, which would be useful when working on local builds that regularly use clean profiles.

The obvious choice would be Ctrl+Shift+1, if not used elsewhere. I guess we'd need to know if the legacy add-on is there, so we don't steal its shortcut.

Bonus points (possibly a separate bug) : Automatically enable the icon if profiler is already started (e.g., for startup profiling).

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

After discussing with the team:

• Yet-another shortcut is probably not good, especially as a very small proportion of our users will use the Profiler, and the rest may be confused with the strange icon appearing.
• A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

In the meantime the icon can be programmatically enabled with the "devtools.performance.popup.feature-flag" pref, which can be set with ./mach run --setpref "devtools.performance.popup.enabled=true"

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gerald Squelart [:gerald] from comment #1)

...
In the meantime the icon can be programmatically enabled with the "devtools.performance.popup.feature-flag" pref, which can be set with ./mach run --setpref "devtools.performance.popup.enabled=true"

This doesn't seem to work on new profiles (until I manually enable it once). 😪

Comment 3

[Julien Wajsberg [:julienw]]

Ah is it possible that this doesn't work on the prefs that aren't defined in all.js? This pref is defined in code only :/

Comment 4

[Julien Wajsberg [:julienw]]

Maybe Andrew would know about this.

Comment 5

[Andrew Halberstadt [:ahal]]

The --setpref option works by writing a user.js file in the profile. So all.js shouldn't affect anything on the tooling side, but maybe it has implications with Firefox that I'm not aware of. On first run (when the pref doesn't work), open the profile and inspect user.js to verify if the pref is in there or not.

Also reading the mach run source, looks like --setpref is only supported if you don't pass in -P/--profile.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Gerald Squelart [:gerald] from comment #1)

After discussing with the team:

• Yet-another shortcut is probably not good, especially as a very small proportion of our users will use the Profiler, and the rest may be confused with the strange icon appearing.
• A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

We do have some 'local development only' shortcuts (like ctrl+alt+r for doing a restart plus session restore): https://searchfox.org/mozilla-central/source/browser/base/content/browser-development-helpers.js. I think it'd be totally fine to add global shortcut for the profile either directly in browser-development-helpers, or guarded behind !AppConstants.MOZILLA_OFFICIAL in the profile code somewhere, as per https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/browser/base/content/browser.xhtml#95-97.

Comment 7

[:Harald Kirschner :digitarald]

A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

I like that solution as well. Nightly might also be fine, which is our prime performance contributor base (at least testing it out and gathering feedback).

Comment 8

[:Harald Kirschner :digitarald]

We touched on this a few times during planning and I didn't hear concerns. Lets enable this for local builds first and then see if devs would also like it for Nightly.

Comment 9

[:Harald Kirschner :digitarald]

Related, bug 1385452 landed the restart button for local builds.

Comment 10

[Greg Tatum [:gregtatum]]

Attachment: Bug 1562082 - Enable profiler menu button on local builds; r?canaltinova

This patch will turn on the profiler button for all local builds.

Depends on D66407

Comment 11

[:Harald Kirschner :digitarald]

I mentioned this work to mattn. He pointed out that adding icons for builds has to be done carefully to avoid changing the toolbar for when tests run. He recommended to tie it into mach run.

Comment 12

[Matthew N. [:MattN]]

Yeah, we've had test failures in the past due to toolbar overflow and UITour availableTargets. The toolbar item could also skew local Talos results.

browser-development-helpers.js is what I was thinking of but that also seems to use AppConstants.MOZILLA_OFFICIAL. It looks like the proposed patch also aligns with how we handle debugging prefs. Maybe we don't actually do any additional changes for mach run anymore? bgrins would know.

I guess as long as tests pass in automation and people don't complain too much about local test failures due to different screen resolutions causing toolbar overflow then you're probably fine.

Comment 13

[Greg Tatum [:gregtatum]]

Yeah, I may hook into mach run. I'm holding off on landing this patch for now.

Here's where that would happen:
https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/python/mozbuild/mozbuild/mach_commands.py#1002-1146

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: Bug 1562082 - Always enable the profiler popup icon in local builds r=canaltinova

Comment 15

[Julien Wajsberg [:julienw]]

I ended up doing something very similar to Greg's previous patch, but in a simpler way. This doesn't use preferences so it doesn't persist if the user removes the popup button, but I believe it's fine.

I pushed a try with "try auto" because I didn't have a good idea of which tests could be affected by adding this button: https://treeherder.allizom.org/jobs?repo=try&revision=6c24571edfbda0f65401e2716baa8e319612373c

Also I'm a bit afraid we might see a performance regression because of this patch, that would be a red herring because that wouldn't happen in official builds. I wonder if we could avoid that.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7eb738540ace
Always enable the profiler popup icon in local builds r=canaltinova,profiler-reviewers

Comment 17

[agoloman]

Backed out for causing bc failures and c2 failures.

• Backout link

Bc failures @browser_914138_widget_API_overflowable_toolbar.js

• Push with failures
• Failure Log

Bc failures @browser_ext_commands_onCommand.js

• Push with failures
• Failure Log

C2 failures @test_keycodes.xhtml.

• Push with failures
• Failure Log

Comment 18

[Julien Wajsberg [:julienw]]

<redacted>

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: Bug 1562082 - Fix possible race when the profiler icon is added in a local build or startup profiling is on r=canaltinova

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1562082 - Try adding a test

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to agoloman from comment #17)

Backed out for causing bc failures and c2 failures.

• Backout link

Bc failures @browser_914138_widget_API_overflowable_toolbar.js

• Push with failures
• Failure Log

Bc failures @browser_ext_commands_onCommand.js

• Push with failures
• Failure Log

C2 failures @test_keycodes.xhtml.

• Push with failures
• Failure Log

It's possible these failures happen because asan doesn't have MOZILLA_OFFICIAL, see bug 1951906.

But I changed the patch to always opt out when in automation now.

Comment 22

[Julien Wajsberg [:julienw]]

Try build: https://treeherder.mozilla.org/jobs?repo=try&revision=48e131ba1ba70f09c9ea8a1a239a36a91f167f3c

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b3ed888a219
Always enable the profiler popup icon in local builds r=canaltinova,profiler-reviewers

Comment 24

[Phabricator Automation]

Comment on attachment 9469925 [details]
Bug 1562082 - Fix possible race when the profiler icon is added in a local build or startup profiling is on r=canaltinova

Revision D240449 was moved to bug 1952214. Setting attachment 9469925 [details] to obsolete.

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3b3ed888a219

Comment 26

[amarc]

Backed out for causing marionette failures @ test_assert_no_toolbar_changes.py

• Backout link
• Push with failures
• Failure Log
• Failure line:

TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/marionette/test_assert_no_toolbar_changes.py TestNoToolbarChanges.test_no_toolbar_changes | AssertionError: Lists differ: ['bac[194 chars]oolbar-button', 'profiler-button', 'unified-extensions-button'] != ['bac[194 chars]oolbar-button', 'unified-extensions-button']

Comment 27

[Julien Wajsberg [:julienw]]

I don't have time to deal with it at the moment, removing the needinfo.