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

Fix TTI logic #19099

Closed
avadacatavra opened this issue Nov 2, 2017 · 6 comments
Closed

Fix TTI logic #19099

avadacatavra opened this issue Nov 2, 2017 · 6 comments
Assignees

Comments

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Nov 2, 2017

At some point in time during the review/rebase process for #18670, I seem to have deleted and missed setting the navigation start for the time to interactive metric.

In hunting down this problem, I'm also suspicious that the window logic may be faulty. I have a local experiment to fix this, but need to figure out what went wrong.

I also need to refer to gecko/chrome spec/implementation again, because the way we've implemented tti, you can't know what it is until you've been on a page for at least 10s (requires a 10s window)--this has not been the case when I try in other browsers. I'm suspicious that domInteractive is simply domContentLoaded fired (https://developer.mozilla.org/en-US/docs/Web/API/PerformanceTiming/domInteractive). It's possible that I missed something, and there's a performance timing metric that I should be using instead (I've been using performance.timing.domInteractive and performance.timing.ConnectStart to calculate TTI)

cc @jdm

@avadacatavra avadacatavra self-assigned this Nov 2, 2017
@jdm
Copy link
Member

@jdm jdm commented Nov 2, 2017

The domInteractive property is very different from the Time to Interactive metric - it is when page stops being parsed, like you speculated. I don't see any way to avoid the 10s delay before the metric is available, since we require a 10s window of availability before we can classify it.

@jdm
Copy link
Member

@jdm jdm commented Nov 2, 2017

Firefox has not implemented TTI yet: https://bugzilla.mozilla.org/show_bug.cgi?id=1299118

@avadacatavra
Copy link
Contributor Author

@avadacatavra avadacatavra commented Nov 2, 2017

It looks like chrome has? GoogleChrome/lighthouse#450

@jdm
Copy link
Member

@jdm jdm commented Nov 2, 2017

https://github.com/GoogleChromeLabs/tti-polyfill is probably relevant. Looks like it's possible to measure TTI via JS in Chrome via the PerformanceObserver API.

@avadacatavra
Copy link
Contributor Author

@avadacatavra avadacatavra commented Nov 2, 2017

And yes, based on the spec, I don't see a way to avoid it. It looks like the chrome pr is from June 16, and then I see https://docs.google.com/document/d/1jbvwxj_GJtiTTqFM8dsVzCIy5XeKL5qtIAvuimcXb1o/edit from Dec...not sure if there's anything more recent?

@avadacatavra
Copy link
Contributor Author

@avadacatavra avadacatavra commented Nov 2, 2017

it looks like this is the most recent: https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/preview#

Should fix what we have and make a followup for consistently interactive

bors-servo added a commit that referenced this issue Nov 14, 2017
added navigation start for interactive metrics

<!-- Please describe your changes on the following line: -->
 follow up from  #18670, fixing #19099

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19099 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19218)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.