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

Replace listeners array with a Map for better performance #4476

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

markerikson
Copy link
Contributor

@markerikson markerikson commented Jan 30, 2023

Redux has always used an array of listener callbacks internally. However, as we found out with React-Redux, using listeners.findIndex(callback) is bad for performance ( reduxjs/react-redux#1523 , reduxjs/react-redux#1869 ).

React-Redux uses a custom linked list internally. That's way more than we need here. But, now that we're assuming modern browsers, it's safe to assume ES2015+ support, and that Map/Sets are available.

I initially tried Set<ListenerCallback>, but found that one test failed... because it was passing in the same jest.fn() instance twice. A Set compares by reference, so the callback got removed in the first unsubscribe() call and was no longer around for the second.

We don't have a formal statement that we support passing in the same callback more than once, but that's the implicit semantics thus far.

Given that, a Map<number, ListenerCallback> works equivalently, and we'll just have an incrementing counter for the key.

Also, I double-checked perf for cloning Sets before I switched over to Maps. Somewhat surprisingly, existing.forEach(item => newSet.add(item)) runs faster than new Set(existing), or using for..of, so I assume the same is true for Maps. (I will note that I was creating Sets with 1M items, and the difference was like 150ms to 120ms. So I don't expect that to have any meaningful cost here.)

In practice, I don't expect this to make a lot of difference, because React-Redux's <Provider> normally subscribes directly to the store, and all child components subscribe to that <Provider>'s internal Subscription instance.

But getting rid of this little perf footgun is nice.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1599aad:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@markerikson markerikson merged commit bc359c9 into master Jan 30, 2023
@markerikson markerikson deleted the feature/listeners-map branch January 30, 2023 01:47
Comment on lines +137 to +140
nextListeners = new Map()
currentListeners.forEach((listener, key) => {
nextListeners.set(key, listener)
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do this?

Suggested change
nextListeners = new Map()
currentListeners.forEach((listener, key) => {
nextListeners.set(key, listener)
})
nextListeners = new Map(currentListeners)

Copy link
Contributor Author

@markerikson markerikson Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in my testing, at least with a Set instead of a Map, .forEach was actually somehow faster 🤷‍♂️ (as mentioned in the original PR description)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, sounds like a bug in the js engine.

@romellem
Copy link

I've been looking at a lot of the react-redux and redux code, and was curious about this implementation split between a Map and the doubly-linked list used in reduxjs/react-redux#1523.

You mention

[a custom linked list is] way more than we need here

Do you just mean that the linked list is probably the fastest, while using a Map is easier since you can rely on native data structures vs rolling your own?

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 this pull request may close these issues.

3 participants