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.

Sign up for GitHub

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

Jump to bottom

core: remove dependency on DevtoolsTimelineModel #5533

Merged
merged 10 commits into from
Jun 28, 2018
Merged

core: remove dependency on DevtoolsTimelineModel #5533

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e2162f5
core: remove dependency on DevtoolsTimelineModel
patrickhulce Jun 20, 2018
b15c188
golden lhr
patrickhulce Jun 21, 2018
279e49d
feedback
patrickhulce Jun 25, 2018
155b3ad
moar docs
patrickhulce Jun 25, 2018
657f7de
golden lhr
patrickhulce Jun 25, 2018
cec72cd
Merge branch 'master' into remove_dtm_dep
patrickhulce Jun 26, 2018
99ab3e1
stack of attributableURLs
patrickhulce Jun 26, 2018
6dd725e
Merge branch 'master' into remove_dtm_dep
patrickhulce Jun 27, 2018
d2def08
rebase
patrickhulce Jun 27, 2018
d712444
last feedback
patrickhulce Jun 28, 2018
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
93 changes: 39 additions & 54 deletions lighthouse-core/audits/bootup-time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
'use strict';

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

class BootupTime extends Audit {
/**
Expand Down Expand Up @@ -41,33 +40,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 {LH.Artifacts.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 originalTime = timingByGroupId[task.group.id] || 0;
timingByGroupId[task.group.id] = originalTime + 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.

feels weird to apply the multiplier in here.
i know it's another loop, but i'd rather apply the multiplier after we loop over all the tasks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh funny I moved it there because it natural in the computation of time instead of a post-step :)

I don't feel that strongly though, done.

Copy link
Member

Choose a reason for hiding this comment

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

ha, it reads better inlined to me too

result.set(task.attributableURL, timingByGroupId);
}

return result;
}
Expand All @@ -80,47 +68,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 = await artifacts.requestMainThreadTasks(trace);
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 +122,6 @@ class BootupTime extends Audit {
rawValue: totalBootupTime,
displayValue: [Util.MS_DISPLAY_VALUE, totalBootupTime],
details,
extendedInfo: {
value: extendedInfo,
},
};
}
}
Expand Down
54 changes: 23 additions & 31 deletions lighthouse-core/audits/mainthread-work-breakdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@

const Audit = require('./audit');
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');

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

/**
* @param {LH.Artifacts.DevtoolsTimelineModel} timelineModel
* @param {LH.Artifacts.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) {
const originalTime = result.get(task.group.id) || 0;
result.set(task.group.id, originalTime + 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.

same comment about the inlined multiplier.

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

}

return result;
}
Expand All @@ -65,40 +67,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 = await artifacts.requestMainThreadTasks(trace);
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 +107,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
View file
Open in desktop

This file was deleted.

Loading