From 38fafe104fb8e90e9b5a676a5726d291ec90b70c Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 17 Jul 2017 15:39:18 -0700 Subject: [PATCH 1/3] feat(computed-artifact): support arbitrarily many inputs --- .../gather/computed/computed-artifact.js | 96 ++++++++++++++++++- .../gather/computed/computed-artifact-test.js | 46 ++++++++- 2 files changed, 134 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/gather/computed/computed-artifact.js b/lighthouse-core/gather/computed/computed-artifact.js index f6cfd76bb343..f8b16017e32b 100644 --- a/lighthouse-core/gather/computed/computed-artifact.js +++ b/lighthouse-core/gather/computed/computed-artifact.js @@ -17,6 +17,10 @@ class ComputedArtifact { this._allComputedArtifacts = allComputedArtifacts; } + get requiredNumberOfArtifacts() { + return 1; + } + /* eslint-disable no-unused-vars */ /** @@ -31,6 +35,87 @@ class ComputedArtifact { throw new Error('compute_() not implemented for computed artifact ' + this.name); } + /** + * This is a helper function for performing cache operations and is responsible for maintaing the + * internal cache structure. This function accepts a path of artifacts, used to find the correct + * nested cache object, and an operation to perform on that cache with the final key. + * + * The cache is structured with the first argument occupying the keys of the toplevel cache that point + * to the Map of keys for the second argument and so forth until the 2nd to last argument's Map points + * to result values rather than further nesting. In the simplest case of a single argument, there + * is no nesting and the keys point directly to result values. + * + * Map( + * argument1A -> Map( + * argument2A -> result1A2A + * argument2B -> result1A2B + * ) + * argument1B -> Map( + * argument2A -> result1B2A + * ) + * ) + * + * @param {!Array<*>} artifacts + * @param {function(!Map, *)} cacheOperation + */ + _performCacheOperation(artifacts, cacheOperation) { + artifacts = artifacts.slice(); + + let cache = this._cache; + while (artifacts.length > 1) { + const nextKey = artifacts.shift(); + if (cache.has(nextKey)) { + cache = cache.get(nextKey); + } else { + const nextCache = new Map(); + cache.set(nextKey, nextCache); + cache = nextCache; + } + } + + return cacheOperation(cache, artifacts.shift()); + } + + /** + * Performs a cache.has operation, see _performCacheOperation for more. + * @param {!Array<*>} artifacts + * @return {boolean} + */ + _cacheHas(artifacts) { + return this._performCacheOperation(artifacts, (cache, key) => cache.has(key)); + } + + /** + * Performs a cache.get operation, see _performCacheOperation for more. + * @param {!Array<*>} artifacts + * @return {*} + */ + _cacheGet(artifacts) { + return this._performCacheOperation(artifacts, (cache, key) => cache.get(key)); + } + + /** + * Performs a cache.set operation, see _performCacheOperation for more. + * @param {!Array<*>} artifacts + * @param {*} result + */ + _cacheSet(artifacts, result) { + return this._performCacheOperation(artifacts, (cache, key) => cache.set(key, result)); + } + + /** + * Asserts that the length of the array is the same as the number of inputs the class expects + * @param {!Array<*>} artifacts + */ + _assertCorrectNumberOfArtifacts(artifacts) { + const actual = artifacts.length; + const expected = this.requiredNumberOfArtifacts; + if (actual !== expected) { + const className = this.constructor.name; + throw new Error(`${className} requires ${expected} artifacts but ${actual} were given`); + } + } + /* eslint-enable no-unused-vars */ /** @@ -38,14 +123,15 @@ class ComputedArtifact { * @param {*} artifact * @return {!Promise<*>} */ - request(artifact) { - if (this._cache.has(artifact)) { - return Promise.resolve(this._cache.get(artifact)); + request(...artifacts) { + this._assertCorrectNumberOfArtifacts(artifacts); + if (this._cacheHas(artifacts)) { + return Promise.resolve(this._cacheGet(artifacts)); } const artifactPromise = Promise.resolve() - .then(_ => this.compute_(artifact, this._allComputedArtifacts)); - this._cache.set(artifact, artifactPromise); + .then(_ => this.compute_(...artifacts, this._allComputedArtifacts)); + this._cacheSet(artifacts, artifactPromise); return artifactPromise; } diff --git a/lighthouse-core/test/gather/computed/computed-artifact-test.js b/lighthouse-core/test/gather/computed/computed-artifact-test.js index 58a51fecc0ff..98c2ada31813 100644 --- a/lighthouse-core/test/gather/computed/computed-artifact-test.js +++ b/lighthouse-core/test/gather/computed/computed-artifact-test.js @@ -12,9 +12,10 @@ const assert = require('assert'); const ComputedArtifact = require('../../../gather/computed/computed-artifact'); class TestComputedArtifact extends ComputedArtifact { - constructor() { - super(); + constructor(...args) { + super(...args); + this.lastArguments = []; this.computeCounter = 0; } @@ -22,12 +23,32 @@ class TestComputedArtifact extends ComputedArtifact { return 'TestComputedArtifact'; } - compute_(_) { + compute_(...args) { + this.lastArguments = args; return this.computeCounter++; } } +class MultipleInputArtifact extends TestComputedArtifact { + get requiredNumberOfArtifacts() { + return 2; + } +} + describe('ComputedArtifact base class', () => { + it('tests correct number of inputs', () => { + const singleInputArtifact = new TestComputedArtifact(); + const multiInputArtifact = new MultipleInputArtifact(); + + return Promise.resolve() + .then(() => singleInputArtifact.request(1)) + .then(() => multiInputArtifact.request(1, 2)) + .then(() => { + assert.throws(() => singleInputArtifact.request(1, 2)); + assert.throws(() => multiInputArtifact.request(1)); + }); + }); + it('caches computed artifacts', () => { const testComputedArtifact = new TestComputedArtifact(); @@ -44,4 +65,23 @@ describe('ComputedArtifact base class', () => { assert.equal(result, 1); }); }); + + it('caches multiple input arguments', () => { + const mockComputed = {computed: true}; + const computedArtifact = new MultipleInputArtifact(mockComputed); + + const obj0 = {value: 1}; + const obj1 = {value: 2}; + const obj2 = {value: 3}; + + return computedArtifact.request(obj0, obj1) + .then(result => assert.equal(result, 0)) + .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj0, obj1, mockComputed])) + .then(() => computedArtifact.request(obj1, obj2)) + .then(result => assert.equal(result, 1)) + .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])) + .then(() => computedArtifact.request(obj0, obj1)) + .then(result => assert.equal(result, 0)) + .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])); + }); }); From 4eca27613274579b777a74f39e1573ededdffa62 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 18 Jul 2017 11:13:17 -0700 Subject: [PATCH 2/3] switch to underscore convention --- .../gather/computed/computed-artifact-test.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/test/gather/computed/computed-artifact-test.js b/lighthouse-core/test/gather/computed/computed-artifact-test.js index 98c2ada31813..a092ada77e0b 100644 --- a/lighthouse-core/test/gather/computed/computed-artifact-test.js +++ b/lighthouse-core/test/gather/computed/computed-artifact-test.js @@ -41,12 +41,10 @@ describe('ComputedArtifact base class', () => { const multiInputArtifact = new MultipleInputArtifact(); return Promise.resolve() - .then(() => singleInputArtifact.request(1)) - .then(() => multiInputArtifact.request(1, 2)) - .then(() => { - assert.throws(() => singleInputArtifact.request(1, 2)); - assert.throws(() => multiInputArtifact.request(1)); - }); + .then(_ => singleInputArtifact.request(1)) + .then(_ => multiInputArtifact.request(1, 2)) + .then(_ => assert.throws(() => singleInputArtifact.request(1, 2))) + .then(_ => assert.throws(() => multiInputArtifact.request(1))); }); it('caches computed artifacts', () => { @@ -76,12 +74,12 @@ describe('ComputedArtifact base class', () => { return computedArtifact.request(obj0, obj1) .then(result => assert.equal(result, 0)) - .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj0, obj1, mockComputed])) - .then(() => computedArtifact.request(obj1, obj2)) + .then(_ => assert.deepEqual(computedArtifact.lastArguments, [obj0, obj1, mockComputed])) + .then(_ => computedArtifact.request(obj1, obj2)) .then(result => assert.equal(result, 1)) - .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])) - .then(() => computedArtifact.request(obj0, obj1)) + .then(_ => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])) + .then(_ => computedArtifact.request(obj0, obj1)) .then(result => assert.equal(result, 0)) - .then(() => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])); + .then(_ => assert.deepEqual(computedArtifact.lastArguments, [obj1, obj2, mockComputed])); }); }); From e7e3da6ef7226512f363b8dcc1e01cba5e7d5bb6 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 18 Jul 2017 11:34:53 -0700 Subject: [PATCH 3/3] jsdoc --- lighthouse-core/gather/computed/computed-artifact.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/computed/computed-artifact.js b/lighthouse-core/gather/computed/computed-artifact.js index f8b16017e32b..fc2306ca6b27 100644 --- a/lighthouse-core/gather/computed/computed-artifact.js +++ b/lighthouse-core/gather/computed/computed-artifact.js @@ -120,7 +120,7 @@ class ComputedArtifact { /** * Request a computed artifact, caching the result on the input artifact. - * @param {*} artifact + * @param {...*} artifacts * @return {!Promise<*>} */ request(...artifacts) {