1589122 - The interval in a new profile is 0.001ms but it should be 1ms.

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

Description

[Julien Wajsberg [:julienw]]

Attachment: screenshot

STR:

1. open a firefox with a new profile.
2. enable the profiler icon from the menu Tools > Web Developer > Enable Profiler Toolbar Icon
3. click the profiler icon

=> notice the interval is set at 0.001ms + the overhead is fully red.

The same happens in the experimental devtools panel.

This comes from a change in bug 1587117 where the default is supposed to be 1000 (usec) but is 1.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Rename getDefaultRecordingSettings to getDefaultRecordingPreferences

This also renames various variables from "settings" to "preferences" to
make it clearer that the values are about actual preferences stored in
the user profile.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Appropriately configure the default interval pref in microseconds

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Fix the default interval in reducers

This value isn't really used, nevertheless it's good to have it to the
right value for documentation reason and consistency.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Add and use functions to translate values between the stored preferences and the state

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Rename some arguments from settings to prefs where appropriate

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Add a simple check to the xpcshell test

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: Bug 1589122 - Add comments to the functions called when using the shortcut keys

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/909df9883d38
Rename getDefaultRecordingSettings to getDefaultRecordingPreferences r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/a99e9fb9591b
Appropriately configure the default interval pref in microseconds r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/59a3aeee924e
Fix the default interval in reducers r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/acee023cadca
Add comments to the functions called when using the shortcut keys r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/7cdd7440a5b3
Add and use functions to translate values between the stored preferences and the state r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0e091e3a56e5
Rename some arguments from settings to prefs where appropriate r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/217dd3192072
Add a simple check to the xpcshell test r=canaltinova

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/909df9883d38
https://hg.mozilla.org/mozilla-central/rev/a99e9fb9591b
https://hg.mozilla.org/mozilla-central/rev/59a3aeee924e
https://hg.mozilla.org/mozilla-central/rev/acee023cadca
https://hg.mozilla.org/mozilla-central/rev/7cdd7440a5b3
https://hg.mozilla.org/mozilla-central/rev/0e091e3a56e5
https://hg.mozilla.org/mozilla-central/rev/217dd3192072

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 9101945 [details]
Bug 1589122 - Rename getDefaultRecordingSettings to getDefaultRecordingPreferences

Beta/Release Uplift Approval Request

• User impact if declined: New profiles will have a bad default value as the "interval" when profiling, which makes firefox very slow (nearly unresponsive) if it's not changed.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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 low risk mainly because this code isn't exposed to end-users but rather to gecko developers. Even for gecko developers it requires enabling a pref.

Several alternatives:

• a simpler patch is probably possible, but because this needs also some manual testing in addition to the automated testing I'd rather use the same patchset.
• not uplifting.

I'd still rather uplift because otherwise the behavior could be surprising if for some reason a gecko developers would want to use the internal profiler popup on a new profile.

• String changes made/needed:

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9101945 [details]
Bug 1589122 - Rename getDefaultRecordingSettings to getDefaultRecordingPreferences

Big patch but has tests and impacts only gecko developers, so let's take it in 71 beta 4 as we are just starting the beta cycle, thanks.

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/92593ea9aa11
https://hg.mozilla.org/releases/mozilla-beta/rev/a15b3b8f9742
https://hg.mozilla.org/releases/mozilla-beta/rev/207c6f7f4223
https://hg.mozilla.org/releases/mozilla-beta/rev/c18192744013
https://hg.mozilla.org/releases/mozilla-beta/rev/eaeef6a3d410
https://hg.mozilla.org/releases/mozilla-beta/rev/9b55fa35e6e1
https://hg.mozilla.org/releases/mozilla-beta/rev/c80e23b44e47