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

perf(config): disable styles gatherer #2153

Merged
merged 2 commits into from
May 5, 2017
Merged

Conversation

patrickhulce
Copy link
Collaborator

in the name of Operation Yaquina Bay (#2146)

shaves off ~40s on heavy sites like www.cnn.com

@brendankenny
Copy link
Member

need to remove smokehouse expectations too

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.

LGTM

I'm wondering if we should remove these altogether (instead of commenting out) and open up an issue tracking disabled audits/gatherers since it's difficult to give context to why these are still here but commented out, what thresholds would open them up to being re-added, etc.

At some point we're going to have to be honest with ourselves about these disabled gatherers/audits, as well. What's the likelihood that they'll come back? CacheStartUrl certainly isn't. If they aren't on anyone's plate, they probably don't have a future, and regardless they're just bitrotting in the meantime. It may be better just to delete them completely and have a link in the disabled-audit issue to the last commit where they existed for a hypothetical future person who'll take them up and make them fast so they can run again

@patrickhulce
Copy link
Collaborator Author

At some point we're going to have to be honest with ourselves about these disabled gatherers/audits, as well. What's the likelihood that they'll come back? If they aren't on anyone's plate, they probably aren't, and they're just bitrotting in the meantime. It may be better just to delete them completely and have a link in the disabled-audit issue to the last commit where they existed for a hypothetical future person who'll take them up and make them fast so they can run again

I agree, but it's at least on my plate after I/O to pick up CSS usage and it's been nice to have the code maintained and thought about during refactors instead of having a larger rewrite later. Not sure if that applies as much to this audit, but I'm in favor of keeping the gatherer at least around in the code.

@brendankenny
Copy link
Member

Oh, sorry, I don't mind at all if it has someone who really is meaning to get back to it with a timeframe (though commenting out in the default config for the project does look a bit janky :), but how much does anyone love old-flexbox? The styles gatherer is going to be difficult to get to run quickly. And then there's CacheStartUrl and friends...

@patrickhulce
Copy link
Collaborator Author

but how much does anyone love old-flexbox

heh, yeah... :)

@paulirish
Copy link
Member

lgtm on disabling this gatherer and old-flexbox audit.

++ on patrick picking up css/js coverage audits which will bring back this gatherer. i am sure there are perf wins to be had.

i prefer commenting out as these cases fall into two options: 1) we have explicit plans to bring it back in the short term or 2) the discussion will take some time to sort out

@paulirish paulirish merged commit f7ea935 into master May 5, 2017
@paulirish paulirish deleted the disable_styles_gatherer branch May 5, 2017 00:28
paulirish pushed a commit to paulirish/lighthouse that referenced this pull request May 5, 2017
@paulirish paulirish mentioned this pull request May 5, 2017
40 tasks
@paulirish paulirish added the OYB label May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants