Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v8] useSelector without custom subscription manager is slower than v7 (performance regression) #1869

Closed
1 task done
Kasdy opened this issue Feb 5, 2022 · 3 comments · Fixed by #1870
Closed
1 task done

Comments

@Kasdy
Copy link

Kasdy commented Feb 5, 2022

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

  • React: 18.0.0-rc0
  • ReactDOM: 18.0.0-rc0
  • Redux: 4.1.2
  • React Redux: 8.0.0-beta2

What is the current behavior?

In React-Redux v8 the custom Subscription manager was removed from useSelector,
and store.subscribe from Redux is used directly in useSyncExternalStore call.

This will bring back a performance problem (#1450), because Redux unsubscription relies on indexOf\splice.

React 18 + React-Redux v8.0.0
Screenshot 2022-02-05 01 36 43

React 17 + React-Redux v7.2.6
Screenshot 2022-02-05 01 24 30

Test code:

const store = createStore(
    (state = 0, action) => (action.type === "INC" ? state + 1 : state)
);
const increment = () => ({ type: "INC" });

function App() {
    const state = useSelector(s => s);
    const dispatch = useDispatch();

    const [children, setChildren] = useState(0);

    const toggleChildren = () => setChildren(c => c ? 0 : 50000);

    return <div>
        <button onClick={toggleChildren}>Toggle Children</button>
        <button onClick={() => dispatch(increment())}>Increment</button>
        {[...Array(children).keys()].map(i => <Child key={i}/>)}
    </div>;

}

function Child() {
    const v = useSelector(s => s);
    return <div className="child">{v}</div>;
}

What is the expected behavior?

The Subscription manager used in v7 has better performance than Redux unsubscription used in v8 with many listeners.
It's still in the bundle, because <Provider> uses it, so it could be used again in useSelector too.

Which browser and OS are affected by this issue?

No response

Did this work in previous versions of React Redux?

  • Yes
@markerikson
Copy link
Contributor

Moving the "unit test" question over here:

About the unit test, that would be really cool, but I can't think of a way to detect it... because it works even though it's slow. measuring the test execution time does not look good to me either. I discovered it by chance, because I was studying the new React18 hooks, and I was digging in React-Redux source.

I suppose given the extreme difference between "quadratic" and "normal" behavior here, it might actually be legit to just copy-paste this example and assert that the elapsed time should be, say, <250ms or something.

@markerikson
Copy link
Contributor

@Kasdy I implemented a fix in #1870 , and included a unit test that tries to capture the perf differences. Can you take a look and also try out the CodeSandbox CI build?

@Kasdy
Copy link
Author

Kasdy commented Feb 5, 2022

@markerikson
I tried the PR with my test code (React v18 + React-Redux v8) and now the unsubscribe is fast again! (~1ms for 50K child)
I tried also React v17 + React-Redux v8 (I had to import react-redux/lib/compat) and it's fast too (same ~1ms)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants