Simplify the event and LoAF entry clean up logic #662
Conversation
| const longestInteractionGroups = | ||
| // Create a set of entries groups that are part of the longest | ||
| // interactions (for faster lookup below). | ||
| const longestInteractionGroups = new Set( |
There was a problem hiding this comment.
This is now a Set rather than an Array for O(1) vs. O(n) lookup.
| interactionManager._longestInteractionList.map((i) => { | ||
| return entryToEntriesGroupMap.get(i.entries[0]); | ||
| }); | ||
| const minIndex = pendingEntriesGroups.length - MAX_PREVIOUS_FRAMES; |
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
Renaming loafsToKeep to intersectingLoAFs because these aren't the only LoAFs that are being kept.
| intersectingLoAFs.add(loaf); | ||
| } | ||
| } | ||
| const prevFrameIndexCutoff = pendingLoAFs.length - 1 - MAX_PREVIOUS_FRAMES; |
There was a problem hiding this comment.
@tunetheweb I removed the use of this prevFrameIndexCutoff because I don't think it's needed.
Since, for INP, LoAF entries are only relevant if they intersect with event entries, the check for intersections with the ~50 pendingEntriesGroups should be sufficient. Also, since there can be event timing entries in cases where there are no LoAF entries, keeping ~50 LoAF entries ends up applying to much more than the previous 50 frames, so I believe that's unnecessary.
There was a problem hiding this comment.
But please double check this, since AFAICT this is the only change in behavior from the previous code logic.
1 participant
This PR updates logic and comments in the
queueCleanup()function forattribution/onINP.tsto make it easier to follow what's happening.