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

PROPOSAL: new structure for metrics + lantern #4676

Closed
wants to merge 1 commit into from
Closed
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
42 changes: 42 additions & 0 deletions lighthouse-core/audits/first-contentful-paint.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @license Copyright 2016 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 Audit = require('./audit');

class FirstContentfulPaint extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
name: 'first-contentful-paint',
description: 'First Contentful Paint',
failureDescription: 'First Contentful Paint',
helpText: 'Foo',
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static async audit(artifacts, context) {
const result = await artifacts.requestFirstContentfulPaint({
trace: artifacts.traces.defaultPass,
devtoolsLog: artifacts.devtoolsLogs.defaultPass,
throttling: context.config.settings.throttling,
})

return {
rawValue: result.timing,
score: 100 - 100 * (result.timing / 10000),
};
}
}

module.exports = FirstContentfulPaint;
36 changes: 1 addition & 35 deletions lighthouse-core/audits/predictive-perf.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,40 +72,6 @@ class PredictivePerf extends Audit {
return scriptUrls;
}

/**
* @param {!Node} dependencyGraph
* @return {!Object}
*/
static computeRTTAndServerResponseTime(dependencyGraph) {
const records = [];
dependencyGraph.traverse(node => {
if (node.type === Node.TYPES.NETWORK) records.push(node.record);
});

// First pass compute the estimated observed RTT to each origin's servers.
const rttByOrigin = new Map();
for (const [origin, summary] of NetworkAnalyzer.estimateRTTByOrigin(records).entries()) {
rttByOrigin.set(origin, summary.min);
}

// We'll use the minimum RTT as the assumed connection latency since we care about how much addt'l
// latency each origin introduces as Lantern will be simulating with its own connection latency.
const minimumRtt = Math.min(...Array.from(rttByOrigin.values()));
// We'll use the observed RTT information to help estimate the server response time
const responseTimeSummaries = NetworkAnalyzer.estimateServerResponseTimeByOrigin(records, {
rttByOrigin,
});

const additionalRttByOrigin = new Map();
const serverResponseTimeByOrigin = new Map();
for (const [origin, summary] of responseTimeSummaries.entries()) {
additionalRttByOrigin.set(origin, rttByOrigin.get(origin) - minimumRtt);
serverResponseTimeByOrigin.set(origin, summary.median);
}

return {additionalRttByOrigin, serverResponseTimeByOrigin};
}

/**
* @param {!Node} dependencyGraph
* @param {!TraceOfTabArtifact} traceOfTab
Expand Down Expand Up @@ -245,7 +211,7 @@ class PredictivePerf extends Audit {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
return Promise.all([
artifacts.requestPageDependencyGraph(trace, devtoolsLogs),
artifacts.requestPageDependencyGraph({trace, devtoolsLogs}),
artifacts.requestTraceOfTab(trace),
]).then(([graph, traceOfTab]) => {
const graphs = {
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ class Config {
this._artifacts = expandArtifacts(configJSON.artifacts);
this._categories = configJSON.categories;
this._groups = configJSON.groups;
this._settings = configJSON.settings || {};

// validatePasses must follow after audits are required
validatePasses(configJSON.passes, this._audits);
Expand Down Expand Up @@ -748,6 +749,11 @@ class Config {
get groups() {
return this._groups;
}

/** @type {Object} */
get settings() {
return this._settings;
}
}

/**
Expand Down
20 changes: 19 additions & 1 deletion lighthouse-core/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,24 @@

/* eslint-disable max-len */

const emulation = require('../lib/emulation');

module.exports = {
settings: {},
settings: {
onlyAudits: ['first-contentful-paint'],
throttling: {
// method: 'devtools',
// requestLatency: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.latency,
Copy link
Member

Choose a reason for hiding this comment

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

i'd like these key names to be more in-line with the ones used for lantern, but i think you already know that. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I named them differently specifically because they operate at different levels and the amount you'd want to apply to one should be very different from the amount you'd apply to the other to get the same experience :)

// downloadThroughput: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.downloadThroughput,
// uploadThroughput: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.uploadThroughput,

method: 'lantern',
rtt: 160,
throughput: 128 * 1000,

cpuSlowdownMultiplier: emulation.settings.CPU_THROTTLE_METRICS.rate,
},
},
passes: [{
passName: 'defaultPass',
recordTrace: true,
Expand Down Expand Up @@ -72,6 +88,7 @@ module.exports = {
],
}],
audits: [
'first-contentful-paint',
'is-on-https',
'redirects-http',
'service-worker',
Expand Down Expand Up @@ -264,6 +281,7 @@ module.exports = {
name: 'Performance',
description: 'These encapsulate your web app\'s current performance and opportunities to improve it.',
audits: [
{id: 'first-contentful-paint', weight: 5, group: 'perf-metric'},
{id: 'first-meaningful-paint', weight: 5, group: 'perf-metric'},
{id: 'first-interactive', weight: 5, group: 'perf-metric'},
{id: 'consistently-interactive', weight: 5, group: 'perf-metric'},
Expand Down
50 changes: 50 additions & 0 deletions lighthouse-core/config/lantern.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* @license Copyright 2017 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';

module.exports = {
extends: 'lighthouse:default',
settings: {
throttling: {
method: 'lantern',
rtt: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.targetLatency,
throughput: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.targetDownloadThroughput,
cpuSlowdownMultiplier: emulation.settings.CPU_THROTTLE_METRICS.rate,
},
skipAudits: [
// disabled for now because their results are not meaningful/cannot be computed anymore
'first-meaningful-paint',
'first-interactive',
'consistently-interactive',
'estimated-input-latency',
'speed-index-metric',
'offscreen-images',
'load-fast-enough-for-pwa',
],
},
passes: [
{
passName: 'defaultPass',
// overwrite the throttling and load wait parameters
useThrottling: false,
pauseAfterLoadMs: 0,
networkQuietThresholdMs: 500,
cpuQuietThresholdMs: 500,
// no need to add any gatherers yet, but this property is required
gatherers: [],
},
],
audits: [
'predictive-perf',
],
categories: {
performance: {
audits: [
{id: 'predictive-perf', weight: 5, group: 'perf-metric'},
],
},
},
};
24 changes: 3 additions & 21 deletions lighthouse-core/gather/computed/computed-artifact.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ class ComputedArtifact {
this._allComputedArtifacts = allComputedArtifacts;
}

get requiredNumberOfArtifacts() {
return 1;
}

/* eslint-disable no-unused-vars */

/**
Expand All @@ -38,34 +34,20 @@ class ComputedArtifact {
throw new Error('compute_() not implemented for computed artifact ' + this.name);
}

/**
* 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 */

/**
* Request a computed artifact, caching the result on the input artifact.
* @param {...*} artifacts
* @param {*} artifacts
* @return {!Promise<*>}
*/
request(...artifacts) {
this._assertCorrectNumberOfArtifacts(artifacts);
request(artifacts) {
if (this._cache.has(artifacts)) {
return Promise.resolve(this._cache.get(artifacts));
}

const artifactPromise = Promise.resolve()
.then(_ => this.compute_(...artifacts, this._allComputedArtifacts));
.then(_ => this.compute_(artifacts, this._allComputedArtifacts));
this._cache.set(artifacts, artifactPromise);

return artifactPromise;
Expand Down
137 changes: 137 additions & 0 deletions lighthouse-core/gather/computed/first-contentful-paint.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/**
* @license Copyright 2017 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 ComputedArtifact = require('./computed-artifact');
const Node = require('../../lib/dependency-graph/node');
const Simulator = require('../../lib/dependency-graph/simulator/simulator');

const COEFFICIENTS = {
Copy link
Member

Choose a reason for hiding this comment

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

comment on where this come from plx

intercept: 1440,
optimistic: -1.75,
pessimistic: 2.73,
};

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

/**
* @param {!Node} dependencyGraph
* @param {function()=} condition
* @return {!Set<string>}
*/
static getScriptUrls(dependencyGraph, condition) {
const scriptUrls = new Set();

dependencyGraph.traverse(node => {
Copy link
Member

Choose a reason for hiding this comment

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

it feels like it'd make sense for a common lib/dependency-graph-predicates.js at some point

if (node.type === Node.TYPES.CPU) return;
if (node.record._resourceType !== WebInspector.resourceTypes.Script) return;
if (condition && !condition(node)) return;
scriptUrls.add(node.record.url);
});

return scriptUrls;
}

/**
* @param {!Node} dependencyGraph
Copy link
Member

Choose a reason for hiding this comment

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

not relevant to the architecture, but we should document how we compute this estimated FCP.

* @param {!TraceOfTabArtifact} traceOfTab
* @return {!Node}
*/
static getOptimisticGraph(dependencyGraph, traceOfTab) {
const fcp = traceOfTab.timestamps.firstContentfulPaint;
const blockingScriptUrls = FirstContentfulPaint.getScriptUrls(dependencyGraph, node => {
return (
node.endTime <= fcp && node.hasRenderBlockingPriority() && node.initiatorType !== 'script'
);
});

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fcp) return false;
// Include EvaluateScript tasks for blocking scripts
if (node.type === Node.TYPES.CPU) return node.isEvaluateScriptFor(blockingScriptUrls);
// Include non-script-initiated network requests with a render-blocking priority
return node.hasRenderBlockingPriority() && node.initiatorType !== 'script';
});
}

/**
* @param {!Node} dependencyGraph
* @param {!TraceOfTabArtifact} traceOfTab
* @return {!Node}
*/
static getPessimisticGraph(dependencyGraph, traceOfTab) {
Copy link
Member

Choose a reason for hiding this comment

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

theres so much similarity between these two fns (optimistic & pessimistic). could we DRY it up? dunno if adding && optionalPred(node) to the end of both would be terrible or not.

const fcp = traceOfTab.timestamps.firstContentfulPaint;
const blockingScriptUrls = FirstContentfulPaint.getScriptUrls(dependencyGraph, node => {
return node.endTime <= fcp && node.hasRenderBlockingPriority();
});

return dependencyGraph.cloneWithRelationships(node => {
if (node.endTime > fcp) return false;
// Include EvaluateScript tasks for blocking scripts
if (node.type === Node.TYPES.CPU) return node.isEvaluateScriptFor(blockingScriptUrls);
// Include all network requests that had render-blocking priority (even script-initiated)
return node.hasRenderBlockingPriority();
});
}

static async computeLantern(data, artifacts) {
const {trace, devtoolsLog} = data;
const graph = await artifacts.requestPageDependencyGraph({trace, devtoolsLog});
const traceOfTab = await artifacts.requestTraceOfTab(trace);
const networkAnalysis = await artifacts.requestNetworkAnalysis(devtoolsLog);

const optimisticGraph = FirstContentfulPaint.getOptimisticGraph(graph, traceOfTab);
const pessimisticGraph = FirstContentfulPaint.getPessimisticGraph(graph, traceOfTab);

const options = {...networkAnalysis, ...data.throttling};
Copy link
Member

@paulirish paulirish Mar 14, 2018

Choose a reason for hiding this comment

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

looks like data.throttling will override the rtt/throughput provided by networkAnalysis, right?
would you ever want just rtt from data.throttling and take throughput from networkAnalysis?

i wonder if networkAnalysis should take data.throttling as a parameter instead? We don't really need to compute throughput if we're using a provided value, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, and not that I can think of. Network analysis artifact is meant to provide you a glimpse into what was observed while throttling is what you want it to be.

In lantern's case we don't really care about what was observed, other than the additional latency observed per origin

const optimisticEstimate = new Simulator(optimisticGraph, options).simulate().timeInMs;
const pessimisticEstimate = new Simulator(pessimisticGraph, options).simulate().timeInMs;

const timing =
COEFFICIENTS.intercept +
COEFFICIENTS.optimistic * optimisticEstimate +
COEFFICIENTS.pessimistic * pessimisticEstimate;

return {
timing,
optimisticEstimate,
pessimisticEstimate,
optimisticGraph,
pessimisticGraph,
};
}

/**
* @param {{trace: Object, devtoolsLog: Object, throttling: Object}} data
* @return {Object}
*/
async compute_(data, artifacts) {
if (data.throttling.method !== 'lantern') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps we should move this to the gatherer runner? this way we don't need to explicitly do if statements inside the compute_.

const throttlingMethod = data.throttling.method;
const computedGathererFunc = throttlingMethod.charAt(0).toUpperCase() + throttlingMethod.slice(1);
if (gatherer[computedGathererFunc]) {
  gatherer[computedGathererFunc]();
} else {
  gatherer.compute_();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm I feel like that introduces a decent amount of extra complexity to where the computed artifacts are coming from when they are already pretty hard to follow. what's the primary concern with having the ifs here in the compute?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no real reason :) just thought it was better to not think about putting ifs everywhere but maybe it's better to make it explicit 😄

const traceOfTab = await artifacts.requestTraceOfTab(data.trace);
return {
timing: traceOfTab.timings.firstContentfulPaint,
timestamp: traceOfTab.timestamps.firstContentfulPaint,
};
}

return FirstContentfulPaint.computeLantern(data, artifacts);
}
}

/**
* @typedef MetricResult
* @property {number} timing
* @property {number|undefined} timestamp
Copy link
Collaborator

Choose a reason for hiding this comment

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

i know it's just an example but shouldn't this be @property {number=} timestamp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nah it should be more like @property {number} [timestamp] docs but point taken yes will spend more time on the real typedef when trying to land 😛

* @property {number|undefined} optimisticEstimate
* @property {number|undefined} pessimisticEstimate
* @property {!Node|undefined} optimisticGraph
* @property {!Node|undefined} pessimisticGraph
*/

module.exports = FirstContentfulPaint;
Loading