core(estimated-input-latency): use a 5s rolling window #4989

patrickhulce · 2018-04-18T16:52:51Z

chose a 5s window because that's what TTI uses and it gave the right "feel" on several test sites, CNN gets ~1s and is punished harshly, pwa.rocks gets ~17ms, 1s was a bit too small and harsh 10s is forgiving of 1s long tasks which is just too lenient

paulirish · 2018-04-19T20:28:18Z

Is what we're computing here functionally equivalent to

the largest 90pctl EQT value of 5s windows between FMP to end of the trace

patrickhulce · 2018-04-19T20:37:47Z

Is what we're computing here functionally equivalent to
the largest 90pctl EQT value of 5s windows between FMP to end of the trace

yes

brendankenny

what's performance like? We could add an iterative version of getMainThreadTopLevelEventDurations() if it's slow as it's doing a lot of redundant work per window

brendankenny · 2018-04-19T21:34:32Z

lighthouse-core/audits/estimated-input-latency.js

-          'of latency, or less. 10% of the time a user can expect additional latency. If your ' +
-          'latency is higher than 50 ms, users may perceive your app as laggy. ' +
-          '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/estimated-input-latency).',
+      helpText:


possible to revert these? don't appear to have changed :)

brendankenny · 2018-04-19T21:38:23Z

lighthouse-core/audits/estimated-input-latency.js

+      const latencyPercentiles = TracingProcessor.getRiskToResponsiveness(
+        events,
+        startEvt.start,
+        startEvt.start + ROLLING_WINDOW_SIZE


if there's a last argument of [0.9] then no need for the find() below (since dropping the extendedInfo...though if we keep EIL, there's arguably usefulness in keeping other percentiles :)

brendankenny · 2018-04-19T21:39:06Z

lighthouse-core/audits/estimated-input-latency.js

+        'input, in milliseconds. There is a 90% probability that a user encounters this amount ' +
+        'of latency, or less. 10% of the time a user can expect additional latency. If your ' +
+        'latency is higher than 50 ms, users may perceive your app as laggy. ' +
+        '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/estimated-input-latency).',


we might want to tweak this, though

brendankenny · 2018-04-19T21:49:04Z

lighthouse-core/audits/estimated-input-latency.js

@@ -75,8 +89,9 @@ class EstimatedInputLatency extends Audit {
  static audit(artifacts, context) {
    const trace = artifacts.traces[this.DEFAULT_PASS];

-    return artifacts.requestTraceOfTab(trace)
-        .then(traceOfTab => EstimatedInputLatency.calculate(traceOfTab, context));
+    return artifacts


patrickhulce · 2018-04-19T22:04:18Z

what's performance like? We could add an iterative version of getMainThreadTopLevelEventDurations() if it's slow as it's doing a lot of redundant work per window

yeah it could definitely be better (~200ms for a CNN trace), but it's marginally better than our old TTI, and the filter on possible start points helps a lot, was planning on tackling this when adding shared eFID/EIL utils but now that it's potentially DOA maybe we just do that as followup separate

paulirish

Is what we're computing here functionally equivalent to

the largest 90pctl EQT value of 5s windows between FMP to end of the trace

yes

tight. let's add that as a comment somewhere, so its clear how we compare to the std metric.

paulirish · 2018-04-20T00:02:59Z

lighthouse-core/audits/estimated-input-latency.js

-    const ninetieth = latencyPercentiles.find(result => result.percentile === 0.9);
-    const rawValue = parseFloat(ninetieth.time.toFixed(1));
+    const events = TracingProcessor.getMainThreadTopLevelEvents(tabTrace, startTime)
+      .filter(evt => evt.duration >= 1);


since a blank trace gets a value of 16, can't we safely remove any events with a duration < 16?

well a trace with constant 16ms events would technically get a EQT of 24ms since we arbitrarily add 16ms to whatever value we compute, I'm all for doing more filtering here though, who cares about your small events

paulirish · 2018-04-20T00:04:23Z

lighthouse-core/audits/estimated-input-latency.js

+    const events = TracingProcessor.getMainThreadTopLevelEvents(tabTrace, startTime)
+      .filter(evt => evt.duration >= 1);
+
+    const candidateStartEvts = events.filter(evt => evt.duration >= 10);


similarly, can we raise 10 to something like 50? Seems like we'd lose precision in the "you're good" area of scores, but that doesn't seem too bad.

yeah sgtm 👍 much faster too

doesn't that make the max score the PODR?

oh, only filtering the start event times

brendankenny · 2018-04-20T01:14:29Z

if we're only concerned with speed there are easier ways of making this faster than sprinkling filters everywhere :P

patrickhulce · 2018-04-20T15:26:32Z

if we're only concerned with speed there are easier ways of making this faster than sprinkling filters everywhere :P

Speed is not a major concern right now. In the unit test which has 20s of CPU time split between 1000 start times and 10000 small tasks it takes ~40-50ms. Of the 200ms for CNN 180 of those are spent in trace-of-tab, not here. We're not filtering because we absolutely have to, it's more why do useless work. I don't really care either way. 1ms and 10ms don't seem to be a bottleneck speed-wise or have any meaningful impact accuracy-wise.

patrickhulce · 2018-04-20T18:08:54Z

friendly ping for re-review! I took no action on filter thresholds since it seemed to be contentious...

brendankenny

LGTM! 🎢 🪟

paulirish

🎢

…4989)

patrickhulce requested review from brendankenny and paulirish as code owners April 18, 2018 16:52

devtools-bot added the waiting4reviewer label Apr 18, 2018

patrickhulce changed the title ~~core(EIL): convert estimated-input-latency to rolling window~~ core(estimated-input-latency): use a 5s rolling window Apr 18, 2018

patrickhulce added the I/O-Expedite label Apr 19, 2018

brendankenny reviewed Apr 19, 2018

View reviewed changes

patrickhulce added 2 commits April 19, 2018 14:55

core(estimated-input-latency): use a 5s rolling window

0fb39ff

feedback

0a33750

patrickhulce force-pushed the rolling_eil branch from bc96c45 to 0a33750 Compare April 19, 2018 22:05

update golden LHR

31b4877

paulirish reviewed Apr 20, 2018

View reviewed changes

patrickhulce added 2 commits April 20, 2018 08:36

proof

b5bb004

Merge branch 'master' into rolling_eil

baede24

brendankenny approved these changes Apr 20, 2018

View reviewed changes

paulirish approved these changes Apr 20, 2018

View reviewed changes

patrickhulce merged commit 2eaafa0 into master Apr 20, 2018

patrickhulce deleted the rolling_eil branch April 20, 2018 19:59

paulirish removed the waiting4reviewer label Apr 30, 2018

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(estimated-input-latency): use a 5s rolling window (GoogleChrome#…

741d945

…4989)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(estimated-input-latency): use a 5s rolling window #4989

core(estimated-input-latency): use a 5s rolling window #4989

patrickhulce commented Apr 18, 2018

paulirish commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

brendankenny left a comment

brendankenny Apr 19, 2018

patrickhulce Apr 19, 2018

brendankenny Apr 19, 2018

patrickhulce Apr 19, 2018

brendankenny Apr 19, 2018

patrickhulce Apr 19, 2018

brendankenny Apr 19, 2018

patrickhulce Apr 19, 2018

patrickhulce commented Apr 19, 2018 •

edited

Loading

paulirish left a comment

paulirish Apr 20, 2018

patrickhulce Apr 20, 2018

paulirish Apr 20, 2018

patrickhulce Apr 20, 2018

brendankenny Apr 20, 2018

brendankenny Apr 20, 2018

brendankenny commented Apr 20, 2018

patrickhulce commented Apr 20, 2018 •

edited

Loading

patrickhulce commented Apr 20, 2018 •

edited

Loading

brendankenny left a comment

paulirish left a comment

core(estimated-input-latency): use a 5s rolling window #4989

core(estimated-input-latency): use a 5s rolling window #4989

Conversation

patrickhulce commented Apr 18, 2018

paulirish commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 19, 2018 • edited Loading

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 20, 2018

patrickhulce commented Apr 20, 2018 • edited Loading

patrickhulce commented Apr 20, 2018 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce commented Apr 19, 2018 •

edited

Loading

patrickhulce commented Apr 20, 2018 •

edited

Loading

patrickhulce commented Apr 20, 2018 •

edited

Loading