ComboBox: Option Performance Optimization

[kysedate]

Pull request checklist

• Addresses an existing issue: Fixes #0000
• Include a change request file using $ npm run change

When investigating performance issues with our ComboBox, I ran into some React behaviour that I was not expecting. Even after breaking out the ComboBox Options into a wrapping component (which uses shouldComponentUpdate to avoid unnecessary updates), render time increased steadily as the number of options increased.

Take the following example:

<ParentComponent>
     <ChildComponent styles={getStyles()}>
          {renderGrandchildComponent()} 
     </ChildComponent>
</ParentComponent>

Even if the ParentComponent doesn't update (shouldComponentUpdate returns false), getStyles() and renderGrandchildComponent() are still executed. This leads to the component computing a bunch of information (calculating styles, building JSX.Elements, etc. ) that is never actually used. Since the component isn't updated in the DOM, this information doesn't need to be computed.

This leads to a performance hit with ComboBoxes that have many options and/or complex onRenderOption functions. To get around this, we can break out the component into it's own private render function, which will be called by the wrapper IF the component is going to be updated. By passing a reference to this function to the wrapper, we can ensure that this logic is never computed until we actually need it.

Focus areas to test

I have tested this ComboBox in the test app, and with complex scenarios (many options, custom styling, etc. ) and found that there is large performance increase with this change without any functionality regressions. The existing functionality of the ComboBox does not change. This PR does not change the times at which ComboBoxOptions are rendered, it only delays the calls of their child components until it is time to render.

[jspurlin]

@dzearing, @joschect, or @cliffkoh any ideas why a components children are being calculated/built when the component returns false from shouldComponentUpdate? Making the children be passed in as the result of a function doesn't hit this issue, shouldComponentUpdate returns false render is not running and hence the function that returns the children is not called.

[jspurlin]

Changes look good to me, but I want to see if anyone else has info on why this is happening

[christiango]

@kysedate are you sure that the component should update is returning false here? Children is a prop passed to ParentComponent. In this case, it needs to call those functions in order to compute the props for ParentComponent

[kysedate]

Christian is right, one of the parameters to shouldComponentUpdate is newProps. This means that the new props have to be computed before they can be passed in to shouldComponentUpdate. Since the child component (and it's child component) was a prop, it had to build. Nonetheless, this change still prevents this from happening, since it is entierly unnecessary.