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

misc(timing): move timing-trace to async events #6440

Merged
merged 5 commits into from
Nov 5, 2018
Merged
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
83 changes: 37 additions & 46 deletions lighthouse-core/lib/timing-trace-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,70 +5,50 @@
*/
'use strict';

/**
* Technically, it's fine for usertiming measures to overlap, however non-async events make
* for a much clearer UI in traceviewer. We do this check to make sure we aren't passing off
* async-like measures as non-async.
* prevEntries are expected to be sorted by startTime
* @param {LH.Artifacts.MeasureEntry} entry user timing entry
* @param {LH.Artifacts.MeasureEntry[]} prevEntries user timing entries
*/
function checkEventOverlap(entry, prevEntries) {
for (const prevEntry of prevEntries) {
const prevEnd = prevEntry.startTime + prevEntry.duration;
const thisEnd = entry.startTime + entry.duration;
const isOverlap = prevEnd > entry.startTime && prevEnd < thisEnd;
if (isOverlap) {
// eslint-disable-next-line no-console
console.error(`Two measures overlap! ${prevEntry.name} & ${entry.name}`);
}
}
}

/**
* Generates a chromium trace file from user timing measures
* `threadId` can be provided to separate a series of trace events into another thread, useful
* if timings do not share the same timeOrigin, but should both be "left-aligned".
* Adapted from https://github.com/tdresser/performance-observer-tracing
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a note about how threadId is used?

* @param {LH.Artifacts.MeasureEntry[]} entries user timing entries
* @param {string=} trackName
* @param {number=} threadId
*/
function generateTraceEvents(entries, trackName = 'measures') {
function generateTraceEvents(entries, threadId = 0) {
if (!Array.isArray(entries)) return [];

/** @type {LH.TraceEvent[]} */
const currentTrace = [];
let id = 0;

entries.sort((a, b) => a.startTime - b.startTime);
entries.forEach((entry, i) => {
checkEventOverlap(entry, entries.slice(0, i));

/** @type {LH.TraceEvent} */
const traceEvent = {
name: entry.name,
cat: entry.entryType,
const startEvt = {
// 1) Remove 'lh:' for readability
// 2) Colons in user_timing names get special handling in traceviewer we don't want. https://goo.gl/m23Vz7
// Replace with a 'Modifier letter colon' ;)
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆

connorjclark marked this conversation as resolved.
Show resolved Hide resolved
name: entry.name.replace('lh:', '').replace(/:/g, '\ua789'),
cat: 'blink.user_timing',
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 doing this should be possible without faking out a browser trace but w/e traceviewer :)

Copy link
Member Author

Choose a reason for hiding this comment

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

traceviewer wont group these but give each measure its own track otherwise
and devtools needs it to view them.

ts: entry.startTime * 1000,
dur: entry.duration * 1000,
args: {},
dur: 0,
pid: 0,
tid: trackName === 'measures' ? 50 : 75,
ph: 'X',
id: '0x' + (id++).toString(16),
tid: threadId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like I'm missing something.

I only see one call to this method changed with a threadId isn't this basically the same as what was happening before then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah totally the same as before. trackName => threadId has no functional changes, really. with these new events we can't label the track.
not a huge deal anyway.

ph: 'b',
id: '0x' + (i++).toString(16),
};

if (entry.entryType !== 'measure') throw new Error('Unexpected entryType!');
if (entry.duration === 0) {
traceEvent.ph = 'n';
traceEvent.s = 't';
}
const endEvt = JSON.parse(JSON.stringify(startEvt));
endEvt.ph = 'e';
endEvt.ts = startEvt.ts + (entry.duration * 1000);

currentTrace.push(traceEvent);
currentTrace.push(startEvt);
currentTrace.push(endEvt);
});

// Add labels
/** @type {LH.TraceEvent} */
const metaEvtBase = {
pid: 0,
tid: trackName === 'measures' ? 50 : 75,
tid: threadId,
ts: 0,
dur: 0,
ph: 'M',
Expand All @@ -77,8 +57,20 @@ function generateTraceEvents(entries, trackName = 'measures') {
args: {labels: 'Default'},
};
currentTrace.push(Object.assign({}, metaEvtBase, {args: {labels: 'Lighthouse Timing'}}));
currentTrace.push(Object.assign({}, metaEvtBase, {name: 'thread_name', args: {name: trackName}}));

// Only inject TracingStartedInBrowser once
if (threadId === 0) {
currentTrace.push(Object.assign({}, metaEvtBase, {
'cat': 'disabled-by-default-devtools.timeline',
'name': 'TracingStartedInBrowser',
'ph': 'I',
'args': {'data': {
'frameTreeNodeId': 1,
'persistentIds': true,
'frames': [],
}},
}));
}
return currentTrace;
}

Expand All @@ -92,13 +84,12 @@ function createTraceString(lhr) {
const entries = lhr.timing.entries.filter(entry => !gatherEntries.includes(entry));

const auditEvents = generateTraceEvents(entries);
const gatherEvents = generateTraceEvents(gatherEntries, 'gather');
const gatherEvents = generateTraceEvents(gatherEntries, 10);
const events = [...auditEvents, ...gatherEvents];

const jsonStr = `
{ "traceEvents": [
${events.map(evt => JSON.stringify(evt)).join(',\n')}
]}`;
const jsonStr = `{"traceEvents":[
${events.map(evt => JSON.stringify(evt)).join(',\n')}
]}`;

return jsonStr;
}
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/scripts/generate-timing-trace.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const {createTraceString} = require('../lib/timing-trace-saver');

/**
* @fileoverview This script takes the timing entries saved during a Lighthouse run and generates
* a trace file that's readable in chrome://tracing.
* a trace file that's readable in DevTools perf panel or chrome://tracing.
*
* input = LHR.json
* output = LHR.timing.trace.json
Expand Down Expand Up @@ -50,7 +50,7 @@ function saveTraceFromCLI() {
// eslint-disable-next-line no-console
console.log(`
> Timing trace file saved to: ${traceFilePath}
> Open this file in chrome://tracing
> Open this file in DevTools perf panel (For --audit-mode runs, use chrome://tracing instead)
`);
}

Expand Down
137 changes: 137 additions & 0 deletions lighthouse-core/test/lib/__snapshots__/timing-trace-saver-test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`createTraceString creates a real trace 1`] = `
Array [
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x0",
"name": "init꞉config",
"ph": "b",
"pid": 0,
"tid": 0,
"ts": 650000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x0",
"name": "init꞉config",
"ph": "e",
"pid": 0,
"tid": 0,
"ts": 860000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x1",
"name": "runner꞉run",
"ph": "b",
"pid": 0,
"tid": 0,
"ts": 870000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x1",
"name": "runner꞉run",
"ph": "e",
"pid": 0,
"tid": 0,
"ts": 990000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x2",
"name": "runner꞉auditing",
"ph": "b",
"pid": 0,
"tid": 0,
"ts": 990000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x2",
"name": "runner꞉auditing",
"ph": "e",
"pid": 0,
"tid": 0,
"ts": 1740000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x3",
"name": "audit꞉is-on-https",
"ph": "b",
"pid": 0,
"tid": 0,
"ts": 1010000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x3",
"name": "audit꞉is-on-https",
"ph": "e",
"pid": 0,
"tid": 0,
"ts": 1020000,
},
Object {
"args": Object {
"data": Object {
"frameTreeNodeId": 1,
"frames": Array [],
"persistentIds": true,
},
},
"cat": "disabled-by-default-devtools.timeline",
"dur": 0,
"name": "TracingStartedInBrowser",
"ph": "I",
"pid": 0,
"tid": 0,
"ts": 0,
},
]
`;

exports[`generateTraceEvents generates a pair of trace events 1`] = `
Array [
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x0",
"name": "init꞉config",
"ph": "b",
"pid": 0,
"tid": 0,
"ts": 650000,
},
Object {
"args": Object {},
"cat": "blink.user_timing",
"dur": 0,
"id": "0x0",
"name": "init꞉config",
"ph": "e",
"pid": 0,
"tid": 0,
"ts": 860000,
},
]
`;
90 changes: 4 additions & 86 deletions lighthouse-core/test/lib/timing-trace-saver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

/* eslint-disable no-console */

const assert = require('assert');
const {generateTraceEvents, createTraceString} = require('../../lib/timing-trace-saver');

const mockEntries = [{
Expand Down Expand Up @@ -37,92 +36,11 @@ const mockEntries = [{
entryType: 'measure',
},
];
const expectedTrace = {
traceEvents: [{
name: 'lh:init:config',
cat: 'measure',
ts: 650000,
dur: 210000,
args: {},
pid: 0,
tid: 50,
ph: 'X',
id: '0x0',
},
{
name: 'lh:runner:run',
cat: 'measure',
ts: 870000,
dur: 120000,
args: {},
pid: 0,
tid: 50,
ph: 'X',
id: '0x1',
},
{
name: 'lh:runner:auditing',
cat: 'measure',
ts: 990000,
dur: 750000,
args: {},
pid: 0,
tid: 50,
ph: 'X',
id: '0x2',
},
{
name: 'lh:audit:is-on-https',
cat: 'measure',
ts: 1010000,
dur: 10000,
args: {},
pid: 0,
tid: 50,
ph: 'X',
id: '0x3',
},
],
};


describe('generateTraceEvents', () => {
let consoleError;
let origConsoleError;

beforeEach(() => {
origConsoleError = console.error;
consoleError = jest.fn();
console.error = consoleError;
});

afterEach(() => {
console.error = origConsoleError;
});

it('generates a single trace event', () => {
const event = generateTraceEvents(mockEntries);
assert.deepStrictEqual(event.slice(0, 1), expectedTrace.traceEvents.slice(0, 1));
});

it('doesn\'t allow overlapping events', () => {
const overlappingEntries = [{
startTime: 10,
name: 'overlap1',
duration: 100,
entryType: 'measure',
},
{
startTime: 30,
name: 'overlap2',
duration: 100,
entryType: 'measure',
},
];

generateTraceEvents(overlappingEntries);
expect(consoleError).toHaveBeenCalled();
expect(consoleError.mock.calls[0][0]).toContain('measures overlap');
it('generates a pair of trace events', () => {
const events = generateTraceEvents([mockEntries[0]]);
expect(events.slice(0, 2)).toMatchSnapshot();
});
});

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

good call. actually we dont need the ph:'n' now that we're on async events.
problem solved. :)


Expand All @@ -135,6 +53,6 @@ describe('createTraceString', () => {
});
const traceJson = JSON.parse(jsonStr);
const eventsWithoutMetadata = traceJson.traceEvents.filter(e => e.cat !== '__metadata');
assert.deepStrictEqual(eventsWithoutMetadata, expectedTrace.traceEvents);
expect(eventsWithoutMetadata).toMatchSnapshot();
});
});