core(preload): use lantern to compute savings #5062

patrickhulce · 2018-04-28T00:26:26Z

this ended up turning into quite the behemoth, I was tempted to break up the lantern/connection-pool test changes, but it's kinda hard to grok without the motivating example.

most of the lines are tests, so that's the good news :)

patrickhulce · 2018-04-28T00:27:31Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

    // initialize the necessary data containers
+    this._ignoreObserved = options.ignoreObserved;


I'm really not sure what to call this thing, basically it's "don't try and stick just to how the page loaded in the trace, feel free to load it as efficiently as you think possible"

yeah i don't love this term. :) as you know i don't love reusing it in different places in diff contexts.

is reconstructTCPConnectionPooling accurate?

patrickhulce · 2018-05-01T18:43:57Z

friendly ping! :)

paulirish · 2018-05-02T00:15:05Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

    // initialize the necessary data containers
+    this._ignoreObserved = options.ignoreObserved;


yeah i don't love this term. :) as you know i don't love reusing it in different places in diff contexts.

is reconstructTCPConnectionPooling accurate?

paulirish · 2018-05-02T00:17:56Z

lighthouse-core/lib/dependency-graph/simulator/connection-pool.js

+      // https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc?type=cs&q="int+g_max_sockets_per_group"
+      while (connections.length < 6) connections.push(connections[0].clone());
+      while (connections.length > 6) connections.pop();
+
      this._connectionsByOrigin.set(origin, connections);
    }
  }

  /**


can we add a few lines to describe what's happening in here?

paulirish · 2018-05-02T00:21:59Z

lighthouse-core/lib/dependency-graph/simulator/connection-pool.js

+    const needsWarmConnection = !options.ignoreObserved &&
+        this._connectionReusedByRequestId.get(record.requestId);
+
+    let connectionToUse = availableWarmConnection || availableColdConnection;


still not sure exactly what logic you're trying to apply here, but it does look odd there's no else case in all 4 lines here.

paulirish · 2018-05-02T00:44:34Z

lighthouse-core/lib/dependency-graph/simulator/connection-pool.js

+      // Make sure each origin has 6 connections available
+      // https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc?type=cs&q="int+g_max_sockets_per_group"
+      while (connections.length < 6) connections.push(connections[0].clone());
+      while (connections.length > 6) connections.pop();


how would we end up with >6? our network records indicate we have >6 even though that should be impossible?

If Chrome killed connections and needed to reopen them, we can kill this line though

paulirish · 2018-05-02T00:48:07Z

lighthouse-core/lib/dependency-graph/simulator/connection-pool.js

-    this._connectionsByRecord.set(record, connection);
-    return connection;
+    const needsColdConnection = !options.ignoreObserved &&
+        !this._connectionReusedByRequestId.get(record.requestId);


meta point: it's really funky to be mixing this "connection reused" state that refers to observed network activity with what's "available" and "needed" for the simulation. I wish these were clearer

brendankenny · 2018-05-02T18:37:16Z

lighthouse-core/lib/dependency-graph/simulator/tcp-connection.js

+   */
+  clone() {
+    // @ts-ignore
+    return Object.assign(new TcpConnection(), this);


I'd probably personally prefer the long way of setting these, but if you prefer this way,
return Object.assign(new TcpConnection(this._rtt, this._throughput), this);
should at least let you avoid the // @ts-ignore (TcpConnection & this should collapse just to TcpConnection externally)

brendankenny · 2018-05-02T18:47:05Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -212,7 +223,7 @@ class Simulator {

    const record = /** @type {NetworkNode} */ (node).record;
    const timingData = this._nodeTimings.get(node);
-    const connection = /** @type {TcpConnection} */ (this._connectionPool.acquire(record));
+    const connection = /** @type {TcpConnection} */ (this._acquireConnection(record));


how is this known not-null at this point?

brendankenny · 2018-05-02T18:47:34Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -255,7 +266,7 @@ class Simulator {
    if (node.type !== Node.TYPES.NETWORK) throw new Error('Unsupported');

    const record = /** @type {NetworkNode} */ (node).record;
-    const connection = /** @type {TcpConnection} */ (this._connectionPool.acquire(record));
+    const connection = /** @type {TcpConnection} */ (this._acquireConnection(record));


also never null?

brendankenny · 2018-05-02T18:49:33Z

lighthouse-core/lib/dependency-graph/simulator/connection-pool.js

@@ -82,15 +82,27 @@ module.exports = class ConnectionPool {
        throw new Error(`Could not find a connection for origin: ${origin}`);
      }

+      // Make sure each origin has 6 connections available
+      // https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc?type=cs&q="int+g_max_sockets_per_group"
+      while (connections.length < 6) connections.push(connections[0].clone());


pull out into a file level constant?

brendankenny · 2018-05-02T18:53:45Z

lighthouse-core/audits/uses-rel-preload.js

      // get all critical requests 2 + mainResourceIndex levels deep
      const mainResourceIndex = mainResource.redirects ? mainResource.redirects.length : 0;

      const criticalRequests = UsesRelPreloadAudit._flattenRequests(critChains,
        3 + mainResourceIndex, 2 + mainResourceIndex);
-      criticalRequests.forEach(request => {
-        const networkRecord = request;
+      const urls = new Set();


I think you'll need an explicit /** @type {Set<string>} */ or it'll be a Set<any>

brendankenny · 2018-05-02T18:55:43Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -281,10 +292,13 @@ class Simulator {
  /**
   * Estimates the time taken to process all of the graph's nodes.
   * @param {Node} graph
+   * @param {{flexibleOrdering?: boolean}=} options


definitely need an explanation of flexibleOrdering added to the docstring above :)

brendankenny · 2018-05-02T18:58:30Z

lighthouse-core/audits/uses-rel-preload.js

+      node.addDependency(mainDocumentNode);
+    }
+
+    const simulationAfterChanges = simulator.simulate(modifiedGraph, {flexibleOrdering: true});


comment why flexibleOrdering should be here would be 👍

brendankenny · 2018-05-02T19:02:12Z

lighthouse-core/audits/uses-rel-preload.js

+    return {
+      // Preload won't necessarily impact the deepest chain/overall time
+      // We'll use the maximum endTime improvement for now
+      wastedMs: Math.max(...results.map(item => item.wastedMs)),


seems like it gets most of the way to an opportunity but then stops here? Is measuring impact on TTI or whatever problematic?

Yeah fair, I was mostly retaining parity with the old way, but we can do max(TTI, load) like the others 👍

having had the hour to think about it, I've reconvinced myself that this is a bad idea and we should keep it to max(...wastedMs), our selection of preloaded items is fairly narrow and in a busy page it will rarely impact the longest chain

given our super ambiguous no-longer-just-on-a-particular-metric approach, it seems OK for now

brendankenny · 2018-05-02T19:06:40Z

lighthouse-core/test/audits/uses-rel-preload-test.js

+    scriptAddedNode.addDependency(scriptNode);
+
+    mockGraph = rootNode;
+    mockSimulator = {


is this reused for the other tests?

nope, just scoped for the mockArtifacts to be able to return it. I added an afterEach that clears it to make it more explciit

wait, but how are the others running with an undefined simulator? (obvs I'm too lazy to debug here :)

the other tests are asserting that no savings are found, so a simulator isn't necessary

brendankenny · 2018-05-02T19:13:30Z

lighthouse-core/test/audits/uses-rel-preload-test.js

+  let mockGraph;
+  let mockSimulator;
+
+  const mockArtifacts = (networkRecords, mockChain, mainResource = defaultMainResource) => {


the audit has some non-trivial uses of the graph and simulator, which means we're doing a lot of testing of the mocks :) Some tests using the full real thing would be 👍 👍

totally agreed, but it requires a trace and devtoolslog of a site that would benefit from preloading which we don't have checked in yet and can be quite large.

my proposal:

we rely on the smoke test + unit tests for now

for cases like this where we need to exercise complicated lantern paths with real traces we use my patent-pending post-I/O approach of storing ~100 traces/devtoolslogs on GCP that can run on-demand/on commits that touch lantern files to assert that opportunities and metrics look decent

WDYT?

it wouldn't give good coverage, but even a simple devtoolslog/trace with little or no savings would be a start until the GCP smoke tests. there's lighthouse-core/tests/fixtures/artifacts/perflog/ and of course lighthouse-core/test/results/artifacts/

we should maybe make a tool to take real artifacts and strip out a bunch of stuff to make nice artifact fixtures and maybe only a few hundred KB at most. e.g. for things like this that are mostly networking we can get rid of a lot of trace events (enough that it was the same page load but with a lot less busy CPU)

(we have a lot of old traces for testing, but without paired devtools logs they're becoming increasingly useless for tests)

yeah that sounds good

patrickhulce · 2018-05-02T23:59:34Z

FYI @brendankenny I rebased against your changes and fixed the remaining type check items

brendankenny

best weak smoke test there ever was :):)

LGTM!

core(preload): use lantern to compute savings

badbe11

patrickhulce requested review from brendankenny and paulirish as code owners April 28, 2018 00:26

patrickhulce commented Apr 28, 2018

View reviewed changes

devtools-bot added the waiting4reviewer label Apr 28, 2018

patrickhulce mentioned this pull request Apr 30, 2018

core(preload): only allow same origin (domain + subdomains) #5065

Merged

paulirish mentioned this pull request Apr 30, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

paulirish removed the waiting4reviewer label Apr 30, 2018

patrickhulce added 3 commits April 30, 2018 11:12

Merge branch 'master' into lantern_preload_opp

51238e7

Merge branch 'master' into lantern_preload_opp

1a03349

rebase and expand tests

08fa322

patrickhulce added the I/O-Expedite label May 1, 2018

paulirish requested changes May 2, 2018

View reviewed changes

patrickhulce added 2 commits May 1, 2018 18:44

feedback

9d0f995

add comment

0dff54d

brendankenny mentioned this pull request May 2, 2018

core(tsc): add type checking to remaining top-level audits #5090

Merged

brendankenny requested changes May 2, 2018

View reviewed changes

paulirish approved these changes May 2, 2018

View reviewed changes

patrickhulce added 2 commits May 2, 2018 15:05

feedback

1b3c2a7

Merge branch 'master' into lantern_preload_opp

a2ef139

weak smoke test

2ec133c

brendankenny approved these changes May 3, 2018

View reviewed changes

patrickhulce merged commit 9cc23a3 into master May 3, 2018

patrickhulce deleted the lantern_preload_opp branch May 3, 2018 00:22

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(preload): use lantern to compute savings (GoogleChrome#5062)

378f348

brendankenny mentioned this pull request Dec 14, 2022

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

Merged

		// initialize the necessary data containers
		this._ignoreObserved = options.ignoreObserved;

core(preload): use lantern to compute savings #5062

core(preload): use lantern to compute savings #5062

Conversation

patrickhulce commented Apr 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented May 1, 2018

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

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 May 2, 2018 • 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

patrickhulce May 2, 2018 • 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

patrickhulce commented May 2, 2018

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce May 2, 2018 •

edited

Loading

patrickhulce May 2, 2018 •

edited

Loading