core(lantern): extract main thread events w/o TraceProcessor #16049
Conversation
| } | ||
|
|
||
| return this._filteredTraceSort(trace.traceEvents, e => rendererPidToTid.get(e.pid) === e.tid); | ||
| } |
There was a problem hiding this comment.
w/o _filteredTraceSort(instead of just array.filter) no tests fail ... so is it needed?
There was a problem hiding this comment.
I don't think it's needed. I looked at how we use main thread events in lantern and we just look for top level CPU tasks. Those should really never have the same timestamp which seems to be the purpose of that function.
There was a problem hiding this comment.
seems convincing
| */ | ||
| static _collectMainThreadEvents(trace, traceEngineResult) { | ||
| const Meta = traceEngineResult.data.Meta; | ||
| const rendererPidToTid = new Map(); |
There was a problem hiding this comment.
rendererPidToTid here gets constructed with different results than TraceProcessor was doing ...
consider the first test in core/test/computed/metrics/speed-index-test.js (should compute a simulated value) as an example, TraceProcessor only has one renderer in this map, but this version has two. the extra one is the initial about:blank renderer. this seems like something to avoid ... it happens b/c the about:blank is in topLevelRendererIds. I'm not yet sure how TraceProcessor ends up excluding it.
the result is more events end up in mainThreadEvents
There was a problem hiding this comment.
ok I think const mainFramePids = Meta.mainFrameNavigations.map(nav => nav.pid); instead of using topLevelRendererIds kinda resolves this. Not sure what would happen w/o any navigation though .... so I'll fall back to topLevelRendererIds in that case.
| } | ||
|
|
||
| return this._filteredTraceSort(trace.traceEvents, e => rendererPidToTid.get(e.pid) === e.tid); | ||
| } |
There was a problem hiding this comment.
I don't think it's needed. I looked at how we use main thread events in lantern and we just look for top level CPU tasks. Those should really never have the same timestamp which seems to be the purpose of that function.
| const Meta = traceEngineResult.data.Meta; | ||
| const mainFramePids = Meta.mainFrameNavigations.length | ||
| ? new Set(Meta.mainFrameNavigations.map(nav => nav.pid)) | ||
| : Meta.topLevelRendererIds; |
There was a problem hiding this comment.
Why not just use Meta.topLevelRendererIds all the time? Seems like it represents what we want?
There was a problem hiding this comment.
see #16049 (comment)
There was a problem hiding this comment.
Approving cuz I'm kinda just throwing ideas against the wall but I also saw rendererProcessesByFrame in the trace processor. It isn't fed by main frame navigations but it is fed by CommitLoad which seems similar.
connorjclark
changed the title
connorjclark
changed the title
Lantern needs just the main thread events (regardless of navigations), so it used
lib/trace-processor. Now Lantern does that itself increateGraphFromTrace.ref #15841