1211433 - [Metrics] Increase thresholds for reflows

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Metrics, defect)

Description

[Tamara Hills [:thills]]

The threshold for jank is a bit too low which seems to cause a ton of entries when logging is enabled in hud.js.  The concern is not so much the logs, but the amount of messages that are being processed/written everytime we receive one of these.  We should see if we can increase the threshold so we don't incur so much processing.

Comment 1

[Tamara Hills [:thills]]

I correct this.  It's not actually the jank that is chatty, it's the reflow: DEVTOOLS_HUD_REFLOWS and DEVTOOLS_HUD_REFLOWS_DURATION.  I did a quick check and there are over 200 messages sent just at startup.

Comment 2

[Jan Keromnes [:janx]]

There is no actual threshold for reflows, all of them are reported undiscriminately.

When launching an app, it's expected to have many incremental reflows, but most of them should be negligible in length.

What is worrying are the more complex reflows that take more time, especially if there are many, because that directly impedes app interactivity. Maybe the telemetry code can report only longer reflows, and discard most of the short incremental reflows? (In my testing, it looks like only about 10% of reflows last more than 10ms, but if they get close to 16ms that's a frame drop).

Maybe such a threshold could also be useful for the purple reflow-counter widget? Then it could be made configurable from the HUD like the jank-threshold.

Comment 3

[Russ Nicoletti [:russn]]

Attachment: reflow-threshold.patch

Hi Jan, do you think you'll be able to take a look at this patch in the near future? If not, I can ask :jryans.

I have the Settings app patch ready, adding a 'reflow-threshold' dropdown. I'd like your feedback on the possible threshold values. Currently, I'm using 5, 10 (default), 30, 60.

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8684465 [details] [diff] [review]
reflow-threshold.patch

Review of attachment 8684465 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Russ Nicoletti [:russn] from comment #3)
> Hi Jan, do you think you'll be able to take a look at this patch in the near
> future? If not, I can ask :jryans.

Looks good to me! Thanks Russ.

> I have the Settings app patch ready, adding a 'reflow-threshold' dropdown.
> I'd like your feedback on the possible threshold values. Currently, I'm
> using 5, 10 (default), 30, 60.

I'm not sure what a 5ms threshold would be useful for. Maybe at this point you're interested in all reflows, regardless of duration? Could have a 0ms threshold? (Or a "No Threshold" option, which is the same thing).

I think the most interesting value for devs is around 16ms, because that's the point where a reflow lasts longer than a frame should (if you want 60 FPS). Maybe we can have 10ms and 20ms, to hover around it?

Did you add 60ms because it helped you significantly reduce the logged reflows? I'd like to make sure that the proposed values are really useful to devs.

Also, could you please use this opportunity to indicate that values are in milliseconds, either by changing the threshold labels to "{Jank,Reflow} Threshold (ms)", or by showing the values as "0 ms", "10 ms", etc? Thanks!

::: b2g/chrome/content/devtools/hud.js
@@ +577,5 @@
>          let {start, end, sourceURL, interruptible} = packet;
>          metric.interruptible = interruptible;
>          let duration = Math.round((end - start) * 100) / 100;
> +
> +        // Record the reflow if the duration exceeds the threshold

Over-nit: Comments in this file tend to end with a full stop. Please add a '.' at the end.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

Comment 6

[Russ Nicoletti [:russn]]

Attachment: reflow-threshold.patch

Updated to address the nit in hud.js. 

In the gaia PR, you can see I have 'no threshold', 16, and 32 as possible values. My thinking was having 10 and 20 was a bit vague if what developers mostly care about is whether the reflows are greater than 16. I imagine reflows approaching 16 might also be useful but I don't know what a good number would be that is less than but close to 16. IMO, 16 is good for the first release of telemetry and we'll adjust the possible values when we get feedback. 32 is for finding the especially bad offenders.

Comment 7

[Russ Nicoletti [:russn]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

Hi Fred, can you review this PR? This is the UI for adding a threshold to the reflow metric. Comment 4 and comment 6 contain some discussion regarding the possible values for the threshold.

Comment 8

[[e-mail]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

Looks good to me, but please rename the id (jank-threshold) when you change the string, or the l10n team can't fetch and localize it.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8685119 [details] [diff] [review]
reflow-threshold.patch

Review of attachment 8685119 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! One nit here, and a few drive-by nits on your pull request.

Also, your code has the nice side-effect that invalid threshold values will act as "no threshold" (e.g. `duration < null`, `duration < undefined`, `duration < -10`, etc. will never cause reflows to be ignored by mistake, which is a good thing). Also `duration < "50"` (notice the string value) will also work as expected. Cool!

::: b2g/chrome/content/devtools/hud.js
@@ +468,5 @@
>          }
>        });
>      }
>  
> +    SettingsListener.observe('devtools.reflowduration.threshold', this._reflowThreshold, threshold => {

Nit: As mentioned in my drive-by review on the pull request, please rename the setting to something like 'hud.reflows.duration'.

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8684465 [details] [diff] [review]
reflow-threshold.patch

(Marking the old patch as obsolete.)

Comment 11

[Russ Nicoletti [:russn]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

I've updated the PR as follows:

* Renamed l10n id 'jank-threshold' to 'hud-jank-threshold'
* Renamed l10n id 'reflow-threshold' to 'hud-reflows-threshold'
* Renamed setting 'devtools.reflowduration.threshold' to 'hud.reflows.duration'
* Changed available reflow duration values to '0, 10 (default), 20, 50, 100'

Comment 12

[Russ Nicoletti [:russn]]

Attachment: reflow-threshold.patch

This patch contains changes to address comment 9 comments. Essentially, the gecko change reflecting that the setting 'devtools.reflowduration.threshold' is now 'hud.reflows.duration'

Note that the now obsolete patch [1] has the r+.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8685119&action=edit

Comment 13

[[e-mail]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

Looks good, thanks.

Comment 14

[Russ Nicoletti [:russn]]

Try for gecko patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab2411284ad5

Comment 15

[Russ Nicoletti [:russn]]

Carsten, can you help move this forward? It seems to be 'checkin-needed' for longer than usual. I have another patch that is blocked until this one lands. Thanks.

Comment 16

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/63f0dc47d179fd709c4162822552d575ff6a58f0

landed now, sorry for the delay!

Comment 17

[Russ Nicoletti [:russn]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This is a new feature.
[User impact] if declined: The 'reflows' metrics (part of the "Enhanced" metrics) will not be very useful, there will be a lot of data points that are not relevant.
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): This is small change to gecko (Developer HUD) that affects only the collection of reflow metrics and a small change to gaia Settings app to expose reflow threshold on the Developer HUD page.
[String changes made]: yes, in addition to adding an l10n ID for "reflow threshold", a new l10n ID for "jank threshold" was introduced to indicate the threshold is in milliseconds.

Comment 18

[Russ Nicoletti [:russn]]

Hi Carsten, my apologies for not being more clear. There are two patches for this bug. One is a gaia patch, which is now landed, and the other is a gecko patch, described in comment 12. Can you land the gecko patch?

Comment 19

[Delphine Lebédel [:delphine - Use needinfo]]

Hi - if this lands on 2.5 it will break string freeze by far (since date was Nov. 2nd). Do we really need this in 2.5 at this point?

Comment 20

[Russ Nicoletti [:russn]]

Attachment: reflow-threshold-updated.patch

This is the rebased gecko patch.

Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa36e59a1af

Comment 21

[Russ Nicoletti [:russn]]

Reopening as the gecko patch is not yet landed.

Comment 22

[Russ Nicoletti [:russn]]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #19)
> Hi - if this lands on 2.5 it will break string freeze by far (since date was
> Nov. 2nd). Do we really need this in 2.5 at this point?

It would be very nice to get this into 2.5. I'm wondering if we can consider uplifting the patch to 2.5 without localizing the affected strings. The two strings are in |Settings|Developer|Developer HUD| -- since this page would only be accessed by developers, perhaps it's possible to have these two strings be English only, at least for the 2.5 release?

Comment 23

[Delphine Lebédel [:delphine - Use needinfo]]

Hi Russ - sorry for the delay, wanted to see with my team about this case first. Since you're saying you need this in 2.5, then it should probably just land. We could have hardcoded the string as to not break string freeze - but since some locales do localize the dev settings, we should give them the choice. Voilà, that's it for l10n side insights!

Comment 24

[Tamara Hills [:thills]]

Hi Carsten,

Are you able to checkin the gecko portion of this?

thanks,

tamara

Comment 25

[Carsten Book [:Tomcat]]

(In reply to Tamara Hills [:thills] from comment #24)
> Hi Carsten,
> 
> Are you able to checkin the gecko portion of this?
> 
> thanks,
> 
> tamara

sure, does this need on master (b2g-inbound) as well or just 2.5. for 2.5 this would need approval from mahe

Comment 26

[Tamara Hills [:thills]]

Hi Carsten,

Let's just land this on master first.  I'm not quite sure I understand Delphine's comment 23 yet.  Also, we have not asked for the approval yet.

Thanks,

-tamara

Comment 27

[Mahendra Potharaju [:mahe]]

Comment on attachment 8685115 [details] [review]
[gaia] russnicoletti:bug-1211433 > mozilla-b2g:master

Approved for 2.5 uplift as l10N team is fine with late string landing. 

Thanks

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/eba071d5d5aa

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eba071d5d5aa

Comment 30

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/b631f43ca1e591a48f8cb9e8516ac538d8f21ee8 for 2.5

Comment 31

[Russ Nicoletti [:russn]]

Hi Carsten, thanks for your help getting these patches checked in. Now that the gaia patch has been uplifted to 2.5, the gecko patch also should get uplifted to 2.5. Do you need me to generate a patch from the 2.5 branch or can you use the master patch?

Comment 32

[Carsten Book [:Tomcat]]

(In reply to Russ Nicoletti [:russn] from comment #31)
> Hi Carsten, thanks for your help getting these patches checked in. Now that
> the gaia patch has been uplifted to 2.5, the gecko patch also should get
> uplifted to 2.5. Do you need me to generate a patch from the 2.5 branch or
> can you use the master patch?

oh the gecko patch would need approval too, mahe can you give approval for the gecko patch too ?

Comment 33

[Mahendra Potharaju [:mahe]]

Russ, Can you please give the bug # with Gecko patch?

Comment 34

[Russ Nicoletti [:russn]]

Mahe, this bug has two patches: one is a gaia patch [1], which has been committed to master (comment 16) and to 2.5 (comment 30). The other patch is a gecko patch [2], which has been committed to master (comment 29), but not to 2.5. The approval request in comment 17 applies to both patches since the bug is not fixed until both patches are committed.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8685115
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8689593

Comment 35

[Russ Nicoletti [:russn]]

Hi Mahe, something I may not have made clear from my previous comment. The gaia and gecko patches are dependent on each other. Therefore, there are two choices. Either the gecko patch gets uplifted to 2.5 or the gaia patch that was already uplifted needs to be backed out.

Comment 36

[Mahendra Potharaju [:mahe]]

Oh ok.. Thanks for this update. Wes - Can you please uplift the Gecko patch here? Thanks

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/462016c61fe6