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(driver): waitForFCP when tracing #6944

Merged
merged 6 commits into from
Jan 8, 2019
Merged

core(driver): waitForFCP when tracing #6944

merged 6 commits into from
Jan 8, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
We don't actually wait for an FCP, and it seems like we should. There could be a lot more to this story of making our waitForFullyLoaded match TTI conditions as close as possible, but this is a good start.

Fully Loaded Conditions After This PR

all [load, FCP, network quiet] --> CPU quiet --> done

Potential Easy Alternative

all [FCP, load] --> network quiet --> CPU quiet --> done

Real TTI Conditions

FCP --> simultaneous network + CPU quiet --> done

Related Issues/PRs
closes #6941

@paulirish
Copy link
Member

node lighthouse-cli/index.js http://localhost:10200/infinite-loop.html --config-path=/home/travis/build/GoogleChrome/lighthouse/lighthouse-cli/test/smokehouse/error-config.js --output-path=smokehouse-19845.report.json --output=json --quiet --port=0 
Runtime error encountered: _waitForLoadEvent.cancel() called before it was defined
Error: _waitForLoadEvent.cancel() called before it was defined
    at Object.cancel (/home/travis/build/GoogleChrome/lighthouse/lighthouse-core/gather/driver.js:182:163)
    at /home/travis/build/GoogleChrome/lighthouse/lighthouse-core/gather/driver.js:236:424
    at Driver._waitForFullyLoaded (/home/travis/build/GoogleChrome/lighthouse/lighthouse-core/gather/driver.js:237:125)
    at <anonymous>

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 like the refactor ideas. _waitForFullyLoaded has gotten pretty terrible to read or add to :)

lighthouse-cli/test/fixtures/delayed-fcp.html Show resolved Hide resolved
lighthouse-core/gather/driver.js Show resolved Hide resolved
@@ -874,23 +918,25 @@ class Driver {
* possible workaround.
* Resolves on the url of the loaded page, taking into account any redirects.
* @param {string} url
* @param {{waitForLoad?: boolean, waitForNavigated?: boolean, passContext?: LH.Gatherer.PassContext}} options
* @param {{waitForFCP?: boolean, waitForLoad?: boolean, waitForNavigated?: boolean, passContext?: LH.Gatherer.PassContext}} options
Copy link
Member

Choose a reason for hiding this comment

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

is this pulled out as another option for performance reasons, or does fcp not fire in certain passes and we need the option so we don't always wait for the full timeout? It's unfortunate mental overhead since none of the wait* options are independent. Is there a way we can reorganize?

Copy link
Collaborator Author

@patrickhulce patrickhulce Jan 7, 2019

Choose a reason for hiding this comment

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

Correcto. On the passes without javascript and offline this can take unnecessarily long when we don't really care about FCP in those passes. In fact, every page that fails the without javascript audit would take 45s+ because all we're doing is checking for the existence of text.

It's unfortunate mental overhead since none of the wait* options are independent. Is there a way we can reorganize?

Agreed. There are really only 4 scenarios despite the 3 total booleans. We could switch to an enum? Opposed to doing it in followup?

Copy link
Member

Choose a reason for hiding this comment

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

There are really only 4 scenarios despite the 3 total booleans. We could switch to an enum? Opposed to doing it in followup?

No, that sounds good. Maybe pair it with a switch to "Potential Easy Alternative" (should mostly match what behavior today) or "Real TTI Conditions" (if we're feeling really brave)

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

Should we do a DZL?

@patrickhulce
Copy link
Collaborator Author

Should we do a DZL?

Fo sho

DZL, do a barrel roll!

@devtools-bot
Copy link

DZL is done! Go check it out http://lh-dzl-6944.surge.sh

@brendankenny
Copy link
Member

ha, well, I don't know how to interpret the DZL. Two sites slowed significantly in gather time, but the others didn't :) I think that's not unexpected?

One other thing (possibly the cause for the above): for LH runs that end up with the NO_FCP error, we'll now hit the pass timeout while we were probably stopping passes earlier before. That's not a huge deal on the CLI, I don't think (since the results are all audit errors regardless), but might be more of a big deal for LR @paulirish

@patrickhulce
Copy link
Collaborator Author

Two sites slowed significantly in gather time, but the others didn't :) I think that's not unexpected?

This is my eternal struggle to get around with DZL for the past month. The comparison hash in that URL was for several days off so the sites were just different 1/4/2019, 8:14:04 PM vs 1/7/2019, 4:48:23 PM. If you take a look at the exact hash from the same day as this was run, you'll find completely different runtime results. I've been doing lots of experiments to figure out how we can get a solid subset, but for now, most runtime results in DZL are pretty much unactionable :(

@patrickhulce
Copy link
Collaborator Author

Yes indeed if the page truly never paints content then we will wait until our timeout instead load/network quiet.

but might be more of a big deal for LR

Interesting, just from a capacity point of view? What would the alternative be?

@brendankenny
Copy link
Member

just from a capacity point of view?

Yeah. I think they were having a bunch of NO_FCP at some point, so the timeout rate might significantly increase in the logs? I have no idea if it would matter, though.

@brendankenny
Copy link
Member

@exterkamp says it doesn't matter and he'll be happy if he can watch the graphs change a bit :)

@paulirish just concurred.

Ready for merge?

@patrickhulce
Copy link
Collaborator Author

baller ⛹️‍♀️

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.

Page load doesn't wait for FCP before network quiet
4 participants