Performance: createListenerCollection

[leshkovichpvl]

On a large application, there are problems with the performance of createListenerCollection.

#1450 I tried to solve this problem using a map, but this led to the wrong order of selector calls.

Any ideas on how to improve performance?

[markerikson]

Can you provide more details on what the specific perf issue is with the existing implementation?

A reproduceable CodeSandbox example would help.

[ckedar]

@leshkovichpvl, try this implementation and see if it helps:

const noop = () => {}

function createListenerCollection() {
  const batch = getBatch()
  let listeners = []
  let toBeRemoved = null

  return {
    clear() {
      listeners = CLEARED
    },

    notify() {
      toBeRemoved = [] // indication to unsubscribe() that removal from listeners should be deferred
      const count = listeners.length // capture the count now, so that any listeners added while we are notifiying are not processed in this round
      batch(() => {
        for (let i = 0; i < count; i++) {
          listeners[i]()
        }
      })

      // do the deferred removal
      for(let i = toBeRemoved.length-1; i >= 0; i--) {
        listeners.splice(toBeRemoved[i], 1)
      }
      toBeRemoved = null // unsubscribe() is now free to remove directly from listeners
    },

    get() {
      return listeners
    },

    subscribe(listener) {
      let isSubscribed = true
      listeners.push(listener)

      return function unsubscribe() {
        if (!isSubscribed || listeners === CLEARED) return
        isSubscribed = false

        const index = listeners.indexOf(listener);
        if(toBeRemoved) { 
          // notify() is working, deffer the removal
          listeners[index] = noop
          toBeRemoved.push(index)
        } else {
          listeners.splice(index, 1)
        }
      }
    }
  }
}

[leshkovichpvl]

@ckedar, Unfortunately, this didn't help improve performance.

[timdorr]

Without a real world example of this issue, we can't really do anything here. It looks like you're connecting a lot of components, so I might recommend refactoring to reduce your connected component count. That's going to be bad for performance in other ways that can't be fundamentally fixed by the library.

[markerikson]

Except that our general recommendation for best perf is to connect as many components as possible.

[timdorr]

No, it's to only connect whenever you have a slice of state that you need for a component. But not so much that you're selecting out the individual properties of an object out of an array of objects. That will hurt your performance, because it's running subscriptions and state updates when you should just be passing props. React will optimize those props changes much better than we can.

[markerikson]

That's... not what all the benchmarks have shown.

In most scenarios, the cost of running a large number of memoized selectors and mapStates is still less than the cost of a single React render.

If you've got fewer connected components selecting larger chunks of data, they're going to re-render more often. If a given component really only needs one field out of a larger slice, then that's all it should subscribe to.

More commonly, the typical pattern is to have a connected parent just read a list of item IDs from the store and render <ListItem id={id} />, and have the child read its own entry in mapState based on that ID.

But seriously, that's why v7 is so much faster than v6 - keeping all the computations outside of React until we know that a component has to render.