component mergeProps are unnecessarily running

[petemill]

Description

Our architecture of a redux-style state and state -> props component connecting is interesting, and helpful in order to read from the state from any level of the component tree. However, it can be wasteful in that each component's mergeProps function is being called every single time any property anywhere in the state changes. Mostly these functions are not doing anything computationally expensive and just reading from the state, so it seems fine (although they occasionally are from what I've seen). However, these very frequent Immutable object searches can add up to cause some jank, and a lot of them are doing exactly the same lookups, repeating them multiple times every time any state changes. More interestingly, we don't seem to be taking advantage of the main feature of the ImmutableJS objects which allows for cheap equality comparison at all levels to know if any descendent properties have been modified anywhere underneath that level, rather than more expensive deep comparisons that we are doing at the moment.

Other react / redux projects I've worked on, especially ones even approaching this size, have benefitted from having their mergeProps-like functions only run when a specific portion of the state tree that the specific UI component depends on is changed.

I did some quick measurement to try to see how much time was being 'wasted'. I isolated invocations of mergeProps which do not result in any change props, measured how long that took, and then reported how much time over a 1 second period that the js thread has spent in those functions. This is the report with about 20 tabs open and just moving the mouse over various UI components:

I don't know how much of this time we can recover - perhaps a lot of it - but it's perhaps worth investigating.

Potential Solutions

Memoize

Using a library like reselect, each component instance can create memoized functions that explicitly define their dependent values / trees in the state. Their mergeProps function will only run if those are not === their previous ImmutableJS Map / List value. The reselect + immutablejs pattern is common

Split containers / components (or smart vs dump components)

Have some high level containers connect to redux store and have a mergeProps, then they pass props directly to pure child components which are not connected to the redux store (and have no mergeProps). This would reduce the occurrence of the problem.

Of course, a mixture of both could be best if we think this is an issue. Probably put some kind of structure in place to help define what should be a container and what a component.

Related immutablejs / redux / selector topics:

• http://redux.js.org/docs/recipes/ComputingDerivedData.html
• http://redux.js.org/docs/recipes/UsingImmutableJS.html
• https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0
• http://www.thegreatcodeadventure.com/the-react-plus-redux-container-pattern/

Would appreciate any feedback from @bsclifton @NejcZdovc or @cezaraugusto as to your opinion of the issue and the suggestions... 😄

[petemill]

Code used to measure this:

--- a/app/renderer/components/reduxComponent.js
+++ b/app/renderer/components/reduxComponent.js
@@ -39,6 +39,20 @@ const didPropsChange = function (oldProps, newProps) {
   return false
 }

+let mergePropsWasteCount = 0
+let totalMergePropsWaste = 0
+function logMergePropsWaste() {
+  const timeWasted = mergePropsWasteCount
+
+  if (timeWasted) {
+    mergePropsWasteCount = 0
+    totalMergePropsWaste+=timeWasted
+    console.log(`wasted ${timeWasted}ms in the last 1 second performing mergeProps that did not change, now wasted a total of ${totalMergePropsWaste}ms`)
+
+  }
+}
+setInterval(logMergePropsWaste, 1000)
+
 class ReduxComponent extends ImmutableComponent {
   constructor (componentType, props) {
     super(props)
@@ -50,9 +64,14 @@ class ReduxComponent extends ImmutableComponent {

   checkForUpdates () {
     if (!this.dontCheck) {
+      const t0 = performance.now()
       const newState = this.buildProps(this.props)
+      const t1 = performance.now()
       if (didPropsChange(this.state, newState)) {
         this.setState(newState)
+      } else {
+        const timeTaken = t1 - t0
+        mergePropsWasteCount += timeTaken
       }
     }
   }

[bridiver]

cc @NejcZdovc who has been working on this and I believe it's likely fixed or mostly fixed by now

[petemill]

I'm currently doing a task that requires creating a stateUtil (used by different mergeProps functions around the components), and realising that the current architecture of putting functions in separate modules (frameStateUtil, etc) reading from state and performing functions e.g. filtering, lends itself well to these functions being converted to selectors, being memoized based on equality of anywhere within the immutable state tree. The reselect library (https://github.com/reactjs/reselect) has a nice way of doing this and creating interdependencies, so these functions are only run once per state update for all components, as well as only firing if their specific state properties change.

[petemill]

@bridiver @NejcZdovc here's an example of changing a frameStateUtil state reading function to be memoized with a cache size of 1: 9e5e457#diff-f5a0b45b8323a469e78cf98fb0c49b8fR52

See the function after for something dependent on multiple parts of state. If we implement state reading using this pattern, we won't have components reading and processing chunks of state that haven't changed, after every single redux action.

[alexwykoff]

closing in favor of other priorities