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(simulator): convert node timings to trace #5350

Merged
merged 13 commits into from
Jun 12, 2018
Merged

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented May 24, 2018

note: test failures blocked on #5377

running the CLI with --save-assets will now output the optimistic FCP and pessimistic TTI traces that lantern computed, waaaaaaay easier for debugging and whatnot

next steps I'll add labels for the various opportunities so those are easier to debug as well

the code's a bit big for asset-saver so it might make sense to pull it out, but I'm drawing a blank where it should live, straight in lib? next to simulator? what do we call it?

CNN TTI

image

CNN FCP

image

@paulirish
Copy link
Member

cc @benschwarz as a model for simulation->HAR

@@ -283,6 +436,13 @@ async function saveAssets(artifacts, audits, pathWithBasename) {
log.log('saveAssets', 'trace file streamed to disk: ' + streamTraceFilename);
});

for (const [label, nodeTimings] of Simulator.COMPUTED_NODE_TIMINGS) {
if (!LANTERN_TRACES_TO_SAVE.includes(label) && !process.env.LANTERN_DEBUG) continue;
Copy link
Member

Choose a reason for hiding this comment

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

let's wrap this whole thing in a conditional to check the env variable.

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 {LH.TraceEvent}
*/
function createFakeTracingStarted() {
const data = {
Copy link
Member

Choose a reason for hiding this comment

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

argsData

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

function createFakeTracingStarted() {
const data = {
frameTreeNodeId: 1,
sessionId: '1.1',
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, when i did this, sessionId was the only required args.data prop, but this was before persistentIds..

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'm not sure they're necessary, but they're all present 🤷‍♂️

...baseEvent,
ts: baseTs - 1e5,
ph: 'I',
s: 't',
Copy link
Member

Choose a reason for hiding this comment

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

i can't find what this "s" is. not seeing it in trace_event source or one of my example traces. you sure about it?

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 see it for all Resource* and TracingStarted* events, but I'm not sure it's necessary. Prefer I remove?

Copy link
Member

Choose a reason for hiding this comment

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

image

sgtm. let's keep it.

* @param {LH.Gatherer.Simulation.Result['nodeTimings']} nodeTimings
* @return {LH.Trace}
*/
function convertNodeTimingsToTrace(nodeTimings) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move these fn's into their own file and require into asset-saver

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

mimeType: record._mimeType,
encodedDataLength: record._transferSize,
fromCache: false,
fromServiceWorker: false,
Copy link
Member

Choose a reason for hiding this comment

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

fromServiceWorker: record.fetchedViaServiceWorker

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

{
...baseRequestEvent,
name: 'ResourceReceiveResponse',
ts: ts(endTime),
Copy link
Member

Choose a reason for hiding this comment

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

wdyt about (startTime + endTime) / 2 instead?? makes for a more attractive visual.

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


const requestData = {requestId: requestId.toString(), frame};
/** @type {Omit<LH.TraceEvent, 'name'|'ts'|'args'>} */
const baseRequestEvent = {...baseEvent, ph: 'I', s: 't', dur: 0};
Copy link
Member

Choose a reason for hiding this comment

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

same question about '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.

same response :)

* @param {LH.Gatherer.Simulation.NodeTiming} timing
* @return {LH.TraceEvent[]}
*/
function createFakeNetwork(record, timing) {
Copy link
Member

Choose a reason for hiding this comment

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

createFakeNetworkTraceEvents

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

* @param {{startTime: number, endTime: number}} timing
* @return {LH.TraceEvent}
*/
function createFakeEvaluateScript(cpuNode, timing) {
Copy link
Member

Choose a reason for hiding this comment

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

createFakeEvaluateScriptEvent

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

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

left some new comments within like 3 of the comment blocks above. awkward UX.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

k given that we're punting child events to a new PR, the only thing left here is the trace length of network nodes.

// 0ms requests get super-messed up rendering
// Use 20ms instead so they're still hoverable
let {startTime, endTime} = timing; // eslint-disable-line prefer-const
if (startTime === endTime) endTime += 20;
Copy link
Member

Choose a reason for hiding this comment

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

I still feel uncomfortable changing the data purely for flamechart UX reasons. E.g. a network request that must start after a previous one finishes won't now. Feels weird, man.

How about... We do know there's pretty high overhead to create a network request. This is something we've discussed, but simulator doesn't acknowledge just yet. Let's add it and say request creation costs ~0.3ms or thereabouts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright 0.3ms it is 👍

@paulirish
Copy link
Member

@patrickhulce can you file an issue for the followups we were talking about? (though you probably already have a branch ready for PR.. :)

@paulirish paulirish merged commit 5237ce7 into master Jun 12, 2018
@paulirish paulirish deleted the lantern_to_trace branch June 12, 2018 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants