From 3aa41ea28038858e16a25518ec7f615212f150ef Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 23 May 2024 14:55:31 -0500 Subject: [PATCH 1/3] INP attribution: allow for LoAFs observed before interaction events --- src/attribution/onINP.ts | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index 361fa4f2..4838e782 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -38,6 +38,12 @@ interface pendingEntriesGroup { entries: PerformanceEventTiming[]; } +/** + * The maximum number of LoAFs to retain on cleanup that don't have any + * overlapping events. + */ +const MAX_UNPAIRED_LOAFS = 20; + // A PerformanceObserver, observing new `long-animation-frame` entries. // If this variable is defined it means the browser supports LoAF. let loafObserver: PerformanceObserver | undefined; @@ -181,8 +187,10 @@ 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 + // - intersect with entries in the newly cleaned up `pendingEntriesGroupMap` + // - occur after all existing pendingEntriesGroups, in case a LoAF came in + // before concurrent entries. const loafsToKeep: Set = new Set(); pendingEntriesGroupMap.forEach((group) => { getIntersectingLoAFs(group.startTime, group.processingEnd).forEach( @@ -191,6 +199,20 @@ const cleanupEntries = () => { }, ); }); + const latestRenderTime = Math.max.apply(Math, previousRenderTimes); + // Start at the end to keep the most recent `MAX_UNPAIRED_LOAFS` LoAFs. + const stoppingPoint = Math.max( + 0, + pendingLoAFs.length - 1 - MAX_UNPAIRED_LOAFS, + ); + for (let i = pendingLoAFs.length - 1; i >= stoppingPoint; i--) { + const loaf = pendingLoAFs[i]; + // If reached LoAFs that overlap with events, no more will be after. + if (loafsToKeep.has(loaf) || loaf.startTime <= latestRenderTime) break; + + loafsToKeep.add(loaf); + } + pendingLoAFs = Array.from(loafsToKeep); // Reset the idle callback handle so it can be queued again. From d41f3bc377a58388c39a96e5a106ee89009a8df6 Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Wed, 5 Jun 2024 21:56:08 -0700 Subject: [PATCH 2/3] Call queueCleanup after LoAF processing --- src/attribution/onINP.ts | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index 4838e782..0973c269 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -42,7 +42,7 @@ interface pendingEntriesGroup { * The maximum number of LoAFs to retain on cleanup that don't have any * overlapping events. */ -const MAX_UNPAIRED_LOAFS = 20; +const MAX_PENDING_ENTRIES = 50; // A PerformanceObserver, observing new `long-animation-frame` entries. // If this variable is defined it means the browser supports LoAF. @@ -65,6 +65,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< @@ -84,7 +87,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 @@ -111,6 +115,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--) { @@ -149,6 +155,8 @@ const groupEntriesByRenderTime = (entry: PerformanceEventTiming) => { if (entry.interactionId || entry.entryType === 'first-input') { entryToRenderTimeMap.set(entry, renderTime); } + + queueCleanup(); }; const queueCleanup = () => { @@ -173,7 +181,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_PENDING_ENTRIES); // Keep all render times that are part of a pending INP candidate or // that occurred within the 50 most recently-dispatched animation frames. @@ -187,10 +195,9 @@ const cleanupEntries = () => { if (!renderTimesToKeep.has(key)) pendingEntriesGroupMap.delete(key); }); - // Keep all pending LoAF entries that - // - intersect with entries in the newly cleaned up `pendingEntriesGroupMap` - // - occur after all existing pendingEntriesGroups, in case a LoAF came in - // before concurrent entries. + // 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( @@ -199,16 +206,13 @@ const cleanupEntries = () => { }, ); }); - const latestRenderTime = Math.max.apply(Math, previousRenderTimes); - // Start at the end to keep the most recent `MAX_UNPAIRED_LOAFS` LoAFs. - const stoppingPoint = Math.max( - 0, - pendingLoAFs.length - 1 - MAX_UNPAIRED_LOAFS, - ); - for (let i = pendingLoAFs.length - 1; i >= stoppingPoint; i--) { - const loaf = pendingLoAFs[i]; - // If reached LoAFs that overlap with events, no more will be after. - if (loafsToKeep.has(loaf) || loaf.startTime <= latestRenderTime) break; + for (let i = 0; i < MAX_PENDING_ENTRIES; 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); } @@ -222,7 +226,6 @@ const cleanupEntries = () => { entryPreProcessingCallbacks.push( saveInteractionTarget, groupEntriesByRenderTime, - queueCleanup, ); const getIntersectingLoAFs = ( From da6f86deeba3517ff97ab6a52b912b3154486dc6 Mon Sep 17 00:00:00 2001 From: Philip Walton Date: Thu, 6 Jun 2024 15:23:06 -0700 Subject: [PATCH 3/3] Address review feedback --- src/attribution/onINP.ts | 29 ++++++++++++----------------- src/onINP.ts | 23 ++++++++++++++++------- test/e2e/onINP-test.js | 3 +++ 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index 0973c269..819adb75 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -38,11 +38,14 @@ interface pendingEntriesGroup { entries: PerformanceEventTiming[]; } -/** - * The maximum number of LoAFs to retain on cleanup that don't have any - * overlapping events. - */ -const MAX_PENDING_ENTRIES = 50; +// 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. @@ -181,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(-1 * MAX_PENDING_ENTRIES); + 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. @@ -206,7 +209,7 @@ const cleanupEntries = () => { }, ); }); - for (let i = 0; i < MAX_PENDING_ENTRIES; i++) { + 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]; @@ -344,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 f8172872..a91826f2 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'; @@ -75,15 +76,23 @@ export const onINP = ( let report: ReturnType; const handleEntries = (entries: INPMetric['entries']) => { - entries.forEach(processInteractionEntry); + // 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(); + const inp = estimateP98LongestInteraction(); - if (inp && inp.latency !== metric.value) { - metric.value = inp.latency; - metric.entries = inp.entries; - report(); - } + 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);