Proposal: Use cached data directly, skip loading state when possible

Question

Proposal: Use cached data directly, skip loading state when possible

hannojg opened this issue 2 years ago · 1 comments

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

What performance issue do we need to solve?

[Slack discussion]

By default withOnyx works like this:
Wrap a component, and set a loading state, while Onyx gets the requested data from the storage (this is async)
When we have the data from the storage we update the state with the real data and set loading to false.
In general this is fine, however, this causes an extra render. And we want to avoid extra re-renders as much as possible to have a good performance.
We have the case that we have data already in the memory, which could be read synchronously at the first render of a onyx connected component. However, we always run though the async connect which causes the extra re-render, switching from loading state to the state with the final data.

What is the impact of this on end-users?

We often start with our components in a loading state, although we could show the content right away (improved UX)
Opening screens often feels jumpy / not-smooth as we have a lot of re-renders

This is for example the current state of opening a chat screen in the new dot app:

main_recording.mov

List any benchmarks that show the severity of the issue

This is a react-devtoosl profiler recording of when the app opens. Roughly 66 commits and 1s until the last commit is made (the SidebarLinks becomes visible)

Also you can measure the time it takes until the report screen is open and ready, which is in our testing around ~800ms.

Proposed solution (if any)

Onyx internally already has a cache map. When a component connects, and the data is already in the cache map we return the data like this:

function get(key) {
    // When we already have the value in cache - resolve right away
    if (cache.hasCacheForKey(key)) {
        return Promise.resolve(cache.getValue(key));
    }

This is good, however, it still runs through the async/callback and thus will cause an extra re-render.
I propose that we, in the constructor of the withOnyx wrapper, access the cache map directly and set the data immediately when we have them. We only set the loading state to true if we don’t have all the data right away.
This way, a withOnyx connected component will most of the times only render initially once, instead of twice.

(Note: once we do that we will notice that there are a few places in the Onyx.js code where we don't compare the data before sending it to the component. As a extension to this proposal we should wrap all those places with equal checks to avoid updating components with the same data twice).

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

Profiling again we get 44 commits, and just 0.6s for the initial render of SidebarLinks to finish!

When it comes to opening the report screen we see 4x improvements, as it only takes 100-200ms to show the whole page:

Screen.Recording.2023-07-05.at.15.33.41.1.mov

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: latest
Reproducible in staging?: yes
Reproducible in production?: yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1688580518417739

Answer 1 · 2023-07-17T07:17:42.000Z

PR is ready for review:

#280