neurobagel / query-tool

User interface for searching across Neurobagel graph
https://query.neurobagel.org/
MIT License
2 stars 1 forks source link

Investigate performance issues #17

Closed rmanaem closed 8 months ago

rmanaem commented 9 months ago

Currently, once an empty query is sent and the result is displayed the application slows down significantly to the point that it affects user experience. We need to investigate and resolve this issue before deployment.

surchs commented 8 months ago

I took a look at this and I think one issue (likely the main issue) is that we keep state for the selected nodes in two places:

https://github.com/neurobagel/react-query-tool/blob/78d37bca7542d5fbd790e48ee2aaa5c25a42ba3a/src/components/QueryForm.tsx#L46-L47

That's no good by itself and should be changed. But I also creates a render loop, i.e. the app rerenders multiple times per second without any input. I haven't fully figured out where, but confirmed that if you disable the setSearchParams lines, the render loop stops. This also removes most performance issues (scrolling is fine again). So my strong sense is that we're just making the poor app sweat too much and when it has to rerender our massive 10MB JSON files several times per seconds, it starts getting slow.

So we should

Quick screenshot of the different useEffects - the node getter/setter effect is what's going crazy: image

rmanaem commented 8 months ago

Addressing the issue above in #25 hasn't solved the lag in performance. After trying a couple of fixes including giving the responsibility of controlling the checkbox state to ResultCard, I realized the issue seems to be stemming from "rendering large lists". This seems to be an issue in the broader react dev community and there are a couple of existing solutions including react-window, react-virtualized, and react-virtuoso.

surchs commented 8 months ago

Breaking the prop passing / state dependency tree in the dumbest possible way, i.e. here: https://github.com/neurobagel/react-query-tool/pull/37 also fixes the rerender issue for the cards in the ResultContainer section.

See: Image

So that's the issue we need to fix / address

surchs commented 8 months ago

I took another look at this, and I think a good starting point is the useCallback hook together with memo: https://react.dev/reference/react/useCallback#skipping-re-rendering-of-components

Child-components rerender under (at least) these conditions:

event handler functions by default get recreated on every rerender of the parent. And they are passed as a prop to the child. Therefore, unless you cache the event handler function / prevent it from being recreated once the parent rerenders, your child will rerender even if you use memo(ize). Take a look at the section in react.dev - I belive that should get us most of the way

surchs commented 7 months ago

:rocket: Issue was released in v0.1.0 :rocket: