Memory leak (#149)

* chore(scan): optimize perf of core scan

- use IntersectionObserver over getClientBoundingRect to minimize reflows

- optimize fast serialize

sample renders to run isRenderUnncessaryOn

dont merge outlines when there are significant active outlines

flush async instead of in hot path/ avoid promise creation every render

dont draw outlines that are out of viewport

* chore(scan): fix memory leak during frequent re-renders

refactor core outline logic to aggregate data at time of collection

optimize toolbar subscriptions to not impact perf when not active

dont waste computation on report render in monitoring

* fix(toolbar): cancel animation on cleanup

* fix log

* fixups

* fix toolbar bug

* cleanup

* add back minification

* add back scan in example

* remove force run in production

* fix(scan): dont draw when tab is not visible

* rm old lint suppression

* disable isRenderUnnecessary

* rm debug file

Author: RobPruzan

.eslintrc.js

@@ -18,6 +18,7 @@ module.exports = {
     sourceType: 'module',
   },
   rules: {
+    "@typescript-eslint/restrict-plus-operands":"off",
     "@typescript-eslint/no-unused-vars":"warn",
     '@typescript-eslint/explicit-function-return-type': 'off',
     'import/no-default-export': 'off',

git

@@ -88,8 +88,8 @@ const applyStealthScripts = async (context: BrowserContext) => {
     window.navigator.permissions.query = (parameters: any) =>
       parameters.name === 'notifications'
         ? Promise.resolve({
-          state: Notification.permission,
-        } as PermissionStatus)
+            state: Notification.permission,
+          } as PermissionStatus)
         : originalQuery(parameters);
   });
 };
@@ -139,7 +139,8 @@ const init = async () => {
       const installPromise = new Promise<void>((resolve, reject) => {
         const runInstall = () => {
           confirm({
-            message: 'No drivers found. Install Playwright Chromium driver to continue?',
+            message:
+              'No drivers found. Install Playwright Chromium driver to continue?',
           }).then((shouldInstall) => {
             if (isCancel(shouldInstall)) {
               cancel('Operation cancelled.');
@@ -157,7 +158,10 @@ const init = async () => {
 
             installProcess.on('close', (code) => {
               if (!code) resolve();
-              else reject(new Error(`Installation process exited with code ${code}`));
+              else
+                reject(
+                  new Error(`Installation process exited with code ${code}`),
+                );
             });
 
             installProcess.on('error', reject);

packages/scan/src/cli.mts

@@ -12,22 +12,26 @@ import {
   traverseFiber,
   detectReactBuildType,
 } from 'bippy';
-import {
-  type ActiveOutline,
-  flushOutlines,
-  type PendingOutline,
-} from '@web-utils/outline';
+import { flushOutlines, type Outline } from '@web-utils/outline';
 import { log, logIntro } from '@web-utils/log';
 import {
   createInspectElementStateMachine,
   type States,
 } from '@web-inspect-element/inspect-state-machine';
 import { playGeigerClickSound } from '@web-utils/geiger';
 import { ICONS } from '@web-assets/svgs/svgs';
-import { updateFiberRenderData, type RenderData } from 'src/core/utils';
+import {
+  aggregateChanges,
+  aggregateRender,
+  updateFiberRenderData,
+  type RenderData,
+} from 'src/core/utils';
 import { readLocalStorage, saveLocalStorage } from '@web-utils/helpers';
 import { initReactScanOverlay } from './web/overlay';
-import { createInstrumentation, type Render } from './instrumentation';
+import {
+  createInstrumentation,
+  type Render,
+} from './instrumentation';
 import { createToolbar } from './web/toolbar';
 import type { InternalInteraction } from './monitor/types';
 import { type getSession } from './monitor/utils';
@@ -70,6 +74,8 @@ export interface Options {
   /**
    * Log renders to the console
    *
+   * WARNING: This can add significant overhead when the app re-renders frequently
+   *
    * @default false
    */
   log?: boolean;
@@ -130,8 +136,8 @@ export interface Options {
   onCommitStart?: () => void;
   onRender?: (fiber: Fiber, renders: Array<Render>) => void;
   onCommitFinish?: () => void;
-  onPaintStart?: (outlines: Array<PendingOutline>) => void;
-  onPaintFinish?: (outlines: Array<PendingOutline>) => void;
+  onPaintStart?: (outlines: Array<Outline>) => void;
+  onPaintFinish?: (outlines: Array<Outline>) => void;
 }
 
 export type MonitoringOptions = Pick<
@@ -169,12 +175,15 @@ interface StoreType {
   lastReportTime: Signal<number>;
 }
 
+export type OutlineKey = `${string}-${string}`;
+
 export interface Internals {
   instrumentation: ReturnType<typeof createInstrumentation> | null;
   componentAllowList: WeakMap<React.ComponentType<any>, Options> | null;
   options: Signal<Options>;
-  scheduledOutlines: Array<PendingOutline>;
-  activeOutlines: Array<ActiveOutline>;
+  scheduledOutlines: Map<Fiber, Outline>; // we clear t,his nearly immediately, so no concern of mem leak on the fiber
+  // outlines at the same coordinates always get merged together, so we pre-compute the merge ahead of time when aggregating in activeOutlines
+  activeOutlines: Map<OutlineKey, Outline>; // we re-use the outline object on the scheduled outline
   onRender: ((fiber: Fiber, renders: Array<Render>) => void) | null;
   Store: StoreType;
 }
@@ -210,8 +219,8 @@ export const ReactScanInternals: Internals = {
     dangerouslyForceRunInProduction: false,
   }),
   onRender: null,
-  scheduledOutlines: [],
-  activeOutlines: [],
+  scheduledOutlines: new Map(),
+  activeOutlines: new Map(),
   Store,
 };
 
@@ -340,7 +349,7 @@ export const reportRender = (fiber: Fiber, renders: Array<Render>) => {
   const { selfTime } = getTimings(fiber.type);
   const displayName = getDisplayName(fiber.type);
 
-  Store.lastReportTime.value = performance.now();
+  Store.lastReportTime.value = Date.now();
 
   const currentFiberData = Store.reportData.get(reportFiber) ?? {
     count: 0,
@@ -367,9 +376,7 @@ export const reportRender = (fiber: Fiber, renders: Array<Render>) => {
       type: getType(fiber.type) || fiber.type,
     };
 
-    existingLegacyData.count =
-      Number(existingLegacyData.count || 0) + Number(renders.length);
-    existingLegacyData.time =
+    existingLegacyData.count = existingLegacyData.time =
       Number(existingLegacyData.time || 0) + Number(selfTime || 0);
     existingLegacyData.renders = renders;
 
@@ -541,18 +548,32 @@ export const start = () => {
     },
     isValidFiber,
     onRender(fiber, renders) {
-      if (Boolean(ReactScanInternals.instrumentation?.isPaused.value) || !ctx) {
-        // don't draw if it's paused
+      if (
+        Boolean(ReactScanInternals.instrumentation?.isPaused.value) ||
+        !ctx ||
+        document.visibilityState !== 'visible'
+      ) {
+        // don't draw if it's paused or tab is not active
         return;
       }
       updateFiberRenderData(fiber, renders);
+      if (ReactScanInternals.options.value.log) {
+        // this can be expensive given enough re-renders
+        log(renders);
+      }
 
       if (isCompositeFiber(fiber)) {
-        reportRender(fiber, renders);
+        // report render has a non trivial cost because it calls Date.now(), so we want to avoid the computation if possible
+        if (
+          ReactScanInternals.options.value.showToolbar !== false &&
+          Store.inspectState.value.kind === 'focused'
+        ) {
+          reportRender(fiber, renders);
+        }
       }
 
       if (ReactScanInternals.options.value.log) {
-        log(renders);
+        renders;
       }
 
       ReactScanInternals.options.value.onRender?.(fiber, renders);
@@ -562,10 +583,38 @@ export const start = () => {
         const domFiber = getNearestHostFiber(fiber);
         if (!domFiber || !domFiber.stateNode) continue;
 
-        ReactScanInternals.scheduledOutlines.push({
-          domNode: domFiber.stateNode,
-          renders,
-        });
+        if (ReactScanInternals.scheduledOutlines.has(fiber)) {
+          const existingOutline =
+            ReactScanInternals.scheduledOutlines.get(fiber)!;
+          aggregateRender(render, existingOutline.aggregatedRender);
+        } else {
+          ReactScanInternals.scheduledOutlines.set(fiber, {
+            domNode: domFiber.stateNode,
+            aggregatedRender: {
+              name:
+                renders.find((render) => render.componentName)?.componentName ??
+                'Unknown',
+              aggregatedCount: 1,
+              changes: aggregateChanges(render.changes),
+              didCommit: render.didCommit,
+              forget: render.forget,
+              fps: render.fps,
+              phase: new Set([render.phase]),
+              time: render.time,
+              // todo: add back a when clear use case in the UI is needed for isRenderUnnecessary, or performance is optimized
+              // unnecessary: isRenderUnnecessary(fiber),
+              unnecessary: false,
+              frame: 0,
+
+              computedKey: null,
+            },
+            alpha: null,
+            groupedAggregatedRender: null,
+            rect: null,
+            totalFrames: null,
+            estimatedTextWidth: null,
+          });
+        }
 
         // - audio context can take up an insane amount of cpu, todo: figure out why
         // - we may want to take this out of hot path

packages/scan/src/core/index.ts

@@ -14,7 +14,7 @@ import {
   traverseProps,
 } from 'bippy';
 import { type Signal, signal } from '@preact/signals';
-import { ReactScanInternals } from './index';
+import { ReactScanInternals, Store } from './index';
 
 let fps = 0;
 let lastTime = performance.now();
@@ -75,23 +75,29 @@ export const isElementInViewport = (
   return isVisible && rect.width && rect.height;
 };
 
-export interface Change {
+export interface RenderChange {
   type: 'props' | 'context' | 'state';
   name: string;
   prevValue: unknown;
   nextValue: unknown;
   unstable: boolean;
 }
 
+// this must grow O(1) w.r.t to renders
+export interface AggregatedChange {
+  type: Set<'props' | 'context' | 'state'>;
+  unstable: boolean;
+}
+
 export type Category = 'commit' | 'unstable' | 'unnecessary';
 
 export interface Render {
-  phase: string;
+  phase: 'mount' | 'update' | 'unmount';
   componentName: string | null;
   time: number | null;
-  count: number;
+  count: number; // uh is this right?
   forget: boolean;
-  changes: Array<Change> | null;
+  changes: Array<RenderChange>;
   unnecessary: boolean | null;
   didCommit: boolean;
   fps: number;
@@ -160,12 +166,11 @@ export function fastSerialize(value: unknown, depth = 0): string {
 }
 
 export const getPropsChanges = (fiber: Fiber) => {
-  const changes: Array<Change> = [];
+  const changes: Array<RenderChange> = [];
 
   const prevProps = fiber.alternate?.memoizedProps || {};
   const nextProps = fiber.memoizedProps || {};
 
-
   const allKeys = new Set([
     ...Object.keys(prevProps),
     ...Object.keys(nextProps),
@@ -181,7 +186,7 @@ export const getPropsChanges = (fiber: Fiber) => {
     ) {
       continue;
     }
-    const change: Change = {
+    const change: RenderChange = {
       type: 'props',
       name: propName,
       prevValue,
@@ -199,13 +204,13 @@ export const getPropsChanges = (fiber: Fiber) => {
 };
 
 export const getStateChanges = (fiber: Fiber) => {
-  const changes: Array<Change> = [];
+  const changes: Array<RenderChange> = [];
 
   traverseState(fiber, (prevState, nextState) => {
     if (Object.is(prevState.memoizedState, nextState.memoizedState)) return;
-    const change: Change = {
+    const change: RenderChange = {
       type: 'state',
-      name: '',
+      name: '', // bad interface should make this a discriminated union
       prevValue: prevState.memoizedState,
       nextValue: nextState.memoizedState,
       unstable: false,
@@ -217,13 +222,13 @@ export const getStateChanges = (fiber: Fiber) => {
 };
 
 export const getContextChanges = (fiber: Fiber) => {
-  const changes: Array<Change> = [];
+  const changes: Array<RenderChange> = [];
 
   traverseContexts(fiber, (prevContext, nextContext) => {
     const prevValue = prevContext.memoizedValue;
     const nextValue = nextContext.memoizedValue;
 
-    const change: Change = {
+    const change: RenderChange = {
       type: 'context',
       name: '',
       prevValue,
@@ -329,7 +334,7 @@ export const createInstrumentation = (
         }
         if (!validInstancesIndicies.length) return null;
 
-        const changes: Array<Change> = [];
+        const changes: Array<RenderChange> = [];
 
         const propsChanges = getPropsChanges(fiber);
         const stateChanges = getStateChanges(fiber);
@@ -359,8 +364,7 @@ export const createInstrumentation = (
           changes,
           time: selfTime,
           forget: hasMemoCache(fiber),
-          // only collect if the render was unnecessary 5% of the time since is isRenderUnnecessary is expensive
-          unnecessary: Math.random() < 0.05 ? isRenderUnnecessary(fiber) : null,
+          unnecessary: Store.monitor ? null : isRenderUnnecessary(fiber),
           didCommit: didFiberCommit(fiber),
           fps,
         };

packages/scan/src/core/instrumentation.ts

@@ -132,7 +132,6 @@ export const startMonitoring = () => {
       updateFiberRenderData(fiber, renders);
 
       if (isCompositeFiber(fiber)) {
-        reportRender(fiber, renders);
         aggregateComponentRenderToInteraction(fiber, renders);
       }
       ReactScanInternals.options.value.onRender?.(fiber, renders);