STR: 1. Cause some jank events > 127ms 2. In about:telemetry -> "Thread Hangs", for the Gecko thread, compare # of jank events in the BHR event histogram vs sum of events reported in each BHR stack These numbers should match
In IRC, :jchen mentioned that when accessing histograms from JS, there's an additional bucket prepended to the time histograms. That means that the ranges we were using in the histograms are incorrect. For example, bucket 7 in JS actually represents 64-127ms, rather than 128-255. That means that the way we're interpreting it puts it in the next bucket, which increases the reported hang duration range. The fix would be to add 1 to the index used in the range. I am currently implementing and testing this in statuser, but about:telemetry also needs to be updated.
The BHR histograms are stored differently from other Telemetry histograms, so the issue was just a matter of interpretation. Anthony fixed the extension in: https://github.com/chutten/statuser/commit/8811418c4e57f3b285a20fe071998b258b7ac842 Fix for about:telemetry coming soon
Created attachment 8711398 [details] [diff] [review] about-telemetry-bhr-hists.v2.patch Fix the display of TimeHistograms at about:telemetry (all and only the histograms of the "Thread hangs" section). The JS representation of those histograms remains unaffected, and as such there's no need to modify any other code which relies on this awkward representation.
Comment on attachment 8711398 [details] [diff] [review] about-telemetry-bhr-hists.v2.patch Review of attachment 8711398 [details] [diff] [review]: ----------------------------------------------------------------- One nit ::: toolkit/content/aboutTelemetry.js @@ +1126,5 @@ > * @param aParent Parent element > * @param aName Histogram name > * @param aHgram Histogram information > * @param aOptions Object with render options > * * exponential: bars follow logarithmic scale Need to update the doc here to describe the new param
Created attachment 8711849 [details] [diff] [review] bug1241362.v2.patch Just updating the patch to the actual one which landed (addresses the review comment), carrying r+. Will use this patch for uplift request to 45 once it lands on m-c.
Comment on attachment 8711849 [details] [diff] [review] bug1241362.v2.patch Approval Request Comment [Feature/regressing bug #]: 1241362 [User impact if declined]: Potential incorrect evaluation (interpretation) of hang histograms at about:telemetry - important for the beta 45 e10s experiment. [Describe test coverage new/current, TreeHerder]: None [Risks and why]: Very low risk - only fixes the display of hang histograms at about:telemetry. [String/UUID change made/needed]: None
Comment on attachment 8711849 [details] [diff] [review] bug1241362.v2.patch Taking it to improve the experiment results.