core(uses-long-cache-ttl): ignore private, must-validate, no-cache #6835

connorjclark · 2018-12-18T16:32:17Z

lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js

patrickhulce · 2018-12-19T01:30:23Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

@@ -202,6 +202,12 @@ class CacheHeaders extends Audit {
          cacheControl
        );

+        // Ignore assets where policy implies they should not be cached long periods
+        if (cacheControl &&
+          (cacheControl['must-validate'] || cacheControl['no-cache'] || cacheControl['private'])) {


Can we figure out how to consolidate some of this with the logic in computeCacheLifetimeInSeconds in L115? right now these type of checks are in there to return a lifetime of 0

maybe? these cache controls could still have ttls, so it'd make computeCacheLifetimeInSeconds a bit of a liar. I get that if we have this method return 0 for all cases we want to ignore, things work. which is cool, I'm just arguing semantics I suppose.

it doesn't have to be moving this there, we can extract the L115 part too (and probably should it's not the most obvious right now). I'm just advocating for collecting the special cases we handle so it's clear what's going on.

Done. How's that?

…cache

connorjclark · 2019-01-09T01:15:39Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

-          cacheControl,
+          // Include cacheControl in results, but cast as any so table types
+          // are happy. cacheControl is not shown in the table so this is OK.
+          cacheControl: /** @type {any} */ (cacheControl),


Fine with me! But I've never really been known as the typecheck police 😉

summoning @brendankenny

cacheControl is not shown in the table so this is OK.

It's not ok, why use a table if you don't want it to be a table, but it's not your fault that you're inheriting this :)

Will you add a TODO(bckenny): fix DetailsItem to the comment?

patrickhulce

thanks @hoten!! this is much, much clearer what's going on now IMO 👍

patrickhulce · 2019-01-09T04:47:03Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+   */
+  static shouldProcessRecord(headers, cacheControl, cacheLifetimeInSeconds, cacheHitProbability) {
+    // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
+    if ((headers.get('pragma') || '').includes('no-cache')) {


doesn't this have to be only if cache-control is not set? or was that wrong?

I didn't mean to remove that. added back.

patrickhulce · 2019-01-09T04:48:16Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

-    } else if ((headers.get('pragma') || '').includes('no-cache')) {
-      // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
-      return 0;
+    if (cacheControl && cacheControl['max-age'] !== undefined) {


we've lost Number.isFinite(maxAge), do we know where that path goes? did it never matter before?

yeah I suppose, if the cache control value for max-age were 111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 it would get parsed as Infinity. I'd like to remove the clamping going on in this fn, so wb moving the finite check to the "shouldProcess" bit?

Oh, just saw that while +Infinity passes the cacheLifetimeInSeconds check, it would fail at the cacheHitProbability check. This is not true for -Infinity. So let's add a call to isFinite to the cacheLifetimeInSeconds check.

patrickhulce · 2019-01-09T04:53:10Z

types/parse-cache-control/index.d.ts

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+declare module 'parse-cache-control' {


👏 👏 👏

patrickhulce · 2019-01-09T04:54:12Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

-          cacheControl,
+          // Include cacheControl in results, but cast as any so table types
+          // are happy. cacheControl is not shown in the table so this is OK.
+          cacheControl: /** @type {any} */ (cacheControl),


Fine with me! But I've never really been known as the typecheck police 😉

patrickhulce · 2019-01-09T04:55:37Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+      return false;
+    }
+
+    if (cacheLifetimeInSeconds !== null && cacheLifetimeInSeconds <= 0) return false;


can we get a few comments explaining these branchs' intention so we don't get confused later again?

maybe...

// Ignore assets whose cache lifetime is explicitly 0 or negative ... // Ignore assets whose cache lifetime is already high enough

?

patrickhulce · 2019-01-09T05:02:55Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+
+    if (cacheLifetimeInSeconds !== null && cacheLifetimeInSeconds <= 0) return false;
+
+    if (cacheHitProbability > IGNORE_THRESHOLD_IN_PERCENT) return false;


I'm wondering if we should just move the cacheHitProbability calculation to after this shouldProcessRecord and switch the ignore threshold to the equivalent time.

That might make it easier to figure out what's going on....

switch the ignore threshold to the equivalent time

I don't understand what you mean by this.

cacheLifetimeInSeconds gets converted into a cacheHitProbability

instead of ignoring records with a cacheHitProbability above 92.5% what if we ignored records with cacheLifetimeInSeconds above 2 weeks (should be the equivalent of 92.5%)

I think I agree with with what @patrickhulce is saying here (if I'm understanding what he's saying :).

I could be making an opposite suggestion to what @patrickhulce has suggested before, but to me there are two different sets of checks in here:

things that we shouldn't be checking for caching suitability (for cache rule reasons) and

things that shouldn't be displaying in the report (for insufficient data or they don't meet an interestingness threshold).

Combining them disrupts the narrative of the code (why calculate cache lifetime of something when we should be ignoring it? etc)

One way audit() might read more easily is kind of like it was before but with this new method incorporated
shouldProcessRecord would just be checking cache checking suitability. Then

for each record gather headers in loop const cacheControl = parseCacheControl(headers.get('cache-control')); if (!this.shouldProcessRecord(headers, cacheControl) continue; cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(headers, cacheControl) if (cacheLifetimeInSeconds === useless) continue; const cacheHitProbability = CacheHeaders.getCacheHitProbability(cacheLifetimeInSeconds) if (cacheHitProbability > useless) continue;

WDYT?

I could be making an opposite suggestion to what @patrickhulce has suggested before, but to me there are two different sets of checks in here:

I didn't do a great job of clearly saying what I was lookin for before (sorry @hoten !!) so it certainly could have been interpreted as opposite to that :)

My primary concern is not sprinkling around the different cases where things get ignored for the same category of reason, before this we checked private somewhere different from no-cache which are both ignored because of the signal in the cache-control directive.

I think it's relatively difficult to segment shouldProcessRecord and computeCacheLifetime as cleanly as you've done in that example @brendankenny because there are some interdependencies, but if everyone's happy with some duplicated logic and think it's easy to read, I'm onboard.

IMO the fact that we're dealing with some mixture of pragma, expires, and cache-control all at the same time also complicates this, but I guess those two other cases are pretty simple.

understood. I ran a bit too much with the idea of only using one continue ;)

connorjclark · 2019-01-09T18:02:43Z

lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js

+  it('ignores nonpositive and nonfinite max-age headers', () => {
+    const infinityMaxAgeStringValue = '1'.repeat(400);
+    assert.equal(Number.parseInt(infinityMaxAgeStringValue), Infinity);
+    const networkRecords = [


so, we wish to ignore all of these records right?

FYI max-age=blahblah would not be ignored.

Shouldn't something silly like max-age=-anything be surfaced? or is that out of scope for this audit

what did the spec say for cacheControl? for expires it was anything invalid is treated as no cache lifetime, so if cache-control is the same we should probably surface them as having no caching?

brendankenny · 2019-01-09T23:23:29Z

Would really love a PR summary on things like this where parsing the conversation in the original issue takes way too long :P

brendankenny · 2019-01-09T23:29:48Z

types/parse-cache-control/index.d.ts

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+declare module 'parse-cache-control' {


brendankenny · 2019-01-09T23:30:36Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

   * @param {Map<string, string>} headers
-   * @param {{'no-cache'?: boolean,'no-store'?: boolean, 'max-age'?: number}} cacheControl Follows the potential settings of cache-control, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
+   * @param {ReturnType<import('parse-cache-control')>} cacheControl


nit: ReturnType<typeof parseCacheControl> reads a little cleaner to me (since it's clear it's from the same, already imported thing)

brendankenny · 2019-01-09T23:33:16Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

-   * 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
-   *
+   * Return max-age if defined, and null if not


also falls back to expires?

brendankenny · 2019-01-09T23:57:28Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+
+    if (cacheLifetimeInSeconds !== null && cacheLifetimeInSeconds <= 0) return false;
+
+    if (cacheHitProbability > IGNORE_THRESHOLD_IN_PERCENT) return false;


I think I agree with with what @patrickhulce is saying here (if I'm understanding what he's saying :).

I could be making an opposite suggestion to what @patrickhulce has suggested before, but to me there are two different sets of checks in here:

things that we shouldn't be checking for caching suitability (for cache rule reasons) and

things that shouldn't be displaying in the report (for insufficient data or they don't meet an interestingness threshold).

Combining them disrupts the narrative of the code (why calculate cache lifetime of something when we should be ignoring it? etc)

One way audit() might read more easily is kind of like it was before but with this new method incorporated
shouldProcessRecord would just be checking cache checking suitability. Then

for each record gather headers in loop const cacheControl = parseCacheControl(headers.get('cache-control')); if (!this.shouldProcessRecord(headers, cacheControl) continue; cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(headers, cacheControl) if (cacheLifetimeInSeconds === useless) continue; const cacheHitProbability = CacheHeaders.getCacheHitProbability(cacheLifetimeInSeconds) if (cacheHitProbability > useless) continue;

WDYT?

brendankenny · 2019-01-10T00:04:13Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

-          cacheControl,
+          // Include cacheControl in results, but cast as any so table types
+          // are happy. cacheControl is not shown in the table so this is OK.
+          cacheControl: /** @type {any} */ (cacheControl),


cacheControl is not shown in the table so this is OK.

It's not ok, why use a table if you don't want it to be a table, but it's not your fault that you're inheriting this :)

Will you add a TODO(bckenny): fix DetailsItem to the comment?

connorjclark · 2019-01-15T23:40:32Z

anything more?

brendankenny

a couple of comment nits, but otherwise LGTM!

@patrickhulce for correctness check, though :P

brendankenny · 2019-01-17T00:32:38Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

@@ -169,6 +160,30 @@ class CacheHeaders extends Audit {
    );
  }

+  /**
+   * @param {Map<string, string>} headers


can you add a jsdoc description to this? Don't want to be redundant with the comments below, just a general category for what would disqualify a record

brendankenny · 2019-01-17T00:33:04Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+   * @param {ReturnType<typeof parseCacheControl>} cacheControl
+   * @returns {boolean}
+   */
+  static shouldProcessRecord(headers, cacheControl) {


should we rename shouldSkipRecord or something since it's only used negated?

brendankenny · 2019-01-17T00:34:31Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js


-        // Ignore assets with an explicit no-cache policy
-        if (cacheLifetimeInSeconds === 0) continue;
+        // Ignore if cacheLifetimeInSeconds is defined and a nonpositive number.


the is defined throws me for a loop when reading this. Ignore if cacheLifetimeInSeconds is a nonpositive number seems sufficient and is still a true description :)

…cache

patrickhulce

LGTM!

core(uses-lonng-cache-ttl): ignore private, must-validate, no-cache (#…

621556c

…6140)

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners December 18, 2018 16:32

patrickhulce reviewed Dec 19, 2018

View reviewed changes

stress parsing in test

5b2e91e

patrickhulce added the waiting4committer label Jan 8, 2019

connorjclark added 3 commits January 8, 2019 15:54

Merge remote-tracking branch 'origin/master' into issue-6140-private-…

976675c

…cache

consolidate logic for skipping records

acd3d67

correct return type

3764974

connorjclark commented Jan 9, 2019

View reviewed changes

patrickhulce reviewed Jan 9, 2019

View reviewed changes

cr changes

e8e0c55

connorjclark commented Jan 9, 2019

View reviewed changes

brendankenny reviewed Jan 10, 2019

View reviewed changes

split up some concerns re: skipping records

162e119

brendankenny approved these changes Jan 17, 2019

View reviewed changes

brendankenny removed the waiting4committer label Jan 17, 2019

connorjclark added 2 commits January 17, 2019 09:57

add comments, negate

5c50509

Merge remote-tracking branch 'origin/master' into issue-6140-private-…

5b9f023

…cache

patrickhulce approved these changes Jan 17, 2019

View reviewed changes

paulirish merged commit 37a68a0 into master Jan 17, 2019

paulirish deleted the issue-6140-private-cache branch January 17, 2019 20:16


		if (cacheLifetimeInSeconds !== null && cacheLifetimeInSeconds <= 0) return false;

		if (cacheHitProbability > IGNORE_THRESHOLD_IN_PERCENT) return false;

core(uses-long-cache-ttl): ignore private, must-validate, no-cache #6835

core(uses-long-cache-ttl): ignore private, must-validate, no-cache #6835

Conversation

connorjclark commented Dec 18, 2018

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 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

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

brendankenny commented Jan 9, 2019

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

connorjclark commented Jan 15, 2019

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

patrickhulce left a comment

Choose a reason for hiding this comment