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

Continuity: Make TTI calculation more reliable and performant #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicjansma
Copy link

@nicjansma nicjansma commented Aug 18, 2021

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.

  1. @scottgifford pointed out that when we're running the TTI calculator, if Boomerang initializes after the page is Visually Ready, startBucket could be negative:
var startBucket = Math.floor((visuallyReady - startTime) / COLLECTION_INTERVAL);

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:

var startBucket = Math.max(Math.floor((visuallyReady - startTime) / COLLECTION_INTERVAL), 0);
  1. 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.

  2. @sukratkashyap opened a PR for some improvements to the TTI calculation as well:

  • Moving idleIntervals outside the loop. Because we want to remember the number of intervals it was free before flushing out the data.
  • Adding 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 improvements
  • Making onBeacon to remove only the data that we have seen. But if c.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.

  1. This also fixes an issue in Confused why TTI was marked when page was still "busy" #307, where if PageBusy metrics may have a race condition with TTI calculation. We now break on the final bucket where Page Busy data is missing (if it's being used).

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.

Copy link

@scottgifford scottgifford left a 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!

@mc-chaos
Copy link

Hi @nicjansma , Could this PR merged?
Regards, Sascha

@mc-chaos
Copy link

mc-chaos commented Jul 1, 2022

Hi @nicjansma ,

would this PR be merged?
Regards, Sascha

@SaschaBrechmannVHV
Copy link

Hi @bluesmoon , @ceckoslab , @andreas-marschke , @ashenoy2014 ,
could this PR be Merged ?

Regards, Sascha

Copy link

@sukratkashyap sukratkashyap left a 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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants