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(lantern): create network graph from trace (experimental) #16026

Merged
merged 65 commits into from
Jun 3, 2024

Conversation

connorjclark
Copy link
Collaborator

This PR does not change the default behavior of Lighthouse.

The most interesting part of this PR is createGraphFromTrace - given a trace (and TraceEngine result), produces the network records from the trace. Also handles making the CPU nodes too and returns the graph.

In Lighthouse, CDP is still used for the network portion of the graph, except in the presence of INTERNAL_LANTERN_USE_TRACE=1 env variable. This includes the lantern smoke tests. The CI scripts have been modified minimally to run both variants. When we switch to using the trace totally, this internal control should be removed.

Note that the unit tests in core/test/lib/lantern only use the trace to build the graph - INTERNAL_LANTERN_USE_TRACE is only for Lighthouse tests.

The only change in these Lantern unit test results is core/test/lib/lantern/metrics/lantern-largest-contentful-paint-test.js - a minor difference in the results due to missing requests in the trace for pre-flight CORs, which doesn't actually impact the simulation except for having one less sample when estimating the RTT/throughput. Adding these requests to the trace is non-trivial and this change in results does not yet warrant further work.

ref #15841

static async createGraphFromTrace(mainThreadEvents, trace, traceEngineResult, URL) {
// TODO: trace engine should handle this.
static _findServiceWorkerThreads(trace) {
// TODO: trace engine should provide a map of service workers, like it does for workers.
Copy link
Member

Choose a reason for hiding this comment

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

it's possible that map already includes (same-process) serviceworkers… do you know it doesn't?
regardless having pid matching for it too seems like a requirement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, by looking at core/test/fixtures/traces/amp-m86.trace.json I see that service worker threads also get the same events a worker thread does for purposes of WorkersHandler

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 modified this method to account for all workers, so we build our own comprehensive pid/tid worker map.

@@ -53,7 +53,7 @@ function getNodesAndTimingByRequestId(nodeTimings) {
for (const [node, nodeTiming] of nodeTimings) {
if (node.type !== 'network') continue;

requestIdToNode.set(node.record.requestId, {node, nodeTiming});
requestIdToNode.set(node.request.requestId, {node, nodeTiming});
Copy link
Member

Choose a reason for hiding this comment

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

yay! i so appreciate this rename. :) thanks

@connorjclark
Copy link
Collaborator Author

so far the test skips from what I've checked seem to be

  1. lacking feature parity in createTestTrace
  2. bug in redirect stuff in createGraphFromTrace
  3. could be other real bugs

@connorjclark connorjclark changed the title core(lantern): create graph from trace (experimental) core(lantern): create network graph from trace (experimental) Jun 3, 2024
const redirectedRequest = structuredClone(request);
if (redirectedRequest.timing) {
// TODO: These are surely wrong for when there is more than one redirect. Would be
// simpler if each redirect remembered it's `timing` object in this `redirects` array.
Copy link
Collaborator Author

@connorjclark connorjclark Jun 3, 2024

Choose a reason for hiding this comment

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

Just realized that our network requests don't even have timing when a request is to be redirected. Only gets set onResponse, which we don't get for redirected requests. This code was written against a test case in redirects-test that did not adhere to that constraint, so this can be disregarded.

I will send a PR that makes sure none of our mock network record tests set a timing on redirected requests, and then update this block of code.

Copy link
Collaborator Author

@connorjclark connorjclark Jun 3, 2024

Choose a reason for hiding this comment

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

I did overlook how onRedirectResponse calls onResponse, but from what I can tell redirectResponse never has a timing object

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

welll.... sometimes the timing is present on redirectResponse and sometimes its not. so my claim that "redirects don't have timings" is wrong

I'll leave this TODO as a followup, it's gonna be a tricky change to get these timings to match.

@@ -52,6 +52,12 @@ Object {

['provided', 'simulate'].forEach(throttlingMethod => {
it(`should fail to compute a value for old trace (${throttlingMethod})`, async () => {
// TODO(15841): fails... but instead w/ "No frames found in trace data" (not NO_LCP).
// ... handle error when calling code uses lantern? or update expected error? idk
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 not sure this test is particularly useful anymore. At this point any pre-LCP trace is going to be complete foobar and we shouldn't need to maintain that NO_LCP is always the first error that comes up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK - removed

package.json Outdated Show resolved Hide resolved
* @return {Map<number, number[]>}
*/
static _findWorkerThreads(trace) {
// TODO: WorkersHandler in TraceEngine needs to be updated to also include `pid` (only had `tid`).
Copy link
Member

Choose a reason for hiding this comment

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

Is this why we have this function instead of using the TraceEngine directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

core/lib/lantern/page-dependency-graph.js Show resolved Hide resolved
// There are some minor differences in the fields, accounted for here.
// Most importantly, there seems to be fewer frames in the trace than the equivalent
// events over the CDP. This results in less accuracy in determining the initiator request,
// which means less edges in the graph, which mean worse results. Should fix.
Copy link
Member

Choose a reason for hiding this comment

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

Should fix

Meaning we should add more info to the trace events?

Copy link
Collaborator Author

@connorjclark connorjclark Jun 3, 2024

Choose a reason for hiding this comment

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

yes - this is chromium work

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should track in b/324059246

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • initiator (and any relevant js callstack)

i think it is known already that these call stack frames are missing data.

cc @paulirish @adrianaixba

relevant: https://chromium-review.googlesource.com/c/chromium/src/+/5280651/7..11/third_party/blink/renderer/core/inspector/inspector_trace_events.cc#b848

"unit-report": "yarn mocha --testMatch report/**/*-test.js",
"unit-treemap": "yarn mocha --testMatch treemap/**/*-test.js",
"unit-viewer": "yarn mocha --testMatch viewer/**/*-test.js",
"unit-flow": "bash flow-report/test/run-flow-report-tests.sh",
"unit": "yarn unit-flow && yarn mocha",
"unit": "yarn unit-flow && yarn mocha && yarn unit-lantern-trace",
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 this will still run the lantern-trace tests as part of the normal unit test step (in addition to the new CI step you just added). IDK maybe it would be to confusing to separate this so I won't die on this hill.

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 don't want to create a new step to really run all the unit tests locally, when it will go away soon.

}

traceEvents.push({
name: 'ResourceReceiveResponse',
Copy link
Member

Choose a reason for hiding this comment

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

What about ResourceReceivedData / ResourceChangePriority? Do we just not need those for this PR?

Copy link
Collaborator Author

@connorjclark connorjclark Jun 3, 2024

Choose a reason for hiding this comment

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

ResourceReceivedData

never used by trace engine

ResourceChangePriority

should be added, but may not be necessary. dont think we even set priority / changing priority in unit tests

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Assuming all of the TODOs will be addressed as follow-ups before closing #15841 I think this is a really good first step! Thanks Connor!

@connorjclark connorjclark merged commit cc62c81 into main Jun 3, 2024
27 checks passed
@connorjclark connorjclark deleted the lantern-from-trace branch June 3, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants