core(config): switch to lantern by default, add presets #5041

patrickhulce · 2018-04-25T20:59:24Z

Changes

converts the default throttling method to lantern 'simulate'
reduces the default network quiet/cpu quiet to 1000ms, this is a bit less aggressive than our old original values but still much improved, and we could increase this if needed, but the full 5s should definitely be unnecessary
replaces --perf and --mixed-content with --preset=perf/mixed-content, there's been some opposition to this before, it wouldn't be incredibly laborious to keep so lmk
old defaults are runnable with --throttling-method=devtools

Yet to Change

The perfy smoke tests made some assumptions about the network/CPU quiet thresholds that don't hold up anymore, so left those testing the devtools path for now and will update those as I finish up the opportunity parity, on the plus side rest of smoketest time went from ~100s to ~80s :)

Also left the golden LHR diffing on observed for now to reduce the noise, seems like we'll want to be checking both in our tests anyhow once there's 100% parity?

paulirish · 2018-04-25T21:56:30Z

lighthouse-cli/bin.js

-  config = mixedContentConfig;
-  // The mixed-content audits require headless Chrome (https://crbug.com/764505).
-  cliFlags.chromeFlags = `${cliFlags.chromeFlags} --headless`;
+} else if (cliFlags.configPreset) {


how about just --preset :D

wfm, @brendankenny ?

--preset

yeah, I think it reads well in context

paulirish · 2018-04-25T21:58:36Z

lighthouse-core/config/perf-config.js

@@ -6,7 +6,7 @@
 'use strict';

 module.exports = {
-  extends: 'lighthouse:default',
+  extends: 'lighthouse:observed',


I feel like the "observed" name refers to observed things (metrics, etc), regardless of throttling method.

Since this config is primarily about changing throttling method I would have expected a name like 'throttled'

I started out with devtools as the name but the primary value of this thing existing is having the 5s+ wait times in the defaultPass which applies to all observed methods. It does indeed change the throttlingMethod as well, but that's easily done without presets/extension. throttled overly communicates the value in changing the throttling IMO, i.e. why would I want --preset=throttled --throttlingMethod=off/provided?

The most consistent might be to make observed not actually apply the throttling by default, and require --throttlingMethod=devtools but that seems to discourage throttling which isn't great.

I just think its confusing that observedMetrics all exist outside of the lighthouse:observed config.

better question: when is observed used? for --preset=perf and for the ttci smoketest, right? why doesn't ttci smoke just use/extend perf config?

agree that observed is a little confusing here. It does sound like it's fairly neutral, observed regardless of throttling method, but if I was told it affected the throttling method I would guess it was closer to none, not using devtools throttling.

But it sounds like you may be rearranging things, so maybe I'll wait to comment further :)

brendankenny · 2018-04-27T23:30:15Z

lighthouse-core/config/config.js

@@ -187,7 +188,7 @@ function deepClone(json) {
  // injection of plugins.
  if (Array.isArray(json.passes)) {
    cloned.passes.forEach((pass, i) => {
-      pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers);
+      pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers || []);


what happened here? Don't we require at least an empty array in the passes?

Well IMO, we shouldn't and the new defaults for passes make sure it's at least an empty array, but that doesn't work with the cloning, I can revert if you feel strongly all passes should declare empty gatherers.

ok, I think that makes sense in the extending case, if I'm reading your response correctly. Can you make gatherers optional in the LH.Config.PassJson side of things, then?

brendankenny · 2018-04-27T23:34:34Z

lighthouse-core/config/perf-config.js

@@ -6,7 +6,7 @@
 'use strict';

 module.exports = {
-  extends: 'lighthouse:default',
+  extends: 'lighthouse:observed',


agree that observed is a little confusing here. It does sound like it's fairly neutral, observed regardless of throttling method, but if I was told it affected the throttling method I would guess it was closer to none, not using devtools throttling.

But it sounds like you may be rearranging things, so maybe I'll wait to comment further :)

patrickhulce · 2018-04-28T00:13:15Z

OK ready for re-review I believe! no more observed config or extension, config.js just forces the defaultPass quiet thresholds to 5.25s+ if the throttling method is devtools or provided

patrickhulce · 2018-05-01T18:03:39Z

friendly ping here, review please! :)

paulirish · 2018-05-02T00:51:52Z

lighthouse-core/config/config.js

+    const defaultPass = config.passes.find(pass => pass.passName === 'defaultPass');
+    if (!defaultPass) return;
+
+    defaultPass.pauseAfterLoadMs = Math.max(5250, defaultPass.pauseAfterLoadMs);


feels like this 5250 magic number should be defined in constants.js

good call done

paulirish · 2018-05-02T00:55:38Z

package.json

@@ -60,7 +60,7 @@
    "changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file",
    "type-check": "tsc -p .",
    "update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G",
-    "update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing",
+    "update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing",


doing this to avoid the mixed settings error, right?
next run of update-report-fixtures will be throttling-method: 'simulate', right?

so a followup PR to update both makes sense to me.

but speaking of. perhaps we should also have 1 golden LHR for simulate and 1 for throttled. Feels like we have a blind spot otherwise.

Also left the golden LHR diffing on observed for now to reduce the noise, seems like we'll want to be checking both in our tests anyhow once there's 100% parity?

totally agree ;)

patrickhulce · 2018-05-02T21:58:32Z

if everyone is afraid of this one we should get it in sooner rather than later to iron out the kinks ;)

patrickhulce · 2018-05-02T23:50:48Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -63,7 +63,7 @@ const SMOKETESTS = [{
  id: 'offline',
  expectations: 'offline-local/offline-expectations.js',
  config: smokehouseDir + 'offline-config.js',
-  batch: 'parallel-second',
+  batch: 'offline',


@paulirish I made this change after too many flakes on travis, now that things aren't throttled it seems too much is trying to happen at once and at least one navstart was getting dropped on ~70% of runs :(

paulirish · 2018-05-03T01:13:40Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -63,7 +63,7 @@ const SMOKETESTS = [{
  id: 'offline',
  expectations: 'offline-local/offline-expectations.js',
  config: smokehouseDir + 'offline-config.js',
-  batch: 'parallel-second',
+  batch: 'offline',


paulirish · 2018-05-03T01:15:12Z

lighthouse-core/config/constants.js

@@ -60,8 +60,15 @@ const defaultPassConfig = {
  gatherers: [],
 };

+const observedPassConfigOverrides = {


throttledPassConfigDefaults
or
nonSimulatedPassConfigOverrides

paulirish · 2018-05-03T01:17:34Z

lighthouse-core/config/config.js

+    if (!defaultPass) return;
+
+    const overrides = constants.observedPassConfigOverrides;
+    defaultPass.pauseAfterLoadMs =


i wouldn't yell bloody murder if you renamed this to dPass or something to avoid all the wrapppppppppppppppping

heh, yeah I spent more time than I should to save this one, basically nothing saves the last one (triple networkQuietThresholdMs is just too much` so I gave up, at least here it's consistent and easy to understand

paulirish · 2018-05-03T01:25:41Z

🍫 😋

brendankenny

just a few things, otherwise looking good!

brendankenny · 2018-05-03T18:39:13Z

lighthouse-core/config/config.js

@@ -187,7 +188,7 @@ function deepClone(json) {
  // injection of plugins.
  if (Array.isArray(json.passes)) {
    cloned.passes.forEach((pass, i) => {
-      pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers);
+      pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers || []);


ok, I think that makes sense in the extending case, if I'm reading your response correctly. Can you make gatherers optional in the LH.Config.PassJson side of things, then?

brendankenny · 2018-05-03T18:43:37Z

lighthouse-core/config/config.js

+      return;
+    }
+
+    const defaultPass = config.passes.find(pass => pass.passName === 'defaultPass');


hopefully in the future we can make a way to tell if it's a metric-measuring pass (similar to gather-runner's hacky isPerfRun) rather than just the default pass, but not sure how to solve the problem if you want to do metric-measuring and have your own thresholds here that are less than the defaults...

yeah that makes sense, I suppose we could do it on all useThrottling passes as a proxy?

discussed offline, it's an impl detail of TTI that we need 5s+ which is hardcoded to defaultPass so we're sticking to defaultPass for now 👍

brendankenny · 2018-05-03T18:46:30Z

lighthouse-core/scripts/assert-golden-lhr-unchanged.sh

@@ -28,7 +28,8 @@ trap teardown EXIT

 colorText "Generating a fresh LHR..." "$purple"
 set -x
-node "$lhroot_path/lighthouse-cli" -A="$lhroot_path/lighthouse-core/test/results/artifacts" --quiet --output=json --output-path="$freshLHRPath" http://localhost/dobetterweb/dbw_tester.html
+# TODO(phulce): add a lantern LHR-differ


we should do this soon

brendankenny · 2018-05-03T18:47:55Z

readme.md

@@ -58,7 +58,6 @@ Configuration:
                                 CHROME_PATH: Explicit path of intended Chrome binary. If set must point to an executable of a build of
                                 Chromium version 54.0 or later. By default, any detected Chrome Canary or Chrome (stable) will be launched.
                                                                                                                                       [default: ""]
-  --perf                         Use a performance-test-only configuration                                                                 [boolean]


need presets and other new changes here?

brendankenny · 2018-05-03T18:52:55Z

lighthouse-extension/app/src/lighthouse-background.js

@@ -22,7 +21,7 @@ const log = require('lighthouse-logger');
 window.runLighthouseForConnection = function(
    connection, url, options, categoryIDs,
    updateBadgeFn = function() { }) {
-  const config = options && options.fastMode ? new Config(fastConfig, options.flags) : new Config({


where was this set before? I can't find it

it's just set by lightrider, all runs for them will be simulated now anyway

brendankenny · 2018-05-03T18:55:24Z

lighthouse-core/test/config/config-test.js

+    });
+
+    assert.equal(config.settings.throttlingMethod, 'devtools');
+    assert.ok(config.passes[0].pauseAfterLoadMs >= 5000, 'did not adjust load quiet ms');


maybe add assertion that this is defaultPass? And maybe a test (or modify this test) with more categories so it can check other passes aren't affected

patrickhulce · 2018-05-03T20:47:24Z

@brendankenny LGTY?

brendankenny

LGTM!

brendankenny · 2018-05-03T21:25:44Z

…#5041)

core(config): switch to lantern by default, add config-preset

5bc5874

patrickhulce requested review from brendankenny and paulirish as code owners April 25, 2018 20:59

devtools-bot added the waiting4reviewer label Apr 25, 2018

continute to use observed for LHR diffing

a7d0089

paulirish reviewed Apr 25, 2018

View reviewed changes

brendankenny reviewed Apr 27, 2018

View reviewed changes

patrickhulce added 3 commits April 27, 2018 17:08

feedback

2b24c22

remove observed

ed63596

remove defaultPass rejigger

08d65f8

patrickhulce added 2 commits April 27, 2018 17:16

lint

b71d604

remove fastMode option from extension

287c94c

paulirish mentioned this pull request Apr 30, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

patrickhulce added the I/O-Expedite label Apr 30, 2018

paulirish removed the waiting4reviewer label Apr 30, 2018

patrickhulce added 2 commits April 30, 2018 15:16

Merge branch 'master' into lantern_default

52792e5

bump to 1000

94f5155

patrickhulce changed the title ~~core(config): switch to lantern by default, add config-preset~~ core(config): switch to lantern by default, add presets May 1, 2018

paulirish reviewed May 2, 2018

View reviewed changes

move to constants

3126132

put offline in its own batch

4b7c1fb

patrickhulce commented May 2, 2018

View reviewed changes

paulirish requested changes May 3, 2018

View reviewed changes

observed -> nonSimulated

0baf005

paulirish approved these changes May 3, 2018

View reviewed changes

brendankenny self-assigned this May 3, 2018

brendankenny requested changes May 3, 2018

View reviewed changes

feedback

3412c82

brendankenny approved these changes May 3, 2018

View reviewed changes

patrickhulce merged commit a6dbd1a into master May 3, 2018

patrickhulce deleted the lantern_default branch May 3, 2018 21:36

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

core(config): switch to lantern by default, add presets (GoogleChrome…

79f6c2c

…#5041)

core(config): switch to lantern by default, add presets #5041

core(config): switch to lantern by default, add presets #5041

Conversation

patrickhulce commented Apr 25, 2018 • edited Loading

Changes

Yet to Change

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 Apr 28, 2018

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

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

paulirish commented May 3, 2018

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

patrickhulce commented May 3, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented May 3, 2018

patrickhulce commented Apr 25, 2018 •

edited

Loading