avoid useless calls to listeners

[splendido]

Avoid useless calls to listeners when they've already been notified by the deepest dispatch call.

Dispatching an action from a listener while responding to a state change starts a recursive call to dispatch. This pauses the loop currently used to notify all listeners and starts a new one at a deeper nested level.

When resuming back to higher levels, all the listeners have already been notified and the paused loop doesn't actually need to be completed.

As an example, consider the following code:

var Redux = require('redux');

var numIncrementers = 3;
var numListeners = 5;

function reducer(state, action) {
  if (typeof state === 'undefined') {
    return 0
  }
  switch (action.type) {
    case 'INCREMENT':
      return state + 1
    default:
      return state
  }
}

var store = Redux.createStore(reducer)


function createListener(store, name, value) {
  var lastState = null;
  return function () {
    var state = store.getState();
    if (state === lastState) {
      console.log(name, 'state changed FALSE');
      return;
    }
    lastState = state;

    if (!!value && state === value) {
      console.log(name, 'state changed TRUE INCREMENT!');
      store.dispatch({ type: 'INCREMENT' });
    } else {
      console.log(name, 'state changed TRUE');
    }
  }
}

var i = 1;
for( ; i <= numIncrementers; i++)
  store.subscribe(createListener(store, 'listener_' + i, i));
for( ; i <= numListeners; i++)
  store.subscribe(createListener(store, 'listener_' + i))


// Trigger first action
store.dispatch({ type: 'INCREMENT' });

Here a number of listeners subscribe to the store and the first three of them trigger an increment action only in case the current value for the state is equal to their id.

The following is the log I get with the current code:

0 listener_1 state changed TRUE INCREMENT!
1 listener_1 state changed TRUE
1 listener_2 state changed TRUE INCREMENT!
2 listener_1 state changed TRUE
2 listener_2 state changed TRUE
2 listener_3 state changed TRUE INCREMENT!
3 listener_1 state changed TRUE
3 listener_2 state changed TRUE
3 listener_3 state changed TRUE
3 listener_4 state changed TRUE
3 listener_5 state changed TRUE
2 listener_4 state changed FALSE
2 listener_5 state changed FALSE
1 listener_3 state changed FALSE
1 listener_4 state changed FALSE
1 listener_5 state changed FALSE
0 listener_2 state changed FALSE
0 listener_3 state changed FALSE
0 listener_4 state changed FALSE
0 listener_5 state changed FALSE

(I've added by hand the recursion level on the leftmost column)

With the proposed modification, the output would be only the following one:

0 listener_1 state changed TRUE INCREMENT!
1 listener_1 state changed TRUE
1 listener_2 state changed TRUE INCREMENT!
2 listener_1 state changed TRUE
2 listener_2 state changed TRUE
2 listener_3 state changed TRUE INCREMENT!
3 listener_1 state changed TRUE
3 listener_2 state changed TRUE
3 listener_3 state changed TRUE
3 listener_4 state changed TRUE
3 listener_5 state changed TRUE

The idea is to prune the exploration of the recursive dispatch calls and stop notifying once the deepest notification loop completes. After this point in time each listener was already notified and it does not need to check the status on the same chain of actions.

Please let me know what do you think about this schema and do not hesitate to start a discussion on it.

[gaearon]

Are there any edge cases this is breaking? In particular I’m concerned that we shouldn’t break this invariant:

The listener should not expect to see all state changes, as the state might have been updated multiple times during a nested dispatch() before the listener is called. It is, however, guaranteed that all subscribers registered before the dispatch() started will be called with the latest state by the time it exits.

This should be true for nested dispatches too.

I always find it hard to wrap my mind around this piece of code so maybe if you could add more test cases verifying the behavior we have in the docs, we can take this.

[splendido]

They only edge case I can think of, is when a subscriberer unsubscribes another subscriber.
Lets make an example.

A, B, and C subscribe to the store in this very same order.

1. An action is dispatched, the new state is generated by the reducer, and the notification loop starts.
2. A is called and it somehow unsubscribes C.
3. B is called and it dispatches an action: the recursion starts! The new state is generated by the reducer, and the notification loop starts with the new snapshot of listeners (which no more includes C!, correct...).
   • 3-a. A is called (...nothing to do)
   • 3-b. B is called (...nothing to do)
   • 3-c. the notification loop ends and needToNotify is set to false: the inner recursion ends.
4. C is called (this is not happening! It's skipped because the loop is abandoned, not because A unregistered C!)
5. the first notification loop ends because needToNotify is false
6. the management of the action originally dispatched at point 1 completes

My personal opinion is that this might be more of a feature than of an issue.
I think that if A needs to unsubscribe C, it is because C should not respond the the action which made A unsubscribing C ...but this might change from case to case. ;-)

What I'm seeing in some tests I'm doing is that with this modification I can easily save up to 10% of the processing time
(FYI, I'm using Redux on the server-side to respond to high-frequency incoming data though, no UIs nor client code involved... I appreciate it could be a very specific use case...)

Certainly the proposed changes break what stated in the quote you reported!
And since it's a breaking change, in case, it should be shipped with the next major release.

What do you think about the above example?

[splendido]

On the other hand, everything works correctly in case a subscriber unsubscribe itself...

[splendido]

...btw, adding a final call to a listener just before it gets unsubscribed (somewhere around here) would be enough to keep the current behaviour unchanged and still granting that

all subscribers registered before the dispatch() started will be called with the latest state by the time it exits.

[gaearon]

Sure, want to give it a try?

[splendido]

Ok, this is how it would look like.
I managed to get all green checks for the tests (it seems tests are failing for other reasons at the moment...) although they're now a little less understandable in my opinion.

I think the documentation should also be updated to let people know about the one last call to listeners within unsubscribe (which could otherwise easily lead to infinite loops...).

[splendido]

There's probably another way to solve the same problem differently: remove unsubscribed listener only at the end of the first notification cycle that completes, somewhere around here, but this requires to modify a little the logic about removing listeners...

I'll get back to this soon!

[timdorr]

Wait, why are you calling the listener on unsubscribe? That's unexpected behavior.

[splendido]

@timdorr what do you mean with unexpected behaviour?
This is not different from what is already happening: the listener is probably being called with a state which was already notified to him and, as you pointed out this is not a problem at all!
But with the other changes in place, this final call is needed to ensure that that listener is always notified with all state changes happened before its unsubscription.

It's not unexpexted behaviour at all ;-)

Edit: the only edge case could be when a listener is registered and soon after unregistered before any state change could happen. In this case I agree with you: the listener is being called even if the state did not change.

[timdorr]

This listener call is entirely unrelated to any state change. It can only be called outside of any dispatch (this is now enforced via #1569). It is impossible to have a state change where the listener is not called.

This is basically creating exactly the thing this PR looks to avoid: an unnecessary listener call.

[timdorr]

I'm 👎 on this. The listener can choose to ignore notifications if it wants (most likely by checking the ref. equality of the state). I'd rather have a guarantee of giving me all notifications, than have some sort of different behavior because of listeners added outside of my control.

[splendido]

I know the proposal is not perfect and I'm trying to improve it.

I think the real point here is that I'm trying to lower the computational complexity of this piece of code, not just saving a few function calls.

When the number of listener is high and many of them are trying to change the state in response to similar changes, things could get really bad...

[splendido]

As promised with this previous comment I've made a new proposal: please see #1867.

I'm happy to discuss about it.

[timdorr]

OK, let's keep this in one PR. #1867 it is.