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): add interface for network request #15845

Merged
merged 20 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ module.exports = {
vars: 'all',
args: 'after-used',
argsIgnorePattern: '(^reject$|^_+$)',
varsIgnorePattern: '(^_$|^LH$)',
varsIgnorePattern: '(^_$|^LH$|^Lantern$)',
}],
'no-cond-assign': 2,
'space-infix-ops': 2,
Expand Down
4 changes: 2 additions & 2 deletions core/audits/byte-efficiency/render-blocking-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import {LCPImageRecord} from '../../computed/lcp-image-record.js';


/** @typedef {import('../../lib/dependency-graph/simulator/simulator').Simulator} Simulator */
/** @typedef {import('../../lib/dependency-graph/base-node.js').Node} Node */
/** @typedef {import('../../lib/dependency-graph/network-node.js')} NetworkNode */
/** @typedef {import('../../lib/dependency-graph/base-node.js').Node<LH.Artifacts.NetworkRequest>} Node */
/** @typedef {import('../../lib/dependency-graph/network-node').NetworkNode<LH.Artifacts.NetworkRequest>} NetworkNode */

// Because of the way we detect blocking stylesheets, asynchronously loaded
// CSS with link[rel=preload] and an onload handler (see https://github.com/filamentgroup/loadCSS)
Expand Down
6 changes: 3 additions & 3 deletions core/computed/metrics/lantern-first-contentful-paint.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import {makeComputedArtifact} from '../computed-artifact.js';
import {LanternMetric} from './lantern-metric.js';
import {BaseNode} from '../../lib/dependency-graph/base-node.js';

/** @typedef {import('../../lib/dependency-graph/base-node.js').Node} Node */
/** @typedef {import('../../lib/dependency-graph/cpu-node').CPUNode} CPUNode */
/** @typedef {import('../../lib/dependency-graph/network-node').NetworkNode} NetworkNode */
/** @typedef {import('../../lib/dependency-graph/base-node.js').Node<LH.Artifacts.NetworkRequest>} Node */
/** @typedef {import('../../lib/dependency-graph/cpu-node').CPUNode<LH.Artifacts.NetworkRequest>} CPUNode */
/** @typedef {import('../../lib/dependency-graph/network-node').NetworkNode<LH.Artifacts.NetworkRequest>} NetworkNode */

class LanternFirstContentfulPaint extends LanternMetric {
/**
Expand Down
4 changes: 2 additions & 2 deletions core/computed/metrics/lantern-metric.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {ProcessedNavigation} from '../processed-navigation.js';
import {PageDependencyGraph} from '../page-dependency-graph.js';
import {LoadSimulator} from '../load-simulator.js';

/** @typedef {import('../../lib/dependency-graph/base-node.js').Node} Node */
/** @typedef {import('../../lib/dependency-graph/network-node').NetworkNode} NetworkNode */
/** @typedef {import('../../lib/dependency-graph/base-node.js').Node<LH.Artifacts.NetworkRequest>} Node */
/** @typedef {import('../../lib/dependency-graph/network-node').NetworkNode<LH.Artifacts.NetworkRequest>} NetworkNode */
/** @typedef {import('../../lib/dependency-graph/simulator/simulator').Simulator} Simulator */

/**
Expand Down
10 changes: 5 additions & 5 deletions core/computed/page-dependency-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import {DocumentUrls} from './document-urls.js';

/**
* @typedef {Object} NetworkNodeOutput
* @property {Array<NetworkNode>} nodes
* @property {Map<string, NetworkNode>} idToNodeMap
* @property {Map<string, Array<NetworkNode>>} urlToNodeMap
* @property {Map<string, NetworkNode|null>} frameIdToNodeMap
* @property {Array<NetworkNode<LH.Artifacts.NetworkRequest>>} nodes
* @property {Map<string, NetworkNode<LH.Artifacts.NetworkRequest>>} idToNodeMap
* @property {Map<string, Array<NetworkNode<LH.Artifacts.NetworkRequest>>>} urlToNodeMap
* @property {Map<string, NetworkNode<LH.Artifacts.NetworkRequest>|null>} frameIdToNodeMap
*/

// Shorter tasks have negligible impact on simulation results.
Expand Down Expand Up @@ -88,7 +88,7 @@ class PageDependencyGraph {
record.requestId += ':duplicate';
}

const node = new NetworkNode(record);
const node = new NetworkNode(record.asLanternNetworkRequest());
nodes.push(node);

const urlList = urlToNodeMap.get(record.url) || [];
Expand Down
11 changes: 8 additions & 3 deletions core/lib/dependency-graph/base-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
/**
* A union of all types derived from BaseNode, allowing type check discrimination
* based on `node.type`. If a new node type is created, it should be added here.
* @typedef {import('./cpu-node.js').CPUNode | import('./network-node.js').NetworkNode} Node
* @template [T=any]
* @typedef {import('./cpu-node.js').CPUNode<T> | import('./network-node.js').NetworkNode<T>} Node
Copy link
Member

Choose a reason for hiding this comment

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

Assuming CPUNode does eventually need a generic, this type asserts that T will be the same between NetworkNode and CPUNode which would never be the case. Maybe we should just remove the generic from CPUNode for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we should just remove the generic from CPUNode for now?

This is a requirement for type narrowing to continue to work.

// in graph.traverse callback...

if (node.type !== 'network') return;

// node.record should be NetworkRequest
if (!CriticalRequestChains.isCritical(node.record, mainResource)) return;

Remove the type parameter from BaseNode, CPUNode, or Node, and in the above node.record is any.

*/

/**
Expand All @@ -22,6 +23,10 @@
* This allows particular optimizations in this class so that we do no need to check for cycles as
* these methods are called and we can always start traversal at the root node.
*/

/**
* @template [T=any]
*/
class BaseNode {
/**
* @param {string} id
Expand Down Expand Up @@ -256,8 +261,8 @@ class BaseNode {
*
* The `getNextNodes` function takes a visited node and returns which nodes to
* visit next. It defaults to returning the node's dependents.
* @param {(node: Node, traversalPath: Node[]) => void} callback
* @param {function(Node): Node[]} [getNextNodes]
* @param {(node: Node<T>, traversalPath: Node<T>[]) => void} callback
* @param {function(Node<T>): Node<T>[]} [getNextNodes]
*/
traverse(callback, getNextNodes) {
for (const {node, traversalPath} of this.traverseGenerator(getNextNodes)) {
Expand Down
4 changes: 4 additions & 0 deletions core/lib/dependency-graph/cpu-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
import * as LH from '../../../types/lh.js';
import {BaseNode} from './base-node.js';

/**
* @template [T=any]
* @extends {BaseNode<T>}
*/
class CPUNode extends BaseNode {
/**
* @param {LH.TraceEvent} parentEvent
Expand Down
80 changes: 80 additions & 0 deletions core/lib/dependency-graph/lantern.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
* @license

Check failure on line 2 in core/lib/dependency-graph/lantern.d.ts

View workflow job for this annotation

GitHub Actions / basics

Cannot find namespace 'LH'.

Check failure on line 2 in core/lib/dependency-graph/lantern.d.ts

View workflow job for this annotation

GitHub Actions / basics

Cannot find namespace 'LH'.
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import * as LH from '../../../types/lh.js'

export type ParsedURL = {
/**
* Equivalent to a `new URL(url).protocol` BUT w/o the trailing colon (:)
*/
scheme: string;
/**
* Equivalent to a `new URL(url).hostname`
*/
host: string;
securityOrigin: string;
};
export type LightriderStatistics = {
/**
* The difference in networkEndTime between the observed Lighthouse networkEndTime and Lightrider's derived networkEndTime.
*/
endTimeDeltaMs: number;
/**
* The time spent making a TCP connection (connect + SSL). Note: this is poorly named.
*/
TCPMs: number;
/**
* The time spent requesting a resource from a remote server, we use this to approx RTT. Note: this is poorly names, it really should be "server response time".
*/
requestMs: number;
/**
* Time to receive the entire response payload starting the clock on receiving the first fragment (first non-header byte).
*/
responseMs: number;
};
export class NetworkRequest<T=any> {
/** The canonical network record. */
record?: T;

static get TYPES(): LH.Util.SelfMap<LH.Crdp.Network.ResourceType>;

isNonNetworkRequest(): boolean;
requestId: string;
connectionId: string;
connectionReused: boolean;
url: string;
protocol: string;
parsedURL: ParsedURL;
/** When the renderer process initially discovers a network request, in milliseconds. */
rendererStartTime: number;
/**
* When the network service is about to handle a request, ie. just before going to the
* HTTP cache or going to the network for DNS/connection setup, in milliseconds.
*/
networkRequestTime: number;
/** When the last byte of the response headers is received, in milliseconds. */
responseHeadersEndTime: number;
/** When the last byte of the response body is received, in milliseconds. */
networkEndTime: number;
transferSize: number;
resourceSize: number;
fromDiskCache: boolean;
fromMemoryCache: boolean;
// TODO(15841): remove from lantern.
/** Extra timing information available only when run in Lightrider. */
lrStatistics: LightriderStatistics | undefined;
finished: boolean;
statusCode: number;
/** The network request that this one redirected to */
redirectDestination: NetworkRequest<T> | undefined;
failed: boolean;
initiator: LH.Crdp.Network.Initiator;
timing: LH.Crdp.Network.ResourceTiming | undefined;
resourceType: LH.Crdp.Network.ResourceType | undefined;
priority: LH.Crdp.Network.ResourcePriority;
}

export {};
33 changes: 33 additions & 0 deletions core/lib/dependency-graph/lantern.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

const NetworkRequest = {
/** @type {LH.Util.SelfMap<LH.Crdp.Network.ResourceType>} */
TYPES: {
XHR: 'XHR',
Fetch: 'Fetch',
EventSource: 'EventSource',
Script: 'Script',
Stylesheet: 'Stylesheet',
Image: 'Image',
Media: 'Media',
Font: 'Font',
Document: 'Document',
TextTrack: 'TextTrack',
WebSocket: 'WebSocket',
Other: 'Other',
Manifest: 'Manifest',
SignedExchange: 'SignedExchange',
Ping: 'Ping',
Preflight: 'Preflight',
CSPViolationReport: 'CSPViolationReport',
Prefetch: 'Prefetch',
},
};

export {
NetworkRequest,
};
46 changes: 28 additions & 18 deletions core/lib/dependency-graph/network-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as LH from '../../../types/lh.js';
import * as Lantern from './lantern.js';
import {BaseNode} from './base-node.js';
import {NetworkRequest} from '../network-request.js';

/**
* @template [T=any]
* @extends {BaseNode<T>}
*/
class NetworkNode extends BaseNode {
/**
* @param {LH.Artifacts.NetworkRequest} networkRecord
* @param {Lantern.NetworkRequest<T>} networkRequest
*/
constructor(networkRecord) {
super(networkRecord.requestId);
constructor(networkRequest) {
super(networkRequest.requestId);
/** @private */
this._record = networkRecord;
this._request = networkRequest;
}

get type() {
Expand All @@ -26,42 +29,49 @@ class NetworkNode extends BaseNode {
* @return {number}
*/
get startTime() {
return this._record.rendererStartTime * 1000;
return this._request.rendererStartTime * 1000;
}

/**
* @return {number}
*/
get endTime() {
return this._record.networkEndTime * 1000;
return this._request.networkEndTime * 1000;
}

/**
* @return {LH.Artifacts.NetworkRequest}
* @return {T}
*/
get record() {
Copy link
Member

@adamraine adamraine Mar 4, 2024

Choose a reason for hiding this comment

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

I think it's kinda confusing to have record and request properties. Could we rename record to internalRecord/recordImpl or something to emphasize which one is the normal use case and which one is for debugging.

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 kept record in this PR just to prevent a big name churn in our usage of Lantern. We can rename them, in another PR to keep this one clearer. I don't know what good names would be right now.

return this._record;
return /** @type {T} */ (this._request.record);
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wouldn't expect this type cast to be necessary, are there errors if it's removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This removes undefined from the type from .record, which is that way to make our NetworkRequest assignable to Lantern.NetworkRequest. redirectDestination: NetworkRequest<T> is the issue. Resolving this won't be trivial, and should be done in a separate PR if at all.

}

/**
* @return {Lantern.NetworkRequest<T>}
*/
get request() {
return this._request;
}

/**
* @return {string}
*/
get initiatorType() {
return this._record.initiator && this._record.initiator.type;
return this._request.initiator && this._request.initiator.type;
}

/**
* @return {boolean}
*/
get fromDiskCache() {
return !!this._record.fromDiskCache;
return !!this._request.fromDiskCache;
}

/**
* @return {boolean}
*/
get isNonNetworkProtocol() {
return NetworkRequest.isNonNetworkRequest(this._record);
return this._request.isNonNetworkRequest();
}


Expand All @@ -78,19 +88,19 @@ class NetworkNode extends BaseNode {
* @return {boolean}
*/
hasRenderBlockingPriority() {
const priority = this._record.priority;
const isScript = this._record.resourceType === NetworkRequest.TYPES.Script;
const isDocument = this._record.resourceType === NetworkRequest.TYPES.Document;
const priority = this._request.priority;
const isScript = this._request.resourceType === Lantern.NetworkRequest.TYPES.Script;
const isDocument = this._request.resourceType === Lantern.NetworkRequest.TYPES.Document;
const isBlockingScript = priority === 'High' && isScript;
const isBlockingHtmlImport = priority === 'High' && isDocument;
return priority === 'VeryHigh' || isBlockingScript || isBlockingHtmlImport;
}

/**
* @return {NetworkNode}
* @return {NetworkNode<T>}
*/
cloneWithoutRelationships() {
const node = new NetworkNode(this._record);
const node = new NetworkNode(this._request);
node.setIsMainDocument(this._isMainDocument);
return node;
}
Expand Down
11 changes: 6 additions & 5 deletions core/lib/dependency-graph/simulator/connection-pool.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import * as LH from '../../../../types/lh.js';
import * as Lantern from '../lantern.js';
import {NetworkAnalyzer} from './network-analyzer.js';
import {TcpConnection} from './tcp-connection.js';

Expand All @@ -17,7 +18,7 @@ const CONNECTIONS_PER_ORIGIN = 6;

export class ConnectionPool {
/**
* @param {LH.Artifacts.NetworkRequest[]} records
* @param {Lantern.NetworkRequest[]} records
* @param {Required<LH.Gatherer.Simulation.Options>} options
*/
constructor(records, options) {
Expand All @@ -26,7 +27,7 @@ export class ConnectionPool {
this._records = records;
/** @type {Map<string, TcpConnection[]>} */
this._connectionsByOrigin = new Map();
/** @type {Map<LH.Artifacts.NetworkRequest, TcpConnection>} */
/** @type {Map<Lantern.NetworkRequest, TcpConnection>} */
this._connectionsByRecord = new Map();
this._connectionsInUse = new Set();
this._connectionReusedByRequestId = NetworkAnalyzer.estimateIfConnectionWasReused(records, {
Expand Down Expand Up @@ -124,7 +125,7 @@ export class ConnectionPool {
* If ignoreConnectionReused is true, acquire will consider all connections not in use as available.
* Otherwise, only connections that have matching "warmth" are considered available.
*
* @param {LH.Artifacts.NetworkRequest} record
* @param {Lantern.NetworkRequest} record
* @param {{ignoreConnectionReused?: boolean}} options
* @return {?TcpConnection}
*/
Expand All @@ -150,7 +151,7 @@ export class ConnectionPool {
* Return the connection currently being used to fetch a record. If no connection
* currently being used for this record, an error will be thrown.
*
* @param {LH.Artifacts.NetworkRequest} record
* @param {Lantern.NetworkRequest} record
* @return {TcpConnection}
*/
acquireActiveConnectionFromRecord(record) {
Expand All @@ -161,7 +162,7 @@ export class ConnectionPool {
}

/**
* @param {LH.Artifacts.NetworkRequest} record
* @param {Lantern.NetworkRequest} record
*/
release(record) {
const connection = this._connectionsByRecord.get(record);
Expand Down
Loading
Loading