-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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): resolve some differences when using trace #16033
Changes from all commits
2961353
508b46a
aa54aa2
19dd64d
a942e0a
2a12f9a
ed0ef5c
4730552
7f85f19
6ccb990
67f55a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -712,6 +712,19 @@ class PageDependencyGraph { | |
if (request.args.data.initiator?.fetchType === 'xmlhttprequest') { | ||
// @ts-expect-error yes XHR is a valid ResourceType. TypeScript const enums are so unhelpful. | ||
resourceType = 'XHR'; | ||
} else if (request.args.data.initiator?.fetchType === 'fetch') { | ||
// @ts-expect-error yes Fetch is a valid ResourceType. TypeScript const enums are so unhelpful. | ||
resourceType = 'Fetch'; | ||
} | ||
|
||
// TODO: set decodedBodyLength for data urls in Trace Engine. | ||
let resourceSize = request.args.data.decodedBodyLength ?? 0; | ||
if (url.protocol === 'data:' && resourceSize === 0) { | ||
const needle = 'base64,'; | ||
const index = url.pathname.indexOf(needle); | ||
if (index !== -1) { | ||
resourceSize = atob(url.pathname.substring(index + needle.length)).length; | ||
} | ||
} | ||
|
||
return { | ||
|
@@ -727,7 +740,7 @@ class PageDependencyGraph { | |
responseHeadersEndTime: request.args.data.syntheticData.downloadStart / 1000, | ||
networkEndTime: request.args.data.syntheticData.finishTime / 1000, | ||
transferSize: request.args.data.encodedDataLength, | ||
resourceSize: request.args.data.decodedBodyLength, | ||
resourceSize, | ||
fromDiskCache: request.args.data.syntheticData.isDiskCached, | ||
fromMemoryCache: request.args.data.syntheticData.isMemoryCached, | ||
isLinkPreload: request.args.data.isLinkPreload, | ||
|
@@ -800,23 +813,42 @@ class PageDependencyGraph { | |
const requestChain = []; | ||
for (const redirect of redirects) { | ||
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. | ||
redirectedRequest.timing.requestTime = redirect.ts / 1000 / 1000; | ||
redirectedRequest.timing.receiveHeadersStart -= redirect.dur / 1000 / 1000; | ||
redirectedRequest.timing.receiveHeadersEnd -= redirect.dur / 1000 / 1000; | ||
redirectedRequest.rendererStartTime = redirect.ts / 1000; | ||
redirectedRequest.networkRequestTime = redirect.ts / 1000; | ||
redirectedRequest.networkEndTime = (redirect.ts + redirect.dur) / 1000; | ||
redirectedRequest.responseHeadersEndTime = | ||
redirectedRequest.timing.requestTime * 1000 + | ||
redirectedRequest.timing.receiveHeadersEnd; | ||
} | ||
|
||
redirectedRequest.networkRequestTime = redirect.ts / 1000; | ||
redirectedRequest.rendererStartTime = redirectedRequest.networkRequestTime; | ||
|
||
redirectedRequest.networkEndTime = (redirect.ts + redirect.dur) / 1000; | ||
redirectedRequest.responseHeadersEndTime = redirectedRequest.networkEndTime; | ||
|
||
redirectedRequest.timing = { | ||
requestTime: redirectedRequest.networkRequestTime / 1000, | ||
receiveHeadersStart: redirectedRequest.responseHeadersEndTime, | ||
receiveHeadersEnd: redirectedRequest.responseHeadersEndTime, | ||
proxyStart: -1, | ||
proxyEnd: -1, | ||
dnsStart: -1, | ||
dnsEnd: -1, | ||
connectStart: -1, | ||
connectEnd: -1, | ||
sslStart: -1, | ||
sslEnd: -1, | ||
sendStart: -1, | ||
sendEnd: -1, | ||
workerStart: -1, | ||
workerReady: -1, | ||
workerFetchStart: -1, | ||
workerRespondWithSettled: -1, | ||
pushStart: -1, | ||
pushEnd: -1, | ||
}; | ||
|
||
redirectedRequest.url = redirect.url; | ||
redirectedRequest.parsedURL = this._createParsedUrl(redirect.url); | ||
// TODO: TraceEngine is not retaining the actual status code. | ||
redirectedRequest.statusCode = 302; | ||
redirectedRequest.resourceType = undefined; | ||
// TODO: TraceEngine is not retaining transfer size of redirected request. | ||
redirectedRequest.transferSize = 400; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this always the transfer size of a redirect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that depends on the headers sent. 400 is just exactly the size of a trace I was debugging. But otherwise it would be the size of the final response and so this avoids an overestimate for the time being. |
||
requestChain.push(redirectedRequest); | ||
lanternRequests.push(redirectedRequest); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,11 +18,6 @@ const trace = readJson('../../fixtures/artifacts/paul/trace.json', import.meta); | |
const devtoolsLog = readJson('../../fixtures/artifacts/paul/devtoolslog.json', import.meta); | ||
|
||
describe('Byte efficiency base audit', () => { | ||
// TODO(15841): investigate failures | ||
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) { | ||
return; | ||
} | ||
|
||
let simulator; | ||
let metricComputationInput; | ||
|
||
|
@@ -131,7 +126,7 @@ describe('Byte efficiency base audit', () => { | |
}, simulator, metricComputationInput, {computedCache: new Map()}); | ||
|
||
assert.equal(result.metricSavings.FCP, 900); | ||
assert.equal(result.metricSavings.LCP, 1350); | ||
assert.equal(result.metricSavings.LCP, 900); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the results changed here b/c createTestTrace is now always producing network events (not just when the env variable is true), so in the graph there is now a new dependency added from a network node to CPU node, cuz of the new ResourceSendRequest event (added in createTestTrace) linking the two ( |
||
|
||
it('should use LCP request savings if larger than LCP graph savings', async () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by, stale comment