1472921 - New performance panel cannot save sampling interval value that is less than 1 ms

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

Step
1. Enable new performance panel
2. Open Performance from devtool menu
3. Set sampling interval to 0.1 ms
4. Close devtool
5. Open devtool, then open performance panel

Result
Sampling interval is 0ms

Expected Result
Sampling interval sets 0.1 ms (that is same value as step 3)

Notes
If this interval is equal or greater than 1ms, it works well.

Comment 1

[Julien Wajsberg [:julienw]]

Ah, we used an int pref, see [1] and [2]. We should use either a char pref or an int pref holding the value multiplied by 10 or 100.

[1] https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/devtools/client/performance-new/browser.js#97-101
[2] https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/devtools/client/performance-new/browser.js#131-134

Comment 2

[Greg Tatum [:gregtatum]]

Char sounds good to me.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bug 1472921 - Fix sampling interval persistence for values <1ms.

Review commit: https://reviewboard.mozilla.org/r/259602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259602/

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

I went with an int pref to simplify things (msec -> usec). In particular I'm not sure if you can change the type of a pref right after reading it without throwing. Let me know if you'd rather have the more complicated migration step.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8995126 [details]
Bug 1472921 - Fix sampling interval persistence for values <1ms.

https://reviewboard.mozilla.org/r/259602/#review267044

You also need to change the default value in [1], otherwise the default is 0.001ms :) Otherwise I think this is good to go! Thanks a lot for this :)

[1] https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/devtools/client/performance-new/store/reducers.js#61

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8995126 [details]
Bug 1472921 - Fix sampling interval persistence for values <1ms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259602/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a485a8db67c
Fix sampling interval persistence for values <1ms. r=julienw

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfc3d5482f79
Backed out changeset 0a485a8db67c for debug-mochitest-chrome failures on test_perf-settings-interval.html

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8995126 [details]
Bug 1472921 - Fix sampling interval persistence for values <1ms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259602/diff/2-3/

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Forgot to rerun the tests after the one line change!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/230936a14d39
Fix sampling interval persistence for values <1ms. r=julienw

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/230936a14d39