-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new_audit(cache-headers): detects savings from leveraging caching #3531
Conversation
// Ignore assets that have low likelihood for cache miss. | ||
const IGNORE_THRESHOLD_IN_PERCENT = 0.1; | ||
// Discount the wasted bytes by some multiplier to reflect that these savings are only for repeat visits. | ||
const WASTED_BYTES_DISCOUNT_MULTIPLIER = 0.1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the screenshot for why this was necessary (even at 1/10th leverage browser caching reports 2s of savings), 10% chance of repeat visit seems reasonable-ish?
b8fd5f7
to
5b59b98
Compare
PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didnt look at tests yet. some comments so far.
|
||
// Ignore assets that have low likelihood for cache miss. | ||
const IGNORE_THRESHOLD_IN_PERCENT = 0.1; | ||
// Discount the wasted bytes by some multiplier to reflect that these savings are only for repeat visits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// As this savings is only for repeat visits, we discount the savings considerably.
// Basically we assume a 10% chance of repeat visit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @param {number} maxAgeInSeconds | ||
* @return {string} | ||
*/ | ||
static toDurationDisplay(maxAgeInSeconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems good if we move this stuff into lighthouse-core/report/v2/renderer/util.js yah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg, done
const upperDecileIndex = RESOURCE_AGE_IN_HOURS_DECILES.findIndex( | ||
decile => decile >= maxAgeInHours | ||
); | ||
if (upperDecileIndex === 11) return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11
=> RESOURCE_AGE_IN_HOURS_DECILES.length - 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
/** | ||
* Computes the user-specified cache lifetime, 0 if explicit no-cache policy is in effect, and null if not | ||
* user-specified. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you nuke the period at the end here. it breaks autolinkers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
/** | ||
* Computes the percent likelihood that a return visit will be within the cache lifetime, based on | ||
* Chrome UMA stats see the note above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know its been our policy to have consts at the top but in this case i think it hurts readability.. would prefer to have those relevant const just defined here inside of getCacheHitLikelihood.
doing that would avoid pingponging back and forth between the top of the file and down here when reading it.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sure sg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess you can drop "see the note above." now
} | ||
|
||
if (headers.has('expires')) { | ||
const expires = new Date(headers.get('expires')).getTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay for standards that enable this parser to handle the http date format. \o/
return ( | ||
CACHEABLE_STATUS_CODES.has(record.statusCode) && | ||
STATIC_RESOURCE_TYPES.has(record._resourceType) && | ||
!resourceUrl.includes('?') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why excluding all of these? there was voodoo around these not being cached at the proxy level but i dont know of a reason a browser has different policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PSI does it based on the claim that resources with query strings are not heuristically cacheable, seems like a reasonable assumption if the asset has a query string and no explicit cache policy then don't cache it, people do weird changes on gets with query string all the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Innnnteresting. I went and found the history of that line. (took a while).
It goes back to here: pagespeed/page-speed@4c4f031#diff-2478b085708a8d438d5057d0365f067fR384
It originally landed with " This is a debatable policy. " :)
I have a feeling like folk's use of query strings are different in 2017 than 2010, but who knows.
Can we leave some comments here that provide some context?
And perhaps a TODO to explore including these records and see what it tells us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
helpText: | ||
'A well-defined cache policy can speed up repeat visits to your page. ' + | ||
'[Learn more](https://developers.google.com/speed/docs/insights/LeverageBrowserCaching).', | ||
description: 'Leverage browser caching', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leverage browser caching for static assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return artifacts.requestNetworkRecords(devtoolsLogs).then(records => { | ||
const results = []; | ||
for (const record of records) { | ||
if (!CacheHeaders.isCacheableAsset(record)) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are redirects just filtered out in that fn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, only 200, 203, 206 allowed
headers.set(header.name, header.value); | ||
} | ||
|
||
// Ignore assets that have an etag since they will not be re-downloaded as long as they are valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm no etag expert but i am not consistently seeing this described behavior
for example: this (tooootally random) page... https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag the html resource has an etag but refreshing it (various ways) does refetch it and get a 200.
then again on http://music.com/ the logo.png has an etag and no other caching headers and it gets served from memory cache on reloads.
so perhaps the document is handled differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "as they are valid" mean? you mean "match the server's etag"?
this comment could probably afford to break into two lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having an etag and the server actually acting on it and doing the right thing are different things :)
when the if-none-match: <etag>
header is sent the server should reply with a 304 if it matches the resource it's planning on sending, perhaps file an issue for a separate audit that checks for improper server handling of etag'd assets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified the comment a bit
feedback addressed, labels switched PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linearInterpolation
is amazingly good. thank you.
I'm still not sold on showing the "Est. Likelihood of Cache Miss" column, but if we do, I think we should flip it to "Cache Hit".
If we omitted the column i think we'd do more in the helpText.. like:
A well-defined cache policy can speed up repeat visits to your page. Estimated Likelihood of Cache Hit is based off collected Chrome statistics, where the median request stays cached for 12 hours and the X is at Y hours. [Learn more]
Regardless we should just show this to some Lighthouse users and see how they interpret it.
|
||
/** | ||
* Computes the percent likelihood that a return visit will be within the cache lifetime, based on | ||
* Chrome UMA stats see the note above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess you can drop "see the note above." now
// are clearly diminishing returns to cache duration i.e. 6 months is not 2x better than 3 months. | ||
// Based on UMA stats for HttpCache.StaleEntry.Validated.Age, see https://www.desmos.com/calculator/7v0qh1nzvh | ||
// Example: a max-age of 12 hours already covers ~50% of cases, doubling to 24 hours covers ~10% more. | ||
const RESOURCE_AGE_IN_HOURS_DECILES = [0, 0.2, 1, 3, 8, 12, 24, 48, 72, 168, 8760, Infinity]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about adding this guy
console.assert(RESOURCE_AGE_IN_HOURS_DECILES.length === 10, 'deci means 10, yo')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need https://eslint.org/docs/rules/no-console#options to allow assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done with require('assert')
if (cacheControl['no-cache'] || cacheControl['no-store']) return 0; | ||
if (Number.isFinite(cacheControl['max-age'])) return Math.max(cacheControl['max-age'], 0); | ||
} else if ((headers.get('pragma') || '').includes('no-cache')) { | ||
// Pragma can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see tools.ietf.org/html/rfc7234#section-5.4
just want to make it clear this shit is from decades ago. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache hit distribution looks over population of all page loads of all sites, which includes sites visited frequently and sites visited rarely and all sort of other visit patterns, which means that the overall distribution of cache hits might not actually resemble the distribution of cache hits for any particular page. Since the goal is to get more caching out there that's not necessarily a problem because the cache hit rate function is strictly increasing, but it does mean the effect size will often be out of step with what happens on the specific test site (we're comparing their assets' cache lengths to what is essentially the distribution of asset age from the mean website revisit)
We'd also be giving implicit ideal cache lengths (at least if the user sets a goal of reducing the red line). "Good" if cache length is at least 7 days, "Average" if cache length is at least 12 hours, and "Poor" for somewhere below that. If we had to write that in the audit description, I'm not sure if we'd want to commit to those numbers?
Just to be clear, I don't know a better way to present the data here :) It's just that the cache hit probabilities end up being very specific claims when we really just want to provide general guidance motivated with real data.
/** | ||
* @return {number} | ||
*/ | ||
static get PROBABILITY_OF_RETURN_VISIT() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need a getter? (e.g. vs IGNORE_THRESHOLD_IN_PERCENT
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes the tests easier than copy pasting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes the tests easier than copy pasting
but it's not used in a test? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is too 😛
const DISCOUNT_MULTIPLIER = CacheHeadersAudit.PROBABILITY_OF_RETURN_VISIT; |
// Based on UMA stats for HttpCache.StaleEntry.Validated.Age, see https://www.desmos.com/calculator/7v0qh1nzvh | ||
// Example: a max-age of 12 hours already covers ~50% of cases, doubling to 24 hours covers ~10% more. | ||
const RESOURCE_AGE_IN_HOURS_DECILES = [0, 0.2, 1, 3, 8, 12, 24, 48, 72, 168, 8760, Infinity]; | ||
assert.ok(RESOURCE_AGE_IN_HOURS_DECILES.length === 12, '1 for each decile, 1 on each boundary'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's 0th, 10th, 20th, ...100th percentiles, shouldn't it be 11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Infinity
might be for a past the upper bound check (boundary somehow past the 100th percentile :), but could also replace upperDecileIndex === RESOURCE_AGE_IN_HOURS_DECILES.length - 1
check with upperDecileIndex === -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Infinity might be for a past the upper bound check (boundary somehow past the 100th percentile
Yeah because 1 year+ is all basically 100th but doing linear interpolation with Infinity isn't quite fair :) I guess it doesn't really matter that much though if we ignore 90% and up, how about I replace with I take it back let's keep this here and just halve the IGNORE_PROBABILITY value so up to 6 months is flaggedNumber.MAX_VALUE
and nuke the check?
if (upperDecileIndex === 0) return 0; | ||
|
||
// Use the two closest decile points as control points | ||
const upperDecile = RESOURCE_AGE_IN_HOURS_DECILES[upperDecileIndex]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe switch these names? upperDecile
/lowerDecile
would refer to 0, 0.1, 0.2, etc while upperDecileValue
or whatever would be the entry in the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure done
* 3. It does not have a query string. | ||
* | ||
* Ignoring assets with a query string is debatable, PSI considered them non-cacheable with a similar | ||
* caveat. Consider experimenting with this requirement to see what changes. See discussion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be worth counting the assets that pass the other cacheable requirements but have a query string. Possible with an HTTP Archive query but a pain; a lot easier to get $.audits.cache-headers.extendedInfo.value.queryStringCount
or whatever :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed more with paul, and we'll just include query string assets for now, count sgtm
cacheLifetimeInSeconds = cacheLifetimeInSeconds || 0; | ||
|
||
let cacheHitProbability = CacheHeaders.getCacheHitProbability(cacheLifetimeInSeconds); | ||
if (cacheHitProbability >= IGNORE_THRESHOLD_IN_PERCENT) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean Lighthouse is implicitly saying that the "correct" cache length is 7 days since that's the only way to bring this down to 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essentially, it's saying any cache policy >= 7 days has 90% of the benefit so we won't flag it
since they're impact on wastedMs is so low though I'm game to put in the table as long as we're showing cache hit likelihood 92% or whatever :)
// This array contains the hand wavy distribution of the age of a resource in hours at the time of | ||
// cache hit at 0th, 10th, 20th, 30th, etc percentiles. This is used to compute `wastedMs` since there | ||
// are clearly diminishing returns to cache duration i.e. 6 months is not 2x better than 3 months. | ||
// Based on UMA stats for HttpCache.StaleEntry.Validated.Age, see https://www.desmos.com/calculator/7v0qh1nzvh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to look at other cache entry stats too? It seems like this is only stale entries (so biases toward later next visits as non-stale entries would just be loaded and not log here?) and only for assets that qualify for 304 checks
|
||
for (const unitLabel of unitLabels) { | ||
const label = /** @type {string} */ (unitLabel[0]); | ||
const unit = /** @type {number} */ (unitLabel[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it seems like overkill to do the type casting instead of just two parallel arrays and a regular for loop or an array of objects and use Object.keys()
(Object.entries
can't come soon enough) or any number of other approaches :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪 🐑 -> 🍖 🎁 -> 🔱
done
static get meta() { | ||
return { | ||
category: 'Caching', | ||
name: 'cache-headers', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshedding on name? It's not just 'cache-headers' but also a judgement of them. asset-cache-length
? asset-caching-ttl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm if we go by consistency with the other byte efficiency audits they basically fall into either <noun of thing being detected>
or uses-<best practice we're encouraging>
how about...
uncached-assets
low-cache-ttl
uses-caching
uses-cache-headers
uses-long-cache-ttl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses-long-cache-ttl
certainly isn't exactly catchy but describes it well :) I like that since it's not just use, it's (if they're used) that they're long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Spoke with brendan about some of this just now. One argument that seems reasonable to me is... What cache lengths do we recommend? IMO if you completely control the URL your resource is accessed by, then you can always afford 1yr TTL. If you don't (because you're metrics.biz/analytics.js) then you can only commit to X hours TTL. And so if there's only two real cases which get unique TTLs then we shouldn't overcomplicate. |
friendly bump on this :) |
🏏 ...🏏 ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @kdzwinel added header support to the smokehouse server, maybe add a cache header test to the byte efficiency smoke test? :):)
I'm not sure what to do with the cache hit rate. With recent and upcoming work to make our opportunities better reflect reality, the platonic reality these come from doesn't seem particularly useful for any particular site beyond just "you should have longer caching"
wastedMs, | ||
wastedKb, | ||
results, | ||
}, | ||
}, result.extendedInfo), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a comment that this merges in any extendedInfo
provided by the derived audit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/** | ||
* @return {number} | ||
*/ | ||
static get PROBABILITY_OF_RETURN_VISIT() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes the tests easier than copy pasting
but it's not used in a test? :)
static get meta() { | ||
return { | ||
category: 'Caching', | ||
name: 'cache-headers', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses-long-cache-ttl
certainly isn't exactly catchy but describes it well :) I like that since it's not just use, it's (if they're used) that they're long
hasn't actually landed yet, but when it does sg :)
It feels like "Repeat visit" is another target where we'll surface savings and this audit will target that. I'd like to ideally follow the same course of action here I'm pushing on other audits which is "let's not agonize over the savings we surface right now since we have it as a high priority item to redo it all" :) |
@brendankenny do you still have requested changes? |
please 🙏 :) |
The PR looks good to me other than the cache miss column, and nothing has really changed in the discussion since the comment 15 days ago :) Maybe we can talk about this in the Monday meeting so we can work out a consensus on moving forward.
To me this is just an argument for leaving it out now and adding it when we've figured it out :) Why add something that's going to be vague or wrong in the near term and removed in the long term? A middle ground could maybe be drop the cache hit column but still have the overall savings, and maybe call out that it's specifically for a repeat visitor at x days later, whatever x works out to. |
I will update with just a score, not the estimated time savings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
closes #3460