feat: byte efficiency estimates based on average throughput #1536

patrickhulce · 2017-01-25T23:33:36Z

addresses #1469 and several tasks of #1517

Switches the approach used previously from adding up the potential ms of each individual resource and instead estimating the time savings based on the average throughput across all requests and the total bytes saved. The previous approach had similar shortcomings to our current links-blocking-first-paint audit where we would report savings longer than the load of the page (because in reality these assets are loaded in parallel, not serially). This should give a more robust and realistic estimate of how many total milliseconds can be shaved off of the load time.

Before:

After:

brendankenny · 2017-01-26T00:11:17Z

This is very cool.

Instead of living on NetworkRecords, does computeAverageThroughput make sense as a computed gatherer?

Would it also make sense to have an average time per domain? That makes certain questions slightly harder to answer (was the connection to a domain in some interval server limited or was it because there were a bunch of connections open to some other domain), but it clears up other things, like recognizing the use of a fast CDN for static assets while ads on the same page are loaded from a much slower server.

patrickhulce · 2017-01-26T00:28:49Z

Instead of living on NetworkRecords, does computeAverageThroughput make sense as a computed gatherer?

Good point I totally forgot about those :) yes, I'll convert it

Would it also make sense to have an average time per domain?

I actually started with this, but ended up just going back to all requests for a couple reasons:

Until we have network request attribution for all data URI images and stylesheets we'll still need to compute the total average throughput, so start there to keep it simple.
If we accept my wild assumption that the client is typically the throughput bottleneck, the differences between origins wouldn't matter much.
The way this metric gets used in the byte efficiency audits for the time being completely ignores response time and the fact that throughput changes over the duration of the request so the benefit you get from a CDN having low latency and quickly saturating the network wouldn't really be surfaced anyway. (Although that's a good idea for another audit, warning about just long server response times!)

Ideally, we'd have some fancy performance experimentation where we could cutoff downloading after X bytes have been received and really tell you what the savings would be

ebidel · 2017-01-26T18:02:02Z

lighthouse-core/formatters/partials/table.html

@@ -26,6 +26,12 @@
 .table_list tr:hover {
  background-color: #fafafa;
 }
+.table_list td * {


Perf isnt a huge deal on the report, but * is always nasty. Is there something more specific we can use? Right now it's just br, code blocks as children elements. RIght?

Interesting what makes it so expensive? Reflows become a lot slower?

Yea. Selectors match right -> left. So * first matches every DOM node, then filters on tds, etc.

ebidel · 2017-01-26T18:02:51Z

lighthouse-core/formatters/table.js

@@ -78,13 +78,18 @@ class Table extends Formatter {

    const rows = results.map(result => {
      const cols = headingKeys.map(key => {
+        let value = result[key];
+        if (typeof value === 'undefined') {


nit: !(key in value)

doesn't let foo; 'foo' in {foo} === true?

not sure how many undefined vals we get, but will a bunch of '--' cells be too noisy?

I found it super helpful for the CLI to differentiate between missing cells and (totally subjectively) more pleasing in the HTML, but open to other ideas

Sounds like we might need some shared helpers for the code and empty cell formatting. Should we do that here or consolidate later?

Not sure what you mean, which code needs to be shared?

https://github.com/GoogleChrome/lighthouse/pull/1536/files#diff-508b9167df0e380cb39ba29ab7deafa2R98 for example. I could use that over in the flexbox audit to fix the whitespace formatting of the code snippet.

Oh the fix for that is most likely the ~ added to the report Handlebars template, those lines were some more aggressive edge case hunting, but yes we totally should. I'm in favor of getting this PR for consistency between the byte efficiency audits through and then doing a bit of refactor cleanup of the stuff that's starting to be repeated everywhere, if that makes sense to you too?

Makes sense. I couldn't get ~ to work but I might be doing Handlebad.

ebidel · 2017-01-26T18:03:49Z

lighthouse-core/formatters/table.js

          case 'lineCol':
            return `${result.line}:${result.col}`;
          default:
-            return result[key];
+            return String(value);


doesnt handlebars do this for us?

Sure, but the results also get directly passed to the pretty formatter above which is where startsWith handler fails with numbers

ebidel · 2017-01-26T18:21:29Z

lighthouse-core/report/report-generator.js

@@ -132,6 +132,7 @@ class ReportGenerator {
      // XSS, define a renderer that only transforms links and code snippets.
      // All other markdown ad HTML is ignored.
      const renderer = new marked.Renderer();
+      renderer.br = () => '<br>';


are you using <br> anywhere?

yes <space><space>\n in between the url and the content preview for stylesheets

Ah ok. I have a PR coming that introduces <pre> to the markdown renderer. We could also use that for block level code snippets, removing the need for <br> and the css you have. WDYT?

Yeah just not sure how to appropriately target the pre tag then for margin only when its following other text. Also I can't get the pre to generate with triple ticks, it still does code for me after cherry-picking :/

Alright I kinda like this, hacky but with a look of intention :)

just comments

paulirish · 2017-01-26T18:59:42Z

lighthouse-core/audits/dobetterweb/uses-optimized-images.js

-      displayValue = `${Math.round(totalWastedBytes / KB_IN_BYTES)}KB potential savings`;
+      const totalWastedKb = Math.round(totalWastedBytes / KB_IN_BYTES);
+      const totalWastedMs = Math.round(totalWastedBytes / networkThroughput * 1000);
+      displayValue = `${totalWastedKb}KB (~${totalWastedMs}ms) potential savings`;


Because this number is super hand-wavy let's reduce the precision some more. Shall we round to the nearest ten? e.g. 497 => 500 and 432 => 430.

patrickhulce · 2017-01-27T18:56:55Z

I'm not getting off this easy am I? PTAL :)

paulirish · 2017-01-27T20:28:32Z

lighthouse-core/audits/unused-css-rules.js

-  static indexStylesheetsById(styles) {
+  static indexStylesheetsById(styles, networkRecords) {
+    const indexedNetworkRecords = networkRecords
+        .filter(record => /css/.test(record.mimeType))


rather than mimeType we might want to use record.resourceType():
https://github.com/ChromeDevTools/devtools-frontend/blob/b84fc38d5618d27b6a5b50d3948cb31a38488bfa/front_end/common/ResourceType.js#L183

paulirish · 2017-01-27T20:30:40Z

lighthouse-core/audits/unused-css-rules.js

+      const firstRuleStart = preview.indexOf('{');
+      const firstRuleEnd = preview.indexOf('}');
+
+      if (firstRuleStart === -1 || firstRuleEnd === -1


Is there anything from https://github.com/ChromeDevTools/devtools-frontend/blob/b84fc38d5618d27b6a5b50d3948cb31a38488bfa/front_end/gonzales/SCSSParser.js we can reuse

not sure, just curious if we can leave some of the parsing/traversal to another project.

Yeah I tried to use these but because everything is expressed in terms of lines and columns translating to absolute length ended up being even more complicated than this (and didn't really handle leading comments such as those used in every lighthouse file :) ) It also focuses a lot more on rules than rule-sets which made for tricky and generally confusing logic to show the first rule-set with its selector.

Also I would love to just totally kill this at some point in the future by attributing where the inline sheet came from and/or maybe showing a few of the selectors

paulirish · 2017-01-27T20:33:06Z

lighthouse-core/gather/computed/network-throughput.js

+
+    let totalBytes = 0;
+    const timeBoundaries = networkRecords.reduce((boundaries, record) => {
+      if (/^data:/.test(record.url) || record.failed || !record.finished ||


could use record.parsedURL().scheme here if you want.

yessss, this is what I've been looking for.

feat: byte efficiency time savings

672d42f

This was referenced Jan 25, 2017

CLI Error: Cannot read property 'startsWith' of undefined #1537

Closed

fix(table-formatter): account for undefined values #1538

Merged

update smokehouse

9888b67

ebidel previously requested changes Jan 26, 2017

View reviewed changes

paulirish reviewed Jan 26, 2017

View reviewed changes

patrickhulce mentioned this pull request Jan 26, 2017

Move no-old-flexbox to table formatter #1545

Merged

move to computed gatherer

bb34ed6

brendankenny changed the title ~~feat: byte efficiency time savings~~ feat: byte efficiency estimates based on average throughput Jan 26, 2017

feedback

c50337d

paulirish requested changes Jan 27, 2017

View reviewed changes

more feedback

db0f7a7

patrickhulce force-pushed the bytes_response_time branch from 62b7d7e to db0f7a7 Compare January 27, 2017 21:52

patrickhulce added 2 commits January 27, 2017 13:54

Merge remote-tracking branch 'origin/master' into bytes_response_time

9b53223

fix merge tests

a449976

paulirish approved these changes Jan 27, 2017

View reviewed changes

paulirish merged commit aa4e711 into master Jan 27, 2017

paulirish deleted the bytes_response_time branch January 27, 2017 22:53

patrickhulce mentioned this pull request Jan 30, 2017

Audits: Detect byte and time savings from bundled resources #1517

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: byte efficiency estimates based on average throughput #1536

feat: byte efficiency estimates based on average throughput #1536

patrickhulce commented Jan 25, 2017 •

edited

Loading

brendankenny commented Jan 26, 2017 •

edited

Loading

patrickhulce commented Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017

ebidel Jan 26, 2017 •

edited

Loading

patrickhulce Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017 •

edited

Loading

ebidel Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017 •

edited

Loading

ebidel Jan 26, 2017

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017 •

edited

Loading

ebidel Jan 26, 2017

patrickhulce Jan 26, 2017

patrickhulce Jan 26, 2017

paulirish Jan 26, 2017

patrickhulce Jan 26, 2017

patrickhulce commented Jan 27, 2017

paulirish Jan 27, 2017

patrickhulce Jan 27, 2017

paulirish Jan 27, 2017

patrickhulce Jan 27, 2017

patrickhulce Jan 27, 2017

paulirish Jan 27, 2017

paulirish Jan 27, 2017

patrickhulce Jan 27, 2017

feat: byte efficiency estimates based on average throughput #1536

feat: byte efficiency estimates based on average throughput #1536

Conversation

patrickhulce commented Jan 25, 2017 • edited Loading

brendankenny commented Jan 26, 2017 • edited Loading

patrickhulce commented Jan 26, 2017

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

ebidel Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Jan 26, 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

Choose a reason for hiding this comment

patrickhulce commented Jan 27, 2017

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 Jan 25, 2017 •

edited

Loading

brendankenny commented Jan 26, 2017 •

edited

Loading

ebidel Jan 26, 2017 •

edited

Loading

patrickhulce Jan 26, 2017 •

edited

Loading

patrickhulce Jan 26, 2017 •

edited

Loading

patrickhulce Jan 26, 2017 •

edited

Loading