Perf: Add context factory helper to speed up chat switching

[marcaaron]

Details

cc @kidroca

This PR is a minimal Context implementation for some of our most problematic Onyx keys.

It should be easy to add more if we need to, but hopefully we won't have to add too many 😅

My idea here is that this will basically "hold us over" until we figure out how to solve this problem better. But at the same time, this solution might reasonably last us a good long while.

Fixed Issues (Related To)

#4101

Tests / QA Steps

1. No specific tests as everything should work the same but faster.
2. Switch some reports and observe

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

iOS.Chat.Switch.-.after.improvements.mov

Android

Android.Chat.Switch.-.After.Improvements.mp4

[marcaaron]

Going to take this out of draft since it is one more set of improvements to speed up chat switching times.

[kidroca]

Overall this works and improves performance. It's intended as a transitional solution, but it can indeed serve as a long term solution as well

I've left some notes regarding props handling and a potential enhancement

[kidroca]

There are some propType inconvenience due to these dynamic prop names
As you've seen this forces you to write the the ONYX key name as prop (e.g. reportActionsDrafts_) in the consumer

Instead it might be handy to leave to consumer name the prop however they like

// Optional params to tweak the behavior
const withOnyxKey = ({propName = onyxKeyName, transformValue} = {}) => (WrappedComponent) => {
  /* ... */
  const propsToPass = {...props, [propName]: transformValue ? transformValue(value, props) : value};
  /* ... */
}

And then in the wrapper component

export default compose(
    withWindowDimensions,
    withReportActionsDrafts({ propName: 'draftMessages' }),
)(ReportActionItem);

Or extract the draft message using the transformValue func

const getDraftMessage = (allDrafts, props) => {
  const {reportID, action} = props;
  const draftKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}_${action.reportActionID}`;        
  const draft = allDrafts[draftKey];
  return draft || '';
}

export default compose(
    withWindowDimensions,
    withReportActionsDrafts({ propName: 'draftMessage', transformValue: getDraftMessage }),
)(ReportActionItem);

This should result in no changes to the draftMessage prop and its usages

[marcaaron]

Ah nice, that's an interesting idea. Thanks! I was having trouble resolving these propTypes errors.

[kidroca]

The comment description would have to change if now the prop here receives all available draft messages

[kidroca]

Alternative OnyxProvider

The thoughts below are another way to achieve the same, though it might be slightly more flexible

Seeing the implementation of createOnyxContext I think it should be possible to extract a single component that subscribes to all necessary keys - instead of creating separate contexts

const OnyxContext = createContext({});

// Define the mass used keys here
const OnyxProvider = withOnyx(
   [NETWORK]: {key: ONYXKEYS.NETWORK}, 
   [PERSONAL_DETAILS]: {key: ONYXKEYS.PERSONAL_DETAILS},
   [REPORT_ACTIONS_DRAFTS]: { key:  ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS},
)(({ children, ...propsFromOnyx }) => (
  <OnyxContext.Provider value={propsFromOnyx}>{children}</OnyxContext.Provider>
));

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

It still allow creating helpers like:

export const withNetwork = WrappedComponent => {
  const mapper = onyxCtx => ({ network: onyxCtx[ONYXKEYS.NETWORK] })
  return withOnyxCtxProps(mapper)(WrappedComponent);
}

As well as an inline usage like:

const mapOnyxProps = (onyxCtx, otherPops) => {
  const { reportID, action } = otherPops;
  const draftMessage = onyxCtx[PATH_TO_ACTION_DRAFT] || '';
  return { draftMessage };
}

export default compose(
    withWindowDimensions,
    withOnyxCtxProps(mapOnyxProps),
)(ReportActionItem);

It isn't restricted to a single Onyx key and can handle a combination of Onyx retrieved keys/values

[kidroca]

I've captured some benchmark data on Android:

This PR

Flow	init	init	init	switch to 71955477	switch to 71955477	switch to 71955477
Total	42.30sec	38.28sec	40.98sec	3.79sec	3.63sec	3.66sec
Last call ended at	9.32sec	8.28sec	8.33sec	8.31sec	9.98sec	7.70sec
Last get ended at	6.36sec	5.84sec	5.64sec	5.15sec	6.01sec	3.81sec
Last long get at	4.98sec	3.77sec	3.83sec	4.38sec	5.69sec	3.40sec
Onyx#get total	29.90sec	26.92sec	28.39sec	2.10sec	1.94sec	1.99sec

• Full data: https://docs.google.com/spreadsheets/d/1RP39XWfMZBV057kQZi_lo-22heK728xwXemUsaNAAB0/edit?usp=sharing

Before

This is not exactly taken at the hash before the changes were introduced, but from my last checkout of main 87cbc24 before starting work on onyx lru cache

Flow	init	init	init	init	switch to 71955477	switch to 71955477	switch to 71955477
Total	54.80sec	53.31sec	56.26sec	52.83sec	11.49sec	11.86sec	18.40sec
Last call ended at	10.26sec	10.33sec	10.27sec	10.26sec	9.85sec	10.43sec	11.00sec
Last get ended at	10.26sec	10.33sec	10.27sec	10.26sec	3.50sec	3.98sec	4.63sec
Last long get at	3.55sec	3.58sec	3.55sec	3.44sec	3.50sec	3.75sec	4.63sec
Onyx#get total	41.01sec	39.42sec	42.48sec	39.30sec	5.24sec	5.35sec	7.63sec

• Full Data: https://docs.google.com/spreadsheets/d/1EdkNhTt4R_oVkUhCqjalYcJwlCTUvCtuFjXG5YoUkBg/edit?usp=sharing

There are obvious improvements for the "init" flow
The benchmark tends to vary a lot for "chat switch" - I'm still improving the process

[marcaaron]

@kidroca I like the idea for the alternative onyx provider. But I'm unsure about something. Wouldn't the wrapped component re-render each time a value changes even if it was not using the value in context?

[marcaaron]

Also you mention

extract a single component that subscribes to all necessary keys - instead of creating separate contexts

but I'm curious if there's anything wrong with creating these separate contexts?

[luacmartins]

Also you mention

extract a single component that subscribes to all necessary keys - instead of creating separate contexts

but I'm curious if there's anything wrong with creating these separate contexts?

I think the only downside in multiple contexts is that we have multiple providers wrapped around our app. However, it is cleaner and prevents unnecessary re-renders, so I'd say we stick with multiple contexts.

[luacmartins]

LGTM! Great work @marcaaron!

[kidroca]

Wouldn't the wrapped component re-render each time a value changes even if it was not using the value in context?

Yes this will cause a re-render, but can be addressed

We don't necessarily want to prevent re-renders:

1. Even when such re-render happens the update would not be translated to the screen as the virtual dom would not change (not all renders produce a view update)
2. All of the expensive components already use some means to skip re-renders, like shouldComponetUpdate

If we must prevent re-renders, we can wrap the component with React.memo

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

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

The component will re-render only when props change or when the values returned by mapContextToProps change.
Note: this is only a shallow comparison. it's fast and should cover a broad range of cases like const mapper = onyxCtx => ({ network: onyxCtx[ONYXKEYS.NETWORK] })

---

but I'm curious if there's anything wrong with creating these separate contexts?

Only small things like:
Not being able to work with 2 or more Onyx props at the same time in functions like mapContextToProps.
You would still be able to do any such logic but it would need to happen inside the WrappedComponent when it receive all the props
The React Tree in Flipper contains many nested entries and can become confusing
Using different HOC for different Onyx keys can be confusing and/or lead to errors. E.g. Having to know to use withNetwork or withReportActionsDrafts. Where mapContextToProps can be more universal

Overall on a small scale like up to 5-6 separate key contexts/HOCs I think separate contexts will work

Another thing to consider is if we're moving to a single OnyxProvider in the future, and it has interface like mapContextToProps we might as well implement it now

• related to: [Performance] [Audit] withOnyx causes bursts of commits, impeding performance #4101 (comment)

[marcaaron]

Even when such re-render happens the update would not be translated to the screen as the virtual dom would not change (not all renders produce a view update)
All of the expensive components already use some means to skip re-renders, like shouldComponetUpdate

Right, those are good points. I guess my one concern is that it might not be obvious to someone using these that it would be the wrapped component's responsibility to prevent the update.

If we must prevent re-renders, we can wrap the component with React.memo

And that takes care of my previous concern. But I'd wonder if that might lead to unexpected lack of updates if someone does decide to have deeply nested props (ideally we'd avoid this by passing simpler ones / maybe not a very likely case in general).

Great points about creating separate contexts! But also wonder if we should restrain ourselves from making this pattern more flexible for now.

This is maybe minor, but one thing I like about the current design (while not super flexible) is that it forces us to be more intentional about whether we need to use context or not. One worry is that it might be confusing to see a pattern that appears to enable generic Onyx key selection, but in reality, only services some subset of frequently accessed keys.

[kidroca]

Noting some small items that might be potential problems

[marcaaron]

Updated!

[luacmartins]

LGTM and feels much faster on chat switching. Great work!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging in version: 1.0.82-8🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅