Perf: allow useMemoAll to cache results independently from deps (#5172)

* Perf: allow useMemoAll to cache results independently from deps * Use more canonical impl for useMemoAll * Rename * Changelog * Self review * Polish * Update packages/component/src/providers/ActivityTree/private/useActivitiesWithRenderer.ts Co-authored-by: William Wong <[email protected]> --------- Co-authored-by: William Wong <[email protected]>
microsoft · May 8, 2024 · d3a6f52 · d3a6f52
1 parent bf8dbd9
commit d3a6f52
Show file tree

Hide file tree

Showing 11 changed files with 294 additions and 101 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -128,6 +128,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 -  Fixes missing exports of `useNotifications`, in PR [#5148](https://github.com/microsoft/BotFramework-WebChat/pull/5148), by [@compulim](https://github.com/compulim)
 -  Fixes suggested actions keyboard navigation skips actions after suggested actions got updated, in PR [#5150](https://github.com/microsoft/BotFramework-WebChat/pull/5150), by [@OEvgeny](https://github.com/OEvgeny)
 -  Fixes [#5155](https://github.com/microsoft/BotFramework-WebChat/issues/5155). Fixed "Super constructor null of anonymous class is not a constructor" error in CDN bundle by bumping to [`[email protected]`](https://www.npmjs.com/package/webpack/v/5.91.0), in PR [#5156](https://github.com/microsoft/BotFramework-WebChat/pull/5156), by [@compulim](https://github.com/compulim)
+-  Improved performance for `useActivityWithRenderer`, in PR [#5172](https://github.com/microsoft/BotFramework-WebChat/pull/5172), by [@OEvgeny](https://github.com/OEvgeny)
 
 ### Changed
 

diff --git a/...rformance-js-batched-renderer-does-not-produce-unnecessary-rerenders-1-snap.png b/...rformance-js-batched-renderer-does-not-produce-unnecessary-rerenders-1-snap.png
diff --git a/__tests__/html/renderActivity.performance.html b/__tests__/html/renderActivity.performance.html
@@ -0,0 +1,106 @@
+<!doctype html>
+<html lang="en-US">
+  <head>
+    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
+    <script crossorigin="anonymous" src="https://unpkg.com/@babel/[email protected]/babel.min.js"></script>
+    <script crossorigin="anonymous" src="https://unpkg.com/[email protected]/umd/react.development.js"></script>
+    <script crossorigin="anonymous" src="https://unpkg.com/[email protected]/umd/react-dom.development.js"></script>
+    <script crossorigin="anonymous" src="/test-harness.js"></script>
+    <script crossorigin="anonymous" src="/test-page-object.js"></script>
+    <script crossorigin="anonymous" src="/__dist__/webchat-es5.js"></script>
+    <style>
+      small {
+        padding-inline: 8px;
+      }
+    </style>
+  </head>
+  <body>
+    <main id="webchat"></main>
+    <script type="text/babel" data-presets="env,stage-3,react">
+      const BATCH_SIZE = 5;
+
+      const timesActivityRendered = new Map();
+
+      function activityRendered() {
+        return next => (...args) => {
+          const [{ activity }] = args;
+          const renderActivity = next(...args)
+          timesActivityRendered.set(activity.id, (timesActivityRendered.get(activity.id) ?? 0) + 1);
+          return (...args) => (
+            <>
+              {renderActivity.call ? renderActivity(...args) : renderActivity}
+              <small> Rendered {timesActivityRendered.get(activity.id)} times</small>
+            </>
+          )
+        }
+      }
+
+      let shownCount = 0;
+      async function postMessagesBatch(directLine) {
+        const promises = [];
+        const timestamp = new Date().toISOString();
+        for (let index = 0; index < BATCH_SIZE; index++) {
+          promises.push(
+            // Plain text message isolate dependencies on Markdown.
+            directLine.emulateIncomingActivity(
+              { id: `activity-${shownCount + index}`, text: `Message ${shownCount + index}.`, textFormat: 'plain', type: 'message', timestamp },
+              { skipWait: true }
+            )
+          );
+        }
+        shownCount += BATCH_SIZE;
+
+        await Promise.all(promises);
+        await pageConditions.numActivitiesShown(shownCount);
+      }
+
+      run(
+        async function () {
+          const {
+            WebChat: { ReactWebChat }
+          } = window; // Imports in UMD fashion.
+
+          const { directLine, store } = testHelpers.createDirectLineEmulator();
+
+          WebChat.renderWebChat({ directLine, store, activityMiddleware: [activityRendered] }, document.querySelector('main'));
+
+          await pageConditions.uiConnected();
+          pageElements.transcript().focus();
+
+          // WHEN: Adding 10 activities.
+          await postMessagesBatch(directLine);
+          await postMessagesBatch(directLine);
+
+          // THEN: Should not re-render activity more than twice.
+          expect(Math.max(...timesActivityRendered.values())).toEqual(2);
+          expect(Math.min(...timesActivityRendered.values())).toEqual(1);
+
+          // WHEN: Scroll and clicked on the 5th activity.
+          const previousTimesActivityRendered = structuredClone(timesActivityRendered)
+          pageElements.activities()[4].scrollIntoView();
+          await host.clickAt(10, 10, pageElements.activities()[4]);
+
+          // THEN: Should focus on the activity.
+          expect(pageElements.focusedActivity()).toEqual(pageElements.activities()[4]);
+
+          // THEN: Should not re-render.          
+          expect(timesActivityRendered).toEqual(previousTimesActivityRendered);
+
+          // WHEN: The 9th activity received an update.
+          const timestamp = new Date().toISOString();
+          const activity9Renders = timesActivityRendered.get('activity-8');
+          await directLine.emulateIncomingActivity(
+              { id: `activity-8`, text: `Activity 8 got updated`, textFormat: 'plain', type: 'message', timestamp },
+              { skipWait: true }
+            );
+
+          // THEN: Should re-render the 9th activity once.
+          expect(timesActivityRendered.get('activity-8')).toBe(activity9Renders + 1);
+          // THEN: Should render the updated 9th activity.
+          pageElements.focusedActivity().scrollIntoView();
+          await host.snapshot();
+        }
+      );
+    </script>
+  </body>
+</html>
diff --git a/__tests__/html/renderActivity.performance.js b/__tests__/html/renderActivity.performance.js
@@ -0,0 +1,5 @@
+/** @jest-environment ./packages/test/harness/src/host/jest/WebDriverEnvironment.js */
+
+describe('Batched renderer', () => {
+  test('does not produce unnecessary rerenders', () => runHTML('renderActivity.performance'));
+});
diff --git a/packages/component/src/Activity/CarouselLayout.js b/packages/component/src/Activity/CarouselLayout.js
@@ -10,7 +10,7 @@ import {
 
 import classNames from 'classnames';
 import PropTypes from 'prop-types';
-import React, { useMemo } from 'react';
+import React, { memo, useMemo } from 'react';
 
 import CarouselFilmStrip from './CarouselFilmStrip';
 import useNonce from '../hooks/internal/useNonce';
@@ -117,4 +117,4 @@ CarouselLayout.propTypes = {
   ...CarouselLayoutCore.propTypes
 };
 
-export default CarouselLayout;
+export default memo(CarouselLayout);
diff --git a/packages/component/src/Activity/StackedLayout.tsx b/packages/component/src/Activity/StackedLayout.tsx
@@ -3,7 +3,7 @@
 import { hooks } from 'botframework-webchat-api';
 import classNames from 'classnames';
 import PropTypes from 'prop-types';
-import React from 'react';
+import React, { memo } from 'react';
 
 import Bubble from './Bubble';
 import isZeroOrPositive from '../Utils/isZeroOrPositive';
@@ -244,4 +244,4 @@ StackedLayout.propTypes = {
   showCallout: PropTypes.bool
 };
 
-export default StackedLayout;
+export default memo(StackedLayout);
diff --git a/packages/component/src/Transcript/ActivityRow.tsx b/packages/component/src/Transcript/ActivityRow.tsx
@@ -1,7 +1,7 @@
 import { hooks } from 'botframework-webchat-api';
 import classNames from 'classnames';
 import PropTypes from 'prop-types';
-import React, { forwardRef, useCallback, useRef } from 'react';
+import React, { forwardRef, memo, useCallback, useRef } from 'react';
 
 import { android } from '../Utils/detectBrowser';
 import FocusTrap from './FocusTrap';
@@ -121,4 +121,4 @@ ActivityRow.propTypes = {
   children: PropTypes.any
 };
 
-export default ActivityRow;
+export default memo(ActivityRow);
diff --git a/packages/component/src/hooks/internal/useMemoAll.spec.jsx b/packages/component/src/hooks/internal/useMemoAll.spec.jsx
@@ -0,0 +1,147 @@
+/** @jest-environment jsdom */
+/* eslint-disable react/prop-types */
+/* eslint-disable no-undef */
+/* eslint no-magic-numbers: "off" */
+import React from 'react';
+import { render } from 'react-dom';
+import { act } from 'react-dom/test-utils';
+import useMemoize from './useMemoAll';
+
+const testHook = fun => {
+  let state;
+  const UseComponent = ({ useTest }) => {
+    useTest();
+    return null;
+  };
+  const TestComponent = () => {
+    state = React.useState();
+    const [useTest] = state;
+    if (useTest) {
+      return <UseComponent useTest={useTest} />;
+    }
+
+    return <React.Fragment />;
+  };
+
+  const root = document.createElement('div');
+  render(<TestComponent />, root);
+
+  return (...args) => {
+    const [_useTest, setTest] = state;
+    return new Promise(resolve => {
+      act(() => {
+        setTest(() => () => resolve(fun(...args)));
+      });
+    });
+  };
+};
+
+test('useMemoize should cache result across runs', async () => {
+  const expensiveSum = jest.fn((x, y) => x + y);
+
+  const render = testHook(doMemoChecks => {
+    // Start a run, all calls to sum() will be cached.
+    useMemoize(expensiveSum, sum => doMemoChecks(sum), []);
+  });
+
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Not cached, return 3.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3.
+    expect(sum(2, 4)).toBe(6); // Not cached, return 6.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3. This is cached because it is inside the same run.
+  });
+  expect(expensiveSum).toHaveBeenCalledTimes(2);
+
+  expect(expensiveSum.mock.calls[0]).toEqual([1, 2]);
+  expect(expensiveSum.mock.calls[1]).toEqual([2, 4]);
+
+  // After the run, 1 + 2 = 3, and 2 + 4 = 6 is cached.
+
+  // Start another run with previous cache
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Cached from previous run, return 3.
+    expect(sum(3, 6)).toBe(9); // Not cached, return 9.
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(3);
+  expect(expensiveSum.mock.calls[2]).toEqual([3, 6]);
+
+  // After the run, only 1 + 2 = 3 and 3 + 6 = 9 is cached. 2 + 4 is dropped.
+
+  // Start another run with previous cache
+  await render(sum => {
+    expect(sum(2, 4)).toBe(6); // Not cached, return 6
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(4);
+  expect(expensiveSum.mock.calls[3]).toEqual([2, 4]);
+});
+
+test('useMemoize should cache result if deps change', async () => {
+  const expensiveSum = jest.fn((x, y) => x + y);
+
+  const render = testHook(doMemoChecks => {
+    // Start a run, all calls to sum() will be cached.
+    useMemoize(expensiveSum, sum => doMemoChecks(sum), [{}]);
+  });
+
+  // Start a run, all calls to sum() will be cached.
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Not cached, return 3.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3.
+    expect(sum(2, 4)).toBe(6); // Not cached, return 6.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3. This is cached because it is inside the same run.
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(2);
+
+  expect(expensiveSum.mock.calls[0]).toEqual([1, 2]);
+  expect(expensiveSum.mock.calls[1]).toEqual([2, 4]);
+
+  // After the run, 1 + 2 = 3, and 2 + 4 = 6 is cached.
+
+  // Start another run with previous cache
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Cached from previous run, return 3.
+    expect(sum(3, 6)).toBe(9); // Not cached, return 9.
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(3);
+  expect(expensiveSum.mock.calls[2]).toEqual([3, 6]);
+
+  // After the run, only 1 + 2 = 3 and 3 + 6 = 9 is cached. 2 + 4 is dropped.
+
+  // Start another run with previous cache
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Cached from previous run, return 3.
+    expect(sum(2, 4)).toBe(6); // Not cached, return 6
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(4);
+  expect(expensiveSum.mock.calls[3]).toEqual([2, 4]);
+});
+
+test('useMemoize should not share cache across hooks', async () => {
+  const expensiveSum = jest.fn((x, y) => x + y);
+
+  const render = testHook(doMemoChecks => {
+    // Start a run, all calls to sum() will be cached.
+    useMemoize(expensiveSum, sum => doMemoChecks(sum), []);
+    useMemoize(expensiveSum, sum => doMemoChecks(sum), []);
+  });
+
+  // Start a run, all calls to sum() will be cached.
+  await render(sum => {
+    expect(sum(1, 2)).toBe(3); // Not cached, return 3.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3.
+    expect(sum(2, 4)).toBe(6); // Not cached, return 6.
+    expect(sum(1, 2)).toBe(3); // Cached, return 3. This is cached because it is inside the same run.
+  });
+
+  expect(expensiveSum).toHaveBeenCalledTimes(4);
+
+  expect(expensiveSum.mock.calls[0]).toEqual([1, 2]);
+  expect(expensiveSum.mock.calls[1]).toEqual([2, 4]);
+  expect(expensiveSum.mock.calls[2]).toEqual([1, 2]);
+  expect(expensiveSum.mock.calls[3]).toEqual([2, 4]);
+});
diff --git a/...omponent/src/hooks/internal/useMemoize.ts → ...omponent/src/hooks/internal/useMemoAll.ts b/...omponent/src/hooks/internal/useMemoize.ts → ...omponent/src/hooks/internal/useMemoAll.ts
@@ -1,4 +1,5 @@
-import { DependencyList, useMemo } from 'react';
+import { useMemo, useRef, type DependencyList } from 'react';
+import { useRefFrom } from 'use-ref-from';
 
 type Cache<TArgs, TResult> = { args: TArgs[]; result: TResult };
 type Fn<TArgs, TResult> = (...args: TArgs[]) => TResult;
@@ -8,11 +9,13 @@ type Fn<TArgs, TResult> = (...args: TArgs[]) => TResult;
  *
  * This is similar to `useMemo`. But instead of calling it once, `useMemoize` enables multiple calls while the `callback` function is executed.
  *
+ * We store cache outside of the memo, so that even in case when dependencies change we're able to use the previous cache for subsequent invocations
+ *
  * @param {Fn<TArgs, TIntermediate>} fn - The function to be memoized.
  * @param {(fn: Fn<TArgs, TIntermediate>) => TFinal} callback - When called, this function should execute the memoizing function.
  * @param {DependencyList[]} deps - Dependencies to detect for chagnes.
  */
-export default function useMemoize<TIntermediate, TFinal>(
+export default function useMemoAll<TIntermediate, TFinal>(
   fn: Fn<unknown, TIntermediate>,
   callback: (fn: Fn<unknown, TIntermediate>) => TFinal,
   deps: DependencyList[]
@@ -25,10 +28,13 @@ export default function useMemoize<TIntermediate, TFinal>(
     throw new Error('The third argument must be an array.');
   }
 
-  const memoizedFn = useMemo(() => {
-    let cache: Cache<unknown, TIntermediate>[] = [];
+  const fnRef = useRefFrom<Fn<unknown, TIntermediate>>(fn);
+  const cacheRef = useRef<Cache<unknown, TIntermediate>[]>([]);
 
-    return (run: (fn: Fn<unknown, TIntermediate>) => TFinal) => {
+  const memoizedFn = useMemo(
+    () => (run: (fn: Fn<unknown, TIntermediate>) => TFinal) => {
+      const { current: fn } = fnRef;
+      const { current: cache } = cacheRef;
       const nextCache: Cache<unknown, TIntermediate>[] = [];
       const result = run((...args) => {
         const { result } = [...cache, ...nextCache].find(
@@ -41,13 +47,14 @@ export default function useMemoize<TIntermediate, TFinal>(
         return result;
       });
 
-      cache = nextCache;
+      cacheRef.current = nextCache;
 
       return result;
-    };
+    },
     // We are manually creating the deps here. The "callback" arg is also designed not to be impact deps, similar to useEffect(fn), where "fn" is not in deps.
     /* eslint-disable-next-line react-hooks/exhaustive-deps */
-  }, [fn, ...deps]);
+    [fnRef, cacheRef, ...deps]
+  );
 
   return memoizedFn(callback);
 }