Repository: GoogleChrome/lighthouse
Pull request #7894 (2019-04-02 00:43:49)

misc: simplifications in simulator/connection-pool
another one I accumulated while stepping through the simulator. Feel free to reject anything @patrickhulce :)

1) remove `records` from `LH.Artifacts.NetworkAnalysis`. The caller of the computed artifact has to already have them to get the `NetworkAnalysis`, and we never use them on any instance of the type anyways.
2) early return in `PageDependencyGraph.linkNetworkNodes()` for the common case where there were no redirects of a particular record
3) split `ConnectionPool.acquire()` into the case where a connection is needed to fetch a record and the case where a connection is already fetching a record.

   This seems ok as a place where the generality hasn't proven necessary...we only call `acquire()` in three places, and the two done that return assigned connections already explicitly call out with a comment and a type cast that it knows a connection already exists for this record.
4) removed `NodeTimingIntermediate.queuedTime` as it isn't used for anything. I can add back if this was useful for debugging or something.


Patch:
diff --git a/lighthouse-core/computed/network-analysis.js b/lighthouse-core/computed/network-analysis.js
index 07784eb78..3db08d2ff 100644
--- a/lighthouse-core/computed/network-analysis.js
+++ b/lighthouse-core/computed/network-analysis.js
@@ -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};
   }
 }
 
diff --git a/lighthouse-core/computed/page-dependency-graph.js b/lighthouse-core/computed/page-dependency-graph.js
index e575940c8..2a9a93442 100644
--- a/lighthouse-core/computed/page-dependency-graph.js
+++ b/lighthouse-core/computed/page-dependency-graph.js
@@ -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);
diff --git a/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js b/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js
index caa4f7e73..8f3e29d2c 100644
--- a/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js
+++ b/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js
@@ -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[]} */
     const connections = this._connectionsByOrigin.get(origin) || [];
     const connectionToUse = this._findAvailableConnectionWithLargestCongestionWindow(connections, {
       ignoreConnectionReused: options.ignoreConnectionReused,
@@ -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
    */
diff --git a/lighthouse-core/lib/dependency-graph/simulator/simulator.js b/lighthouse-core/lib/dependency-graph/simulator/simulator.js
index 9c6651520..714e43f48 100644
--- a/lighthouse-core/lib/dependency-graph/simulator/simulator.js
+++ b/lighthouse-core/lib/dependency-graph/simulator/simulator.js
@@ -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,
@@ -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,
@@ -503,7 +501,7 @@ module.exports = Simulator;
  * @typedef NodeTimingIntermediate
  * @property {number} [startTime]
  * @property {number} [endTime]
- * @property {number} [queuedTime]
+ * @property {number} [queuedTime] Helpful for debugging.
  * @property {number} [estimatedTimeElapsed]
  * @property {number} [timeElapsed]
  * @property {number} [timeElapsedOvershoot]
diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts
index 6bd614d4d..c1ae0516e 100644
--- a/types/artifacts.d.ts
+++ b/types/artifacts.d.ts
@@ -376,7 +376,6 @@ declare global {
       }
 
       export interface NetworkAnalysis {
-        records: Array<NetworkRequest>;
         rtt: number;
         additionalRttByOrigin: Map<string, number>;
         serverResponseTimeByOrigin: Map<string, number>;


Test patch:
diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js
index 601334af8..23157362c 100644
--- a/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js
+++ b/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js
@@ -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]);
@@ -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', () => {