[Performance] withOnyx initial state from cache

[kidroca]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Overview

We can resolve initial component state from cache instead of starting with empty and the receive a burst of updates
Related to #4101

What performance issue do we need to solve?

• inefficient React component rendering

What is the impact of this on end-users?

• app boot time
• chat switch time

List any benchmarks that show the severity of the issue

1. Start the web app and be on a chat
2. Open the dev console
3. Go to the Performance tab
4. Start capturing data
5. Switch to another chat
6. Wait a few seconds for loading to settle
7. Stop capturing data
8. Review the first long running task after renderApplication

See the `sendDataToConnection` calls that make the majority of the long running Task We should see a reduction in the amount of time they take

Profile data (you can load this in Chrome/Performance): https://u.pcloud.link/publink/show?code=XZCiqwXZco50EdMOjDXI1D8DhgzxGmPMTSV7

Proposed solution (if any)

Import the cache service in withOnyx
Use cache and mapOnyxToState to try and map initial state from cache
If we've mapped everything from cache we can set loading: false and render the wrapped component
Leave the existing logic that would call setState with values from storage

• when the values are already in state a re-render won't be caused

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

Note: These should be the same as the benchmarks collected before any changes.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.0.85-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @TomatoToaster (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[kidroca]

cc @marcaaron

[marcaaron]

See the sendDataToConnection calls that make the majority of the long running Task We should see a reduction in the amount of time they take

Looks like the setState() call is blocking? Like things randomly hang up i.e. one call to setState() will maybe take 1ms but then another will take 100ms. Any ideas about why? I like the solution to minimize the calls, but also curious why this happens.

Regardless, I think we can try to pre-fill the state from the cache and then connect to anything that did not return a value.

[marcaaron]

Ah I just thought of something... I am not positive but I think some of them block more than others because the setState() includes the componentDidUpdate() and eventual rendering of the WrappedComponent e.g. check this call tree here:

First thought... I wonder if we can improve this (for any keys that are not cached) by simply setting loading: false from inside sendDataToConnection() instead of in componentDidUpdate() like this...

function sendDataToConnection(config, val, key) {
    if (config.withOnyxInstance) {
        const component = config.withOnyxInstance;
        component.setState((prevState) => {
            const requiredKeysForInit = _.chain(component.mapOnyxToState)
                .omit(c => c.initWithStoredValues === false)
                .keys()
                .value();

            const newState = {
                ...prevState,
                [config.statePropertyName]: val,
            };

            return {
                ...newState,
                // // All state keys should exist and at least have a value of null
                loading: !_.every(requiredKeysForInit, key => !_.isUndefined(newState[key])),
            };
        });
    } else if (_.isFunction(config.callback)) {
        config.callback(val, key);
    }
}

Second thought... if a call to sendDataToConnection() does include the render of the child component then it might be hard to say how much slower the setting state itself is compared to the rendering of the child component. Is the problem really that we are setting state or something about the way it's all being rendered?

Last thought... if it takes time to gather data needed to render a child component and we can only start the next process once a child component has rendered - then each child (if also wrapped in withOnyx) must wait for it's parent to complete. So, would it maybe be more efficient in some cases to load all the required keys in a parent level and then just pass them through instead of choking things up each time a new withOnyx() wrapped component appears in the tree?

I followed up on that last idea and did some quick changes where more keys were moved into context to replace some of the staggering withOnyx components. This front loading of the data seems like it led to less blocking time and seems to confirm the theory that wrapping everything in withOnyx() is not efficient.

Before removing some withOnyx() calls and replacing them with context providers

After

The difference isn't that great yet.. but I only moved a few things and 100ms dropped off the total time so seems like this is the right thread to pull on.

[TomatoToaster]

Hey I know some of the performance upgrade related work is its own project. Is this issue just for tracking? Or should I tag this as External so an upwork listing can be created?

[marcaaron]

It's a conversation for now. This should probably have a Planning label put on it until it's actionable and not an Engineering label.

[marcaaron]

Tested out my theory on an Android release build and startup time got worse 😄 I'm done for today...

Just confirms my feeling the web profiler can't always be trusted.

[kidroca]

I wonder if we can improve this (for any keys that are not cached) by simply setting loading: false from inside sendDataToConnection() instead of in componentDidUpdate() like this...

Actually thanks to your input I don't think we should call setState at all

Do we really want to trigger a re-render (using setState) when loading will remain true? I don't think so

Here's a rough proposal

1. Onyx and particularly code that uses withOnyxInstance should not call setState directly on that instance
2. We should add a method like onOnyxUpdate on the withOnyxInstance - this method receives connection updates
3. We should drop component state in favor plain instance properties (this is optional)
4. Instead of calling setState, sendDataToConnection calls onOnyxUpdate
5. onOnyxUpdate saves incoming data and compares it to local data, it only calls setState when all the mappings are resolved

In summary onOnyxUpdate would only trigger a re-render when loading becomes false, instead for each individual connection update
This can work purely without React state (force refreshes) or mix with local and React state - we collect local state until loading should become false at that point we call setState with everything we have for the mapping
This should allow us to drop some of the existing logic tries to manage this as well. Or restructure so that it doesn't bother React as much or at all

[kidroca]

Last thought... if it takes time to gather data needed to render a child component and we can only start the next process once a child component has rendered - then each child (if also wrapped in withOnyx) must wait for it's parent to complete.

This is a valid point - a child can only mount when the parent is done loading, otherwise withOnyx would not render the WrappedComponent at all
This might be a problem for React as it can't compute the full App Tree - it can only see how the branch continues after loading: false and then if these are nested withOnyx components this would repeat a bunch

Thought it's likely the same thing would happen if the Children themselves don't render anything because they lack the data...

As to how to supply the data to the children instantly, yes one way would be to capture everything needed in the parent. Another would be to pre populate cache
The parent approach's PRO is it leads to less HOCs
While the hidrate approach's PRO is multi-getting everything in one go
Both can be applied together

[marcaaron]

This can work purely without React state (force refreshes) or mix with local and React state - we collect local state until loading should become false at that point we call setState with everything we have for the mapping

I like the former idea since it means we do not need to yet look into managing keyChanged() events which also setState(). Gonna open a PR with those changes so we can do some testing.