From d58dbab8b8a6315fec0f7eebadb22847bf5c205b Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 6 Jun 2024 17:32:40 -0500 Subject: [PATCH] INP attribution: allow for LoAFs observed before interaction events (#487) * INP attribution: allow for LoAFs observed before interaction events * Call queueCleanup after LoAF processing * Address review feedback --------- Co-authored-by: Philip Walton --- src/attribution/onINP.ts | 50 ++++++++++++++++++++++++++++------------ src/onINP.ts | 27 ++++++++++++++-------- test/e2e/onINP-test.js | 3 +++ 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index 361fa4f2..819adb75 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -38,6 +38,15 @@ interface pendingEntriesGroup { entries: PerformanceEventTiming[]; } +// The maximum number of previous frames for which data is kept in regards to. +// Storing data about previous frames is necessary to handle cases where event +// and LoAF entries are dispatched out or order, and so a buffer of previous +// frame data is needed to determine various bits of INP attribution once all +// 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; + // A PerformanceObserver, observing new `long-animation-frame` entries. // If this variable is defined it means the browser supports LoAF. let loafObserver: PerformanceObserver | undefined; @@ -59,6 +68,9 @@ const pendingEntriesGroupMap: Map = new Map(); // are removed. let previousRenderTimes: number[] = []; +// The `processingEnd` time of most recently-processed event, chronologically. +let latestProcessingEnd: number; + // A WeakMap so you can look up the `renderTime` of a given entry and the // value returned will be the same value used by `pendingEntriesGroupMap`. const entryToRenderTimeMap: WeakMap< @@ -78,7 +90,8 @@ let idleHandle: number = -1; * Adds new LoAF entries to the `pendingLoAFs` list. */ const handleLoAFEntries = (entries: PerformanceLongAnimationFrameTiming[]) => { - entries.forEach((entry) => pendingLoAFs.push(entry)); + pendingLoAFs = pendingLoAFs.concat(entries); + queueCleanup(); }; // Get a reference to the interaction target element in case it's removed @@ -105,6 +118,8 @@ const groupEntriesByRenderTime = (entry: PerformanceEventTiming) => { let renderTime = entry.startTime + entry.duration; let previousRenderTime; + latestProcessingEnd = Math.max(latestProcessingEnd, entry.processingEnd); + // Iterate of all previous render times in reverse order to find a match. // Go in reverse since the most likely match will be at the end. for (let i = previousRenderTimes.length - 1; i >= 0; i--) { @@ -143,6 +158,8 @@ const groupEntriesByRenderTime = (entry: PerformanceEventTiming) => { if (entry.interactionId || entry.entryType === 'first-input') { entryToRenderTimeMap.set(entry, renderTime); } + + queueCleanup(); }; const queueCleanup = () => { @@ -167,7 +184,7 @@ const cleanupEntries = () => { // events are dispatched out of order. When this happens they're generally // only off by a frame or two, so keeping the most recent 50 should be // more than sufficient. - previousRenderTimes = previousRenderTimes.slice(-50); + previousRenderTimes = previousRenderTimes.slice(-1 * MAX_PREVIOUS_FRAMES); // Keep all render times that are part of a pending INP candidate or // that occurred within the 50 most recently-dispatched animation frames. @@ -181,8 +198,9 @@ const cleanupEntries = () => { if (!renderTimesToKeep.has(key)) pendingEntriesGroupMap.delete(key); }); - // Remove all pending LoAF entries that don't intersect with entries in - // the newly cleaned up `pendingEntriesGroupMap`. + // Keep all pending LoAF entries that either: + // 1) intersect with entries in the newly cleaned up `pendingEntriesGroupMap` + // 2) occur after the most recently-processed event entry. const loafsToKeep: Set = new Set(); pendingEntriesGroupMap.forEach((group) => { getIntersectingLoAFs(group.startTime, group.processingEnd).forEach( @@ -191,6 +209,17 @@ const cleanupEntries = () => { }, ); }); + 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]; + + // 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); + } + pendingLoAFs = Array.from(loafsToKeep); // Reset the idle callback handle so it can be queued again. @@ -200,7 +229,6 @@ const cleanupEntries = () => { entryPreProcessingCallbacks.push( saveInteractionTarget, groupEntriesByRenderTime, - queueCleanup, ); const getIntersectingLoAFs = ( @@ -319,15 +347,7 @@ export const onINP = ( loafObserver = observe('long-animation-frame', handleLoAFEntries); } unattributedOnINP((metric: INPMetric) => { - // Queue attribution and reporting in the next idle task. - // This is needed to increase the chances that all event entries that - // occurred between the user interaction and the next paint - // have been dispatched. Note: there is currently an experiment - // running in Chrome (EventTimingKeypressAndCompositionInteractionId) - // 123+ that if rolled out fully would make this no longer necessary. - whenIdle(() => { - const metricWithAttribution = attributeINP(metric); - onReport(metricWithAttribution); - }); + const metricWithAttribution = attributeINP(metric); + onReport(metricWithAttribution); }, opts); }; diff --git a/src/onINP.ts b/src/onINP.ts index 17707716..cafa6d2d 100644 --- a/src/onINP.ts +++ b/src/onINP.ts @@ -27,6 +27,7 @@ import {observe} from './lib/observe.js'; import {onHidden} from './lib/onHidden.js'; import {initInteractionCountPolyfill} from './lib/polyfills/interactionCountPolyfill.js'; import {whenActivated} from './lib/whenActivated.js'; +import {whenIdle} from './lib/whenIdle.js'; import {INPMetric, MetricRatingThresholds, ReportOpts} from './types.js'; @@ -85,15 +86,23 @@ export const onINP = ( let report: ReturnType; const handleEntries = (entries: INPMetric['entries']) => { - entries.forEach(processInteractionEntry); - - const inp = estimateP98LongestInteraction(); - - if (inp && inp.latency !== metric.value) { - metric.value = inp.latency; - metric.entries = inp.entries; - report(); - } + // Queue the `handleEntries()` callback in the next idle task. + // This is needed to increase the chances that all event entries that + // occurred between the user interaction and the next paint + // have been dispatched. Note: there is currently an experiment + // running in Chrome (EventTimingKeypressAndCompositionInteractionId) + // 123+ that if rolled out fully may make this no longer necessary. + whenIdle(() => { + entries.forEach(processInteractionEntry); + + const inp = estimateP98LongestInteraction(); + + if (inp && inp.latency !== metric.value) { + metric.value = inp.latency; + metric.entries = inp.entries; + report(); + } + }); }; const po = observe('event', handleEntries, { diff --git a/test/e2e/onINP-test.js b/test/e2e/onINP-test.js index e2e77aaf..3bdc691b 100644 --- a/test/e2e/onINP-test.js +++ b/test/e2e/onINP-test.js @@ -326,6 +326,9 @@ describe('onINP()', async function () { await browser.keys(['a', 'b', 'c']); + // Ensure the interaction completes. + await nextFrame(); + await stubForwardBack(); await beaconCountIs(1);