core(fr): handle 0 throughput in timespan

[adamraine]

Closes #13321

This appears to be the correct solution given how we calculate savings in timespan mode, but it also means that wastedMs will be 0 for every BE audit in timespan mode using the desktop config.

[brendankenny]

I'm on my phone so can't check for myself, sorry, but what's the difference between timespan and navigation for this? I thought we estimated from the download rate in cases like this... Is it we don't have that info in a timespan?

[adamraine]

We estimate from the throughput in timespan, which is calculated using throttling.downloadThroughputKbps for DevTools throttling (timespan always uses DevTools throttling). throttling.downloadThroughputKbps is 0 in the desktop config, this leads to some divide by 0 when calculating wastedMs:

lighthouse/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

Lines 207 to 212 in 5d8ea1a

	static computeWastedMsWithThroughput(wastedBytes, simulator) {
	const bitsPerSecond = simulator.getOptions().throughput;
	const wastedBits = wastedBytes * 8;
	const wastedMs = wastedBits / bitsPerSecond * 1000;
	return wastedMs;
	}

[brendankenny]

but what's the difference between timespan and navigation for this

for myself: the difference is that estimating via throughput isn't done for navigation, that uses the full graph simulation:

lighthouse/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

Lines 227 to 234 in 5d8ea1a

	if (gatherContext.gatherMode === 'navigation') {
	if (!graph) throw Error('Page dependency graph should always be computed in navigation mode');
	wastedMs = this.computeWasteWithTTIGraph(results, graph, simulator, {
	providedWastedBytesByUrl: result.wastedBytesByUrl,
	});
	} else {
	wastedMs = this.computeWastedMsWithThroughput(wastedBytes, simulator);
	}

If instead we did call computeWastedMsWithThroughput in a navigation with --preset desktop, we'd have the same NaN issue, but we don't do that so we don't :)

We estimate from the throughput in timespan, which is calculated using throttling.downloadThroughputKbps for DevTools throttling

sorry, the missing object in I thought we estimated from the download rate was supposed to be "throughput" :)

It looks like we only do that for 'provided':

lighthouse/lighthouse-core/computed/load-simulator.js

Lines 40 to 43 in 5d8ea1a

	options.rtt = networkAnalysis.rtt;
	options.throughput = networkAnalysis.throughput;
	options.cpuSlowdownMultiplier = 1;
	options.layoutTaskMultiplier = 1;

which is estimated over in network-analyzer from whatever network records are available.

We know we have network records due to this check:

lighthouse/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

Lines 120 to 128 in 68ba77a

	// Requesting load simulator requires non-empty network records.
	// Timespans are not guaranteed to have any network activity.
	// There are no bytes to be saved if no bytes were downloaded, so mark N/A if empty.
	if (!hasContentfulRecords && gatherContext.gatherMode === 'timespan') {
	return {
	score: 1,
	notApplicable: true,
	};
	}

what if we altered computeWastedMsWithThroughput to fallback to the calculated throughput if one wasn't set?

[brendankenny]

kind of ugly version of what I mean, ideally we can do this nicer (but the file might need a refactor for the FR world it lives in now that we don't want to take on at this point):

diff --git a/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js b/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
index 4c6e17abe..bae49d2ba 100644
--- a/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
+++ b/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
@@ -12,6 +12,7 @@ const i18n = require('../../lib/i18n/i18n.js');
 const NetworkRecords = require('../../computed/network-records.js');
 const LoadSimulator = require('../../computed/load-simulator.js');
 const PageDependencyGraph = require('../../computed/page-dependency-graph.js');
+const NetworkAnalysis = require('../../computed/network-analysis.js');
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, {});
 
@@ -127,16 +128,17 @@ class UnusedBytes extends Audit {
       };
     }
 
-    const [result, graph, simulator] = await Promise.all([
+    const [result, graph, simulator, networkAnalysis] = await Promise.all([
       this.audit_(artifacts, networkRecords, context),
       // Page dependency graph is only used in navigation mode.
       gatherContext.gatherMode === 'navigation' ?
         PageDependencyGraph.request({trace, devtoolsLog}, context) :
         null,
       LoadSimulator.request(simulatorOptions, context),
+      NetworkAnalysis.request(devtoolsLog, context),
     ]);
 
-    return this.createAuditProduct(result, graph, simulator, gatherContext);
+    return this.createAuditProduct(result, graph, simulator, gatherContext, networkAnalysis);
   }
 
   /**
@@ -203,9 +205,13 @@ class UnusedBytes extends Audit {
   /**
    * @param {number} wastedBytes
    * @param {Simulator} simulator
+   * @param {LH.Artifacts.NetworkAnalysis} networkAnalysis
    */
-  static computeWastedMsWithThroughput(wastedBytes, simulator) {
-    const bitsPerSecond = simulator.getOptions().throughput;
+  static computeWastedMsWithThroughput(wastedBytes, simulator, networkAnalysis) {
+    let bitsPerSecond = simulator.getOptions().throughput;
+    if (bitsPerSecond === 0) {
+      bitsPerSecond = networkAnalysis.throughput;
+    }
     const wastedBits = wastedBytes * 8;
     const wastedMs = wastedBits / bitsPerSecond * 1000;
     return wastedMs;
@@ -216,9 +222,10 @@ class UnusedBytes extends Audit {
    * @param {Node|null} graph
    * @param {Simulator} simulator
    * @param {LH.Artifacts['GatherContext']} gatherContext
+   * @param {LH.Artifacts.NetworkAnalysis} networkAnalysis
    * @return {LH.Audit.Product}
    */
-  static createAuditProduct(result, graph, simulator, gatherContext) {
+  static createAuditProduct(result, graph, simulator, gatherContext, networkAnalysis) {
     const results = result.items.sort((itemA, itemB) => itemB.wastedBytes - itemA.wastedBytes);
 
     const wastedBytes = results.reduce((sum, item) => sum + item.wastedBytes, 0);
@@ -230,7 +237,7 @@ class UnusedBytes extends Audit {
         providedWastedBytesByUrl: result.wastedBytesByUrl,
       });
     } else {
-      wastedMs = this.computeWastedMsWithThroughput(wastedBytes, simulator);
+      wastedMs = this.computeWastedMsWithThroughput(wastedBytes, simulator, networkAnalysis);
     }
 
     let displayValue = result.displayValue || '';

extra computation cost is minimal because simulator creation also runs NetworkAnalysis.request so it'll be cached for one call or the other.

We know we have network records due to this check:

this should cover us, but networkAnalysis.throughput also falls back to Infinity if there are no usable network records, which is equivalent to computeWastedMsWithThroughput returning 0 as this PR does, so we're covered either way

[adamraine]

If instead we did call computeWastedMsWithThroughput in a navigation with --preset desktop, we'd have the same NaN issue, but we don't do that so we don't :)

The simulator doesn't have any NaN issues with navigations, but does it make sense to have simulator options throughput set to 0? My whole confusion here stems from the fact that 0 throughput in the config represents "unset". Should we be setting throughput to the network analysis value in all cases where config throughput is 0?

[adamraine]

I assumed this is true for rtt but it shouldn't be necessary to fix the issue at hand.

[brendankenny]

Well, now you've made me come all the way around and I think your initial proposal is the right one, haha. Most of this you already said above but to try to summarize for posterity:

The reason the simulator only uses the estimated throughput value for provided is because provided is used for cases like WPT (where it does network throttling at a deeper level in the OS) or a real device on a mobile network (where you're already on the real network you want to be testing). In those cases there's no way for Lighthouse to know what network conditions to use except to use the rates discernible from the network requests themselves, and that lets us support throttling external to Lighthouse.

When throttling is devtools or simulated we know exactly our own settings so it makes more sense to use that over empirical values, because (in theory) that can help the simulator smooth out some of the variances introduced by the particular testing machine/network/etc. For desktop that means running it as fast as it can go, which loses some of the testing-environment-independence, but that's made up for by simulating over the full dependency graph, which means for each byte-efficiency audit there can still actually be ms to be saved relative to the end of the graph.

But since the timespan case is just doing a super naive calculation (bits/(bitsPerSecond)) of what can be saved relative to the end of this single request, we'll run into this problem.

So I think:

• we shouldn't change the simulator options to preserve what's described in the first two paragraphs above

• for timespans in byte efficiency we should do something like if (bitsPerSecond === 0) return 0; (your original solution, equivalent to bitsPerSecond = simulator.getOptions().throughput || Infinity but more explicit about it's doing).

  That does mean no byte savings in desktop, but I think that's the best we can do right now given we're only looking at byte efficiency for each request in isolation and we've explicitly said desktops can download things at an infinite Kbps

• we should seriously consider giving desktop a non-zero downloadThroughputKbps/uploadThroughputKbps in the near future. This would fix this problem and possibly reduce some variance across machines in the desktop preset. Basically have an opinionated desktop config rather than whatever the machine being run on can do. This might be more controversial :) cc @paulirish

[paulirish]

• we should seriously consider giving desktop a non-zero downloadThroughputKbps/uploadThroughputKbps in the near future.

One notable difference is... in lantern, throughput and rtt are used "correctly", in that our simulated environment ensures the throughput affects network transmission to the best of our abilities. However in our devtools throttling, both the rtt and throttling numbers are additive. We'd just adding extra latency and constraining throughput beyond what's happening in real life. (put another way... if you wanted to know what rtt of 2ms and throughput of 1gb looks like... lantern would be ~accurate, but w/ devtools your results would be basically indicative of your machine's actual network conditions)

So... given that we have additive throttling in devtools.. it makes me think our BE computeWastedMsWithThroughput calculation is currently pretty innacurate for configs assuming devtools throttling..

[paulirish]

three possibilities

1. super minimal: return 0 (current state of PR)
2. smallish impact: use networkAnalyzer.throughput in BE's computeWastedMs
3. impact to be determined: change simulator so both 'provided' + 'devtools' throttling use networkSimulator results.
   • we don't have smoketest coverage that'd capture this so we want to verify the impact.

[paulirish]

@adamraine if you wanna land this with option 1 (as listed above), i'm only comfortable doing that as a super quick solution to avoid NaN.

options 2 and 3 are the only options that provide "correct" results, so i'd expect we'd followup in the shortterm with one of those.

[adamraine]

Lets do option 1 for v9, that's what's implemented now