-
Notifications
You must be signed in to change notification settings - Fork 294
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
Continuity: Make TTI calculation more reliable and performant #326
base: master
Are you sure you want to change the base?
Conversation
d2fe644
to
6596e9e
Compare
6596e9e
to
62372c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes those changes look good to me!
Hi @nicjansma , Could this PR merged? |
Hi @nicjansma , would this PR be merged? |
Hi @bluesmoon , @ceckoslab , @andreas-marschke , @ashenoy2014 , Regards, Sascha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 . (Thank you for adding the tests.)
Addresses a few issues seen in calculating Time To Interactive (TTI) in the Continuity plugin.
This PR refactors the TTI calculation code into a new function called
determineTti()
(so it's easier to test), and contains fixes proposed by both @sukratkashyap and @scottgifford.startBucket
could be negative:This generally doesn't cause a problem with the TTI calculation, as "negative" (missing) buckets will always fail the FPS-is-atleast-20-check, but it's still a loop over negative indicies that could be skipped.
This PR ensures
startBucket
is not negative before looping:Another issue @scottgifford pointed out is that the whole TTI calculation may be off-by-1 (100ms). I agree with his assessment, and have adjusted the TTI calculation to be at the correct bucket.
@sukratkashyap opened a PR for some improvements to the TTI calculation as well:
idleIntervals
outside the loop. Because we want to remember the number of intervals it was free before flushing out the data.bucketsVisited
to track buckets we have already visited. So that we don't go through the data again for which we didn't see any improvementsonBeacon
to remove only the data that we have seen. But ifc.tti
was calculated then flush as we use to before.Essentially, this allows the TTI calculator to be "restarted" if TTI wasn't found (e.g. it happens post-page-load, so Boomerang re-runs TTI calculation until it happens). These changes improve the performance of the TTI calculation.
@sukratkashyap's PR is great, and just needed some tests.
In an effort to make this code more reliable, all of the changes above were moved into a new function
determineTti()
, which allowed for unit test coverage.