core(lantern): always use flexible network ordering (#14612)

Co-authored-by: Connor Clark <cjamcl@google.com>

cli/test/smokehouse/test-definitions/byte-efficiency.js

@@ -96,10 +96,8 @@ const expectations = {
       'uses-http2': {
         score: 1,
         details: {
-          items: {
-            // localhost gets a free pass on uses-h2
-            length: 0,
-          },
+          // localhost gets a free pass on uses-h2
+          items: [],
         },
       },
       'unminified-css': {
@@ -153,10 +151,13 @@ const expectations = {
       },
       'unused-javascript': {
         score: '<1',
-        details: {
+        metricSavings: {
           // the specific ms value here is not meaningful for this smoketest
           // *some* savings should be reported
-          overallSavingsMs: '>0',
+          FCP: '>0',
+        },
+        details: {
+          overallSavingsMs: '>=0',
           overallSavingsBytes: '35000 +/- 1000',
           items: [
             {

core/audits/dobetterweb/uses-http2.js

@@ -78,11 +78,9 @@ class UsesHTTP2Audit extends Audit {
     options = Object.assign({label: ''}, options);
     const beforeLabel = `${this.meta.id}-${options.label}-before`;
     const afterLabel = `${this.meta.id}-${options.label}-after`;
-    const flexibleOrdering = true;
 
     const urlsToChange = new Set(results.map(result => result.url));
-    const simulationBefore =
-      simulator.simulate(graph, {label: beforeLabel, flexibleOrdering});
+    const simulationBefore = simulator.simulate(graph, {label: beforeLabel});
 
     // Update all the protocols to reflect implementing our recommendations
     /** @type {Map<string, string>} */
@@ -95,7 +93,7 @@ class UsesHTTP2Audit extends Audit {
       node.request.protocol = 'h2';
     });
 
-    const simulationAfter = simulator.simulate(graph, {label: afterLabel, flexibleOrdering});
+    const simulationAfter = simulator.simulate(graph, {label: afterLabel});
 
     // Restore the original protocol after we've done our simulation
     graph.traverse(node => {

core/audits/prioritize-lcp-image.js

@@ -194,8 +194,8 @@ class PrioritizeLcpImage extends Audit {
     modifiedLCPNode.removeAllDependencies();
     modifiedLCPNode.addDependency(mainDocumentNode);
 
-    const simulationBeforeChanges = simulator.simulate(graph, {flexibleOrdering: true});
-    const simulationAfterChanges = simulator.simulate(modifiedGraph, {flexibleOrdering: true});
+    const simulationBeforeChanges = simulator.simulate(graph);
+    const simulationAfterChanges = simulator.simulate(modifiedGraph);
     const lcpTimingsBefore = simulationBeforeChanges.nodeTimings.get(lcpNode);
     if (!lcpTimingsBefore) throw new Error('Impossible - node timings should never be undefined');
     const lcpTimingsAfter = simulationAfterChanges.nodeTimings.get(modifiedLCPNode);

core/audits/uses-rel-preload.js

@@ -143,9 +143,9 @@ class UsesRelPreloadAudit extends Audit {
       return {wastedMs: 0, results: []};
     }
 
-    // Preload changes the ordering of requests, simulate the original graph with flexible ordering
+    // Preload changes the ordering of requests, simulate the original graph
     // to have a reasonable baseline for comparison.
-    const simulationBeforeChanges = simulator.simulate(graph, {flexibleOrdering: true});
+    const simulationBeforeChanges = simulator.simulate(graph);
     const modifiedGraph = graph.cloneWithRelationships();
 
     /** @type {Array<LH.Gatherer.Simulation.GraphNetworkNode>} */
@@ -174,8 +174,8 @@ class UsesRelPreloadAudit extends Audit {
       node.addDependency(mainDocumentNode);
     }
 
-    // Once we've modified the dependencies, simulate the new graph with flexible ordering.
-    const simulationAfterChanges = simulator.simulate(modifiedGraph, {flexibleOrdering: true});
+    // Once we've modified the dependencies, simulate the new graph.
+    const simulationAfterChanges = simulator.simulate(modifiedGraph);
     const originalNodesByRecord = Array.from(simulationBeforeChanges.nodeTimings.keys())
         // @ts-expect-error we don't care if all nodes without a record collect on `undefined`
         .reduce((map, node) => map.set(node.record, node), new Map());

core/lib/lantern-trace-saver.js

@@ -255,7 +255,6 @@ export default {
     'unlabeled',
     // These node timings should be nearly identical to the ones produced for Interactive
     'optimisticSpeedIndex',
-    'optimisticFlexSpeedIndex',
     'pessimisticSpeedIndex',
   ],
   convertNodeTimingsToTrace,

core/lib/lantern/metric.js

@@ -101,19 +101,15 @@ class Metric {
     const optimisticGraph = this.getOptimisticGraph(graph, processedNavigation);
     const pessimisticGraph = this.getPessimisticGraph(graph, processedNavigation);
 
-    /** @type {{flexibleOrdering?: boolean, label?: string}} */
     let simulateOptions = {label: `optimistic${metricName}`};
     const optimisticSimulation = simulator.simulate(optimisticGraph, simulateOptions);
 
-    simulateOptions = {label: `optimisticFlex${metricName}`, flexibleOrdering: true};
-    const optimisticFlexSimulation = simulator.simulate(optimisticGraph, simulateOptions);
-
     simulateOptions = {label: `pessimistic${metricName}`};
     const pessimisticSimulation = simulator.simulate(pessimisticGraph, simulateOptions);
 
     const optimisticEstimate = this.getEstimateFromSimulation(
-      optimisticSimulation.timeInMs < optimisticFlexSimulation.timeInMs ?
-        optimisticSimulation : optimisticFlexSimulation, {...extras, optimistic: true}
+      optimisticSimulation,
+      {...extras, optimistic: true}
     );
 
     const pessimisticEstimate = this.getEstimateFromSimulation(

core/lib/lantern/metrics/interactive.js

@@ -21,8 +21,8 @@ class Interactive extends Metric {
   static get COEFFICIENTS() {
     return {
       intercept: 0,
-      optimistic: 0.5,
-      pessimistic: 0.5,
+      optimistic: 0.45,
+      pessimistic: 0.55,
     };
   }
 

core/lib/lantern/metrics/speed-index.js

@@ -18,12 +18,11 @@ class SpeedIndex extends Metric {
    */
   static get COEFFICIENTS() {
     return {
-      // Negative intercept is OK because estimate is Math.max(FCP, Speed Index) and
-      // the optimistic estimate is based on the real observed speed index rather than a real
-      // lantern graph.
-      intercept: -250,
+      // Note that the optimistic estimate is based on the real observed speed index rather than a
+      // real lantern graph (and the final estimate will be Math.max(FCP, Speed Index)).
+      intercept: 0,
       optimistic: 1.4,
-      pessimistic: 0.65,
+      pessimistic: 0.4,
     };
   }
 

core/lib/lantern/simulator/connection-pool.js

@@ -85,24 +85,13 @@ export class ConnectionPool {
 
   /**
    * @param {Array<TcpConnection>} connections
-   * @param {{ignoreConnectionReused?: boolean, observedConnectionWasReused: boolean}} options
    */
-  _findAvailableConnectionWithLargestCongestionWindow(connections, options) {
-    const {ignoreConnectionReused, observedConnectionWasReused} = options;
-
+  _findAvailableConnectionWithLargestCongestionWindow(connections) {
     /** @type {TcpConnection|null} */
     let maxConnection = null;
     for (let i = 0; i < connections.length; i++) {
       const connection = connections[i];
 
-      // Normally, we want to make sure the connection warmth matches the state of the record
-      // we're acquiring for. Do this check first since it's the common case and cheaper than our
-      // "in use" check below.
-      // Use the _warmed property instead of the getter because this is a surprisingly hot code path.
-      if (!ignoreConnectionReused && connection._warmed !== observedConnectionWasReused) {
-        continue;
-      }
-
       // Connections that are in use are never available.
       if (this._connectionsInUse.has(connection)) {
         continue;
@@ -121,23 +110,15 @@ export class ConnectionPool {
    * if no connection was available. If returned, connection will not be available for other network
    * records until release is called.
    *
-   * 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 {Lantern.NetworkRequest} record
-   * @param {{ignoreConnectionReused?: boolean}} options
    * @return {?TcpConnection}
    */
-  acquire(record, options = {}) {
+  acquire(record) {
     if (this._connectionsByRecord.has(record)) throw new Error('Record already has a connection');
 
     const origin = record.parsedURL.securityOrigin;
-    const observedConnectionWasReused = !!this._connectionReusedByRequestId.get(record.requestId);
     const connections = this._connectionsByOrigin.get(origin) || [];
-    const connectionToUse = this._findAvailableConnectionWithLargestCongestionWindow(connections, {
-      ignoreConnectionReused: options.ignoreConnectionReused,
-      observedConnectionWasReused,
-    });
+    const connectionToUse = this._findAvailableConnectionWithLargestCongestionWindow(connections);
 
     if (!connectionToUse) return null;
 

core/lib/lantern/simulator/simulator.js

@@ -80,7 +80,6 @@ class Simulator {
     this._cachedNodeListByStartPosition = [];
 
     // Properties reset on every `.simulate` call but duplicated here for type checking
-    this._flexibleOrdering = false;
     this._nodeTimings = new SimulatorTimingMap();
     /** @type {Map<string, number>} */
     this._numberInProgressByType = new Map();
@@ -197,9 +196,7 @@ class Simulator {
    * @return {?TcpConnection}
    */
   _acquireConnection(record) {
-    return this._connectionPool.acquire(record, {
-      ignoreConnectionReused: this._flexibleOrdering,
-    });
+    return this._connectionPool.acquire(record);
   }
 
   /**
@@ -425,13 +422,13 @@ class Simulator {
    * Estimates the time taken to process all of the graph's nodes, returns the overall time along with
    * each node annotated by start/end times.
    *
-   * If flexibleOrdering is set, simulator/connection pool are allowed to deviate from what was
+   * Simulator/connection pool are allowed to deviate from what was
    * observed in the trace/devtoolsLog and start requests as soon as they are queued (i.e. do not
    * wait around for a warm connection to be available if the original record was fetched on a warm
    * connection).
    *
    * @param {Node} graph
-   * @param {{flexibleOrdering?: boolean, label?: string}=} options
+   * @param {{label?: string}=} options
    * @return {Lantern.Simulation.Result<T>}
    */
   simulate(graph, options) {
@@ -441,11 +438,9 @@ class Simulator {
 
     options = Object.assign({
       label: undefined,
-      flexibleOrdering: false,
     }, options);
 
     // initialize the necessary data containers
-    this._flexibleOrdering = !!options.flexibleOrdering;
     this._dns = new DNSCache({rtt: this._rtt});
     this._initializeConnectionPool(graph);
     this._initializeAuxiliaryData();
@@ -470,11 +465,9 @@ class Simulator {
       }
 
       if (!nodesInProgress.size) {
-        // interplay between fromDiskCache and connectionReused can be incorrect
-        // proceed with flexibleOrdering if we can, otherwise give up
-        if (this._flexibleOrdering) throw new Error('Failed to start a node');
-        this._flexibleOrdering = true;
-        continue;
+        // Interplay between fromDiskCache and connectionReused can be incorrect,
+        // have to give up.
+        throw new Error('Failed to start a node');
       }
 
       // set the available throughput for all connections based on # inflight

core/test/audits/__snapshots__/metrics-test.js.snap

@@ -49,7 +49,7 @@ Object {
   "observedTimeOriginTs": 760620643599,
   "observedTraceEnd": 4778,
   "observedTraceEndTs": 760625421283,
-  "speedIndex": 6330,
+  "speedIndex": 5546,
   "speedIndexTs": undefined,
   "timeToFirstByte": 2394,
   "timeToFirstByteTs": undefined,
@@ -124,7 +124,7 @@ Object {
   "firstContentfulPaintTs": undefined,
   "firstMeaningfulPaint": 2764,
   "firstMeaningfulPaintTs": undefined,
-  "interactive": 4457,
+  "interactive": 4607,
   "interactiveTs": undefined,
   "largestContentfulPaint": 2764,
   "largestContentfulPaintAllFrames": undefined,
@@ -163,7 +163,7 @@ Object {
   "observedTimeOriginTs": 713037023064,
   "observedTraceEnd": 7416,
   "observedTraceEndTs": 713044439102,
-  "speedIndex": 3684,
+  "speedIndex": 3172,
   "speedIndexTs": undefined,
   "timeToFirstByte": 611,
   "timeToFirstByteTs": undefined,
@@ -238,7 +238,7 @@ Object {
   "firstContentfulPaintTs": undefined,
   "firstMeaningfulPaint": 1541,
   "firstMeaningfulPaintTs": undefined,
-  "interactive": 4206,
+  "interactive": 3955,
   "interactiveTs": undefined,
   "largestContentfulPaint": undefined,
   "largestContentfulPaintAllFrames": undefined,
@@ -277,7 +277,7 @@ Object {
   "observedTimeOriginTs": 225414172015,
   "observedTraceEnd": 12540,
   "observedTraceEndTs": 225426711887,
-  "speedIndex": 1676,
+  "speedIndex": 1511,
   "speedIndexTs": undefined,
   "timeToFirstByte": 760,
   "timeToFirstByteTs": undefined,

core/test/audits/__snapshots__/predictive-perf-test.js.snap

@@ -11,14 +11,14 @@ Object {
   "pessimisticFMP": 3233,
   "pessimisticLCP": 3233,
   "pessimisticSI": 3052,
-  "pessimisticTTI": 5119,
+  "pessimisticTTI": 5272,
   "roughEstimateOfFCP": 2294,
   "roughEstimateOfFMP": 2764,
   "roughEstimateOfLCP": 2764,
   "roughEstimateOfLCPLoadEnd": undefined,
   "roughEstimateOfLCPLoadStart": undefined,
-  "roughEstimateOfSI": 3684,
+  "roughEstimateOfSI": 3172,
   "roughEstimateOfTTFB": 611,
-  "roughEstimateOfTTI": 4457,
+  "roughEstimateOfTTI": 4607,
 }
 `;

core/test/audits/byte-efficiency/render-blocking-resources-test.js

@@ -33,8 +33,8 @@ describe('Render blocking resources audit', () => {
     const computedCache = new Map();
     const result = await RenderBlockingResourcesAudit.audit(artifacts, {settings, computedCache});
     assert.equal(result.score, 0);
-    assert.equal(result.numericValue, 316);
-    assert.deepStrictEqual(result.metricSavings, {FCP: 316, LCP: 0});
+    assert.equal(result.numericValue, 304);
+    assert.deepStrictEqual(result.metricSavings, {FCP: 304, LCP: 0});
   });
 
   it('evaluates correct wastedMs when LCP is text', async () => {
@@ -59,7 +59,7 @@ describe('Render blocking resources audit', () => {
     const settings = {throttlingMethod: 'simulate', throttling: mobileSlow4G};
     const computedCache = new Map();
     const result = await RenderBlockingResourcesAudit.audit(artifacts, {settings, computedCache});
-    assert.deepStrictEqual(result.metricSavings, {FCP: 316, LCP: 316});
+    assert.deepStrictEqual(result.metricSavings, {FCP: 304, LCP: 304});
   });
 
   it('evaluates amp page correctly', async () => {
@@ -82,7 +82,7 @@ describe('Render blocking resources audit', () => {
     const settings = {throttlingMethod: 'simulate', throttling: mobileSlow4G};
     const computedCache = new Map();
     const result = await RenderBlockingResourcesAudit.audit(artifacts, {settings, computedCache});
-    expect(result.numericValue).toEqual(316);
+    expect(result.numericValue).toEqual(0);
     expect(result.details.items).toEqual([
       {
         totalBytes: 389629,
@@ -96,7 +96,7 @@ describe('Render blocking resources audit', () => {
         wastedMs: 311,
       },
     ]);
-    expect(result.metricSavings).toEqual({FCP: 316, LCP: 0});
+    expect(result.metricSavings).toEqual({FCP: 0, LCP: 0});
   });
 
   describe('#estimateSavingsWithGraphs', () => {
@@ -155,8 +155,8 @@ describe('Render blocking resources audit', () => {
       documentNode.addDependent(styleNode);
 
       const result = estimate(simulator, documentNode, deferredIds, wastedBytesMap, Stacks);
-      // Saving 1000 + 1000 + 100ms for TCP handshake + 1 RT savings + server response time
-      assert.equal(result, 2100);
+      // Saving 1000 + 100ms for 1 RT savings + server response time
+      assert.equal(result, 1100);
     });
 
     it('does not report savings from AMP-stack when document already exceeds 2.1s', () => {

core/test/audits/predictive-perf-test.js

@@ -25,7 +25,7 @@ describe('Performance: predictive performance audit', () => {
     const context = {computedCache: new Map(), settings: {locale: 'en'}};
 
     const output = await PredictivePerf.audit(artifacts, context);
-    expect(output.displayValue).toBeDisplayString('4,460 ms');
+    expect(output.displayValue).toBeDisplayString('4,610 ms');
     const metrics = output.details.items[0];
     for (const [key, value] of Object.entries(metrics)) {
       metrics[key] = value === undefined ? value : Math.round(value);

core/test/computed/metrics/__snapshots__/interactive-test.js.snap

@@ -3,7 +3,7 @@
 exports[`Metrics: TTI should compute a simulated value 1`] = `
 Object {
   "optimistic": 4178,
-  "pessimistic": 4234,
-  "timing": 4206,
+  "pessimistic": 3773,
+  "timing": 3955,
 }
 `;