Skip to content
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

core: initial refactor of computedArtifact import/caching #5907

Merged
merged 5 commits into from
Sep 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll refrain from going promises->async/await in future changes to cut down the diffs as well :)

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe someone once told me that he thought it was silly we made classes with all static members instead of plain objects ;)

FWIW I kinda like them as classes for the flashy require name consistency, but I was mostly persuaded back then for plain objects, thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe someone once told me that he thought it was silly we made classes with all static members instead of plain objects ;)

haha, I didn't even think about it as I changed things. There used to be some tsc benefit due to polymorphic this (but not an issue with this implementation) and then from augmenting things in javascript passed out on module.exports (but that's all fixed in 3.1).

I'll admit to stockholm syndrome in that I don't see it as ugly and unnecessary any more, but I also don't have any problem with a plain object. The main thing might be the name property, which I think we still want, whether explicit or from just the StaticClass.name default from declaring a class.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true true, this WFM :)

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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fn is for the gulpfile. wont browserify already discover this manifest-values module without being added manually?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff obscures it, but this is the list of filenamesToSkip :)

];

const fileList = [
Expand Down
Loading