fixes for TTFB #2231

wardpeet · 2017-05-11T19:07:58Z

I changed a few things in this PR I moved the critical requests to a new computed gatherer and moved a small part to the chain gatherer. I tried rewriting all tests for this as well.

I also added a fix to check to check if _timing is available on the network record so ttfb does not fail.

paulirish · 2017-05-15T03:48:21Z

lighthouse-core/audits/time-to-first-byte.js

+
+    return artifacts.requestNetworkRecords(devtoolsLogs).then(
+      networkRecords => artifacts.requestCriticalRequests(networkRecords)
+        .then(criticalRequests => [


you can use promise.all to avoid this little dance here. a number of other audits employ the pattern.

Are you referring to something like this? Or am I missing something?

return Promsise.all([ networkRecords, artifacts.requestCriticalRequests(networkRecords) ])

yup

return Promise.all([ artifacts.requestNetworkRecords(devtoolsLogs), artifacts.requestCriticalRequests(networkRecords), ]).then(([networkRecords, criticalRequests]) => ...

wardpeet · 2017-05-29T20:16:22Z

@paulirish @patrickhulce should I try to run smoke tests on this audit?

brendankenny · 2017-08-14T21:47:06Z

@wardpeet no rush on this cause this is your reviewers' fault, but can you rebase when you get a chance? Things have changed so much since this was opened that it's hard to tell what's a real change and what's not :):) ❤️

wardpeet · 2017-08-25T21:45:01Z

@brendankenny done :) hopefully travis doesn't fail (fingers crossed)
I have to do some extra runs just to make sure it works 100% (but at least tests aren't failing)

patrickhulce · 2017-08-25T21:53:40Z

lighthouse-core/config/default.js

@@ -227,12 +229,13 @@ module.exports = {
        {id: 'estimated-input-latency', weight: 1, group: 'perf-metric'},
        {id: 'link-blocking-first-paint', weight: 0, group: 'perf-hint'},
        {id: 'script-blocking-first-paint', weight: 0, group: 'perf-hint'},
-        {id: 'uses-responsive-images', weight: 0, group: 'perf-hint'},
+        // {"id": "unused-css-rules", "weight": 0},


you can revert these

patrickhulce · 2017-08-25T21:54:07Z

lighthouse-core/audits/time-to-first-byte.js

+      artifacts.requestNetworkRecords(devtoolsLogs),
+      artifacts.requestCriticalRequests(devtoolsLogs)
+    ])
+      .then(([criticalRequests, networkRecords]) => {


I think these are flipped, shouldn't it be [networkRecords, criticalRequests]?

patrickhulce · 2017-08-25T21:56:00Z

lighthouse-core/gather/computed/critical-requests.js

+    };
+  }
+
+  static extractRequests(networkRecords) {


This split seems weird to me, shouldn't this belong with the *Chains computed artifact and this class just filters to the requests? It's been a while, but I thought we decided to split based on the filtering/chain creation.

The world has also changed quite a bit by now with fast mode introducing a new full-blown dependency graph, instead of all this work we could probably just get away with using the isCritical static method on the critical request chains class

I still need to drop requests that start from a low priority request. Or am I wrong?
The critical requests artifact gives a flat tree of requests and the critical-request-chain artifact creates a tree

reference: (if this is outdated info, I can change my approach)
#2192 (comment)

if I look at #931 it looks like we just want all critical resources so maybe it makes sense to just look at the priority and not an image and do more logic in the critical-request-chains gatherer

Oh right right. That really puts a wrinkle in wanting to split these then since you need to construct the chain even just to determine the list of requests. I suppose this is fine for now. We'll probably end up migrating this to use the dependency graph from #2720 at some point.

OK @wardpeet talked it over with @paulirish and @brendankenny, and we think we should stick to measuring the TTFB for just the primary HTML request to keep it simple and avoid messing with the critical request chains gatherers for now.

We're about to get better infrastructure for dealing with the graph of requests that should make this much easier and we can revisit it then. So sorry for the churn here, and my bad for not checking we really needed all the critical requests before suggesting the split 😅 You're a champ 💪

yes, you will still need to construct the chain but critical resources would give a flat request tree so it's easier to model other audits/gatherers around it.

I will revert the changes and only measure ttfb on primary request

patrickhulce · 2017-08-29T17:07:34Z

lighthouse-core/audits/time-to-first-byte.js

+const Util = require('../report/v2/renderer/util');
+const URL = require('../lib/url-shim');
+
+const TTFB_THRESHOLD = 200;


Are we intending to run this audit only on unthrottled runs? Won't it flag all resources with the default throttling settings on?

I believe I don't understand this comment. TTFB should be below 200, this is merely a constant that I use in the audit.

Well if throttling is on won't TTFB always be at least as high as our throttling value?

that's a really good question. I believer you are right. Should we higher the max ttfb? I believe @paulirish suggested using 200ms

I'll do some audits based on throttling and no throttling

When throttling is the TTFB that we observe should be minimum 150ms. And if there was any server latency, that is just absorbed in there.

So if the observed TTFB is > 151ms, then ALL of that was real network latency + server response.

That much sound correct to yall?

Then, 200 is a number we can evaluate separate from our throttling and we just have to decide when its slow enough for us to care.
PSI uses either 200 or 400, I forget.

Ah based on https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/emulation.js#L47 our set latency should be 562ms. So whatever i just say but s/150/562/ :)

well... then our threshold has to be like 600ms
eesh.

wardpeet · 2017-09-05T20:01:21Z

Should be fixed and using 600ish

patrickhulce

so much simpler this is great!

patrickhulce · 2017-09-05T20:25:11Z

lighthouse-core/audits/time-to-first-byte.js

+const Util = require('../report/v2/renderer/util');
+
+const TTFB_THRESHOLD = 600;
+const TTFB_THRESHOLD_BUFFER = 15;


at this point I'm not sure splitting buffer is worth it when our threshold is 600, haha

patrickhulce · 2017-09-05T20:27:09Z

lighthouse-core/audits/time-to-first-byte.js

+    return {
+      category: 'Performance',
+      name: 'time-to-first-byte',
+      description: 'Time To First Byte (TTFB)',


this description nicely summarizes what we're really doing but it feels weird along with the other perf opportunities, maybe we should convert it into something like "Keep server response times low" and mention TTFB in the description or debugString?

I know @paulirish doesn't like "response time" though ;) open to whatever doesn't make it feel out of place there.

Keep server response times low (TTFB)?

wardpeet · 2017-09-12T15:54:09Z

ping @patrickhulce

patrickhulce

LGTM % nit 🎉

patrickhulce · 2017-09-12T16:39:28Z

lighthouse-core/audits/time-to-first-byte.js

+
+        if (!passed) {
+          debugString = `Root document (${URL.getURLDisplayName(finalUrl)}) went over` +
+            ` the ${thresholdDisplay} threshold`;


if it's over the threshold then we can be confident it took at least that amount of time, so let's report the actual ttfb here e.g. Root document request took X ms

something like Root document took ${Util.formatMilliseconds(ttfb, 1)} ms to get the first byte. or keep it short as you stated above

yours sgtm too 👍

brendankenny

Successfully emerges from the distant past! :)

wardpeet force-pushed the feature/audit-ttfb branch from b1b2161 to 80d68c6 Compare May 12, 2017 05:00

wardpeet mentioned this pull request May 12, 2017

TTFB Audit followup #2192

Closed

wardpeet force-pushed the feature/audit-ttfb branch from 80d68c6 to 1108575 Compare May 12, 2017 19:12

paulirish reviewed May 18, 2017

View reviewed changes

wardpeet force-pushed the feature/audit-ttfb branch from 1108575 to e0d305f Compare May 23, 2017 21:21

wardpeet added 5 commits August 25, 2017 22:33

Rename time to first byte

830c4a1

Change critical request gatherer

28524ca

Review changes

7018e81

Review changes

c1425dd

Fixes for ttfb after rebase

830037c

wardpeet force-pushed the feature/audit-ttfb branch from e0d305f to 830037c Compare August 25, 2017 21:44

Fix for promise rewrite

afde577

patrickhulce reviewed Aug 25, 2017

View reviewed changes

Revert default.js

efcf5c1

wardpeet force-pushed the feature/audit-ttfb branch from 52b0429 to efcf5c1 Compare August 28, 2017 20:19

patrickhulce reviewed Aug 29, 2017

View reviewed changes

wardpeet added 2 commits August 30, 2017 18:04

Only check ttfb on finalUrl

45171cd

Fix audits time to first byte

56dab1a

patrickhulce reviewed Sep 5, 2017

View reviewed changes

Review changes

f053799

patrickhulce approved these changes Sep 12, 2017

View reviewed changes

wardpeet added 2 commits September 12, 2017 19:51

Review changes

ab8b571

Fixes eslint

8b06a37

brendankenny approved these changes Sep 12, 2017

View reviewed changes

brendankenny merged commit 0c2e69e into GoogleChrome:master Sep 12, 2017

brendankenny mentioned this pull request Sep 12, 2017

test(smokehouse): add TTFB smoke test #3310

Merged

wardpeet deleted the feature/audit-ttfb branch September 12, 2017 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes for TTFB #2231

fixes for TTFB #2231

wardpeet commented May 11, 2017

paulirish May 15, 2017

wardpeet May 18, 2017 •

edited

Loading

patrickhulce May 18, 2017

wardpeet commented May 29, 2017

brendankenny commented Aug 14, 2017

wardpeet commented Aug 25, 2017

patrickhulce Aug 25, 2017

patrickhulce Aug 25, 2017

patrickhulce Aug 25, 2017

wardpeet Aug 28, 2017 •

edited

Loading

wardpeet Aug 29, 2017

patrickhulce Aug 29, 2017 •

edited

Loading

patrickhulce Aug 30, 2017

wardpeet Aug 30, 2017

patrickhulce Aug 29, 2017

wardpeet Aug 30, 2017

patrickhulce Aug 30, 2017 •

edited

Loading

wardpeet Sep 1, 2017 •

edited

Loading

paulirish Sep 1, 2017

paulirish Sep 1, 2017

wardpeet commented Sep 5, 2017

patrickhulce left a comment

patrickhulce Sep 5, 2017

patrickhulce Sep 5, 2017

wardpeet Sep 5, 2017

wardpeet commented Sep 12, 2017

patrickhulce left a comment

patrickhulce Sep 12, 2017

wardpeet Sep 12, 2017

patrickhulce Sep 12, 2017

brendankenny left a comment

fixes for TTFB #2231

fixes for TTFB #2231

Conversation

wardpeet commented May 11, 2017

Choose a reason for hiding this comment

wardpeet May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented May 29, 2017

brendankenny commented Aug 14, 2017

wardpeet commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Aug 29, 2017 • edited Loading

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 Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

wardpeet Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Sep 5, 2017

patrickhulce 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

wardpeet commented Sep 12, 2017

patrickhulce 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

brendankenny left a comment

Choose a reason for hiding this comment

wardpeet May 18, 2017 •

edited

Loading

wardpeet Aug 28, 2017 •

edited

Loading

patrickhulce Aug 29, 2017 •

edited

Loading

patrickhulce Aug 30, 2017 •

edited

Loading

wardpeet Sep 1, 2017 •

edited

Loading