From 43d1cc61c0ed257fb8d6ddcb6691020b64f0210b Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sat, 5 Feb 2022 13:17:10 -0500 Subject: [PATCH] Switch back to Subscription in useSelector to fix unsubscribe perf The Subscription impl originally used an array of listeners just like the actual Redux store, but in #1523 we changed it to be a linked list to fix perf issues. The use of `indexOf+splice` caused a quadratic cost to unmounting many components. In v8, I originally dropped use of `Subscription` to potentially save on bundle size. However, `Provider` still uses `Subscription`, so that will always be in the bundle. For this issue, a user pointed out that directly subscribing to the store brought back the original quadratic unsubscription behavior, which I've confirmed locally. Swapping `store.subscribe` for `subscription.addNestedSub` fixes that issue. I've added a unit test that mounts and unmounts a massive tree and measures the elapsed time. There's a distinct difference between the "correct" and quadratic behavior. Weirdly, there's also a big diff in "correct" time between React 18, and 17 + the "compat" entry point, and I have no idea why. It's not as bad as the quadratic time, but it's still pretty expensive. I've written the test to set an acceptable max unmount time based on which React version we're running against, with some buffer added. --- src/hooks/useSelector.ts | 7 +- test/hooks/useSelector.spec.tsx | 141 +++++++++++++++++++++----------- 2 files changed, 95 insertions(+), 53 deletions(-) diff --git a/src/hooks/useSelector.ts b/src/hooks/useSelector.ts index e05525be3..13db2b503 100644 --- a/src/hooks/useSelector.ts +++ b/src/hooks/useSelector.ts @@ -1,4 +1,4 @@ -import { useContext, useDebugValue } from 'react' +import { useContext, useDebugValue, useCallback } from 'react' import { useReduxContext as useDefaultReduxContext } from './useReduxContext' import { ReactReduxContext } from '../components/Context' @@ -48,12 +48,11 @@ export function createSelectorHook( } } - const { store, getServerState } = useReduxContext()! + const { store, subscription, getServerState } = useReduxContext()! const selectedState = useSyncExternalStoreWithSelector( - store.subscribe, + subscription.addNestedSub, store.getState, - // TODO Need a server-side snapshot here getServerState || store.getState, selector, equalityFn diff --git a/test/hooks/useSelector.spec.tsx b/test/hooks/useSelector.spec.tsx index 59cf034cb..9d406d875 100644 --- a/test/hooks/useSelector.spec.tsx +++ b/test/hooks/useSelector.spec.tsx @@ -1,14 +1,23 @@ /*eslint-disable react/prop-types*/ -import React, { useCallback, useReducer, useLayoutEffect } from 'react' +import React, { + useCallback, + useReducer, + useLayoutEffect, + useState, + useContext, +} from 'react' import { createStore } from 'redux' import * as rtl from '@testing-library/react' import { Provider as ProviderMock, useSelector, + useDispatch, shallowEqual, connect, createSelectorHook, + ReactReduxContext, + Subscription, } from '../../src/index' import type { TypedUseSelectorHook, @@ -29,7 +38,6 @@ describe('React', () => { let renderedItems: any[] = [] type RootState = ReturnType let useNormalSelector: TypedUseSelectorHook = useSelector - type VoidFunc = () => void beforeEach(() => { normalStore = createStore( @@ -123,29 +131,18 @@ describe('React', () => { }) it('subscribes to the store synchronously', () => { - const listeners = new Set() - const originalSubscribe = normalStore.subscribe - - jest - .spyOn(normalStore, 'subscribe') - .mockImplementation((callback: VoidFunc) => { - listeners.add(callback) - const originalUnsubscribe = originalSubscribe(callback) - - return () => { - listeners.delete(callback) - originalUnsubscribe() - } - }) + let appSubscription: Subscription | null = null - const Parent = () => { + const Child = () => { const count = useNormalSelector((s) => s.count) - return count === 1 ? : null + return
{count}
} - const Child = () => { + const Parent = () => { + const { subscription } = useContext(ReactReduxContext) + appSubscription = subscription const count = useNormalSelector((s) => s.count) - return
{count}
+ return count === 1 ? : null } rtl.render( @@ -153,35 +150,23 @@ describe('React', () => { ) - // Provider + 1 component - expect(listeners.size).toBe(2) + // Parent component only + expect(appSubscription!.getListeners().get().length).toBe(1) rtl.act(() => { normalStore.dispatch({ type: '' }) }) - // Provider + 2 components - expect(listeners.size).toBe(3) + // Parent component + 1 child component + expect(appSubscription!.getListeners().get().length).toBe(2) }) it('unsubscribes when the component is unmounted', () => { - const originalSubscribe = normalStore.subscribe - - const listeners = new Set() - - jest - .spyOn(normalStore, 'subscribe') - .mockImplementation((callback: VoidFunc) => { - listeners.add(callback) - const originalUnsubscribe = originalSubscribe(callback) - - return () => { - listeners.delete(callback) - originalUnsubscribe() - } - }) + let appSubscription: Subscription | null = null const Parent = () => { + const { subscription } = useContext(ReactReduxContext) + appSubscription = subscription const count = useNormalSelector((s) => s.count) return count === 0 ? : null } @@ -196,15 +181,15 @@ describe('React', () => { ) - // Provider + 2 components - expect(listeners.size).toBe(3) + // Parent + 1 child component + expect(appSubscription!.getListeners().get().length).toBe(2) rtl.act(() => { normalStore.dispatch({ type: '' }) }) - // Provider + 1 component - expect(listeners.size).toBe(2) + // Parent component only + expect(appSubscription!.getListeners().get().length).toBe(1) }) it('notices store updates between render and store subscription effect', () => { @@ -504,12 +489,7 @@ describe('React', () => { ) const doDispatch = () => normalStore.dispatch({ type: '' }) - // Seems to be an alteration in behavior - not sure if 17/18, or shim/built-in - if (IS_REACT_18) { - expect(doDispatch).not.toThrowError() - } else { - expect(doDispatch).toThrowError() - } + expect(doDispatch).not.toThrowError() spy.mockRestore() }) @@ -660,6 +640,69 @@ describe('React', () => { expect(renderedItems.length).toBe(2) expect(renderedItems[0]).toBe(renderedItems[1]) }) + + it('should have linear or better unsubscribe time, not quadratic', () => { + const reducer = (state: number = 0, action: any) => + action.type === 'INC' ? state + 1 : state + const store = createStore(reducer) + const increment = () => ({ type: 'INC' }) + + const numChildren = 100000 + + function App() { + useSelector((s: number) => s) + const dispatch = useDispatch() + + const [children, setChildren] = useState(numChildren) + + const toggleChildren = () => + setChildren((c) => (c ? 0 : numChildren)) + + return ( +
+ + + {[...Array(children).keys()].map((i) => ( + + ))} +
+ ) + } + + function Child() { + useSelector((s: number) => s) + // Deliberately do not return any DOM here - we want to isolate the cost of + // unsubscribing, and tearing down thousands of JSDOM nodes is expensive and irrelevant + return null + } + + const { getByText } = rtl.render( + + + + ) + + const timeBefore = Date.now() + + const button = getByText('Toggle Children') + rtl.act(() => { + rtl.fireEvent.click(button) + }) + + const timeAfter = Date.now() + const elapsedTime = timeAfter - timeBefore + + // Seeing an unexpected variation in elapsed time between React 18 and React 17 + the compat entry point. + // With 18, I see around 75ms with correct implementation on my machine, with 100K items. + // With 17 + compat, the same correct impl takes about 4200-5000ms. + // With the quadratic behavior, this is at least 13000ms (or worse!) under 18, and 22000ms+ with 17. + // The 13000ms time for 18 stays the same if I use the shim, so it must be a 17 vs 18 difference somehow, + // although I can't imagine why, and if I remove the `useSelector` calls both tests drop to ~50ms. + // So, we'll modify our expectations here depending on whether this is an 18 or 17 compat test, + // and give some buffer time to allow for variations in test machines. + const expectedMaxUnmountTime = IS_REACT_18 ? 500 : 7000 + expect(elapsedTime).toBeLessThan(expectedMaxUnmountTime) + }) }) describe('error handling for invalid arguments', () => {