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: remove dependency on DevtoolsTimelineModel #5533

Merged
merged 10 commits into from
Jun 28, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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
96 changes: 42 additions & 54 deletions lighthouse-core/audits/bootup-time.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
'use strict';

const Audit = require('./audit');
const WebInspector = require('../lib/web-inspector');
const TraceProcessor = require('../lib/traces/tracing-processor');
const Util = require('../report/html/renderer/util');
const {groupIdToName, taskToGroup} = require('../lib/task-groups');
const {taskGroups} = require('../lib/task-groups');

/** @typedef {import('../lib/traces/tracing-processor.js').TaskNode} TaskNode */

class BootupTime extends Audit {
/**
Expand Down Expand Up @@ -41,33 +43,22 @@ class BootupTime extends Audit {
}

/**
* Returns a mapping of URL to counts of event groups.
* @param {LH.Artifacts.DevtoolsTimelineModel} timelineModel
* @return {Map<string, Object<string, number>>}
* @param {TaskNode[]} tasks
* @param {number} multiplier
* @return {Map<string, Object<keyof taskGroups, number>>}
Copy link
Member

Choose a reason for hiding this comment

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

if you use keyof in the Object<> type (or anything reasonably complicated, it seems), tsc gets confused and promotes to any, so should use Record<> here (maybe we should just give up on Object and always use Record)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

had to widen it to string anyhow

*/
static getExecutionTimingsByURL(timelineModel) {
const bottomUpByURL = timelineModel.bottomUpGroupBy('URL');
static getExecutionTimingsByURL(tasks, multiplier) {
/** @type {Map<string, Object<string, number>>} */
const result = new Map();

bottomUpByURL.children.forEach((perUrlNode, url) => {
// when url is "" or about:blank, we skip it
if (!url || url === 'about:blank') {
return;
}

/** @type {Object<string, number>} */
const taskGroups = {};
perUrlNode.children.forEach((perTaskPerUrlNode) => {
// eventStyle() returns a string like 'Evaluate Script'
const task = WebInspector.TimelineUIUtils.eventStyle(perTaskPerUrlNode.event);
// Resolve which taskGroup we're using
const groupName = taskToGroup[task.title] || groupIdToName.other;
const groupTotal = taskGroups[groupName] || 0;
taskGroups[groupName] = groupTotal + (perTaskPerUrlNode.selfTime || 0);
});
result.set(url, taskGroups);
});
for (const task of tasks) {
if (!task.attributableURL || task.attributableURL === 'about:blank') continue;

const timingByGroupId = result.get(task.attributableURL) || {};
const original = timingByGroupId[task.group.id] || 0;
Copy link
Member

Choose a reason for hiding this comment

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

bikeshed: originalTime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

timingByGroupId[task.group.id] = original + task.selfTime * multiplier;
result.set(task.attributableURL, timingByGroupId);
}

return result;
}
Expand All @@ -80,47 +71,47 @@ class BootupTime extends Audit {
static async audit(artifacts, context) {
const settings = context.settings || {};
const trace = artifacts.traces[BootupTime.DEFAULT_PASS];
const devtoolsTimelineModel = await artifacts.requestDevtoolsTimelineModel(trace);
const executionTimings = BootupTime.getExecutionTimingsByURL(devtoolsTimelineModel);
let totalBootupTime = 0;
/** @type {Object<string, Object<string, number>>} */
const extendedInfo = {};

const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'scripting', granularity: 1, itemType: 'ms', text: groupIdToName.scripting},
{key: 'scriptParseCompile', granularity: 1, itemType: 'ms',
text: groupIdToName.scriptParseCompile},
];

const tasks = TraceProcessor.getMainThreadTasks(trace.traceEvents);
Copy link
Member

Choose a reason for hiding this comment

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

should this be a computed artifact?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah probably, ☠️ 2️⃣ 🐦 1️⃣ 🗿

const multiplier = settings.throttlingMethod === 'simulate' ?
settings.throttling.cpuSlowdownMultiplier : 1;
// map data in correct format to create a table

const executionTimings = BootupTime.getExecutionTimingsByURL(tasks, multiplier);

let totalBootupTime = 0;
const results = Array.from(executionTimings)
.map(([url, groups]) => {
.map(([url, timingByGroupId]) => {
// Add up the totalBootupTime for all the taskGroups
for (const [name, value] of Object.entries(groups)) {
groups[name] = value * multiplier;
totalBootupTime += value * multiplier;
let bootupTimeForURL = 0;
for (const timespanMs of Object.values(timingByGroupId)) {
Copy link
Member

Choose a reason for hiding this comment

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

reduce?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not since we're moving the multiplier down, otherwise yes :)

bootupTimeForURL += timespanMs;
}

extendedInfo[url] = groups;
totalBootupTime += bootupTimeForURL;

const scriptingTotal = timingByGroupId[taskGroups.ScriptEvaluation.id] || 0;
const parseCompileTotal = timingByGroupId[taskGroups.ScriptParseCompile.id] || 0;

const scriptingTotal = groups[groupIdToName.scripting] || 0;
const parseCompileTotal = groups[groupIdToName.scriptParseCompile] || 0;
return {
url: url,
sum: scriptingTotal + parseCompileTotal,
// Only reveal the javascript task costs
// Later we can account for forced layout costs, etc.
total: bootupTimeForURL,
// Highlight the JavaScript task costs
scripting: scriptingTotal,
scriptParseCompile: parseCompileTotal,
};
})
.filter(result => result.sum >= context.options.thresholdInMs)
.sort((a, b) => b.sum - a.sum);
.filter(result => result.total >= context.options.thresholdInMs)
.sort((a, b) => b.total - a.total);

const summary = {wastedMs: totalBootupTime};

const headings = [
{key: 'url', itemType: 'url', text: 'URL'},
{key: 'total', granularity: 1, itemType: 'ms', text: 'Total'},
{key: 'scripting', granularity: 1, itemType: 'ms', text: taskGroups.ScriptEvaluation.label},
{key: 'scriptParseCompile', granularity: 1, itemType: 'ms',
text: taskGroups.ScriptParseCompile.label},
];

const details = BootupTime.makeTableDetails(headings, results, summary);

const score = Audit.computeLogNormalScore(
Expand All @@ -134,9 +125,6 @@ class BootupTime extends Audit {
rawValue: totalBootupTime,
displayValue: [Util.MS_DISPLAY_VALUE, totalBootupTime],
details,
extendedInfo: {
value: extendedInfo,
},
};
}
}
Expand Down
56 changes: 25 additions & 31 deletions lighthouse-core/audits/mainthread-work-breakdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
'use strict';

const Audit = require('./audit');
const TraceProcessor = require('../lib/traces/tracing-processor');
const Util = require('../report/html/renderer/util');
// We group all trace events into groups to show a highlevel breakdown of the page
const {taskToGroup} = require('../lib/task-groups');
const {taskGroups} = require('../lib/task-groups');

/** @typedef {import('../lib/traces/tracing-processor.js').TaskNode} TaskNode */

class MainThreadWorkBreakdown extends Audit {
/**
Expand Down Expand Up @@ -43,15 +45,17 @@ class MainThreadWorkBreakdown extends Audit {
}

/**
* @param {LH.Artifacts.DevtoolsTimelineModel} timelineModel
* @param {TaskNode[]} tasks
* @param {number} multiplier
* @return {Map<string, number>}
*/
static getExecutionTimingsByCategory(timelineModel) {
const bottomUpByName = timelineModel.bottomUpGroupBy('EventName');

static getExecutionTimingsByGroup(tasks, multiplier) {
/** @type {Map<string, number>} */
const result = new Map();
bottomUpByName.children.forEach((event, eventName) =>
result.set(eventName, event.selfTime));

for (const task of tasks) {
result.set(task.group.id, (result.get(task.group.id) || 0) + task.selfTime * multiplier);
Copy link
Member

Choose a reason for hiding this comment

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

split second part into a variable(s)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did the first part

}

return result;
}
Expand All @@ -65,40 +69,33 @@ class MainThreadWorkBreakdown extends Audit {
const settings = context.settings || {};
const trace = artifacts.traces[MainThreadWorkBreakdown.DEFAULT_PASS];

const devtoolsTimelineModel = await artifacts.requestDevtoolsTimelineModel(trace);
const executionTimings = MainThreadWorkBreakdown.getExecutionTimingsByCategory(
devtoolsTimelineModel
);
let totalExecutionTime = 0;

const tasks = TraceProcessor.getMainThreadTasks(trace.traceEvents);
const multiplier = settings.throttlingMethod === 'simulate' ?
settings.throttling.cpuSlowdownMultiplier : 1;

const extendedInfo = {};
const executionTimings = MainThreadWorkBreakdown.getExecutionTimingsByGroup(tasks, multiplier);

let totalExecutionTime = 0;
const categoryTotals = {};
const results = Array.from(executionTimings).map(([eventName, duration]) => {
duration *= multiplier;
const results = Array.from(executionTimings).map(([groupId, duration]) => {
totalExecutionTime += duration;
extendedInfo[eventName] = duration;
const groupName = taskToGroup[eventName];

const categoryTotal = categoryTotals[groupName] || 0;
categoryTotals[groupName] = categoryTotal + duration;
const categoryTotal = categoryTotals[groupId] || 0;
categoryTotals[groupId] = categoryTotal + duration;

return {
category: eventName,
group: groupName,
group: groupId,
groupLabel: taskGroups[groupId].label,
duration: duration,
};
});

const headings = [
{key: 'group', itemType: 'text', text: 'Category'},
{key: 'category', itemType: 'text', text: 'Work'},
{key: 'duration', itemType: 'ms', granularity: 1, text: 'Time spent'},
{key: 'groupLabel', itemType: 'text', text: 'Category'},
{key: 'duration', itemType: 'ms', granularity: 1, text: 'Time Spent'},
];
// @ts-ignore - stableSort added to Array by WebInspector
results.stableSort((a, b) => categoryTotals[b.group] - categoryTotals[a.group]);

results.sort((a, b) => categoryTotals[b.group] - categoryTotals[a.group]);
const tableDetails = MainThreadWorkBreakdown.makeTableDetails(headings, results);

const score = Audit.computeLogNormalScore(
Expand All @@ -112,9 +109,6 @@ class MainThreadWorkBreakdown extends Audit {
rawValue: totalExecutionTime,
displayValue: [Util.MS_DISPLAY_VALUE, totalExecutionTime],
details: tableDetails,
extendedInfo: {
value: extendedInfo,
},
};
}
}
Expand Down
25 changes: 0 additions & 25 deletions lighthouse-core/gather/computed/dtm-model.js

This file was deleted.

33 changes: 10 additions & 23 deletions lighthouse-core/gather/computed/screenshots.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,39 +7,26 @@

const ComputedArtifact = require('./computed-artifact');

const SCREENSHOT_TRACE_NAME = 'Screenshot';

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

/**
* @param {{imageDataPromise: function(): Promise<string>}} frame
* @return {Promise<string>}
*/
fetchScreenshot(frame) {
return frame
.imageDataPromise()
.then(data => 'data:image/jpg;base64,' + data);
}

/**
* @param {LH.Trace} trace
* @param {LH.ComputedArtifacts} computedArtifacts
* @return {Promise<Array<{timestamp: number, datauri: string}>>}
*/
compute_(trace, computedArtifacts) {
return computedArtifacts.requestDevtoolsTimelineModel(trace).then(model => {
const filmStripFrames = model.filmStripModel().frames();
const frameFetches = filmStripFrames.map(frame => this.fetchScreenshot(frame));

return Promise.all(frameFetches).then(images => {
const result = filmStripFrames.map((frame, i) => ({
timestamp: frame.timestamp,
datauri: images[i],
}));
return result;
async compute_(trace) {
return trace.traceEvents
.filter(evt => evt.name === SCREENSHOT_TRACE_NAME)
.map(evt => {
return {
timestamp: evt.ts / 1000,
datauri: `data:image/jpg;base64,${evt.args.snapshot}`,
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the compiler doesn't object to accessing snapshot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well I added snapshot to traceevent definition :)

Copy link
Member

@brendankenny brendankenny Jun 25, 2018

Choose a reason for hiding this comment

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

I meant because it was optional but looks like the compiler's ok passing undefined into template strings :)

};
});
});
}
}

Expand Down
Loading