Refactor INP attribution code to fix errors on Windows 10 #495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendankenny
commented
Jun 11, 2024
| @@ -35,12 +35,13 @@ interface pendingEntriesGroup { | |||
| startTime: DOMHighResTimeStamp; | |||
| processingStart: DOMHighResTimeStamp; | |||
| processingEnd: DOMHighResTimeStamp; | |||
| renderTime: DOMHighResTimeStamp; | |||
There was a problem hiding this comment.
technically the code below could use group.entries[0].startTime + group.entries[0].duration and not add a new property here, but it seemed better to put this here as a convenience
There was a problem hiding this comment.
Yeah, new property is better I think.
philipwalton
approved these changes
Jun 11, 2024
There was a problem hiding this comment.
Thanks @brendankenny! Overall these changes LGTM andI think do simplify things.
| @@ -35,12 +35,13 @@ interface pendingEntriesGroup { | |||
| startTime: DOMHighResTimeStamp; | |||
| processingStart: DOMHighResTimeStamp; | |||
| processingEnd: DOMHighResTimeStamp; | |||
| renderTime: DOMHighResTimeStamp; | |||
There was a problem hiding this comment.
Yeah, new property is better I think.
| // 2) occur after the most recently-processed event entry. | ||
| const loafsToKeep: Set<PerformanceLongAnimationFrameTiming> = new Set(); | ||
| pendingEntriesGroupMap.forEach((group) => { | ||
| for (let i = 0; i < pendingEntriesGroups.length; i++) { |
There was a problem hiding this comment.
Didn't like a forEach() right above a regular for loop, eh? :)
You could alternatively do something like this to keep it one line (and which is common in the Google JS style guide):
for (let i = 0, group; group = pendingEntriesGroups[i]; i++) {
philipwalton
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UPDATE (2024-06-20): while intended to just be an internal refactor, the changes in this PR do appear to fix the source of errors being thrown for some users on Windows 10. The cause of these errors is still unknown.
I wanted to do this while the code was still fresh in my mind. There are no expected functional changes here, just code simplification.
Currently,
previousRenderTimesis used for easier lookup into event-timing-entry groups inpendingEntriesGroupMapand for quick event grouping by render time, but it's also not quite the same set of render times inpendingEntriesGroupMap(the times and groups grow together as events come in, but the map actually contains all the events referenceable bypreviousRenderTimesand groups with events fromlongestInteractionList). The results in more bookkeeping and (speaking as someone recently getting their head around this code :) it can make some of this logic harder to follow because effects aren't always collocated.This PR proposes getting rid of
previousRenderTimesso that there are only two main data structures, an array tracking LoAFs and an array tracking groups of event timing entries. I think this makes it a little simpler to understand how events are grouped as they come in and which event groups are being preserved during cleanup, without a runtime or clarity tradeoff elsewhere.latestProcessingEndand the entry WeakMap are still here as secondary data sources because they're convenient ways to skip extra work and they're both only ever updated in a single place (the WeakMap now maps directly to entry groups, though).