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(metrics): switch to speedIndex from perceptualSpeedIndex #4980

Merged
merged 5 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion lighthouse-cli/test/smokehouse/perf/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = [
initialUrl: 'http://localhost:10200/preload.html',
url: 'http://localhost:10200/preload.html',
audits: {
'speed-index-metric': {
'speed-index': {
score: '>=0.80',
extendedInfo: {
value: {
Expand Down
27 changes: 25 additions & 2 deletions lighthouse-core/audits/metrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,21 @@ class Metrics extends Audit {
const metricComputationData = {trace, devtoolsLog, settings: context.settings};

const traceOfTab = await artifacts.requestTraceOfTab(trace);
const speedline = await artifacts.requestSpeedline(trace);
const firstContentfulPaint = await artifacts.requestFirstContentfulPaint(metricComputationData);
const firstMeaningfulPaint = await artifacts.requestFirstMeaningfulPaint(metricComputationData);
const timeToInteractive = await artifacts.requestConsistentlyInteractive(metricComputationData);
const speedIndex = await artifacts.requestSpeedIndex(metricComputationData);
const metrics = [];

// Include the simulated/observed performance metrics
const metricsMap = {firstContentfulPaint, firstMeaningfulPaint, timeToInteractive};
const metricsMap = {firstContentfulPaint, firstMeaningfulPaint, timeToInteractive, speedIndex};
for (const [metricName, values] of Object.entries(metricsMap)) {
metrics.push(Object.assign({metricName}, values));
metrics.push({
metricName,
timing: Math.round(values.timing),
Copy link
Member

Choose a reason for hiding this comment

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

nice

timestamp: values.timestamp,
});
}

// Include all timestamps of interest from trace of tab
Expand All @@ -50,6 +56,23 @@ class Metrics extends Audit {
metrics.push({metricName, timing, timestamp});
}

// Include some visual metrics from speedline
metrics.push({
metricName: 'traceFirstVisualChange',
Copy link
Member

Choose a reason for hiding this comment

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

I feel like these aren't from the trace so i think we want a diff prefix.

Copy link
Collaborator Author

@patrickhulce patrickhulce Apr 18, 2018

Choose a reason for hiding this comment

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

open to ideas, I'd like to be consistent here on the fact that these are the raw observed values compared to the (potentially) simulated metric values

WDYT about observedMetricName

Copy link
Member

Choose a reason for hiding this comment

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

hmm okay. maybe we use 'observed' prefix?
(don't tell me i suggested we use 'trace' instead of 'observed' 😨 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no no you're 😎 I originally had raw haha which was just worse, I didn't think to suggest observed until now :)

timing: speedline.first,
timestamp: (speedline.first + speedline.beginning) * 1000,
});
metrics.push({
metricName: 'traceLastVisualChange',
timing: speedline.complete,
timestamp: (speedline.complete + speedline.beginning) * 1000,
});
metrics.push({
metricName: 'traceSpeedIndex',
timing: speedline.speedIndex,
timestamp: (speedline.speedIndex + speedline.beginning) * 1000,
});

const headings = [
{key: 'metricName', itemType: 'text', text: 'Name'},
{key: 'timing', itemType: 'ms', granularity: 10, text: 'Value (ms)'},
Expand Down
106 changes: 0 additions & 106 deletions lighthouse-core/audits/speed-index-metric.js

This file was deleted.

62 changes: 62 additions & 0 deletions lighthouse-core/audits/speed-index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/**
* @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');
const Util = require('../report/v2/renderer/util');

class SpeedIndex extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
name: 'speed-index',
description: 'Speed Index',
helpText: 'Speed Index shows how quickly the contents of a page are visibly populated. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/speed-index).',
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

/**
* @return {LH.Audit.ScoreOptions}
*/
static get defaultOptions() {
return {
// see https://www.desmos.com/calculator/mdgjzchijg
scorePODR: 1250,
scoreMedian: 5500,
};
}

/**
* Audits the page to give a score for the Speed Index.
* @see https://github.com/GoogleChrome/lighthouse/issues/197
* @param {Artifacts} artifacts The artifacts from the gather phase.
* @param {LH.Audit.Context} context
* @return {Promise<AuditResult>}
*/
static async audit(artifacts, context) {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const metricComputationData = {trace, devtoolsLog, settings: context.settings};
const metricResult = await artifacts.requestSpeedIndex(metricComputationData);

return {
score: Audit.computeLogNormalScore(
metricResult.timing,
context.options.scorePODR,
context.options.scoreMedian
),
rawValue: metricResult.timing,
displayValue: Util.formatMilliseconds(metricResult.timing),
};
}
}

module.exports = SpeedIndex;
4 changes: 2 additions & 2 deletions lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ module.exports = {
'first-contentful-paint',
'first-meaningful-paint',
'load-fast-enough-for-pwa',
'speed-index-metric',
'speed-index',
'screenshot-thumbnails',
'estimated-input-latency',
'errors-in-console',
Expand Down Expand Up @@ -268,7 +268,7 @@ module.exports = {
{id: 'first-meaningful-paint', weight: 3, group: 'perf-metric'},
{id: 'first-interactive', weight: 5, group: 'perf-metric'},
{id: 'consistently-interactive', weight: 5, group: 'perf-metric'},
{id: 'speed-index-metric', weight: 1, group: 'perf-metric'},
{id: 'speed-index', weight: 1, group: 'perf-metric'},
{id: 'estimated-input-latency', weight: 1, group: 'perf-metric'},
{id: 'link-blocking-first-paint', weight: 0, group: 'perf-hint'},
{id: 'script-blocking-first-paint', weight: 0, group: 'perf-hint'},
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/fast-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module.exports = {
'first-interactive',
'consistently-interactive',
'estimated-input-latency',
'speed-index-metric',
'speed-index',
'offscreen-images',
'load-fast-enough-for-pwa',
],
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/plots-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module.exports = {
settings: {
onlyAudits: [
'first-meaningful-paint',
'speed-index-metric',
'speed-index',
'estimated-input-latency',
'first-interactive',
'consistently-interactive',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class SpeedIndex extends MetricArtifact {
getEstimateFromSimulation(simulationResult, extras) {
const fcpTimeInMs = extras.fcpResult.timing;
const estimate = extras.optimistic
? extras.speedline.perceptualSpeedIndex
? extras.speedline.speedIndex
: SpeedIndex.computeLayoutBasedSpeedIndex(simulationResult.nodeTiming, fcpTimeInMs);
return {
timeInMs: estimate,
Expand Down
28 changes: 28 additions & 0 deletions lighthouse-core/gather/computed/metrics/speed-index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* @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 MetricArtifact = require('./metric');

class SpeedIndex extends MetricArtifact {
get name() {
return 'SpeedIndex';
}

/**
* @param {LH.Artifacts.MetricComputationData} data
* @param {Object} artifacts
* @return {Promise<LH.Artifacts.Metric>}
*/
async computeObservedMetric(data, artifacts) {
const speedline = await artifacts.requestSpeedline(data.trace);
Copy link
Member

Choose a reason for hiding this comment

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

should we just collapse these 2 into 1?

not sure if we use speedline for non-speedindex purposes.
i guess if we'd want firstVC or lastVC but no speedindex computed?

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'd like to keep them separate for exactly that reason, also if we want to do other stuff with the frames

Copy link
Member

@paulirish paulirish Apr 18, 2018

Choose a reason for hiding this comment

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

aiight groovy. 🕺

const timing = Math.round(speedline.speedIndex);
const timestamp = (timing + speedline.beginning) * 1000;
return Promise.resolve({timing, timestamp});
}
}

module.exports = SpeedIndex;
12 changes: 11 additions & 1 deletion lighthouse-core/gather/computed/speedline.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,24 @@ class Speedline extends ComputedArtifact {
return speedline(traceEvents, {
timeOrigin: navStart,
fastMode: true,
include: 'perceptualSpeedIndex',
include: 'speedIndex',
});
}).catch(err => {
if (/No screenshots found in trace/.test(err.message)) {
throw new LHError(LHError.errors.NO_SCREENSHOTS);
}

throw err;
}).then(speedline => {
if (speedline.frames.length === 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was done just in the audit, but it makes sense to be doing this everywhere speedline is needed, moved the test as well

throw new LHError(LHError.errors.NO_SPEEDLINE_FRAMES);
}

if (speedline.speedIndex === 0) {
throw new LHError(LHError.errors.SPEEDINDEX_OF_ZERO);
}

return speedline;
});
}
}
Expand Down
Loading