Replace listeners array with a Map for better performance

[markerikson]

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]

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

[nstepien]

Why not do this?

Suggested change

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

[markerikson]

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)

[nstepien]

Huh, sounds like a bug in the js engine.

[romellem]

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?