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

Missing LoAF attributions for INP due to violating internal invariants #511

Closed
brendankenny opened this issue Aug 6, 2024 · 0 comments · Fixed by #512
Closed

Missing LoAF attributions for INP due to violating internal invariants #511

brendankenny opened this issue Aug 6, 2024 · 0 comments · Fixed by #512

Comments

@brendankenny
Copy link
Member

brendankenny commented Aug 6, 2024

#487 introduced a bug that we didn't notice that reduces (often significantly) the number of INPs that get LoAF attribution.

The extra loop added to keep LoAFs that happened after all known events

for (let i = 0; i < MAX_PREVIOUS_FRAMES; i++) {
// Look at pending LoAF in reverse order so the most recent are first.
const loaf = pendingLoAFs[pendingLoAFs.length - 1 - i];
// If we reach LoAFs that overlap with event processing,
// we can assume all previous ones have already been handled.
if (!loaf || loaf.startTime < latestProcessingEnd) break;
loafsToKeep.add(loaf);
}

works as expected and is only additive to the LoAFs kept for potential attribution before #487. The problem is that it adds to the loafsToKeep set as a second batch of LoAFs, after the first loop through the LoAFs.

When the new array of LoAFs is created from this set (pendingLoAFs = Array.from(loafsToKeep)), the pendingLoAFs array is now ordered by how values were inserted into loafsToKeep. This is bad because pendingLoAFs is assumed to be in startTime order by getIntersectingLoAFs, which will quit looking for intersecting LoAFs early if it thinks it's reached the end of possible intersections:

// If the LoAF starts after the given end time, ignore it and all
// subsequent pending LoAFs (because they're in time order).
if (loaf.startTime > end) break;

All the correct LoAFs are in pendingLoAFs, but getIntersectingLoAFs gives up before seeing them. I believe this tends to get worse for busier or longer-lived pages because LoAFs that happened after any known interaction events get put last in this array initially, then they get paired with some event group as events come in so get put into the first batch of LoAFs in the loafsToKeep Set. But they're now (potentially) out of order in that batch, spreading the disorder throughout the pendingLoAFs array.

Fix: don't do that :) e.g. sort or filter from the existing sorted array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant