Refactor hazardMetric

Refactor hazardMetric

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/80132ac6935a2b537964714fb194ca481e5f14ca

[benjhayden, 2016-05-20 00:03:26]

Patchset #1 (id:1) has been deleted

[benjhayden, 2016-05-20 00:03:30]

Patchset #1 (id:20001) has been deleted

[benjhayden, 2016-05-20 00:03:34]

Patchset #1 (id:40001) has been deleted

[benjhayden, 2016-05-20 00:05:35]

Patchset #1 (id:60001) has been deleted

[benjhayden, 2016-05-20 00:07:54]

Description was changed from

==========
computeHazardForLongTasksInRangeOnThread

BUG=catapult:#2335
==========

to

==========
computeHazardForLongTasksInRangeOnThread

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for
Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335
==========

[benjhayden, 2016-05-23 23:27:22]

Description was changed from

==========
computeHazardForLongTasksInRangeOnThread

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for
Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335
==========

to

==========
Refactor hazardMetric

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for
Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335
==========

[benjhayden, 2016-05-23 23:31:27]

benjhayden@chromium.org changed reviewers:
+ bckenny@chromium.org, paulirish@chromium.org

[benjhayden, 2016-05-23 23:31:28]

PTAL

[paulirish, 2016-05-25 00:13:09]

this mostly looks how i've been thinking. but two questions below.

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (right):

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:37: 300 / MS_PER_S, 100
/ MS_PER_S);
Are these numbers defining the shape of the curve to score individual tasks?
They seem pretty big.

We talk about 50ms being the goal of maximum task length, so i'd expect that
would be our dim-returns point.

and 300 seems big for a 50% score. Here's a random sample of tasks lengths
during load: http://imgur.com/nngelLH

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:137: taskHazardScores,
perceptualBlendSmallerIsBetter);
I've been thinking that this long-task situation is similar to the throughput &
discrepancy calculations we do for smoothness?
I'm curious if that calculation is closer to the data.

[bckenny2, 2016-05-25 00:37:00]

bckenny@google.com changed reviewers:
+ bckenny@google.com

[bckenny2, 2016-05-25 00:37:01]

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (right):

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:137: taskHazardScores,
perceptualBlendSmallerIsBetter);
I was curious about this as well. Does the weighted mean have any meaning in
itself? Or just that it's a usefully correlated value? It seems like the
distribution of task scores is what you're more interested in, so percentiles
(90% of the time you'll hit a task of at least score x) or a fancier smoothness
metric that paulirish mentions above.

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:182: __proto__:
Function.prototype
this might just be a catapult-ism I don't know the reason for (since this is
pretty common in existing code), but what does this do here? When functions are
declared as above their __proto__ is already Function.prototype

[benjhayden, 2016-05-25 18:01:24]

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (right):

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:37: 300 / MS_PER_S, 100
/ MS_PER_S);
On 2016/05/25 at 00:13:09, paulirish wrote:
> Are these numbers defining the shape of the curve to score individual tasks?
They seem pretty big.
> 
> We talk about 50ms being the goal of maximum task length, so i'd expect that
would be our dim-returns point.

Tim had reasoned that we can't give less than a perfect score to 50ms tasks
because that is the recommendation. If a site follows our recommendations and we
don't give them a perfect score, then we might look confused.
But we can do that if you want.

> 
> and 300 seems big for a 50% score. Here's a random sample of tasks lengths
during load: http://imgur.com/nngelLH

How does 100 sound?

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:137: taskHazardScores,
perceptualBlendSmallerIsBetter);
On 2016/05/25 at 00:13:09, paulirish wrote:
> I've been thinking that this long-task situation is similar to the throughput
& discrepancy calculations we do for smoothness?
> I'm curious if that calculation is closer to the data.

That might be a good idea, I don't know if it's been discussed enough yet.

When I proposed that to Tim and Sami in that VC about time-to-interactive, it
seemed to me that they agreed that tasks are not quite like animation frames. As
long as there's an opportunity for an input to slip in between two tasks, then
the scheduler will prioritize the input. So it seems to me to make more sense to
think of each task separately, and consider a different speculative response for
each task.

It might make sense to apply animation theory when we need to blend the per-task
risks. A site with consistently long tasks might be said to have low scheduler
throughput. This point of view might argue for taking an unweighted mean of the
per-task risk, since animation throughput is computed as an unweighted mean.

There's also an argument to be made for computing animation metrics using the
perceptualBlend of scores statistic that hazard uses now instead of
throughput+smoothness. My understanding is that nobody is thrilled with the
current smoothness metrics. To briefly summarize the argument, we could try to
say that we should compute a percentile/score for each frame of an animation,
and perceptualBlend the scores using the peak-end rule, since users are more
likely to remember the bad frames than the good ones. The weighted mean would
let a few bad frames really bring down the overall score, even if there are
hundreds of good frames.
I think brianderson might also be exploring animation statistics. He might have
some ideas.

I really don't know what the right theory is for either hazard or animation, or
even if they should use the same statistics.

Would it be ok if this CL continued with the current statistics while we
continue discussing better ones?

Maybe followup CLs can add experimental metrics to explore all combinations of
the [long tasks, animations] x [perceptualBlend percentiles, unweighted mean of
percentiles, throughput+discrepancy] matrix?

> Does the weighted mean have any meaning in itself?

The main idea behind weighted mean is that each part of the sum (whether those
parts are responsiveness scores or long tasks) should affect the sum, and that
worse parts should affect the sum more than good parts due to the peak-end rule.
Humans are more likely to remember the worst parts of an experience than the
good parts. If there are a hundred Responses in a trace, and most of them were
pretty good but a few of them were really bad, then a human is going to
primarily remember the really bad ones, so they should bring the overall score
down more than the good scores bring the overall score up.

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:182: __proto__:
Function.prototype
On 2016/05/25 at 00:37:00, bckenny2 wrote:
> this might just be a catapult-ism I don't know the reason for (since this is
pretty common in existing code), but what does this do here? When functions are
declared as above their __proto__ is already Function.prototype

Yeah, this is definitely a wart. It's due to how the extension_registry_base
enforces the mandatoryBaseClass, which the MetricRegistry sets to Function.

[paulirish, 2016-05-25 21:34:49]

On 2016/05/25 at 18:01:24, benjhayden wrote:
>
https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
> File tracing/tracing/metrics/system_health/hazard_metric.html (right):
> 
>
https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
> tracing/tracing/metrics/system_health/hazard_metric.html:37: 300 / MS_PER_S,
100 / MS_PER_S);
> On 2016/05/25 at 00:13:09, paulirish wrote:
> > Are these numbers defining the shape of the curve to score individual tasks?
They seem pretty big.
> > 
> > We talk about 50ms being the goal of maximum task length, so i'd expect that
would be our dim-returns point.
> 
> Tim had reasoned that we can't give less than a perfect score to 50ms tasks
because that is the recommendation. If a site follows our recommendations and we
don't give them a perfect score, then we might look confused.
> But we can do that if you want.

all good. 50ms is a score of .999, so yeah I think we're good. :)

> > and 300 seems big for a 50% score. Here's a random sample of tasks lengths
during load: http://imgur.com/nngelLH
> 
> How does 100 sound?

100 sgtm.

> 
>
https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
> tracing/tracing/metrics/system_health/hazard_metric.html:137:
taskHazardScores, perceptualBlendSmallerIsBetter);
> On 2016/05/25 at 00:13:09, paulirish wrote:
> > I've been thinking that this long-task situation is similar to the
throughput & discrepancy calculations we do for smoothness?
> > I'm curious if that calculation is closer to the data.
> 
> That might be a good idea, I don't know if it's been discussed enough yet.
> ...
> 
> Would it be ok if this CL continued with the current statistics while we
continue discussing better ones?

YUP!

> 
> Maybe followup CLs can add experimental metrics to explore all combinations of
the [long tasks, animations] x [perceptualBlend percentiles, unweighted mean of
percentiles, throughput+discrepancy] matrix?

Love that idea. Try a few things, evaluate against what it feels like. (same
thing is happening in speed index right now)

lgtm from my side % the change to 100ms

[bckenny2, 2016-05-25 22:21:43]

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/hazard_metric.html (right):

https://codereview.chromium.org/1992303003/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/hazard_metric.html:182: __proto__:
Function.prototype
On 2016/05/25 at 18:01:23, benjhayden_chromium wrote:
> On 2016/05/25 at 00:37:00, bckenny2 wrote:
> > this might just be a catapult-ism I don't know the reason for (since this is
pretty common in existing code), but what does this do here? When functions are
declared as above their __proto__ is already Function.prototype
> 
> Yeah, this is definitely a wart. It's due to how the extension_registry_base
enforces the mandatoryBaseClass, which the MetricRegistry sets to Function.

should this be dropped since you fixed the wart?

[paulirish, 2016-06-02 07:38:57]

hows this going? Lots of conversations around this metric recently. I'd love to
get it landed so we can start trying it out.

[bckenny2, 2016-06-02 19:15:14]

On 2016/06/02 at 07:38:57, paulirish wrote:
> hows this going? Lots of conversations around this metric recently. I'd love
to get it landed so we can start trying it out.

last question from me above, but otherwise LGTM

[bckenny2, 2016-06-02 19:15:18]

lgtm

[benjhayden, 2016-06-03 16:18:12]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-06-03 16:18:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulirish@chromium.org, bckenny@google.com
Link to the patchset: https://codereview.chromium.org/1992303003/#ps260001
(title: "fixing")

[commit-bot: I haz the power, 2016-06-03 16:18:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992303003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992303003/260001

[commit-bot: I haz the power, 2016-06-03 16:42:26]

Description was changed from

==========
Refactor hazardMetric

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for
Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335
==========

to

==========
Refactor hazardMetric

Currently, hazardMetric computes a single overall score with diagnostic values.
Diagnostic values are being deprecated.
This CL removes the diagnostic values and adds a Numeric histogram of the raw
long task durations.
This CL also refactors the implementation in order to be more useful for
Lighthouse,
and uses LogNormalDistribution instead of the old getInterpolatedCountAt.

BUG=catapult:#2335

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-06-03 16:42:28]

Committed patchset #10 (id:260001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...