[Performance] [Audit] withOnyx causes bursts of commits, impeding performance

[jsamr]

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

---

This report is part of #3957, scenario "Rendering Individual chat messages".

Commit log excerpt

The full commit log can be inspected with Flipper or React Devtools, see #3957 (comment) for instructions. This excerpt takes only the first 31 commits of the 292-long log.

SHOW LOG

1. Renders BaseNavigationContainer because of hook change (124ms)
2. Renders BaseNavigationContainer because of hook change, but don't re-renders subtree (6ms)
3. Renders withOnyx(Component) because preferredLocale state changed (0.1ms)
4. Renders withOnyx(Component) because loading state changed (0.2ms)
5. Renders SideBarLinks (withOnyx) because currentlyViewedReportID prop changed (12ms)
6. Renders withOnyx(HeaderView) because report state changed (0.1ms)
7. Renders withOnyx(HeaderView) because personalData state changed (0.1ms)
8. Renders withOnyx(HeaderView) because policies state changed (0.1ms)
9. Renders withOnyx(HeaderView) because loading state changed (13.2ms)
10. Renders withOnyx(Component) inside HeaderView because preferredLocale state changed (0.1ms)
11. Renders withOnyx(Component), parent of VideoChatButtonAndMenu because loading state changed (5.8ms)
12. Renders ReportScreen because isLoading state changed (13ms)
13. no apparent cause (NAC)
14. Renders withOnyx(ReportView), because state changed session (0.1ms)
15. Render withOnyx(ReportView), because state changed loading (3.6ms)
16. Render withOnyx(ReportView), because state changed preferredLocale (0.1ms)
17. Render withOnyx(Component), list cell, because state changed loading (0.6ms)
18. Render withOnyx(Component), list cell, because state changed preferredLocale (0.1ms)
19. Render withOnyx(Component), list cell, because state changed loading (0.1ms)
20. Render withOnyx(ReportActionView), list cell, because state changed report (0.1ms)
21. Render withOnyx(ReportActionView), list cell, because state changed reportActions (0.1ms)
22. Render withOnyx(ReportActionView), list cell, because state changed session (0.1ms)
23. Render withOnyx(ReportActionView), list cell, because state changed loading (19ms)
24. Render withOnyx(ReportActionCompose), list cell, because state changed comment (0.1ms)
25. Render withOnyx(ReportActionCompose), list cell, because state changed betas (0.1ms)
26. Render withOnyx(ReportActionCompose), list cell, because state changed modal (0.1ms)
27. Render withOnyx(ReportActionCompose), list cell, because state changed network (0.1ms)
28. Render withOnyx(ReportActionCompose), list cell, because state changed myPersonalDetails (0.1ms)
29. Render withOnyx(ReportActionCompose), list cell, because state changed personalDetails (0.1ms)
30. Render withOnyx(ReportActionCompose), list cell, because state changed reportActions (0.1ms)
31. Render withOnyx(ReportActionCompose), list cell, because state changed report (0.1ms)
    ...

#4022 could be related to this

withOnyx HOC triggers a lot of commit bursts such as seen in the commit log excerpt. You can also notice this pattern with the commit graph in Flipper / Devtools:

In the below graph, we see that from commit 24 to 52, a span of 400ms was occupied by those commit bursts, happening for each cell. This will cause performances issues because React needs to run its reconciliation algorithm each time setState is invoked. Moreover, each commit can cause children to re-render unless they are pure (and assuming props are memoized). withOnyx is so widespread in this application that every commit must count!

Proposal 1: batch Onyx state mutations

Proposal: Perhaps events could be initially batched to avoid these bursts. For example, we could cache every subscribed key and trigger a state update only when all keys have been loaded.

Proposal 2: avoid extraneous tailing commit (loaded state becomes true)

withOnyx HOC causes at least n + 2 commits, where n is the number of subscribed keys (initial, ...nth key available, and loaded). With Proposal 1, we could go down to at least 3 commits (initial, first batch, loaded). We could go down to 2 if loaded came along with the first batch.

If proposal 1 cannot be considered, we could still spare one commit by using getDerivedStateFromProps to derive loaded.

Proposal 3, access cache synchronously to prevent extraneous commits

Onyx has a cache which returns cached values as promises.

https://github.com/Expensify/react-native-onyx/blob/86be75945a47f9015b9a37ca738e8fdd36e42de3/lib/Onyx.js#L40

If it would return cached values synchronously, we could set some keys early (in withOnyx constructor) and in many instances where all keys are cached, end up with only one initial render (if we include the first two proposals) instead of the current n + 2 figure.

Proposal 4, test rendering performance of withOnyx

Test performance-critical withOnyx with react-performance-testing to enforce rendering metrics, such as number of renders in controlled conditions.

Proposal 5, (experiment) use React Context for keys that change rarely

A lot of components subscribe to keys which rarely change (preferredLocale, session, beta). If the first 3 proposals could not be considered, an alternative would be to limit the number of components subscribing directly to Onyx, and use a store to share access to these values which rarely change and have a low memory footprint. Thus, most of those components would require one render instead of the n + 2 pattern identified before.

The context would be connected to Onyx, and update slices of its state on value updates. Moreover, you could also take advantage of useContextSelector third party which should land to React soon, narrowing down subscriptions to slices of the state.

EDIT 1: Added Proposal 4
EDIT 2: Added Proposal 5

[MelvinBot]

Triggered auto assignment to @stephanieelliott (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[marcaaron]

Proposal 1: batch Onyx state mutations

This is an interesting idea, but I'm not yet sure how it would work given the architecture we have now. Let's dive a little deeper...

Presently, we are calling setState() directly on any connected components (which we store references to in a subscriber map) in several places. When a stored key changes we will loop over the map and setState() on each subscriber. These updates could be batched maybe, but I think the intention here was that it would be ideal to update a subscriber as soon as the data changes or is fetched from storage asynchronously (since "asynchronous" would always be better than "synchronous").

That said, I think what we are looking at here in the profile is the state when a withOnyx component first mounts and calls Onyx.connect() for each key. So without changing too much we could start by changing the logic in withOnyx where we send the data to a connection. So that for the connected components we:

1. Gather all this data from AsyncStorage or the cache
2. Call withOnyxInstance.setState() once

Proposal 2: avoid extraneous tailing commit (loaded state becomes true)

Doing the above should allow us to also set loaded when the rest of the data is set and we won't need to do the dance where we wait for data to become available and then perform another state update.

Proposal 3, access cache synchronously to prevent extraneous commits

I'm not too sure how to reliably achieve this without creating a big front-loaded event where all data from storage is first pulled into the cache on app init, but maybe there's some other way or that's not a real problem.

I think the main arguments why we have not done it is:

1. memory cache will, well, take up a lot of memory
2. it will add extra lag to app boot time while we slurp up all the data from storage into the cache

Maybe @kidroca has thoughts on this. I think if the rendering trade-offs of being able to go from n+2 commits to n+1 then eventually to n are great enough then we can compare that to the cost of app boot + memory consumption. I'm not sure anyone has evaluated whether those are real or imagined problems - and maybe if they are real there are other ways to solve them.

Proposal 5, (experiment) use React Context for keys that change rarely

This sounds interesting. I'm less familiar with how a solution like this might work and having trouble seeing how it would fit in with the logic we already have.

Does it kind of beg the question of whether we should just be using context for everything? Seems like this is what Redux uses. And makes me wonder whether switching from the setState() on a subscriber model should not be replaced with a context solution - and what that would look like.

[kidroca]

Proposal 1: batch Onyx state mutations

This sounds like a case for the initial connection (mount time)
withOnyx would have to wait to get everything from storage and then render the wrapped component
This is perceived like something slow compared to render a prop as soon as possible, but if
all props need to come from disc or from cache - they will take about the same time to arrive so we might as
well wait for that. The only time a prop can come significantly faster is when one comes from cache and the
other from storage, but IMO it's still worth to wait all to arrive

Something very similar happens for subscriptions to collection keys

@marcaaron I somewhat did this with multiGet, but preserved the logic
that called keysChanged in a foreach
I just want to note that this is something that would probably go hand in hand with get/multiGet batching

Proposal 2: avoid extraneous tailing commit (loaded state becomes true)

If loading can be derived without using state then this make sense

Proposal 3, access cache synchronously to prevent extraneous commits

After initially adding cache we triggered keysChanges synchronously.
It resulted in bugs through the app we added Promise.resolve() to
preserve the previous async behavior
Though this might work if serves only to get data

Proposal 5, (experiment) use React Context for keys that change rarely

I'm making updates that would keep some keys like defaultKeyStates forever in cache
they take low space and are frequently fetched, we could identify all such keys and
add them to this list/map

---

Does it kind of beg the question of whether we should just be using context for everything?

On the broader topic of Context I thought of that as well
Why don't we have a store in memory (promote cache or succeed it)
It doesn't have to contain all the items we have on disc, just whatever is used ATM
Components can connect directly to it and receive the data
Writes/merges are first sent there and then a patch is sent to disc
Any updates to it would cause a re-render and pass the changes down without having to maintain an explicit connection
This is where redux suffered in the past, and where the store can be divided into multiple
substores, so that updating one would not cause a re-render to components that aren't subscribed to keys from this substore
It's a long topic...

[jsamr]

@kidroca

This is where redux suffered in the past, and where the store can be divided into multiple
substores, so that updating one would not cause a re-render to components that aren't subscribed to keys from this substore

This is precisely where useContextSelector comes to the rescue! (see Proposal 5)

[MelvinBot]

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

[stephanieelliott]

Setting this to monthly due to @marcaaron's comment around uncertainties on how it would work. @chiragsalian please update the label as needed based on how things develop!

[marcaaron]

Adding a planning label until we can make this more actionable. One idea I shared that might be useful (or not)...

Maybe someone can try a POC where we collect all the data for a withOnyx wrapped component FIRST and then call setState on it just once - instead of calling setState() n times per key we are subscribing to?

https://expensify.slack.com/archives/C01GTK53T8Q/p1626895469088100

Which is basically to test out Proposal 2 in the issue description and then report the results.

It's less clear to me how we should go about:

• incorporating the Context API
• synchronous changes to Onyx (since the implications might be far reaching)

But I think we can look at more detailed proposals for those if someone is inclined to make them.

[marcaaron]

Quick update.

I ended up doing a POC in this branch that I suggested we try in that last comment. It is hacked together and doesn't use any caching, but basically collects all the data a subscriber needs and then passes it to withOnyx so we only setState once. The results were sort of inconclusive but we still need to try hooking that up with caching as we are discussing here.

I also tried some things with the Context API and reported about here that @kidroca is also investigating for withLocalize(). Basically the problem he identified there could be applied to Onyx more generally. Still, I'm not quite sure how to best do it.

That POC showed about a ~400ms improvement in chat switching time so I'm bumping the priority of this back to Daily.

I think for some actionable next steps we can:

Implement Context API usage to improve list performance in the short term

Basically just audit all the subscribers in lists and start switching them over to use Context API + a single withOnyx() per unique key. It is really easy to do this so we don't need to get too fancy yet.

Improve withOnyx / Onyx.connect

• Make @kidroca's multiGet improvements to Onyx.connect() - some discussions here and here
• Also update withOnyx() so it gets all subscribers keys at once and only sets state once.

Longer Term:

Think about how to incorporate Context better into Onyx's architecture. I don't quite have the vision for how that should work yet, but I'm pretty convinced we shouldn't be afraid to use it 😄 and I'm interested in learning more about useContextSelector etc.

[kidroca]

Implement Context API usage to improve list performance in the short term

Basically just audit all the subscribers in lists and start switching them over to use Context API + a single withOnyx() per unique key. It is really easy to do this so we don't need to get too fancy yet.

I've posted a reply on the slack thread but here's the gist:

Instead of making a Context Provider for each key that is mass used we can make a single OnyxContextProvider that wraps the App

• It will manage connections similarly to how that happens now
• Its state would be aggregated from all connections
• withOnyx would be receive a slice of props using OnyxContextConsumer and will provide them to the WrappedComponent
• This way even when OnyxContextProvider re-renders, consumer components won't re-render if their particular slice of props/state didn't change

This eliminates duplicate connections to the same key, which as @marcaaron pointed, are a lot
It will also result in less changes to the general app code, as we won't have to change each component that uses a frequently used key

---

useContextSelector is very similar to the proposal above, but works with hooks.

• we have one big context wrapping the App
• we don't want our context users to re-render each time the context changes, but only when their slice of state changes

Components subscribe using a Context and a selector function that picks items from the actual context value

const myProps = useContextSelector(Context, everything => {
  return {
     network: everything[ONYXKEYS.NETWORK],
     betas: everything[ONYXKEYS.BETAS],
  }
})

return <WrappedComponent {...myProps} />

It's still experimental and not a officially a part of React

• Feature request: useContextGetter facebook/react#21329
• [Experiment] Context Selectors facebook/react#20646

There are libraries that expose or implement the useContextSelector functionality themselves

[marcaaron]

Great insights @kidroca and I LOVE this idea!

I think there might be an easy alternative while we continue to research everything we want in an Onyx 2.0 that will let us test this out with not a lot of code. Here's what I had in mind to propose for a v1 which gets us to experience benefits immediately without changing too many low level things.

1. Create a factory method like this to create contexts for any keys that we are mass subscribing to

export default (onyxKeyName) => {
    const Context = createContext();
    const Provider = props => (
        <Context.Provider value={props[onyxKeyName]}>
            {props.children}
        </Context.Provider>
    );

    const ProviderWithOnyx = withOnyx({
        key: onyxKeyName,
    })(Provider);

    const withOnyxKey = WrappedComponent => props => (
        <Context.Consumer>
            {(value) => {
                const propsToPass = {...props, [onyxKeyName]: value};
                return (
                    <WrappedComponent {...propsToPass} />
                );
            }}
        </Context.Consumer>
    );

    return {
        withOnyxKey,
        Provider: ProviderWithOnyx,
    };
};

Which would let us create a context for the problematic key easily like this ..

const context = createOnyxContext(ONYXKEYS.NETWORK);
export const NetworkProvider = context.Provider;
export default context.withOnyxKey;

And we can compose the providers like this to clean things up a bit

const App = () => (
    <ComposeProviders
        components={[
            NetworkProvider,
            SafeAreaProvider,
            PersonalDetailsProvider,
        ]}
    >
        <CustomStatusBar />
        <ErrorBoundary errorMessage="E.cash crash caught by error boundary">
            <Expensify />
        </ErrorBoundary>
    </ComposeProviders>
);

I kind of like starting with this because (while not perfect) it's also not hack by any means and will give us a sense of the benefits with few changes.

[kidroca]

Yep, that seems good

It might be clearer to capture all Onyx providers in a single Component file and expose the ComposedProvider from that file

OnyxProvider

const NetworkContext = createOnyxContext(ONYXKEYS.NETWORK);
const NetworkProvider = context.Provider;
const PersonalDetailsContext = ...

// BTW we can also move `Onyx.init` here
const OnyxProvider = (props)  => (
      <ComposeProviders
        components={[
            NetworkProvider,
            PersonalDetailsProvider,
            ...
        ]}
    >
      {props.children}
    </ComposeProviders>
)

export default OnyxProvider;

[MelvinBot]

@chiragsalian Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[jsamr]

@kidroca

withOnyx would be receive a slice of props using OnyxContextConsumer and will provide them to the WrappedComponent

I really love the ideas you are proposing here. Have a little technical question though! How would you prevent the WrappedComponent to re-render when other slices would update? My understanding is that useContextSelector prevents this issue by bailing out early in a reducer (basically by returning the previous state):

https://github.com/dai-shi/use-context-selector/blob/2dd334d727fc3b4cbadf7876b6ce64e0c633fd25/src/index.ts#L155-L157

@marcaaron
I don't know yet if it would be relevant for grouping updates with Onyx, but there is an interesting API (unstable_bachedUpdates) to batch updates outside of the render loop (e.g. in callbacks / subscribers), see this comment: reduxjs/react-redux#1091 (comment) The huge benefit is that if one onyx key has, let's say, 1000 subscribers, you could end up with only 1 commit instead of 1000!. This technique is actually used in useContextSelector user land implementation.

[kidroca]

@jsamr

I really love the ideas you are proposing here. Have a little technical question though! How would you prevent the WrappedComponent to re-render when other slices would update? My understanding is that useContextSelector prevents this issue by bailing out early in a reducer (basically by returning the previous state):

Thanks for the question.
I didn't get into details in the initial post but here's an idea

The withOnyx HOC would have a mapContextToProps interface similar to the one used in useContextSelector mapper function

A component using a "slice" containing just network and details

const ComponentUsingOnyx = withOnyx(everything => ({ 
  details: everything[ONYXKEYS.PERSONAL_DETAILS],
  network: everything[ONYXKEYS.NETWORK,
}))(SomeComponent)

A key/value in everything (The aggregated context) would not change reference unless a connection triggers an update for it. Which means we could use a shallow comparison as the one provided by React.memo

Basic withOnyx implementation

const withOnyx = mapContextToProps => WrappedComponent => {
 const MemoizedWrap = React.memo(WrappedComponent);

  const WithOnyxCtx = props => (
    <OnyxContext.Consumer>
        {value => <MemoizedWrap {...props} {...mapContextToProps(value, props)} />}
    </OnyxContext.Consumer>
  );

  return WithOnyxCtx;
};

As long as mapContextToProps returns the same props and their storage values haven't changed the component should not re-render

@marcaaron had the same question. I've mentioned why it might not be so bad even if a re-render happens to slip by: #4348 (comment)

I think what's causing the biggest impact ATM are the duplicate connections as each connection introduces some overhead and async handling.
If we capture a single context it might not be an issue if it causes a re-render as the expensive components already use some kind of re-render prevention

[MelvinBot]

@chiragsalian Huh... This is 4 days overdue. Who can take care of this?

[marcaaron]

Not sure what is left to do here but discuss whether there are some more long term improvements to make to withOnyx using Context or something else. For now, we should not be experiencing this issue as much as before due to recent changes so I'm going to lower the priority.

[MelvinBot]

@jsamr, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.