Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: update to zustand 4 to prevent re-renders #4885

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

darrelhong
Copy link
Contributor

Problem

#4766 #4764 highlights some performance issues that form builder has. Both videos show that lag that occurs after changing input. (#4766 unable to scroll to bottom fully is probably a separate issue fixed in #4860). This should be caused by a long JS task blocking interaction. I also experience this lag in local development, and this issue might exist across devices as it is relatively easy to increase the lag by adding more form fields.

Findings

Did a bunch of random digging to see whats the problem and I try to summarise them here. Note: the numbers here are in local environment, so I'm not sure how it translates in production

  • Using the chrome profiler, we can see what tasks are running and for how long. The method was to simply change one input and see what was run. Was hoping to find some rogue functions that might be causing the issue but it was mostly just react doing its thing, so it's likely due to re-renders happening
  • Using the react profiler, we can see pretty much the entire page is re-rendering upon one input change which is not a good sign
  • The profiler recordings unfortunately was not much help either as there every profiling session was very different even though the action was the same. Random components everywhere would take unusually long to render according to the flamegraph and that component would be different between each run. I'm not sure if this is an issue with the profiler or not, but do let me know if you guys try it and see if you get the same results.
  • As such the lag seems to be not be caused by a specific component but instead is a result if just having a lot of components re-render
  • Lag is not just contributed by the form fields but also the surrounding parent elements as well. A form with 8 fields, including complicated ones like Table causes a task duration of 800ms, and commenting out the return values in BuilderFields still causes the task to be 80ms. Returning an empty div in FieldRowContainer ups the duration to ~150ms meaning the hooks themselves take about 70ms? Might need to go deeper here to see if a specific component is causing problems.

Solution

One thing I noticed from the profiler sessions is that the zustand library uses an increasing counter to force updates when state has changed. That's why there's a number of components that seemingly don't need to update but still do as they call the zustand hook. It turns out that the library has already been update to v4 which uses a different way of state management, so I simply updated to see if it helps.

I'm actually surprised to see how much improvement there is after the update. The JS task duration drop by about 30-40% from ~800ms to ~500ms and there is definitely less re-renders in React.

Therefore this, this PR is just a simple low-hanging fruit optimisation, but one that reaps noticeable benefit. If its worth it, there is definitely more room for improvement. Based on the findings above, I not sure if there is a simple solution to optimise performance. I'm still not too familiar with the overall state management in the app, but I feel some rejigging of the the state management should help. Maybe you guys can give your opinion on this.

@karrui karrui mentioned this pull request Sep 19, 2022
3 tasks
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for the detailed PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants