1416161 - Using redux-batched-subscribe + unstable_batchedUpdates to reduce Redux’s subscribed notifications

Status: RESOLVED WONTFIX

(DevTools :: Netmonitor, enhancement, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Follow bug 1416096 Option 2. unstable_batchedUpdates can be a nice approach to batch / debounce the real React component updates especially when UI update happens so frequently.

Using redux-batched-subscribe [1] + ReactDOM.unstable_batchedUpdates to reduce Redux’s subscribed notifications.

[1] https://github.com/tappleby/redux-batched-subscribe

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1416161 - Using redux-batched-subscribe + unstable_batchedUpdates to reduce Redux’s subscribed notifications

Review commit: https://reviewboard.mozilla.org/r/198546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198546/

Comment 2

[Ricky Chien [:rickychien] (inactive)]

The DAMP outcome looks great! 

damp simple.netmonitor.requestsFinished.DAMP opt e10s   graph	195.89 ± 5.32%	> 143.60 ± 3.19%	-26.69% (high confidence)


https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=2eeb948da971&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

Comment 3

[Julien Wajsberg [:julienw]]

You can also look at https://github.com/mathieudutour/redux-throttle. I like the approach where the decision to throttle is decided at the dispatch time (with a "throttle" boolean) instead of batching everything.
There are other options at https://redux.js.org/docs/faq/Performance.html#how-can-i-reduce-the-number-of-store-update-events
It would be nice to know why you decided to choose one over another ?
Also, is it necessary to use the unstable batchedUpdates API? Maybe batching actions is already enough?

Comment 4

[Alexandre Poirot [:ochameau]]

This is also quite significant:
  complicated.netmonitor.requestsFinished   9,744.04 ± 0.75%   >   9,078.48 ± 0.70%   -6.83%

I thought that as you dispatch multiple actions synchronously, it only updated Store and React once.
But it looks like it is not.

Ricky, could you confirm what happened before this patch?
 * what are the typical actions we dispatch with their timestamp ?
 * are they throttled or not? (are they executed all at once synchronously, or individually every xx ms? timestamp will help knowing)
 * when some are throttled, do we update store more than once? do we update React more than once?
To debug store, you can do: store.subscribe(()=>console.log("store update"));
And react, you can log in any render or shouldComponentUpdate method.

I imagine we could get rid of action throttling with this patch...?

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8927246 [details]
Bug 1416161 - Using redux-batched-subscribe + unstable_batchedUpdates to reduce Redux’s subscribed notifications

https://reviewboard.mozilla.org/r/198546/#review203694

Thanks Ricky for working on this, looks promising!

A few general comments:
1) Can we avoid using unstable_batchedUpdates?
2) We should proably insert the `redux-batched-subscribe` into client/shared/vendor, so other panels (especially the Console) can benefit from it too.
3) What happens with the performance if we remove the action-debouncing logic? Could we have some damp data?

Honza

Comment 6

[Ricky Chien [:rickychien] (inactive)]

(In reply to Ricky Chien [:rickychien] from comment #2)
> The DAMP outcome looks great! 
> 
> damp simple.netmonitor.requestsFinished.DAMP opt e10s   graph	195.89 ± 5.32%
> > 143.60 ± 3.19%	-26.69% (high confidence)

I probably mess up my patch with this https://hg.mozilla.org/try/rev/4691ca5dde3fc7a07518fe66e9a1faaffe487ece#l1.13

So the damp result is incorrect since I didn't revert the change of REQUESTS_REFRESH_RATE = 150ms. 

After reverting this part, damp result doesn't indicate so much high confidence.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=fe23c386c932&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

But I can still see the perceive perf of network request is faster than before.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

I was probably wrong in last observation :(

I did a research by inserting log in store.subscribe() to see how Redux notification happens:

* Our current batching middleware is really helpful (it throttles and reduces actions). It is able to reduce UI update efficiently.
* It could be just my feeling about the perceived perf improvement with using unstable_batchedUpdates, but the real damp test (comment 6) doesn't show significant improvement.
* unstable_batchedUpdates doesn't expose as a standard API for general case, we should avoid using it.
* Documentation of unstable_batchedUpdates is unclear and brittle, sometimes I believe that React can optimize itself by their internal batchedUpdates.

I'm going to close this issue since I believe this is not the way we want to go. Thanks for all your suggestions and feedback.