Cache SW: Serve version with most responses (ampproject#6475)

* Cache SW: Serve version with most responses

I want to avoid the situation where
`/v0/some-rarely-used-components.js` serves up a very old RTV, when we
have multiple files of the newer version that could be served.

This heavily weights the main binary, places a small weight on the
first requested file, and treats all other responses as normal. The
version with the max score is what we’ll serve.

* Comments

* Update tests

src/service-worker/core.js

@@ -58,9 +58,9 @@ let cache;
  * A mapping from a Client's (unique per tab _and_ refresh) ID to the AMP
  * release version we are serving it.
  *
- * @type {!Object<string, RtvVersion>}
+ * @type {!Object<string, !Promise<RtvVersion>>}
  */
-const clientsMap = Object.create(null);
+const clientsVersion = Object.create(null);
 
 /**
  * A mapping from a client's referrer into the time that referrer last made a
@@ -268,28 +268,63 @@ export function fetchAndCache(cache, request, requestPath, requestVersion) {
 }
 
 /**
- * Gets the version we have cached for this file. It's either:
- * - The requestVersion, meaning we have this explicit version cached.
- * - Some older version
- * - An empty string, meaning we have nothing cached for this file.
+ * Gets the version we want to serve for this client. We attempt to serve the
+ * version with the most cached files, with a additional weight given to the
+ * main binary and the first requested file.
  *
  * @param {!Cache} cache
  * @param {string} requestPath
+ * @param {RtvVersion} requestVersion
  * @return {!Promise<RtvVersion>}
  * @visibleForTesting
  */
-export function getCachedVersion(cache, requestPath) {
- // TODO(jridgewell): We should make this a bit smarter, so that it selects
- // the version that has a lot of matches, not just this request file.
+export function getCachedVersion(cache, requestPath, requestVersion) {
  return cache.keys().then(requests => {
+ // TODO(jridgewell): This should really count the bytes of the response,
+ // but there's no efficient way to do that.
+ const counts = {};
+ let most = requestVersion;
+ let mostCount = 0;
+
+ // Generates a weighted maximum version, ie the version with the most
+ // cached files. Given every file we've cached, determine what version
+ // it is, and increment the number of files we have for that version.
  for (let i = 0; i < requests.length; i++) {
  const url = requests[i].url;
- if (requestPath === pathname(url)) {
- return rtvVersion(url);
+ const path = pathname(url);
+ const version = rtvVersion(url);
+
+ // We do not want to stale serve blacklisted files. If nothing else is
+ // cached, we will end up serving whatever version is requested.
+ if (isBlacklisted(version)) {
+ continue;
+ }
+
+ let count = counts[version] || 0;
+
+ // Incrementing the number of "files" that have this version with a
+ // weight.
+ // The main binary (arguably the most important file to cache) is given a
+ // heavy weight, while the first requested file is given a slight weight.
+ // Everything else increments normally.
+ if (path.indexOf('/', 1) === -1) {
+ // Main binary
+ count += 5;
+ } else if (requestPath === path) {
+ // Give a little precedence to the requested file
+ count += 2;
+ } else {
+ count++;
+ }
+
+ counts[version] = count;
+ if (count > mostCount) {
+ most = version;
+ mostCount = count;
  }
  }
 
- return '';
+ return most;
  });
 }
 
@@ -328,30 +363,13 @@ export function handleFetch(request, maybeClientId) {
  return cachePromise.then(() => {
  // If we already registered this client, we must always use the same
  // version.
- if (clientsMap[clientId]) {
- return clientsMap[clientId];
+ if (clientsVersion[clientId]) {
+ return clientsVersion[clientId];
  }
 
- // If not, do we have this version cached?
- return getCachedVersion(cache, requestPath).then(version => {
- // We have a cached version! Serve it up!
- if (version && !isBlacklisted(version)) {
- return version;
- }
-
- // Tears! We have nothing cached, so we'll have to make a request.
- return requestVersion;
- }).then(version => {
- // Determining the version to serve is racey, since there are parallel
- // requests coming in for all the CDN JS files. If one of them "won"
- // the race, respect the winner.
- if (clientsMap[clientId]) {
- return clientsMap[clientId];
- }
-
- clientsMap[clientId] = version;
- return version;
- });
+ // If not, let's find the version to serve up.
+ return clientsVersion[clientId] = getCachedVersion(cache, requestPath,
+ requestVersion);
  }).then(version => {
  const versionedRequest = normalizedRequest(request, version);
 

test/functional/test-cache-sw-core.js

@@ -15,7 +15,6 @@
  */
 
 import * as sinon from 'sinon';
-import {timerFor} from '../../src/timer';
 
 /**
  * Cache SW has some side-effects, so we've got to do a little jig to test.
@@ -320,20 +319,58 @@ runner.run('Cache SW', () => {
  });
 
  describe('getCachedVersion', () => {
+ let keys;
  beforeEach(() => {
+ keys = [{url}];
  sandbox.stub(cache, 'keys', () => {
- return Promise.resolve([{url}]);
+ return Promise.resolve(keys);
  });
  });
+
  it('returns cached rtv version, if file is cached', () => {
- return sw.getCachedVersion(cache, '/v0.js').then(version => {
+ return sw.getCachedVersion(cache, '/v0.js', prevRtv).then(version => {
  expect(version).to.equal(rtv);
  });
  });
 
- it('returns empty string, if file is not cached', () => {
- return sw.getCachedVersion(cache, '/amp-comp.js').then(version => {
- expect(version).to.equal('');
+ it('defaults to requested version, if file no files are cached', () => {
+ keys.length = 0;
+ return sw.getCachedVersion(cache, '/v0.js', prevRtv).then(version => {
+ expect(version).to.equal(prevRtv);
+ });
+ });
+
+ it('returns version with most responses', () => {
+ keys.splice(0, 1,
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-1-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-2-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${rtv}/v0/amp-3-0.1.js`});
+ return sw.getCachedVersion(cache, '/v0.js', rtv).then(version => {
+ expect(version).to.equal(prevRtv);
+ });
+ });
+
+ it('weights requested file cached version', () => {
+ keys.splice(0, 1,
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-1-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-2-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${rtv}/v0/amp-3-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${rtv}/v0/amp-4-0.1.js`});
+
+ return sw.getCachedVersion(cache, '/v0/amp-4-0.1.js', '123')
+ .then(version => {
+ expect(version).to.equal(rtv);
+ });
+ });
+
+ it('heavily weights main binary version', () => {
+ keys.splice(0, 1,
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-1-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-2-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${prevRtv}/v0/amp-3-0.1.js`},
+ {url: `https://cdn.ampproject.org/rtv/${rtv}/v0.js`});
+ return sw.getCachedVersion(cache, '/v0.js', '123').then(version => {
+ expect(version).to.equal(rtv);
  });
  });
  });
@@ -426,31 +463,25 @@ runner.run('Cache SW', () => {
  });
  });
 
- describe('with multiple parallel requests', () => {
- it('forces uniform RTV version of winner', () => {
- const timer = timerFor(window);
- // First call will resolve after the first.
- const keys = sandbox.stub(cache, 'keys');
- keys.returns(Promise.resolve([]));
- keys.onCall(0).returns(timer.promise(100, []));
- return Promise.all([
- sw.handleFetch(request, clientId),
- sw.handleFetch(prevRequest, clientId),
- ]).then(responses => {
- expect(sw.rtvVersion(responses[0].url)).to.equal(
- sw.rtvVersion(prevRequest.url));
- expect(sw.rtvVersion(responses[1].url)).to.equal(
- sw.rtvVersion(prevRequest.url));
- });
- });
- });
-
  describe('with cached files', () => {
  beforeEach(() => {
- cache.cached.push([request, responseFromRequest(request)]);
  cache.cached.push([prevRequest, responseFromRequest(prevRequest)]);
  });
 
+ describe('with multiple parallel requests', () => {
+ it('forces uniform RTV version of winner', () => {
+ return Promise.all([
+ sw.handleFetch(request, clientId),
+ sw.handleFetch(compRequest, clientId),
+ ]).then(responses => {
+ expect(sw.rtvVersion(responses[0].url)).to.equal(
+ sw.rtvVersion(prevRequest.url));
+ expect(sw.rtvVersion(responses[1].url)).to.equal(
+ sw.rtvVersion(prevRequest.url));
+ });
+ });
+ });
+
  it('fulfills with cached version', () => {
  return sw.handleFetch(compRequest, clientId).then(resp => {
  expect(resp.url).to.equal(prevRequest.url);
@@ -468,7 +499,7 @@ runner.run('Cache SW', () => {
 
  describe('with blacklisted file', () => {
  beforeEach(() => {
- cache.cached.splice(1, 1,
+ cache.cached.splice(0, 1,
  [blacklistedRequest, responseFromRequest(blacklistedRequest)]);
  });
 
@@ -494,7 +525,7 @@ runner.run('Cache SW', () => {
  setTimeout(resolve, 50);
  });
  }).then(() => {
- expect(cache.cached[1][0]).to.equal(prodRequest);
+ expect(cache.cached[0][0]).to.equal(prodRequest);
  });
  });
 
@@ -511,6 +542,22 @@ runner.run('Cache SW', () => {
  });
  });
  });
+
+ describe('without cached files', () => {
+ describe('with multiple parallel requests', () => {
+ it('forces uniform RTV version of winner', () => {
+ return Promise.all([
+ sw.handleFetch(request, clientId),
+ sw.handleFetch(prevRequest, clientId),
+ ]).then(responses => {
+ expect(sw.rtvVersion(responses[0].url)).to.equal(
+ sw.rtvVersion(request.url));
+ expect(sw.rtvVersion(responses[1].url)).to.equal(
+ sw.rtvVersion(request.url));
+ });
+ });
+ });
+ });
  });
  });
 });