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): resolve some differences when using trace #16033

Merged
merged 11 commits into from
Jun 4, 2024
2 changes: 1 addition & 1 deletion core/audits/byte-efficiency/byte-efficiency-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class ByteEfficiencyAudit extends Audit {
const originalTransferSizes = new Map();
graph.traverse(node => {
if (node.type !== 'network') return;
const wastedBytes = wastedBytesByUrl.get(node.record.url);
const wastedBytes = wastedBytesByUrl.get(node.request.url);
if (!wastedBytes) return;

const original = node.request.transferSize;
Expand Down
2 changes: 1 addition & 1 deletion core/audits/byte-efficiency/offscreen-images.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
if (node.type === 'cpu' && timing.duration >= 50) {
lastLongTaskStartTime = Math.max(lastLongTaskStartTime, timing.startTime);
} else if (node.type === 'network') {
startTimesByURL.set(node.record.url, timing.startTime);
startTimesByURL.set(node.request.url, timing.startTime);
}
}

Expand Down
10 changes: 5 additions & 5 deletions core/audits/byte-efficiency/render-blocking-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function computeStackSpecificTiming(node, nodeTiming, Stacks) {
// https://github.com/ampproject/amphtml/blob/8e03ac2f315774070651584a7e046ff24212c9b1/src/font-stylesheet-timeout.js#L54-L59
// Any potential savings must only include time spent on AMP stylesheet nodes before 2.1 seconds.
if (node.type === BaseNode.TYPES.NETWORK &&
node.record.resourceType === NetworkRequest.TYPES.Stylesheet &&
node.request.resourceType === NetworkRequest.TYPES.Stylesheet &&
nodeTiming.endTime > 2100) {
stackSpecificTiming.endTime = Math.max(nodeTiming.startTime, 2100);
stackSpecificTiming.duration = stackSpecificTiming.endTime - stackSpecificTiming.startTime;
Expand Down Expand Up @@ -228,11 +228,11 @@ class RenderBlockingResources extends Audit {
if (node.type !== BaseNode.TYPES.NETWORK) return !canDeferRequest;

const isStylesheet =
node.record.resourceType === NetworkRequest.TYPES.Stylesheet;
node.request.resourceType === NetworkRequest.TYPES.Stylesheet;
if (canDeferRequest && isStylesheet) {
// We'll inline the used bytes of the stylesheet and assume the rest can be deferred
const wastedBytes = wastedCssBytesByUrl.get(node.record.url) || 0;
totalChildNetworkBytes += (node.record.transferSize || 0) - wastedBytes;
const wastedBytes = wastedCssBytesByUrl.get(node.request.url) || 0;
totalChildNetworkBytes += (node.request.transferSize || 0) - wastedBytes;
}
return !canDeferRequest;
});
Expand All @@ -247,7 +247,7 @@ class RenderBlockingResources extends Audit {
));

// Add the inlined bytes to the HTML response
const originalTransferSize = minimalFCPGraph.record.transferSize;
const originalTransferSize = minimalFCPGraph.request.transferSize;
const safeTransferSize = originalTransferSize || 0;
minimalFCPGraph.request.transferSize = safeTransferSize + totalChildNetworkBytes;
const estimateAfterInline = simulator.simulate(minimalFCPGraph).timeInMs;
Expand Down
4 changes: 2 additions & 2 deletions core/audits/dobetterweb/uses-http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ class UsesHTTP2Audit extends Audit {
const originalProtocols = new Map();
graph.traverse(node => {
if (node.type !== 'network') return;
if (!urlsToChange.has(node.record.url)) return;
if (!urlsToChange.has(node.request.url)) return;

originalProtocols.set(node.request.requestId, node.record.protocol);
originalProtocols.set(node.request.requestId, node.request.protocol);
node.request.protocol = 'h2';
});

Expand Down
2 changes: 1 addition & 1 deletion core/audits/prioritize-lcp-image.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ class PrioritizeLcpImage extends Audit {
wastedMs,
results: [{
node: Audit.makeNodeItem(lcpElement.node),
url: lcpNode.record.url,
url: lcpNode.request.url,
wastedMs,
}],
};
Expand Down
4 changes: 2 additions & 2 deletions core/audits/uses-rel-preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,14 @@ class UsesRelPreconnectAudit extends Audit {
/** @type {Set<string>} */
const lcpGraphURLs = new Set();
lcpGraph.traverse(node => {
if (node.type === 'network') lcpGraphURLs.add(node.record.url);
if (node.type === 'network') lcpGraphURLs.add(node.request.url);
});

const fcpGraph =
await LanternFirstContentfulPaint.getPessimisticGraph(pageGraph, processedNavigation);
const fcpGraphURLs = new Set();
fcpGraph.traverse(node => {
if (node.type === 'network') fcpGraphURLs.add(node.record.url);
if (node.type === 'network') fcpGraphURLs.add(node.request.url);
});

/** @type {Map<string, LH.Artifacts.NetworkRequest[]>} */
Expand Down
1 change: 1 addition & 0 deletions core/computed/metrics/lantern-metric.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ async function getComputationDataParamsFromTrace(data, context) {
}

const {trace, URL} = data;
// TODO(15841): use computed artifact.
const {graph} = await createGraphFromTrace(URL, trace, context);
const processedNavigation = await ProcessedNavigation.request(data.trace, context);
const simulator = data.simulator || (await LoadSimulator.request(data, context));
Expand Down
1 change: 0 additions & 1 deletion core/lib/lantern/network-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ const NON_NETWORK_SCHEMES = [
];

/**
* Use `NetworkRequest.isNonNetworkRequest(req)` if working with a request.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

drive by, stale comment

* Note: the `protocol` field from CDP can be 'h2', 'http', (not 'https'!) or it'll be url's scheme.
* https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/protocol/network_handler.cc;l=598-611;drc=56d4a9a9deb30be73adcee8737c73bcb2a5ab64f
* However, a `new URL(href).protocol` has a colon suffix.
Expand Down
60 changes: 46 additions & 14 deletions core/lib/lantern/page-dependency-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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;
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 always the transfer size of a redirect?

Copy link
Collaborator Author

@connorjclark connorjclark Jun 4, 2024

Choose a reason for hiding this comment

The 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);
}
Expand Down
2 changes: 1 addition & 1 deletion core/lib/lantern/simulator/simulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const NodeState = {
Complete: 3,
};

/** @type {Record<NetworkNode['record']['priority'], number>} */
/** @type {Record<NetworkNode['request']['priority'], number>} */
const PriorityStartTimePenalty = {
VeryHigh: 0,
High: 0.25,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 (linkCPUNodes)


it('should use LCP request savings if larger than LCP graph savings', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,11 +313,6 @@ describe('DuplicatedJavascript computed artifact', () => {
});

it('.audit', async () => {
// TODO(15841): investigate failures
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

const artifacts = await loadArtifacts(`${LH_ROOT}/core/test/fixtures/artifacts/cnn`);
const ultraSlowThrottling = {rttMs: 150, throughputKbps: 100, cpuSlowdownMultiplier: 8};
const settings = {throttlingMethod: 'simulate', throttling: ultraSlowThrottling};
Expand Down
5 changes: 0 additions & 5 deletions core/test/audits/byte-efficiency/offscreen-images-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ function generateImage({
}

describe('OffscreenImages audit', () => {
// TODO(15841): investigate test failures
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

let context;
const DEFAULT_DIMENSIONS = {innerWidth: 1920, innerHeight: 1080};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ const devtoolsLog = readJson('../../fixtures/artifacts/render-blocking/devtoolsl
const mobileSlow4G = constants.throttling.mobileSlow4G;

describe('Render blocking resources audit', () => {
// TODO(15841): investigate failures
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

it('evaluates render blocking input correctly', async () => {
const artifacts = {
URL: getURLArtifactFromDevtoolsLog(devtoolsLog),
Expand Down
13 changes: 3 additions & 10 deletions core/test/audits/dobetterweb/dom-size-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,17 @@ describe('DOMSize audit', () => {

beforeEach(() => {
const mainDocumentUrl = 'https://example.com/';

const networkRecords = [{url: mainDocumentUrl, priority: 'High'}];
const trace = createTestTrace({
topLevelTasks: [
{ts: 1000, duration: 1000, children: [
{ts: 1100, duration: 200, eventName: 'ScheduleStyleRecalculation'},
]},
],
networkRecords,
});

const devtoolsLog = networkRecordsToDevtoolsLog([{
url: mainDocumentUrl,
priority: 'High',
}]);
const devtoolsLog = networkRecordsToDevtoolsLog(networkRecords);

artifacts = {
DOMStats: {
Expand All @@ -55,11 +53,6 @@ describe('DOMSize audit', () => {
});

it('calculates score hitting mid distribution', async () => {
// TODO(15841): investigate failures
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

const auditResult = await DOMSize.audit(artifacts, context);
assert.equal(auditResult.score, 0.43);
assert.equal(auditResult.numericValue, 1500);
Expand Down
17 changes: 6 additions & 11 deletions core/test/audits/largest-contentful-paint-element-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function mockNetworkRecords() {
isLinkPreload: false,
networkRequestTime: 0,
networkEndTime: 500,
timing: {sendEnd: 0, receiveHeadersEnd: 500},
responseHeadersEndTime: 500,
responseHeadersTransferSize: 400,
transferSize: 400,
url: requestedUrl,
Expand Down Expand Up @@ -68,11 +68,6 @@ function mockNetworkRecords() {
}

describe('Performance: largest-contentful-paint-element audit', () => {
// TODO(15841): investigate failures
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

it('correctly surfaces the LCP element', async () => {
const networkRecords = mockNetworkRecords();
const artifacts = {
Expand Down Expand Up @@ -110,8 +105,8 @@ describe('Performance: largest-contentful-paint-element audit', () => {

expect(auditResult.score).toEqual(0);
expect(auditResult.notApplicable).toBeUndefined();
expect(auditResult.displayValue).toBeDisplayString('5,800\xa0ms');
expect(auditResult.metricSavings).toEqual({LCP: 3304}); // 5804 - 2500 (p10 mobile)
expect(auditResult.displayValue).toBeDisplayString('5,340\xa0ms');
expect(auditResult.metricSavings).toEqual({LCP: 2837}); // calculated LCP - 2500 (p10 mobile)
expect(auditResult.details.items).toHaveLength(2);
expect(auditResult.details.items[0].items).toHaveLength(1);
expect(auditResult.details.items[0].items[0].node.path).toEqual('1,HTML,3,BODY,5,DIV,0,HEADER');
Expand All @@ -123,11 +118,11 @@ describe('Performance: largest-contentful-paint-element audit', () => {
expect(auditResult.details.items[1].items[0].phase).toBeDisplayString('TTFB');
expect(auditResult.details.items[1].items[0].timing).toBeCloseTo(800, 0.1);
expect(auditResult.details.items[1].items[1].phase).toBeDisplayString('Load Delay');
expect(auditResult.details.items[1].items[1].timing).toBeCloseTo(651, 0.1);
expect(auditResult.details.items[1].items[1].timing).toBeCloseTo(534.2, 0.1);
expect(auditResult.details.items[1].items[2].phase).toBeDisplayString('Load Time');
expect(auditResult.details.items[1].items[2].timing).toBeCloseTo(1813.7, 0.1);
expect(auditResult.details.items[1].items[2].timing).toBeCloseTo(1667.8, 0.1);
expect(auditResult.details.items[1].items[3].phase).toBeDisplayString('Render Delay');
expect(auditResult.details.items[1].items[3].timing).toBeCloseTo(2539.2, 0.1);
expect(auditResult.details.items[1].items[3].timing).toBeCloseTo(2334.9, 0.1);
});

it('doesn\'t throw an error when there is nothing to show', async () => {
Expand Down
5 changes: 0 additions & 5 deletions core/test/audits/prioritize-lcp-image-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ const scriptUrl = 'http://www.example.com/script.js';
const imageUrl = 'http://www.example.com/image.png';

describe('Performance: prioritize-lcp-image audit', () => {
// TODO(15841): fix createTestTrace, cycles
if (process.env.INTERNAL_LANTERN_USE_TRACE !== undefined) {
return;
}

const mockArtifacts = (networkRecords, URL) => {
return {
GatherContext: {gatherMode: 'navigation'},
Expand Down
Loading
Loading