refactor: DRY byte efficiency audits #1635

patrickhulce · 2017-02-04T01:31:15Z

Moves the byte efficiency audits to their own folder
Creates a base audit class for byte efficiency audits to use
Sorts unused byte rows by their total savings in descending order (biggest bang for buck first)
Relaxes the thresholds for failure
- CSS Usage will now only fail if at least 10ms would have been saved by eliminating the unused CSS
- Optimized Images will now fail at 1MB total waste (up from 100k) or a JPEG wastes 25k (up from 1 byte) or an image would have saved 100k from conversion to WebP (up from 50k)
- Responsive Images will only show images that are larger than their displayed size with DPR accounted for and will only fail if one of those images is 25k larger than necessary (changed from 10% of the pixels)

How it looks now

(CSS Usage is green because this was run without throttling and it wouldn't have saved at least 10ms)

Examples of Verge de-noising

Before

After

The smokehouse tests will fail thanks to us ignoring negligible results now. Not sure if we can find an elegant way of lowering the threshold or we have to just use full-size examples in smokehouse.

ebidel · 2017-02-07T06:53:07Z

@patrickhulce this will need a rebase. Rebasing against the latest master will also pick up #1654.

patrickhulce · 2017-02-10T01:07:28Z

PTAL, I'll update smoke tests once there's general agreement on the more lax thresholds.

brendankenny

I'm liking this. My only concern vs if this was just done with a helper function would be what happens when we get a byte efficiency audit that doesn't completely fit this mold (e.g. JS usage...we'd also want to report potential savings on compile/parse time) but I think this refactor leaves us in a good position to handle that, actually.

I haven't thought at all about the new thresholds so others will have to comment there :)

For smokehouse, it would be a shame to lose it for testing these audits. We probably do want to add some assets big enough to hit the thresholds, but replacing everything might be excessive.

One option (tell me if this is crazy) would be to add something to the extended info that doesn't end up in the table, so you'd still have value: {results, tableHeadings}, but also a negligibleResults or whatever property with the filtered out stuff. It would add noise (and size) to the raw results for results that ostensibly don't matter mostly just to make tests happy, so might not be worth it.

brendankenny · 2017-02-10T23:37:11Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

+
+const KB_IN_BYTES = 1024;
+
+class UnusedBytes extends Audit {


can you add a brief class description, basically this is the base class for byte efficiency audits, subclass and override audit_ to write your own?

patrickhulce · 2017-02-11T01:07:39Z

Thanks for braving the scary diff @brendankenny! :)

what happens when we get a byte efficiency audit that doesn't completely fit this mold

I think we can refactor the shared pieces between that audit and these to a helper, but there's so much shared between generating the audit results here it felt like a waste to do anything less.

For smokehouse, it would be a shame to lose it for testing these audits. We probably do want to add some assets big enough to hit the thresholds, but replacing everything might be excessive.

Agreed, what about moving these to a new folder and start matching our smokehouse expectations files to our folder structure? That way there's less noise too. We'd only really introduce a minimum of ~100k total to exercise failure in each of these.

patrickhulce · 2017-02-13T18:57:00Z

Yeah sounds good, I like 'loading unnecessary CSS'

…

On Mon, Feb 13, 2017 at 10:40 AM, Eric Bidelman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lighthouse-core/audits/byte-efficiency/unused-css-rules.js <#1635 (review)> : > @@ -32,7 +29,7 @@ class UnusedCSSRules extends Audit { return { category: 'CSS', name: 'unused-css-rules', - description: 'Uses 90% of its CSS rules', + description: 'Avoids unnecessary CSS', "unnecessary CSS" might not be descriptive enough. What do you think about Avoids including unnecessary CSS or Avoids loading unnecessary CSS to correlate with the theme of reducing bytes on page load? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1635 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACMdEkBGNc0rfk4Qxjyb7dX_C1g9c-NLks5rcKOpgaJpZM4L2_SE> .

ebidel · 2017-02-13T18:46:51Z

lighthouse-core/audits/byte-efficiency/uses-optimized-images.js

@@ -41,7 +40,7 @@ class UsesOptimizedImages extends Audit {
    return {
      category: 'Images',
      name: 'uses-optimized-images',
-      description: 'Has optimized images',
+      description: 'Avoids unoptimized images',


Affirmative descriptions still feel better to me (where we can use them). How about aligning with PSI? They use a singular "Optimize images". WDYT about "Optimizes images" for us?

I switched them because I sympathized with the idea that telling users they did something when that thing doesn't apply to them doesn't make a lot of sense. I like the affirmative descriptions in isolation, but I do think consistency between audits within a category that's all about not doing something (shipping bytes) feels a lot smoother.

I definitely prefer "Optimizes images" to the double negative here, but could use guidance on the CSS affirmative, Uses most of its CSS is pretty lame

telling users they did something when that thing doesn't apply to them

Sort of why I think "Uses optimized images" is better than PSI's "Optimizes images".

The first says: "All the images LH found were optimized. Good job!"
The second says: "You're (actively) optimizing images" ...which as you say, may not be the case.

Any thoughts on CSS wording changes to also be affirmative? I'll give up soon if you don't have any ideas either I'll just live with the mismatch to push this through, haha :P

Yea......hmm. We can stick with "avoids *" for now.

ebidel · 2017-02-13T18:47:04Z

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

@@ -32,7 +29,7 @@ class UnusedCSSRules extends Audit {
    return {
      category: 'CSS',
      name: 'unused-css-rules',
-      description: 'Uses 90% of its CSS rules',
+      description: 'Avoids unnecessary CSS',


"unnecessary CSS" might not be descriptive enough.
What do you think about Avoids including unnecessary CSS or Avoids loading unnecessary CSS to correlate with the theme of reducing bytes on page load?

ebidel · 2017-02-13T18:49:37Z

lighthouse-core/audits/byte-efficiency/uses-optimized-images.js

+        preview: '',
+        url: 'URL',
+        totalKb: 'Original (KB)',
+        wastedKb: 'Savings (KB)',


just a thought: should we put total savings last...as a way to indicate it's representative of the webp/jpeg column that provides the most impact.

+1 I like that idea, also makes the math clear ORIG x PERCENT = SAVINGS

ebidel · 2017-02-13T18:58:58Z

lighthouse-core/audits/byte-efficiency/uses-responsive-images.js

@@ -39,7 +38,7 @@ class UsesResponsiveImages extends Audit {
    return {
      category: 'Images',
      name: 'uses-responsive-images',
-      description: 'Has appropriately sized images',
+      description: 'Avoids oversized images',


"Uses appropriately sized images" or "Loads appropriately sized images"?

patrickhulce · 2017-02-14T19:59:38Z

updated smokehouse to have a new byte efficiency tester since there were no objections, tests should pass now PTAL :)

ebidel

from me

googlebot · 2017-02-14T22:25:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

paulirish

A few smallish things

kill "potential savings %" column and mostly merge it in to last column. So, the last column reads as "Potential Savings" and values are like 186 KB (60%).
Since all the values in these tables include their own units, we don't need the headers to indicate units too. It also makes the headers longer, when we'd prefer to give that horizontal space to the URL.
Title is a little awkward to read. I'd go with "Avoids loading unnecessary CSS: Potential savings of 15KB (~70ms of page load)"

patrickhulce · 2017-02-15T00:29:23Z

with @paulirish's improvements:

paulirish · 2017-02-15T00:42:48Z

love it.

refactor: DRY byte efficiency tests

ed81f34

patrickhulce changed the title ~~refactor: DRY byte efficiency tests~~ refactor: DRY byte efficiency audits Feb 4, 2017

patrickhulce added 5 commits February 9, 2017 14:31

resolve merge conflicts

f43acdf

loosen responsive images conditions

c956725

reduce optimized images aggressiveness

227ff38

consistency with optimized images threshold

81f33fe

more updates

2681d84

brendankenny reviewed Feb 11, 2017

View reviewed changes

patrickhulce mentioned this pull request Feb 13, 2017

Uses 90% of its CSS rules > Lighthouse is very different to DevTools #1712

Closed

brendankenny mentioned this pull request Feb 13, 2017

Image Optimisation - Which Tools to Use? #1709

Closed

add class overview

f0b5763

update unused css description

a2cecf4

ebidel reviewed Feb 13, 2017

View reviewed changes

patrickhulce added 2 commits February 13, 2017 12:07

more feedback

b646421

added smoke tests

08e6697

ebidel approved these changes Feb 14, 2017

View reviewed changes

adjust perf.json

8f93c45

paulirish requested changes Feb 14, 2017

View reviewed changes

pauls feedback

909d013

paulirish approved these changes Feb 15, 2017

View reviewed changes

paulirish merged commit d1b9110 into master Feb 15, 2017

paulirish deleted the dry_byte_efficiency branch February 15, 2017 00:43

brendankenny mentioned this pull request Mar 23, 2023

core: collect fetchpriority for images and link rel preload #14925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: DRY byte efficiency audits #1635

refactor: DRY byte efficiency audits #1635

patrickhulce commented Feb 4, 2017 •

edited

Loading

ebidel commented Feb 7, 2017

patrickhulce commented Feb 10, 2017

brendankenny left a comment

brendankenny Feb 10, 2017

patrickhulce Feb 11, 2017

patrickhulce commented Feb 11, 2017

patrickhulce commented Feb 13, 2017 via email

ebidel Feb 13, 2017

patrickhulce Feb 13, 2017

ebidel Feb 13, 2017

patrickhulce Feb 14, 2017 •

edited

Loading

ebidel Feb 14, 2017

ebidel Feb 13, 2017

ebidel Feb 13, 2017

patrickhulce Feb 13, 2017

ebidel Feb 13, 2017

patrickhulce commented Feb 14, 2017

ebidel left a comment

googlebot commented Feb 14, 2017

paulirish left a comment

patrickhulce commented Feb 15, 2017

paulirish commented Feb 15, 2017

refactor: DRY byte efficiency audits #1635

refactor: DRY byte efficiency audits #1635

Conversation

patrickhulce commented Feb 4, 2017 • edited Loading

How it looks now

Examples of Verge de-noising

ebidel commented Feb 7, 2017

patrickhulce commented Feb 10, 2017

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

patrickhulce commented Feb 11, 2017

patrickhulce commented Feb 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Feb 14, 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 Feb 14, 2017

ebidel left a comment

Choose a reason for hiding this comment

googlebot commented Feb 14, 2017

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce commented Feb 15, 2017

paulirish commented Feb 15, 2017

patrickhulce commented Feb 4, 2017 •

edited

Loading

patrickhulce Feb 14, 2017 •

edited

Loading