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

misc: simplifications in simulator/connection-pool #7894

Merged
merged 2 commits into from Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-core/computed/network-analysis.js
Expand Up @@ -57,7 +57,7 @@ class NetworkAnalysis {
const records = await NetworkRecords.request(devtoolsLog, context);
const throughput = NetworkAnalyzer.estimateThroughput(records);
const rttAndServerResponseTime = NetworkAnalysis.computeRTTAndServerResponseTime(records);
return {records, throughput, ...rttAndServerResponseTime};
return {throughput, ...rttAndServerResponseTime};
}
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/computed/page-dependency-graph.js
Expand Up @@ -133,9 +133,9 @@ class PageDependencyGraph {
rootNode.addDependent(node);
}

const redirects = Array.from(node.record.redirects || []);
redirects.push(node.record);
if (!node.record.redirects) return;

const redirects = [...node.record.redirects, node.record];
for (let i = 1; i < redirects.length; i++) {
const redirectNode = networkNodeOutput.idToNodeMap.get(redirects[i - 1].requestId);
const actualNode = networkNodeOutput.idToNodeMap.get(redirects[i].requestId);
Expand Down
22 changes: 16 additions & 6 deletions lighthouse-core/lib/dependency-graph/simulator/connection-pool.js
Expand Up @@ -139,14 +139,10 @@ module.exports = class ConnectionPool {
* @return {?TcpConnection}
*/
acquire(record, options = {}) {
if (this._connectionsByRecord.has(record)) {
// @ts-ignore
return this._connectionsByRecord.get(record);
}
if (this._connectionsByRecord.has(record)) throw new Error('Record already has a connection');

const origin = String(record.parsedURL.securityOrigin);
const origin = record.parsedURL.securityOrigin;
const observedConnectionWasReused = !!this._connectionReusedByRequestId.get(record.requestId);
/** @type {TcpConnection[]} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't necessary anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't necessary anymore?

no, its inferred from the first part of the init and the _findAvailableConnectionWithLargestCongestionWindow callsite

const connections = this._connectionsByOrigin.get(origin) || [];
const connectionToUse = this._findAvailableConnectionWithLargestCongestionWindow(connections, {
ignoreConnectionReused: options.ignoreConnectionReused,
Expand All @@ -160,6 +156,20 @@ module.exports = class ConnectionPool {
return connectionToUse;
}

/**
* 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
* @return {TcpConnection}
*/
acquireActiveConnectionFromRecord(record) {
const activeConnection = this._connectionsByRecord.get(record);
if (!activeConnection) throw new Error('Could not find an active connection for record');

return activeConnection;
}

/**
* @param {LH.Artifacts.NetworkRequest} record
*/
Expand Down
8 changes: 3 additions & 5 deletions lighthouse-core/lib/dependency-graph/simulator/simulator.js
Expand Up @@ -300,8 +300,7 @@ class Simulator {
const sizeInMb = (record.resourceSize || 0) / 1024 / 1024;
timeElapsed = 8 + 20 * sizeInMb - timingData.timeElapsed;
} else {
// If we're estimating time remaining, we already acquired a connection for this record, definitely non-null
const connection = /** @type {TcpConnection} */ (this._acquireConnection(record));
const connection = this._connectionPool.acquireActiveConnectionFromRecord(record);
const dnsResolutionTime = this._dns.getTimeUntilResolution(record, {
requestedAt: timingData.startTime,
shouldUpdateCache: true,
Expand Down Expand Up @@ -352,8 +351,7 @@ class Simulator {
if (node.type !== BaseNode.TYPES.NETWORK) throw new Error('Unsupported');

const record = node.record;
// If we're updating the progress, we already acquired a connection for this record, definitely non-null
const connection = /** @type {TcpConnection} */ (this._acquireConnection(record));
const connection = this._connectionPool.acquireActiveConnectionFromRecord(record);
const dnsResolutionTime = this._dns.getTimeUntilResolution(record, {
requestedAt: timingData.startTime,
shouldUpdateCache: true,
Expand Down Expand Up @@ -503,7 +501,7 @@ module.exports = Simulator;
* @typedef NodeTimingIntermediate
* @property {number} [startTime]
* @property {number} [endTime]
* @property {number} [queuedTime]
* @property {number} [queuedTime] Helpful for debugging.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

* @property {number} [estimatedTimeElapsed]
* @property {number} [timeElapsed]
* @property {number} [timeElapsedOvershoot]
Expand Down
Expand Up @@ -85,8 +85,8 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
const connectionForA = pool.acquire(recordA);
const connectionForB = pool.acquire(recordB);
for (let i = 0; i < 10; i++) {
assert.equal(pool.acquire(recordA), connectionForA);
assert.equal(pool.acquire(recordB), connectionForB);
assert.equal(pool.acquireActiveConnectionFromRecord(recordA), connectionForA);
assert.equal(pool.acquireActiveConnectionFromRecord(recordB), connectionForB);
}

assert.deepStrictEqual(pool.connectionsInUse(), [connectionForA, connectionForB]);
Expand Down Expand Up @@ -152,7 +152,8 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
}

assert.ok(pool.acquire(coldRecord, opts), 'should have acquired connection');
assert.ok(pool.acquire(warmRecord, opts), 'should have acquired connection');
assert.ok(pool.acquireActiveConnectionFromRecord(warmRecord, opts),
'should have acquired connection');
});

it('should acquire in order of warmness', () => {
Expand Down
1 change: 0 additions & 1 deletion types/artifacts.d.ts
Expand Up @@ -376,7 +376,6 @@ declare global {
}

export interface NetworkAnalysis {
records: Array<NetworkRequest>;
rtt: number;
additionalRttByOrigin: Map<string, number>;
serverResponseTimeByOrigin: Map<string, number>;
Expand Down