-
Notifications
You must be signed in to change notification settings - Fork 401
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 missing LoAF attribution when entries are dispatched before event entries #487
Conversation
Oh, also I could not get a test for this working reliably. Inherently flaky and subject to future Chrome scheduler changes anyways. I verified the issue repro no longer fails, but looking for other ideas on this. Time to bust out some unit tests? Override |
Maybe we should do this clean up in |
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.
Thanks for taking a look at this @brendankenny, and for the detailed repro. It was very helpful for testing this!
I took Barry's suggestion and made a few other minor tweaks to the code, but other than that LGTM.
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 but some nits on comments.
Updated, and also moved the |
fixes #485
As described there, I added an extra condition to keep any LoAFs around that are after any existing known interactions, in case those interactions then come in later.
I then realized that would leave the LoAF collection unbounded, since a page can have a bunch of slow animation frames without the user interacting. I added a
MAX_UNPAIRED_LOAFS
of 20, so it will only keep the 20 most recent LoAFs after any known user interaction events. The number isn't hugely important, that just allows a one second delay in event-timing reporting even if the page somehow has filled the time with the shortest (50ms) long animation frames possible. LoAF entries are fairly light weight, so that's not so bad.As I was implementing this, however, I realized that even without this change the
pendingLoAFs
array can grow without bound on a page with e.g. animations and a busy main thread, since LoAFs are appended as they come in but clean up is only scheduled by user interactions. Seems like we should fix? @philipwalton @tunethewebMy only idea right now is to split the event timing and LoAF cleanups into two separate functions, and run each as their respective entries come in, since that's the only time each list of entries grows.