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(fr): add computed cache to pass context #12427

Merged
merged 21 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const artifacts = {
GlobalListeners: '',
IFrameElements: '',
InstallabilityErrors: '',
LinkElements: '',
MetaElements: '',
NetworkUserAgent: '',
PasswordInputsWithPreventedPaste: '',
Expand Down Expand Up @@ -60,6 +61,7 @@ const defaultConfig = {
{id: artifacts.GlobalListeners, gatherer: 'global-listeners'},
{id: artifacts.IFrameElements, gatherer: 'iframe-elements'},
{id: artifacts.InstallabilityErrors, gatherer: 'installability-errors'},
{id: artifacts.LinkElements, gatherer: 'link-elements'},
{id: artifacts.MetaElements, gatherer: 'meta-elements'},
{id: artifacts.NetworkUserAgent, gatherer: 'network-user-agent'},
{id: artifacts.PasswordInputsWithPreventedPaste, gatherer: 'dobetterweb/password-inputs-with-prevented-paste'},
Expand Down Expand Up @@ -94,6 +96,7 @@ const defaultConfig = {
artifacts.GlobalListeners,
artifacts.IFrameElements,
artifacts.InstallabilityErrors,
artifacts.LinkElements,
artifacts.MetaElements,
artifacts.NetworkUserAgent,
artifacts.PasswordInputsWithPreventedPaste,
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/gather/base-gatherer.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ class FRGatherer {
* @return {Promise<LH.Gatherer.PhaseResultNonPromise>}
*/
async afterPass(passContext, loadData) {
if ('dependencies' in this.meta) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
throw Error('Gatherer with dependencies should override afterPass');
adamraine marked this conversation as resolved.
Show resolved Hide resolved
}
await this.stopSensitiveInstrumentation({...passContext, dependencies: {}});
await this.stopInstrumentation({...passContext, dependencies: {}});
return this.getArtifact({...passContext, dependencies: {}});
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ async function navigation(options) {
{
url: requestedUrl,
config,
computedCache: new Map(),
}
);
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/fraggle-rock/gather/runner-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ async function collectPhaseArtifacts(options) {
gatherMode,
driver,
dependencies,
computedCache: new Map(),
});
});

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/fraggle-rock/gather/snapshot-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ async function snapshot(options) {
{
url,
config,
computedCache: new Map(),
}
);
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/fraggle-rock/gather/timespan-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ async function startTimespan(options) {
{
url: finalUrl,
config,
computedCache: new Map(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we be sharing computed artifacts between phases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but I don't imagine it will really matter in practice as there are no dependencies for anything but getArtifact

}
);
},
Expand Down
8 changes: 5 additions & 3 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
const log = require('lighthouse-logger');
const LHError = require('../lib/lh-error.js');
const NetworkAnalyzer = require('../lib/dependency-graph/simulator/network-analyzer.js');
const NetworkRecorder = require('../lib/network-recorder.js');
const NetworkRecords = require('../computed/network-records.js');
const emulation = require('../lib/emulation.js');
const constants = require('../config/constants.js');
const i18n = require('../lib/i18n/i18n.js');
Expand Down Expand Up @@ -55,6 +55,7 @@ const SLOW_CPU_BENCHMARK_INDEX_THRESHOLD = 1000;
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/** @typedef {import('../gather/driver.js')} Driver */
/** @typedef {import('../lib/arbitrary-equality-map.js')} ArbitraryEqualityMap */

/**
* Each entry in each gatherer result array is the output of a gatherer phase:
Expand Down Expand Up @@ -480,7 +481,7 @@ class GatherRunner {
};
log.time(status);
const devtoolsLog = driver.endDevtoolsLog();
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
const networkRecords = await NetworkRecords.request(devtoolsLog, passContext);
log.timeEnd(status);

return {
Expand Down Expand Up @@ -710,7 +711,7 @@ class GatherRunner {

/**
* @param {Array<LH.Config.Pass>} passConfigs
* @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings}} options
* @param {{driver: Driver, requestedUrl: string, settings: LH.Config.Settings, computedCache: Map<string, ArbitraryEqualityMap>}} options
* @return {Promise<LH.Artifacts>}
*/
static async run(passConfigs, options) {
Expand Down Expand Up @@ -740,6 +741,7 @@ class GatherRunner {
settings: options.settings,
passConfig,
baseArtifacts,
computedCache: options.computedCache,
LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings,
};
const passResults = await GatherRunner.runPass(passContext);
Expand Down
64 changes: 48 additions & 16 deletions lighthouse-core/gather/gatherers/link-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
'use strict';

const LinkHeader = require('http-link-header');
const Gatherer = require('./gatherer.js');
const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const {URL} = require('../../lib/url-shim.js');
const NetworkRecords = require('../../computed/network-records.js');
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const pageFunctions = require('../../lib/page-functions.js');
const DevtoolsLog = require('./devtools-log.js');

/* globals HTMLLinkElement getNodeDetails */

Expand Down Expand Up @@ -79,15 +81,28 @@ function getLinkElementsInDOM() {
}
/* c8 ignore stop */

class LinkElements extends Gatherer {
class LinkElements extends FRGatherer {
constructor() {
super();
/**
* This needs to be in the constructor.
* https://github.com/GoogleChrome/lighthouse/issues/12134
* @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>}
*/
this.meta = {
supportedModes: ['timespan', 'navigation'],
dependencies: {DevtoolsLog: DevtoolsLog.symbol},
};
adamraine marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.FRTransitionalContext} context
* @return {Promise<LH.Artifacts['LinkElements']>}
*/
static getLinkElementsInDOM(passContext) {
static getLinkElementsInDOM(context) {
// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize
// the values like access from JavaScript does.
return passContext.driver.executionContext.evaluate(getLinkElementsInDOM, {
return context.driver.executionContext.evaluate(getLinkElementsInDOM, {
args: [],
useIsolation: true,
deps: [
Expand All @@ -98,14 +113,13 @@ class LinkElements extends Gatherer {
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {LH.Artifacts.NetworkRequest[]} networkRecords
* @return {LH.Artifacts['LinkElements']}
*/
static getLinkElementsInHeaders(passContext, loadData) {
const finalUrl = passContext.url;
const records = loadData.networkRecords;
const mainDocument = NetworkAnalyzer.findMainDocument(records, finalUrl);
static getLinkElementsInHeaders(context, networkRecords) {
const finalUrl = context.url;
const mainDocument = NetworkAnalyzer.findMainDocument(networkRecords, finalUrl);

/** @type {LH.Artifacts['LinkElements']} */
const linkElements = [];
Expand All @@ -131,13 +145,13 @@ class LinkElements extends Gatherer {
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {LH.Artifacts.NetworkRequest[]} networkRecords
* @return {Promise<LH.Artifacts['LinkElements']>}
*/
async afterPass(passContext, loadData) {
const fromDOM = await LinkElements.getLinkElementsInDOM(passContext);
const fromHeaders = LinkElements.getLinkElementsInHeaders(passContext, loadData);
async _getArtifact(context, networkRecords) {
const fromDOM = await LinkElements.getLinkElementsInDOM(context);
const fromHeaders = LinkElements.getLinkElementsInHeaders(context, networkRecords);
const linkElements = fromDOM.concat(fromHeaders);

for (const link of linkElements) {
Expand All @@ -147,6 +161,24 @@ class LinkElements extends Gatherer {

return linkElements;
}

/**
* @param {LH.Gatherer.PassContext} context
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts['LinkElements']>}
*/
async afterPass(context, loadData) {
return this._getArtifact({...context, dependencies: {}}, loadData.networkRecords);
}

/**
* @param {LH.Gatherer.FRTransitionalContext<'DevtoolsLog'>} context
* @return {Promise<LH.Artifacts['LinkElements']>}
*/
async getArtifact(context) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const records = await NetworkRecords.request(context.dependencies.DevtoolsLog, context);
return this._getArtifact(context, records);
}
}

module.exports = LinkElements;
3 changes: 2 additions & 1 deletion lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ async function lighthouse(url, flags = {}, configJSON, userConnection) {
log.setLevel(flags.logLevel);

const config = generateConfig(configJSON, flags);
const options = {url, config};
const computedCache = new Map();
const options = {url, config, computedCache};
const connection = userConnection || new ChromeProtocol(flags.port, flags.hostname);

// kick off a lighthouse run
Expand Down
13 changes: 8 additions & 5 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@ const generateReport = require('./report/report-generator.js').generateReport;
const LHError = require('./lib/lh-error.js');

/** @typedef {import('./gather/connections/connection.js')} Connection */
/** @typedef {import('./lib/arbitrary-equality-map.js')} ArbitraryEqualityMap */
/** @typedef {LH.Config.Config} Config */

class Runner {
/**
* @template {LH.Config.Config | LH.Config.FRConfig} TConfig
* @param {(runnerData: {requestedUrl: string, config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: TConfig, url?: string, driverMock?: Driver}} runOpts
* @param {{config: TConfig, computedCache: Map<string, ArbitraryEqualityMap>, url?: string, driverMock?: Driver}} runOpts
* @return {Promise<LH.RunnerResult|undefined>}
*/
static async run(gatherFn, runOpts) {
Expand Down Expand Up @@ -100,7 +101,7 @@ class Runner {
throw new Error('No audits to evaluate.');
}
const auditResultsById = await Runner._runAudits(settings, runOpts.config.audits, artifacts,
lighthouseRunWarnings);
lighthouseRunWarnings, runOpts.computedCache);

// LHR construction phase
const resultsStatus = {msg: 'Generating results...', id: 'lh:runner:generate'};
Expand Down Expand Up @@ -217,7 +218,7 @@ class Runner {
/**
* Establish connection, load page and collect all required artifacts
* @param {string} requestedUrl
* @param {{config: Config, driverMock?: Driver}} runnerOpts
* @param {{config: Config, computedCache: Map<string, ArbitraryEqualityMap>, driverMock?: Driver}} runnerOpts
* @param {Connection} connection
* @return {Promise<LH.Artifacts>}
*/
Expand All @@ -230,6 +231,7 @@ class Runner {
driver,
requestedUrl,
settings: runnerOpts.config.settings,
computedCache: runnerOpts.computedCache,
};
const artifacts = await GatherRunner.run(runnerOpts.config.passes, gatherOpts);
return artifacts;
Expand All @@ -241,9 +243,10 @@ class Runner {
* @param {Array<LH.Config.AuditDefn>} audits
* @param {LH.Artifacts} artifacts
* @param {Array<string | LH.IcuMessage>} runWarnings
* @param {Map<string, ArbitraryEqualityMap>} computedCache
* @return {Promise<Record<string, LH.RawIcu<LH.Audit.Result>>>}
*/
static async _runAudits(settings, audits, artifacts, runWarnings) {
static async _runAudits(settings, audits, artifacts, runWarnings, computedCache) {
const status = {msg: 'Analyzing and running audits...', id: 'lh:runner:auditing'};
log.time(status);

Expand All @@ -267,7 +270,7 @@ class Runner {
// Members of LH.Audit.Context that are shared across all audits.
const sharedAuditContext = {
settings,
computedCache: new Map(),
computedCache,
};

// Run each audit sequentially
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/fraggle-rock/api-test-pptr.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe('Fraggle Rock API', () => {

const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`24`);
expect(auditResults.length).toMatchInlineSnapshot(`25`);
adamraine marked this conversation as resolved.
Show resolved Hide resolved

expect(erroredAudits).toHaveLength(0);
expect(failedAudits.map(audit => audit.id)).toContain('errors-in-console');
Expand Down Expand Up @@ -159,7 +159,7 @@ describe('Fraggle Rock API', () => {
const {lhr} = result;
const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`102`);
expect(auditResults.length).toMatchInlineSnapshot(`103`);
expect(erroredAudits).toHaveLength(0);

const failedAuditIds = failedAudits.map(audit => audit.id);
Expand Down
15 changes: 15 additions & 0 deletions lighthouse-core/test/fraggle-rock/gather/base-gatherer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,20 @@ describe('BaseGatherer', () => {
expect(gatherer.stopInstrumentation).toHaveBeenCalled();
expect(gatherer.stopSensitiveInstrumentation).toHaveBeenCalled();
});

it('throws if no override and has dependencies', async () => {
class MyGatherer extends Gatherer {
/** @type {LH.Gatherer.GathererMeta<'Trace'>} */
meta = {supportedModes: ['timespan'], dependencies: {Trace: Symbol('Trace')}};
stopInstrumentation = jest.fn();
stopSensitiveInstrumentation = jest.fn();
}

const gatherer = new MyGatherer();
const afterPassPromise = gatherer.afterPass(fakeParam, fakeParam);
await expect(afterPassPromise).rejects.toThrowError(/Gatherer with dependencies/);
expect(gatherer.stopInstrumentation).not.toHaveBeenCalled();
expect(gatherer.stopSensitiveInstrumentation).not.toHaveBeenCalled();
});
});
});