Optimize createListenerCollection

[wurstbonbon]

Follow-up to #1454. Using a linked list to store listeners for fast removal. For comparison unmounting 50k connected children under a single parent.

Before:

Afterwards:

[wurstbonbon]

I removed current/next based on this comment #1450 (comment)

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 47b5fb9

https://deploy-preview-1523--react-redux-docs.netlify.com

[markerikson]

I would like to see the actual benchmarks you're using to compare these results first, and get a better understanding of the behavior characteristics may be different between the current implementation and this PR.

[wurstbonbon]

This is the benchmark code:

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

const App = connect(null, { increment })(
    class extends React.Component {
        state = { numChildren: 0 };

        toggleChildren = () => {
            this.setState(state => ({ numChildren: state.numChildren ? 0 : 50000 }));
        };

        render() {
            return (
                <div>
                    <button onClick={this.toggleChildren}>Toggle Children</button>
                    {[...Array(this.state.numChildren).keys()].map(i => (
                        <Child key={i} />
                    ))}
                </div>
            );
        }
    }
);

const Child = connect(state => ({ state }))(
    class extends React.Component {
        render() {
            return <div className="child">{this.props.state}</div>;
        }
    }
);

ReactDOM.render(
    <Provider store={store}>
        <App />
    </Provider>,
    document.getElementById("root")
);

What I'm profiling is the unmounting. This is obviously specifically designed to expose the quadratic complexity of the unsubscribe function.

[markerikson]

Can you clarify what specifically about the current implementation is "quadratic"?

[markerikson]

Okay, yeah, that definitely looks like an issue.

[wurstbonbon]

This statement next.splice(next.indexOf(listener), 1) for every listener that is remove we need look at all other listeners. And the splice should also scale with size of the array.

[wurstbonbon]

[timdorr]

LGTM. It may be a tiny bit less memory efficient (there is a closure around listener that keeps the ref alive), but I think that's an acceptable tradeoff when this gets noticeable on something like a route change.

[markerikson]

Yeah, I vote we put this in, merge #1506 , and put out 7.2.0.

[markerikson]

I'll publish shortly.

[markerikson]

And https://github.com/reduxjs/react-redux/releases/tag/v7.2.0 is live.