-
Notifications
You must be signed in to change notification settings - Fork 491
Simplify the event and LoAF entry clean up logic #662
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ interface pendingEntriesGroup { | |
| // the frame-related data has come in. | ||
| // In most cases this out-of-order data is only off by a frame or two, so | ||
| // keeping the most recent 50 should be more than sufficient. | ||
| const MAX_PREVIOUS_FRAMES = 50; | ||
| const MAX_PENDING_FRAMES = 50; | ||
|
|
||
| /** | ||
| * Calculates the [INP](https://web.dev/articles/inp) value for the current | ||
|
|
@@ -202,39 +202,45 @@ export const onINP = ( | |
| }; | ||
|
|
||
| const cleanupEntries = () => { | ||
| // Keep all render times that are part of a pending INP candidate or | ||
| // that occurred within the 50 most recently-dispatched groups of events. | ||
| const longestInteractionGroups = | ||
| // Create a set of entries groups that are part of the longest | ||
| // interactions (for faster lookup below). | ||
| const longestInteractionGroups = new Set( | ||
| interactionManager._longestInteractionList.map((i) => { | ||
| return entryToEntriesGroupMap.get(i.entries[0]); | ||
| }); | ||
| const minIndex = pendingEntriesGroups.length - MAX_PREVIOUS_FRAMES; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed this variable and just put the logic directly into the filter conditional. To me that makes it easier to understand what's happening, because having a variable with the term "index" in it—that can be negative—is more confusing to me than just looking at the logic directly. |
||
| pendingEntriesGroups = pendingEntriesGroups.filter((group, index) => { | ||
| if (index >= minIndex) return true; | ||
| return longestInteractionGroups.includes(group); | ||
| }), | ||
| ); | ||
|
|
||
| // Clean up the `pendingEntriesGroups` list so it doesn't grow endlessly. | ||
| // Keep any groups that: | ||
| // 1) Correspond to one of the current longest interactions, OR | ||
| // 2) Any group that's part of the most recent set of frames (based on | ||
| // `MAX_PENDING_FRAMES`). | ||
| pendingEntriesGroups = pendingEntriesGroups.filter((group, i) => { | ||
| return ( | ||
| longestInteractionGroups.has(group) || | ||
| i >= pendingEntriesGroups.length - MAX_PENDING_FRAMES | ||
| ); | ||
| }); | ||
|
|
||
| // Keep all pending LoAF entries that either: | ||
| // 1) intersect with entries in the newly cleaned up `pendingEntriesGroups` | ||
| // 2) occur after the most recently-processed event entry (for up to MAX_PREVIOUS_FRAMES) | ||
| const loafsToKeep: Set<PerformanceLongAnimationFrameTiming> = new Set(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renaming |
||
| // Create a set of LoAF entries that intersect with entries in the newly | ||
| // cleaned up `pendingEntriesGroups` (for faster lookup below). | ||
| const intersectingLoAFs: Set<PerformanceLongAnimationFrameTiming> = | ||
| new Set(); | ||
| for (const group of pendingEntriesGroups) { | ||
| const loafs = getIntersectingLoAFs(group.startTime, group.processingEnd); | ||
| for (const loaf of loafs) { | ||
| loafsToKeep.add(loaf); | ||
| intersectingLoAFs.add(loaf); | ||
| } | ||
| } | ||
| const prevFrameIndexCutoff = pendingLoAFs.length - 1 - MAX_PREVIOUS_FRAMES; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tunetheweb I removed the use of this Since, for INP, LoAF entries are only relevant if they intersect with event entries, the check for intersections with the ~50
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But please double check this, since AFAICT this is the only change in behavior from the previous code logic. |
||
| // Filter `pendingLoAFs` to preserve LoAF order. | ||
| pendingLoAFs = pendingLoAFs.filter((loaf, index) => { | ||
| if ( | ||
| loaf.startTime > latestProcessingEnd && | ||
| index > prevFrameIndexCutoff | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return loafsToKeep.has(loaf); | ||
| // Clean up the `pendingLoAFs` list so it doesn't grow endlessly. | ||
| // Keep all LoAFs that either: | ||
| // 1) Intersect with one of the above pending entries groups, OR | ||
| // 2) Occurred more recently than the most recently process event entry | ||
| pendingLoAFs = pendingLoAFs.filter((loaf) => { | ||
| return ( | ||
| intersectingLoAFs.has(loaf) || loaf.startTime > latestProcessingEnd | ||
| ); | ||
| }); | ||
|
|
||
| cleanupPending = false; | ||
|
|
||
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.
This is now a
Setrather than anArrayfor O(1) vs. O(n) lookup.