feat(predictive-perf): add network estimation #3187

patrickhulce · 2017-08-30T00:34:35Z

in the name of fast mode #2703

~~blocked on #2720~~

split out from #2720 for easier reviewing

paulirish

more to come, but flushing early... 🚽 😺

paulirish · 2017-08-31T20:29:47Z

lighthouse-core/audits/predictive-perf.js

+        sum += values[key];
+      });
+
+      const rawValue = sum / 4;


magic number. Object.keys(graphs).length ?

paulirish · 2017-08-31T20:32:49Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+const DEFAULT_MAXIMUM_CONCURRENT_REQUESTS = 10;
+const DEFAULT_RESPONSE_TIME = 30;
+const DEFAULT_RTT = 150;
+const DEFAULT_THROUGHPUT = 1600 * 1024; // 1.6 Mbps


these two numbers are identical to our throttling presets. should we pull from emulation.js instead?

paulirish · 2017-08-31T20:52:22Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+      {
+        rtt: DEFAULT_RTT,
+        throughput: DEFAULT_THROUGHPUT,
+        defaultResponseTime: DEFAULT_RESPONSE_TIME,


post-discussion: naming-wise this value is very unclear. defaultServerResponseTime is still ambiguous wether it includes latency or not. (and 1-way or 2-way?)

defaultTTFB is very clear as a definition, so I'd prefer using that as the argument. That means some adding/subtracting of latency here and there, but i think it's worth it so the terms are clear.

renamed to fallbackTTFB and added latency subtraction 👍

ok hang on, reopening the discussion :)

i'm gonna push back against this latency subtraction from TTFB deal a bit, I cherrypicked the change to my final fixes branch and it regressed accuracy by 0.3%

given we're significantly under reporting rtt in most cases i'm not sure we should be further reducing the download time estimates, how about as future exploration we try to do something fancy to identify/separate connection latency from server response time instead (I can imagine trying to determine the built-in connection latency by looking at all the requests, or comparing the TCP handshake time to the TTFB time etc but its all more researchy) and leave it be for now?

proposed comment:

// We'll approximate how much time the server for a connection took to respond after receiving // the request by computing the minimum TTFB time for requests on that connection. // TTFB = one way latency + server response time + one way latency // Even though TTFB is greater than server response time, the RTT is underaccounted for by // by not varying per-server and so the difference roughly evens out. // TODO(patrickhulce): investigate a way to identify per-server RTT

as discsussed offline, i think it'd be great to explore identifing per-server RTT later. this comment sg. let's add it. :)

update: saw the comment is added already. 👍

paulirish · 2017-08-31T20:53:16Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+   * @param {!WebInspector.NetworkRequest} record
+   * @return {number}
+   */
+  static getResponseTime(record) {


yes, this value is the definition of TTFB, so let's s/responseTime/ttfb/ :)

paulirish · 2017-08-31T20:55:07Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+const DEFAULT_RTT = 150;
+const DEFAULT_THROUGHPUT = 1600 * 1024; // 1.6 Mbps
+
+function groupBy(items, keyFunc) {


I wish we could install this to Object.groupBy so its as usable everywhere. the named props and array of obj dances get old. oh well.

heh yeah...I'm still all for a util lib :)

paulirish · 2017-08-31T20:57:30Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+    );
+
+    for (const [connectionId, records] of recordsByConnection.entries()) {
+      const isSsl = records[0].parsedURL.scheme === 'https';


wss? if that should be included we have SECURE_SCHEMES elsewhere :)

confirm that we dont care about ws in this simulation.

how about

const isTLS = records[0].parsedURL.scheme === 'https'; // wss deliberately excluded

paulirish

flushing..

Also can I just say... estimator/tcp-connection.js is one of my favorite files I've seen in a while. Love the research.

paulirish · 2017-08-31T21:03:19Z

lighthouse-core/audits/predictive-perf.js

+    return graph.cloneWithRelationships(node => {
+      if (node.endTime > fmp) return false;
+      if (node.type !== Node.TYPES.NETWORK) return true;
+      return node.record.priority() === 'VeryHigh'; // proxy for render-blocking


FWIW this will exclude external scripts (and a few others)

https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc/edit

Maybe it's time we write the isResourceRenderBlocking(resourceType, priority) method finally?

yup this is implemented in next PR :) FWIW in the future PR the remaining optimistic/pessimistic split is also excluding render blocking priorities if they were script initiated

paulirish · 2017-08-31T21:05:09Z

lighthouse-core/audits/predictive-perf.js

+   */
+  static getOptimisticTTCIGraph(graph) {
+    return graph.cloneWithRelationships(node => {
+      return node.record._resourceType && node.record._resourceType._name === 'script' ||


with modern devtools this is node.record.resourceType() === Common.resourceTypes.Script
But with ours it'll need some WebInspector.*... :)

heh, do we want to go with the public accessors here? (this specific condition is also removed in future work so I'll add this there if we want to switch)

this is same as node.record._resourceType === Common.resourceTypes.Script (which is fine). The benefit is just dont have to reach all the way to _name

ah ok, will re-open in the final version then where i've used node.resourceType then 👍

paulirish · 2017-08-31T21:06:30Z

lighthouse-core/audits/predictive-perf.js

+  static getOptimisticTTCIGraph(graph) {
+    return graph.cloneWithRelationships(node => {
+      return node.record._resourceType && node.record._resourceType._name === 'script' ||
+          node.record.priority() === 'High' ||


why does this one use both priorities but optimisticFMP only uses veryhigh? Would love a comment to explain some of these or link into the relevant part of the doc.

future version has comments explaining each condition when more thought started going into them :) but will flesh out more

paulirish · 2017-08-31T21:07:52Z

lighthouse-core/audits/predictive-perf.js

+   */
+  static getOptimisticTTCIGraph(graph) {
+    return graph.cloneWithRelationships(node => {
+      return node.record._resourceType && node.record._resourceType._name === 'script' ||


style wise the if (..) return... approach used on getOptimisticFMPGraph's predicate is more clear and avoids questions around operator precedence.

this particular condition is removed in next PR but will update latest 👍

paulirish · 2017-08-31T21:11:33Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+    });
+
+    this._networkRecords = records;
+    return records;


does the return value matter? if it doesn't then might as well ditch records and push right into this._networkRecords ?

nope it doesn't, done.

paulirish · 2017-08-31T21:37:27Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+      }
+    } else {
+      auxiliaryData.timeElapsed += calculation.timeElapsed;
+      auxiliaryData.timeElapsedOvershoot +=


yeah lets rename, especially so this var's length doesn't force wrapping. :)

paulirish · 2017-08-31T21:44:24Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+
+  _updateNetworkCapacity() {
+    for (const connection of this._connectionsInUse) {
+      connection.setThroughput(this._throughput / this._nodesInProcess.size);


trying this out..

1000KB/sec throughput
10 connections
20 requests in flight

Each connection then get is set to 50 kb/s of availableThroughput. K sgtm. :)

however, thinking forward to CPU nodes in the graph, they shouldn't be included in this .size should they?

yeah they won't be I keep track of number of in process by type separately 👍

paulirish · 2017-08-31T21:48:27Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+
+    let handshakeAndRequest = oneWayLatency;
+    if (!this._warmed) {
+      handshakeAndRequest =


as part of the earlier TTFB comments, can this be just handShake ? (and move one of the OWL's into the TTFB) 🦉

I'm not a huge fan of splitting it since the handshake is in fact concluding in the same trip as the request is being made, adding a comment with // technically the ACK is still happening blah blah seems a tad more to process than a variable name indicating the two are conjoined IMO. It also slightly complicates the later http2 case where neither are required once the connection has been warmed.

Do you think addressing the other TTFB issues makes this clearer or you'd still like to see the split?

agreed to keep handshakeAndRequest as a unit as it's more accurate to the reality of TCP. 👍

paulirish · 2017-08-31T21:53:31Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+        oneWayLatency +
+        // ACK + Application Data
+        oneWayLatency +
+        // ClientHello/ServerHello with TLS False Start


how about this for a comment

// We assume TLS False Start is enabled. https://istlsfastyet.com/#server-performance

paulirish · 2017-08-31T22:09:35Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+   * @param {number=} maximumTimeToElapse
+   * @return {{timeElapsed: number, roundTrips: number, bytesDownloaded: number, congestionWindow: number}}
+   */
+  calculateTimeToDownload(bytesToDownload, timeAlreadyElapsed = 0, maximumTimeToElapse = Infinity) {


naming threw me... simulateDownloadingUntil is way clearer.

kinda weird that args 1 and 3 are the constraints but not next to eachother. should we just move to an object instead? Seems like we usually would at this point. strawman:

simulateDownloadingUntil({maxBytes, maxMs, msElapsed})

done, how about switching to an object param in the "incremental PR" after I can rebase against the name change?

how about switching to an object param in the "incremental PR" after I can rebase against the name change?

sure that's fine.

paulirish

ok, Finished looking at this PR. great stuff. :)

paulirish · 2017-08-31T22:25:42Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+
+  _updateNetworkCapacity() {
+    for (const connection of this._connectionsInUse) {
+      connection.setThroughput(this._throughput / this._nodesInProcess.size);


setThroughput vs availableThroughput makes me think there may be multiple... like currentTHroughput, defaultThroughput, etc... since there's only one should we just use that same name?

paulirish · 2017-09-01T20:33:20Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+        oneWayLatency +
+        // ACK + Application Data
+        oneWayLatency +
+        // ClientHello/ServerHello assuming TLS Flase Start is enabled (https://istlsfastyet.com/#server-performance).


paulirish · 2017-09-01T20:34:45Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+    }
+
+    let roundTrips = Math.ceil(handshakeAndRequest / twoWayLatency);
+    const timeToFirstByte = handshakeAndRequest + this._responseTime + oneWayLatency;


_responseTime => _serverLatency

everywhere? :)

paulirish · 2017-09-01T20:35:01Z

lighthouse-core/gather/computed/dependency-graph/estimator/tcp-connection.js

+        oneWayLatency +
+        // SYN ACK
+        oneWayLatency +
+        // ACK + Application Data


ACK + initial request

patrickhulce · 2017-09-05T19:18:40Z

I think everything's addressed here, so back to you @paulirish

build failures seem to be bundlesize step failing due to a github 404

brendankenny · 2017-09-05T19:29:14Z

sorry, I'd still like to take a look :D:D

brendankenny

initial comment flush, though this is looking good :)

brendankenny · 2017-09-05T20:10:30Z

lighthouse-core/audits/predictive-perf.js

@@ -31,14 +32,77 @@ class PredictivePerf extends Audit {
  }

  /**
+   * @param {!Node} graph


maybe dependencyGraph to give a local hint?

brendankenny · 2017-09-05T20:15:45Z

lighthouse-core/audits/predictive-perf.js

+        sum += values[key];
+      });
+
+      const rawValue = sum / Object.keys(values).length;


nit: meanDuration for clarity?

brendankenny · 2017-09-05T20:25:00Z

lighthouse-core/gather/computed/dependency-graph/node.js

+      maxDepth--;
+    }
+
+    if (!maxDepth) {


if this happens it seems like a more serious bug would be present?

It means something is wrong with graph that's been created, see #3187 (comment) I think it's still more useful than having a fatal infinite loop on bad input.

yeah, which I guess is good :) (especially during ongoing development) but it does feel like a check for a properly formed graph at construction time is the right solution (plus we could probably give better error messages for what went wrong with graph construction)

brendankenny · 2017-09-05T20:28:01Z

lighthouse-core/gather/computed/dependency-graph/node.js

@@ -121,6 +147,11 @@ class Node {
      if (!shouldIncludeNode(originalNode)) return;
      const clonedNode = originalNode.cloneWithoutRelationships();
      idToNodeMap.set(clonedNode.id, clonedNode);
+    });
+
+    rootNode.traverse(originalNode => {


why is splitting it like this necessary?

it's not guaranteed that a node will be visited by traverse before all of its dependencies, it's just doing a traversal of the edges not necessarily a topological sort ordered traversal

brendankenny · 2017-09-05T20:34:41Z

lighthouse-core/gather/computed/page-dependency-graph.js

+   *
+   * @param {!Node} rootNode
+   */
+  static printGraph(rootNode, widthInCharacters = 100) {


not used? just for debugging?

brendankenny · 2017-09-05T20:47:42Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+  return grouped;
+}
+
+class Estimator {


needs some jsdoc description strings :)

brendankenny · 2017-09-05T20:52:49Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+      // Even though TTFB is greater than server response time, the RTT is underaccounted for by
+      // not varying per-server and so the difference roughly evens out.
+      // TODO(patrickhulce): investigate a way to identify per-server RTT
+      let estimatedResponseTime = records.reduce(


Math.min(...records.map(Estimator.getTTFB))?

brendankenny · 2017-09-05T20:57:59Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+        );
+      }
+
+      if (depth > 10000) {


also seems like a code smell/from an earlier development stage?

While it was also very useful for early debugging with a small value, this is just a sensibility check to avoid looping forever and instead fail in the event of illogical network record or trace event relationships (or bug in graph construction though at this point it's synonymous with illogic event relationships given the interpretations we're taking).

this is just a sensibility check to avoid looping forever and instead fail in the event of illogical network record or trace event relationships

same as above, maybe we can move to an assertValidDepGraph method in the future

brendankenny · 2017-09-05T21:00:28Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+  _initializeAuxiliaryData() {
+    this._nodeTiming = new Map();
+    this._nodesCompleted = new Set();
+    this._nodesInProcess = new Set();


in the same process? if not, may be a bit confusing naming-wise. nodesInProgress?

hahaha, good point, done

brendankenny · 2017-09-05T21:11:23Z

lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js

+  _enqueueNodeIfPossible(node) {
+    const dependencies = node.getDependencies();
+    if (
+      !this._nodesCompleted.has(node) &&


if all dependencies must have been completed to be enqueued, shouldn't it not be possible for a node to already be completed here (since it's being added as a dependent of another node)

This is cleaned up a little bit in #3162 but the summary here is that it was the only check against duplicate processing of a node. By definition, if a node has been completed, all of its dependencies have been completed, so while we're enqueing don't start on a node we've already finished (really should be if a node we've already started which is fixed later but it didn't have an impact when only network records were considered).

Once again, this logic is re-arranged in the next PR to be clearer.

By definition, if a node has been completed, all of its dependencies have been completed, so while we're enqueing don't start on a node we've already finished

right, I was trying to say that always checking dependencies.every(dependency => this._nodesCompleted.has(dependency)) before enqueuing should be sufficient, because it means if a dependent of a newly-completed node is being enqueued, it can't have even been in the queue before (let alone completed), because one of its dependencies wasn't completed until just now.

We can wait as it changes, but it feels like it's a // should never happen situation and it should throw rather than ignoring just in case it does ever happen :)

Oh oh I see my confusion here now :)

In another version, this method was called in a loop over all nodes, not just the dependents of recently completed nodes. Yup, yup in this snapshot it's a 100% superfluous check.

brendankenny

good to go from me!

brendankenny · 2017-09-06T18:40:59Z

lighthouse-core/gather/computed/dependency-graph/node.js

@@ -177,4 +208,9 @@ class Node {
  }
 }

+Node.TYPES = {
+  UNKNOWN: 'unknown',


paulirish requested changes Aug 31, 2017

View reviewed changes

patrickhulce mentioned this pull request Aug 31, 2017

feat(predictive-perf): add shell and base audit #2720

Merged

paulirish requested changes Aug 31, 2017

View reviewed changes

patrickhulce force-pushed the predictive_perf__network branch from 3d35420 to cf44696 Compare September 1, 2017 00:03

patrickhulce added 2 commits September 1, 2017 09:16

feat(predictive-perf): add network estimation

1342bb1

feedback

a632442

patrickhulce force-pushed the predictive_perf__network branch from cf44696 to a632442 Compare September 1, 2017 16:18

paulirish reviewed Sep 1, 2017

View reviewed changes

address feedback

3c8ee6f

brendankenny requested changes Sep 5, 2017

View reviewed changes

brendan feedback

1e56a89

brendankenny approved these changes Sep 6, 2017

View reviewed changes

paulirish approved these changes Sep 6, 2017

View reviewed changes

remove unkown type

6f28492

patrickhulce merged commit fab1ce5 into master Sep 6, 2017

patrickhulce deleted the predictive_perf__network branch September 6, 2017 21:31

patrickhulce mentioned this pull request May 24, 2018

core(simulator): improved timing typedef #5347

Merged

brendankenny mentioned this pull request Nov 28, 2022

core(lantern): divide throughput only on network node count #14564

Merged

feat(predictive-perf): add network estimation #3187

feat(predictive-perf): add network estimation #3187

Conversation

patrickhulce commented Aug 30, 2017 • edited Loading

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Sep 5, 2017 • edited Loading

brendankenny commented Sep 5, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Aug 30, 2017 •

edited

Loading

patrickhulce Aug 31, 2017 •

edited

Loading

patrickhulce Aug 31, 2017 •

edited

Loading

patrickhulce commented Sep 5, 2017 •

edited

Loading