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

polish: listen for network idle after DCL #2271

Merged
merged 4 commits into from
May 24, 2017
Merged

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented May 16, 2017

@brendankenny has observed flakiness of TTCI and TTFI on simpler sites since we've switched to waiting on 2-quiet, my hypothesis is that under the new harsher throttling, we'll see this even more frequently because 2-idle is true as soon as we request the initial page (which if it takes too long 2-quiet could fire before we even load any other content!). Worst case sites already run up to the timeout, so this shouldn't impact them as much. Simpler sites see longer times, but that's the goal since we're ending the trace too early in some cases.

UPDATE: waits for DCL before listening for network idle per @deepanjanroy 's suggestion, this seems to mostly fix the issue for sites like vevo and NASA who were reaching "network idle" while the primary script request was in-flight

@brendankenny
Copy link
Member

brendankenny commented May 16, 2017

#2238 might throw another wrench in the works as well :)

I can do some testing with both this afternoon.

https://react-hn.appspot.com/ was a common flaky test site if others want to try some things out.

@tdresser
Copy link

@deepanjanroy FYI.

@deepanjanroy
Copy link
Contributor

I don't know if this is an urgent fix for I/O so maybe you shouldn't block this change, but many many sites do long polling / intermittent polling and they may never hit network quiet if we use 0-quiet. Technically you're supposed to wait after FMP, but our current definition of FMP also needs a quietness condition so I can see there is a circular dependency here.

Could we wait for FCP/DCL before looking for network 2-quiet? Would that fix the issue?

@deepanjanroy
Copy link
Contributor

Or maybe I'm not understanding how lighthouse works - is there a hard timeout?

@patrickhulce
Copy link
Collaborator Author

@tdresser @deepanjanroy FWIW the signals we used before weren't quite as sophisticated since we don't actually get live network quietness (protocol events are delayed while there's main thread activity). This shouldn't be a problem once we have the quietness signal from Chrome plumbed through and can just listen for it over the protocol. We took a shortcut in #2220 to try to get the run time down.

Could we wait for FCP/DCL before looking for network 2-quiet? Would that fix the issue?

waiting for DCL before kicking off our 2-quiet 5s window would probably fix this too it just wasn't part of the "it worked before" fix :) I'll compare some times with that and the old (current PR) approach

@deepanjanroy
Copy link
Contributor

Oh I see - I was missing a lot of context on this PR. Thanks for clarifying :)

@patrickhulce patrickhulce changed the title fix: switch back to waiting for 0-quiet [DISCUSSION NEEDED] polish: listen for network idle after DCL May 18, 2017
@patrickhulce
Copy link
Collaborator Author

updated to just wait for DCL instead PTAL @brendankenny :)

@patrickhulce
Copy link
Collaborator Author

@brendankenny
Copy link
Member

2Fast4DCL

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 do wish we had monitoring set up for tracking general failure rates (for whatever metric). This should only push out end of trace, though, so it should only help things (at the expense of sometimes tracing too long).

@brendankenny brendankenny removed the #BOB label May 24, 2017
@brendankenny brendankenny merged commit 131df27 into master May 24, 2017
@brendankenny brendankenny deleted the switch_to_0_quiet branch May 24, 2017 00:14
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.

None yet

4 participants