Rerender only when data for given reduxes has changed #2564

rawagner · 2019-09-03T10:11:29Z

ConnectToState updates children everytime k8s map is changed, which means that even if watched resource isn't changed, but something else in k8s map changed, we unnecessarily rerender child component. For example, we have two components - A and B. A is watching all pods, B is watching all nodes. Pods are updated, k8s map is changed and A and B are rerendered. but B doesnt have to be.

To optimize rerenders we can check only parts of k8s map and rerender only when the parts we are interested in have changed.

Or maybe I misread something :)

@spadgett

spadgett · 2019-09-03T14:19:26Z

/cc @christianvogt

christianvogt · 2019-09-04T18:47:34Z

frontend/public/components/utils/firehose.jsx

+});
+
+const propsAreEqual = (prevProps, nextProps) => {
+  if (nextProps.children === prevProps.children && nextProps.reduxes === prevProps.reduxes) {


There are several other props missing from this check: filters, loaded, loadError, resources

how so ? ConnectToState has only 3 props - children, reduxes (both from Firehose) and k8s (from mapStateToProps). Am I wrong ?

Sorry you're right. Not sure how I came to that conclusion before.

christianvogt · 2019-09-05T13:15:56Z

@rawagner i haven't tried locally but it would seem that if k8s changes then Firehose would re-render and create a new instance of children, due to inject in its render function, which would be passed down to ConnectToState and still bypass your propsAreEqual check.
What scenario did you try in the app to reproduce this? Otherwise will need to contrive something to test. Would be great if we had some unit tests here :)

spadgett · 2019-09-05T13:39:00Z

I'd suggest we push this to 4.3 to give it more soak time.

rawagner · 2019-09-05T14:20:06Z

@rawagner i haven't tried locally but it would seem that if k8s changes then Firehose would re-render

not necessarily, Firehose takes k8sModels, loaded and inFlight from stateToProps but there is areStatePropsEqual function which actually compares these props to know if it should rerender. So even if k8s changes (ie some k8s resource changed) but models and k8s.getIn(['RESOURCES', 'inFlight']) is the same Firehose wont rerender, but ConnectToState would rerender. With this change ConnectToState additionally checks if a resource we are interested in changed.

I've tried with and without this change on Dashboards Page where a lot of resources are fetched via various cards (all pods, all events etc..) and it reduced the amount of rerenders

christianvogt · 2019-09-06T16:19:47Z

Tried out the changes. While sitting on the dashboard page it almost looks like the same cards are being updated but visually nothing has changed as before applying the fix.
I put conditional breakpoints in the new code and indeed it looks to be working and preventing some re-renders. There are also, as mentioned above, the checks for children are failing and component re-renders are triggered although visually there are no changes.

Seems to me this is an improvement but still more work could be done.

Do you mind posting a specific scenario where you compare the before and after?

rawagner · 2019-09-06T17:39:01Z

@christianvogt thanks for trying it out!

The easiest scenario would probably be

checkout PR Update Inventory card design #2601 which changes the behavior of Inventory card. Currently Inventory Card fetches all resources (all pods, all nodes, all pvcs) and passes them to specific inventory items. Which means that if any resource changes, all items are updated. The PR changes the behavior in a way that not card, but item fetches the need resource ie Pod inventory item fetches all pods, Node item fetches all nodes and so on. I was expecting that this would optimize the number of rerenders - pods changes, only pod item is rerendered, however that is not the case as we already know due to behavior of ConnectToState
apply this PR on top of 1. and you will see that now it behaves as expected - if pods change only pod item is rerendered and others are not affected. Its easier to try this on some resource which does not change often, for example try creating and deleting PVC at see that only PVC item is updated. Without this change, all of them would be.

rawagner · 2019-09-06T17:42:50Z

Tried out the changes. While sitting on the dashboard page it almost looks like the same cards are being updated but visually nothing has changed as before applying the fix.
I put conditional breakpoints in the new code and indeed it looks to be working and preventing some re-renders. There are also, as mentioned above, the checks for children are failing and component re-renders are triggered although visually there are no changes.

Im not sure how to optimize children, but reduxes can be easily done

christianvogt · 2019-09-06T22:35:48Z

Tried out the fix with the above mentioned PR and it was an improvement. I didn't see any side effects.

christianvogt · 2019-09-06T22:35:55Z

/approve

christianvogt · 2019-09-06T22:36:15Z

@spadgett what would you like to do?

spadgett

Given that this is a small change and easy to rollback if needed, let’s go ahead. We have a little bit of soak time.

/lgtm

openshift-ci-robot · 2019-09-07T00:09:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, rawagner, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2019-09-07T00:09:24Z

/retest

openshift-bot · 2019-09-07T01:38:08Z

/retest