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

fix(ui): perf improvements #5411

Merged
merged 14 commits into from
Jan 5, 2024
Merged

Conversation

psychedelicious
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Description

Investigation of performance issues indicates the main factor is the change of reselect's default memoization function from lruMemoize to weakMapMemoize. This changed in the RTK v2 release (v2 depends on the reselect v5, which includes this change).

lruMemoize provides a less sophisticated memoize strategy, with a configurable, but finite, cache size. We've never changed it from the default cache size of 1. Instead, we wrap parameterized selectors in useMemo and rely on closures to provide the selector parameters. The cache size never grows.

weakMapMemoize provides an effectively unbounded cache size out of the box. When selectors are reused with different parameters, the cache grows.

For our particular use patterns, with many parameterized selectors, this substantially increases memory allocations and causes a frequent major GCs. This is my best estimation of the situation and there may be more nuance of course.

Previously (between v3.4.0post2/v3.5.1 and now), I had configured most of the app to use lruMemoize, but there were two missing spots:

  • createEntityAdapter's getSelectors() defaults to weakMapMemoize, changed to lruMemoize in this PR.
  • RTK Query uses weakMapMemoize internally, but there's no way to change. Pending a change upstream to allow customizing the memoize function, I've directly patched the builds so that lruMemoize is always used.

In my profiling, this has smoothed out the remaining bumps, but it's possible that other changes introduced after v3.4.0post2 were also contributing and we still have a few places to fix up.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Test canvas and workflow editor. Should be smooth. If there are still performance issues I'll need to continue investigating.

Merge Plan

This PR can be merged when approved.

Copy link
Member

@hipsterusername hipsterusername left a comment

Choose a reason for hiding this comment

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

@psychedelicious @maryhipp - only thing blocking atm is lint checks are failing.

@psychedelicious psychedelicious marked this pull request as draft January 5, 2024 12:26
@psychedelicious psychedelicious force-pushed the fix/ui/patch-reselect-weakmapmemoize branch from aa74411 to b1370bd Compare January 5, 2024 12:41
@psychedelicious psychedelicious marked this pull request as ready for review January 5, 2024 12:41
@psychedelicious psychedelicious enabled auto-merge (rebase) January 5, 2024 12:42
@psychedelicious
Copy link
Collaborator Author

I've also reviewed every single redux selector in the app and optimized all but a couple of them.

The useNextPrevImage hook needs to be refactored to avoid using a state => state input selector, and there's a canvas selector I ran out of steam on. Neither are causing issues though (I commented them out and nothing changed), so I will tackle those in the future.

Our redux implementation is better now than it ever has been. Sweet!

Pending resolution of reduxjs/reselect#635, we can patch `reselect` to use `lruMemoize` exclusively.

Pin RTK and react-redux versions too just to be safe.

This reduces the major GC events that were causing lag/stutters in the app, particularly in canvas and workflow editor.
Accidentally removed it last commit.
These were just using overly verbose syntax - like explicitly typing `state: RootState`, which is unnecessary.
Do not memoize unless absolutely necessary. Minor perf improvement
Just a few stragglers left. Good enough for now.
@hipsterusername hipsterusername force-pushed the fix/ui/patch-reselect-weakmapmemoize branch from b1370bd to c5c2d75 Compare January 5, 2024 13:01
@psychedelicious psychedelicious merged commit 4716632 into main Jan 5, 2024
7 checks passed
@psychedelicious psychedelicious deleted the fix/ui/patch-reselect-weakmapmemoize branch January 5, 2024 13:03
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.

None yet

2 participants