Skip to content

Commit

Permalink
core: initial refactor of computedArtifact import/caching (#5907)
Browse files Browse the repository at this point in the history
  • Loading branch information
brendankenny committed Sep 21, 2018
1 parent 7e969b2 commit 489da22
Show file tree
Hide file tree
Showing 17 changed files with 247 additions and 141 deletions.
6 changes: 4 additions & 2 deletions lighthouse-core/audits/manifest-short-name-length.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

const Audit = require('./audit');
const ManifestValues = require('../gather/computed/manifest-values');

class ManifestShortNameLength extends Audit {
/**
Expand All @@ -25,10 +26,11 @@ class ManifestShortNameLength extends Audit {

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts) {
const manifestValues = await artifacts.requestManifestValues(artifacts.Manifest);
static async audit(artifacts, context) {
const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
// If there's no valid manifest, this audit is not applicable
if (manifestValues.isParseFailure) {
return {
Expand Down
9 changes: 6 additions & 3 deletions lighthouse-core/audits/multi-check-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ const Audit = require('./audit');
class MultiCheckAudit extends Audit {
/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static audit(artifacts) {
return Promise.resolve(this.audit_(artifacts)).then(result => this.createAuditProduct(result));
static async audit(artifacts, context) {
const multiProduct = await this.audit_(artifacts, context);
return this.createAuditProduct(multiProduct);
}

/**
Expand Down Expand Up @@ -63,9 +65,10 @@ class MultiCheckAudit extends Audit {

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, warnings?: Array<string>, manifestValues?: LH.Artifacts.ManifestValues}>}
*/
static audit_(artifacts) {
static audit_(artifacts, context) {
throw new Error('audit_ unimplemented');
}

Expand Down
17 changes: 9 additions & 8 deletions lighthouse-core/audits/splash-screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

const MultiCheckAudit = require('./multi-check-audit');
const ManifestValues = require('../gather/computed/manifest-values');

/**
* @fileoverview
Expand Down Expand Up @@ -64,20 +65,20 @@ class SplashScreen extends MultiCheckAudit {

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
*/
static audit_(artifacts) {
static async audit_(artifacts, context) {
/** @type {Array<string>} */
const failures = [];

return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
SplashScreen.assessManifest(manifestValues, failures);
const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
SplashScreen.assessManifest(manifestValues, failures);

return {
failures,
manifestValues,
};
});
return {
failures,
manifestValues,
};
}
}

Expand Down
21 changes: 11 additions & 10 deletions lighthouse-core/audits/themed-omnibox.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const MultiCheckAudit = require('./multi-check-audit');
const validColor = require('../lib/web-inspector').Color.parse;
const ManifestValues = require('../gather/computed/manifest-values');

/**
* @fileoverview
Expand Down Expand Up @@ -63,22 +64,22 @@ class ThemedOmnibox extends MultiCheckAudit {

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues, themeColor: ?string}>}
*/
static audit_(artifacts) {
static async audit_(artifacts, context) {
/** @type {Array<string>} */
const failures = [];

return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
ThemedOmnibox.assessManifest(manifestValues, failures);
ThemedOmnibox.assessMetaThemecolor(artifacts.ThemeColor, failures);
const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
ThemedOmnibox.assessManifest(manifestValues, failures);
ThemedOmnibox.assessMetaThemecolor(artifacts.ThemeColor, failures);

return {
failures,
manifestValues,
themeColor: artifacts.ThemeColor,
};
});
return {
failures,
manifestValues,
themeColor: artifacts.ThemeColor,
};
}
}

Expand Down
39 changes: 20 additions & 19 deletions lighthouse-core/audits/webapp-install-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const MultiCheckAudit = require('./multi-check-audit');
const SWAudit = require('./service-worker');
const ManifestValues = require('../gather/computed/manifest-values');

/**
* @fileoverview
Expand Down Expand Up @@ -118,33 +119,33 @@ class WebappInstallBanner extends MultiCheckAudit {

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, warnings: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
*/
static audit_(artifacts) {
static async audit_(artifacts, context) {
/** @type {Array<string>} */
let offlineFailures = [];
/** @type {Array<string>} */
let offlineWarnings = [];

return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
const manifestFailures = WebappInstallBanner.assessManifest(manifestValues);
const swFailures = WebappInstallBanner.assessServiceWorker(artifacts);
if (!swFailures.length) {
const {failures, warnings} = WebappInstallBanner.assessOfflineStartUrl(artifacts);
offlineFailures = failures;
offlineWarnings = warnings;
}
const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
const manifestFailures = WebappInstallBanner.assessManifest(manifestValues);
const swFailures = WebappInstallBanner.assessServiceWorker(artifacts);
if (!swFailures.length) {
const {failures, warnings} = WebappInstallBanner.assessOfflineStartUrl(artifacts);
offlineFailures = failures;
offlineWarnings = warnings;
}

return {
warnings: offlineWarnings,
failures: [
...manifestFailures,
...swFailures,
...offlineFailures,
],
manifestValues,
};
});
return {
warnings: offlineWarnings,
failures: [
...manifestFailures,
...swFailures,
...offlineFailures,
],
manifestValues,
};
}
}

Expand Down
12 changes: 4 additions & 8 deletions lighthouse-core/gather/computed/manifest-values.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

const ComputedArtifact = require('./computed-artifact');
const makeComputedArtifact = require('./new-computed-artifact');
const icons = require('../../lib/icons');

const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone'];
Expand All @@ -14,11 +14,7 @@ const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone'];
// For more discussion, see https://github.com/GoogleChrome/lighthouse/issues/69 and https://developer.chrome.com/apps/manifest/name#short_name
const SUGGESTED_SHORTNAME_LENGTH = 12;

class ManifestValues extends ComputedArtifact {
get name() {
return 'ManifestValues';
}

class ManifestValues {
static get validityIds() {
return ['hasManifest', 'hasParseableManifest'];
}
Expand Down Expand Up @@ -88,7 +84,7 @@ class ManifestValues extends ComputedArtifact {
* @param {LH.Artifacts['Manifest']} manifest
* @return {Promise<LH.Artifacts.ManifestValues>}
*/
async compute_(manifest) {
static async compute_(manifest) {
// if the manifest isn't there or is invalid json, we report that and bail
let parseFailureReason;

Expand Down Expand Up @@ -125,4 +121,4 @@ class ManifestValues extends ComputedArtifact {
}
}

module.exports = ManifestValues;
module.exports = makeComputedArtifact(ManifestValues);
40 changes: 40 additions & 0 deletions lighthouse-core/gather/computed/new-computed-artifact.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* 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.
*/
'use strict';

const ArbitraryEqualityMap = require('../../lib/arbitrary-equality-map.js');

/**
* @template {string} N
* @template A
* @template R
* @param {{name: N, compute_: (artifact: A) => Promise<R>}} computableArtifact
* @return {{name: N, request: (context: LH.Audit.Context, artifact: A) => Promise<R>}}
*/
function makeComputedArtifact(computableArtifact) {
/**
* @param {LH.Audit.Context} context
* @param {A} artifact
*/
const request = ({computedCache}, artifact) => {
const cache = computedCache.get(computableArtifact.name) || new ArbitraryEqualityMap();
computedCache.set(computableArtifact.name, cache);

const computed = /** @type {Promise<R>|undefined} */ (cache.get(artifact));
if (computed) {
return computed;
}

const artifactPromise = computableArtifact.compute_(artifact);
cache.set(artifact, artifactPromise);

return artifactPromise;
};

return Object.assign(computableArtifact, {request});
}

module.exports = makeComputedArtifact;
18 changes: 10 additions & 8 deletions lighthouse-core/lib/arbitrary-equality-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ const isEqual = require('lodash.isequal');
* It is not meant to be performant and is well-suited to use cases where the number of entries is
* likely to be small (like computed artifacts).
*/
module.exports = class ArbitraryEqualityMap {
class ArbitraryEqualityMap {
constructor() {
this._equalsFn = /** @type {function(any,any):boolean} */ ((a, b) => a === b);
/** @type {Array<{key: string, value: *}>} */
this._equalsFn = /** @type {function(*,*):boolean} */ ((a, b) => a === b);
/** @type {Array<{key: *, value: *}>} */
this._entries = [];
}

Expand All @@ -27,15 +27,15 @@ module.exports = class ArbitraryEqualityMap {
}

/**
* @param {string} key
* @param {*} key
* @return {boolean}
*/
has(key) {
return this._findIndexOf(key) !== -1;
}

/**
* @param {string} key
* @param {*} key
* @return {*}
*/
get(key) {
Expand All @@ -44,7 +44,7 @@ module.exports = class ArbitraryEqualityMap {
}

/**
* @param {string} key
* @param {*} key
* @param {*} value
*/
set(key, value) {
Expand All @@ -54,7 +54,7 @@ module.exports = class ArbitraryEqualityMap {
}

/**
* @param {string} key
* @param {*} key
* @return {number}
*/
_findIndexOf(key) {
Expand All @@ -76,4 +76,6 @@ module.exports = class ArbitraryEqualityMap {
static deepEquals(objA, objB) {
return isEqual(objA, objB);
}
};
}

module.exports = ArbitraryEqualityMap;
21 changes: 15 additions & 6 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,17 @@ class Runner {
}
}

// Members of LH.Audit.Context that are shared across all audits.
const sharedAuditContext = {
settings,
LighthouseRunWarnings: runWarnings,
computedCache: new Map(),
};

// Run each audit sequentially
const auditResults = [];
for (const auditDefn of audits) {
const auditResult = await Runner._runAudit(auditDefn, artifacts, settings, runWarnings);
const auditResult = await Runner._runAudit(auditDefn, artifacts, sharedAuditContext);
auditResults.push(auditResult);
}

Expand All @@ -220,12 +227,11 @@ class Runner {
* Otherwise returns error audit result.
* @param {LH.Config.AuditDefn} auditDefn
* @param {LH.Artifacts} artifacts
* @param {LH.Config.Settings} settings
* @param {Array<string>} runWarnings
* @param {Pick<LH.Audit.Context, 'settings'|'LighthouseRunWarnings'|'computedCache'>} sharedAuditContext
* @return {Promise<LH.Audit.Result>}
* @private
*/
static async _runAudit(auditDefn, artifacts, settings, runWarnings) {
static async _runAudit(auditDefn, artifacts, sharedAuditContext) {
const audit = auditDefn.implementation;
const status = `Evaluating: ${i18n.getFormatted(audit.meta.title, 'en-US')}`;

Expand Down Expand Up @@ -274,8 +280,7 @@ class Runner {
const auditOptions = Object.assign({}, audit.defaultOptions, auditDefn.options);
const auditContext = {
options: auditOptions,
settings,
LighthouseRunWarnings: runWarnings,
...sharedAuditContext,
};

const product = await audit.audit(artifacts, auditContext);
Expand Down Expand Up @@ -380,6 +385,10 @@ class Runner {
'metrics', // the sub folder that contains metric names
'metrics/lantern-metric.js', // lantern metric base class
'metrics/metric.js', // computed metric base class

// Computed artifacts switching to the new system.
'new-computed-artifact.js',
'manifest-values.js',
];

const fileList = [
Expand Down
Loading

0 comments on commit 489da22

Please sign in to comment.