Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: stricter LR desktop metric thresholds #6969

Closed
wants to merge 2 commits into from

Conversation

patrickhulce
Copy link
Collaborator

Summary
Uses data from HTTPArchive desktop runs to get new score thresholds

CNN Before

image

CNN After

image |

Related Issues/PRs
fixes #6836

@@ -137,7 +137,6 @@ function getFlags(manualArgv) {
// default values
.default('chrome-flags', '')
.default('output', ['html'])
.default('emulated-form-factor', 'mobile')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an erroneous double default that makes it impossible to run lr-desktop-config from the CLI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was the cli flag winning over the config setting? Yeah, that seems bad...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does output have the same problem? I don't see any other of these defaults in SharedFlagsSettings

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I can't support this because it lowered my site's perf from green to orange. So I think this is broken 😉

nit: linter wants a trailing comma

lighthouse-core/config/lr-desktop-config.js Outdated Show resolved Hide resolved
Co-Authored-By: patrickhulce <patrick.hulce@gmail.com>
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion on the new curves, so LGTM! :)

(probably at least @paulirish should take an actual look at them)

@@ -137,7 +137,6 @@ function getFlags(manualArgv) {
// default values
.default('chrome-flags', '')
.default('output', ['html'])
.default('emulated-form-factor', 'mobile')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was the cli flag winning over the config setting? Yeah, that seems bad...

@@ -137,7 +137,6 @@ function getFlags(manualArgv) {
// default values
.default('chrome-flags', '')
.default('output', ['html'])
.default('emulated-form-factor', 'mobile')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does output have the same problem? I don't see any other of these defaults in SharedFlagsSettings

// 75th and 95th percentiles -> median and PODR
// SELECT QUANTILES(fullyLoaded, 21) FROM [httparchive:summary_pages.2018_12_15_desktop] LIMIT 1000
{path: 'metrics/interactive', options: {scorePODR: 2000, scoreMedian: 4500}},
{path: 'metrics/first-cpu-idle', options: {scorePODR: 2000, scoreMedian: 4500}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wooooo good luck desktop sites!

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looked at the curves.
They are aggressive but their selection criteria is totally consistent with our mobile scores.

Shane's site is a little slow to load anyway, so this all seems fine. :)

@exterkamp
Copy link
Member

How do I downvote a review?

@brendankenny
Copy link
Member

ok, no clue what's going on with github. Merged in 161519a, PR isn't closing, so closing it manually :)

@brendankenny brendankenny deleted the lr_desktop_thresholds branch January 11, 2019 20:37
mattzeunert pushed a commit to mattzeunert/lighthouse that referenced this pull request Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lr-desktop preset is too generous
4 participants