Downshift: Performance issues

[Dr-Nikson]

Hello! Currently Downshift's performance isn't the best. In many cases we need to use memoization to avoid unnecessary rerenders. For example -- it https://github.com/downshift-js/downshift/blob/master/src/downshift.js#L763-L770 will create callback every time when menu is open and user is moving mouse over the elements. So, basically in current version everything inside Downshift is being rerendered. It causes a lack of the performanse even if there are a 5 elements in the menu.

And as far as I know -- there's nothing to do at the userland.

• downshift version: 3.2.10
• node version: 10.9.0
• npm (or yarn) version: 6.4.1

[RoslerWS]

In Chrome for me the performance is downright terrible since some of the latest Chrome updates. The latest Chrome updates seems to have amplified the effect of reflows in the browser.

[silviuaavram]

Any ideas on improving it?

[silviuaavram]

Synced with a colleague about it, expect a fixing PR soon.

[Dr-Nikson]

@silviuavram did you thought to move some of the "interactive stuff" to context instead of passing it through props? For example "selectedIndex": we can achieve much higher performance in this area if we switch to context (because rerender whole list of items is expensive even there are two items changed -- previously selected and currently selected). Maybe we can provide our own react-hook for that.

[levithomason]

Haven't thoroughly caught up on this issue yet, however, one strategy for optimizing mouse move listeners is to remove them after the first invocation. This makes it work more like a mouse enter listener since it fires once and removes itself. Pair this with a mouse leave listener to achieve your highlighting.

Currently, the code fires the mouse move listener constantly which causes all mouse move events (thousands) to re-render the menu items. This is one source of perf issues that is easily solved.

[Dr-Nikson]

@levithomason I mean if we are using props to passthrough "highlighted" state to menu items - we'll still rerender all of the menu items and it doesn't really matter are they pure components or not.

But in other hand if we'll achieve memoizations for all stuff that passed to render function we can implement context-thing that I described above in custom menu component (e.g. at the user-land)

[silviuaavram]

Maybe we should just concentrate on writing Downshift with hooks. And then see how that behaves. I am currently working on useSelect(): https://github.com/silviuavram/downshift-hooks

[Dr-Nikson]

@silviuavram Yep, 146% agree. Can I help you with hooks? I have some ideas about performance optimisation.

[alexkirsz]

I wanted to use downshift for my custom autocomplete component, but faced the same performance issue with re-rendering my whole list of items whenever an innocuous state property changed (i.e. selectedItem).

I recently gave react-final-form a try, and found it dealt with this issue in an elegant manner: the whole form state is passed down through context, and individual components in the hierarchy can choose to subscribe to one or more properties of the form state by calling the useFormState hook with a subscriptions: Array<keyof FormState> option.

My dream API for an autocomplete helper would be:

• A <Downshift {...props} children={(s: DownshiftState) => React.ReactNode}> that I can setup at the top of my autocomplete component tree;
• A useDownshift({ ...opts, subscriptions: Array<keyof DownshiftState> }) => DownshiftState hook that I can use to subscribe to the downshift state and to updates whenever one of the keys I subscribe to changes.

[silviuaavram]

@Dr-Nikson you can check the performance of useSelect in one of the codesandboxes. Links in the useSelect readme / downshift docsite.

@alexkirsz isn't context an overkill here? It makes sense for a form, but for one autocomplete widget?

I have some issues with the controlled part of useSelect right now but I would also like to see it how it's performance-wise.

[alexkirsz]

@silviuavram I imagine it depends on the complexity of the widget. Most users might not see a benefit to doing this, so perhaps it is not worth the burden, but it should be possible to set this up in a completely retro-compatible way for power users. For now it's just a thought, it might not even make sense at all when actually implementing and testing this.

[silviuaavram]

Unfortunately the hooks may suffer from the same issue, the fact that the callbacks are returned as new functions after each re-render. Do you think it's worth to add a small guide on this problem in the Readme? On how to use a memoization library to return the same callbacks as long as the item and its position in the list (index) is the same? @Dr-Nikson

[Dr-Nikson]

@silviuavram oh... I see. From my point of view -- there not much to do at user-land with these problem. Actually I have a concept in my mind how to make everything fast and reliable, but I have a lot of business at my job... so, I really would like to dive into this issue, but a little bit later

[josephkeiser]

Unfortunately the hooks may suffer from the same issue, the fact that the callbacks are returned as new functions after each re-render.

The callback behavior (new function each render) definitely seems to be impacting all items rendering. @silviuavram I have tried a few things to attempt to memoize the functions, but it's so awkward and definitely not ideal. Have you tried anything and been successful, if nothing else as a stopgap?

@Dr-Nikson I am definitely interested in what you come up with as a solution.

[kilrain]

I'm seeing a similar issue: all the callbacks returned from getItemProps are new instances... forcing a re-render on every item. Can this ticket be re-opened? No fix was submitted, nor was documentation updated...

Also, if I were to workaround this in user-land, is it safe to assume we can re-use old callbacks as long as item and position haven't changed as @silviuaavram suggests above??

[silviuaavram]

Let's see if we can up with an official solution. I would suggest to memoize the results of getItemProps if: items are the same, item is the same, index is the same. From the top of my head this makes sense.

If you use the hooks, useMemo otherwise maybe fast-memoize.

I don't have time to work at this at the moment, but if you come up with a working solution we should definitely post it in the documentation. I would like to keep this part out of the production code, it's already big enough and not everybody needs memoization.

Does it make sense? :)

[kilrain]

Thanks for the quick response... I'll keep a documentation PR in the back of my mind while optimizing for my use case.

[jorgemoya]

Having issues with performance as well, hoping we can find a fix directly in the library.

[kilrain]

Do you think it's worth to add a small guide on this problem in the Readme? On how to use a memoization library to return the same callbacks as long as the item and its position in the list (index) is the same?

I've tried memoizing the return value of getInputProps and I get funky behavior around the item's onClick handler (it is not invoked). So I don't think that's a viable option... unless someone can come up with a working snippet.

[silviuaavram]

I don't want to add this in the library. Reasons: bundle size, complexity of API (another props for generating cache key), not tested enough, not needed by everyone. Plus that it is more advised to fix your render performance before fixing re-renders.

https://codesandbox.io/s/usecombobox-with-memoization-ryzy9?file=/src/index.js

I am using here fast memoize to cache results for getItemProps, as long as an item stays on its position. Will invalidate the whole operation if the inputItems change. Take a look and tell me how it works for you. Thanks!

[silviuaavram]

As a side note, I am still thinking if we can turn onMouseMove into onMouseEnter and somehow reset it when user changes highlightedIndex by arrow key. Otherwise when moving mouse again on the same item, the highlightedIndex will not change to it, since it already had onMouseMove triggered and there was no onMouseLeave fired in the meantime.

Anyone has any idea on how to improve this?

[silviuaavram]

Closing this as a result of https://github.com/downshift-js/downshift/pull/1051. Hooks will be memoized starting with 5.4.0. Downshift stays the same, we are not going to change it. So if you want fast performance, you should migrate to the hooks. Thank you!

[kentcdodds]

Just FYI, you actually can memoize things with the regular Downshift component. Here's an example of how you could do that:

https://github.com/kentcdodds/react-performance/blob/6e91f10f453e66b537e4f6643dbf66ff55dbe23a/src/final/03.js

And here's an example using a custom comparator: https://github.com/kentcdodds/react-performance/blob/6e91f10f453e66b537e4f6643dbf66ff55dbe23a/src/final/03.extra-1.js

And I even show you how to memoize the Downshift component itself: https://github.com/kentcdodds/react-performance/blob/6e91f10f453e66b537e4f6643dbf66ff55dbe23a/src/final/03.extra-2.js

But I updated to use the useCombobox hook instead now that things are memoized with useCallback.

Here's how to memoize things: https://github.com/kentcdodds/react-performance/blob/fc7e0f334efe2e364b82439011921f25a559bac6/src/final/03.js

And how to use the custom comparator: https://github.com/kentcdodds/react-performance/blob/fc7e0f334efe2e364b82439011921f25a559bac6/src/final/03.extra-1.js

And how to avoid needing the custom comparator by providing primitive values instead: https://github.com/kentcdodds/react-performance/blob/fc7e0f334efe2e364b82439011921f25a559bac6/src/final/03.extra-2.js

I hope that helps!