Perf: remove Onyx usage from `withLocalize` #4253

kidroca · 2021-07-27T13:40:30Z

Details

This component is one of the most used components and has more than 310 instances depending on chat size
Instead of creating a new connection for each withLocalize usage - use the already cached value in translate
This significantly reduces the Onyx.get load that is caused by all the connections

Fixed Issues (Related To)

Fixes #4268

Tests

Measure performance (init time) before this change
Measure performance (init time) with the change in this PR

I'm using Onyx.printMetrics() to monitor the Onyx.get calls that happen during init: https://github.com/Expensify/react-native-onyx#benchmarks

My Benchmarks

Before

Total:	123.80sec			Last	17.49sec			ReportID	71955477

method	total time spent	max	min	avg	time last call completed	calls made
Onyx:get	96.85sec	2.92sec	0.10ms	144.77ms	17.48sec	669

see the individual Onyx.get calls here: https://docs.google.com/spreadsheets/d/1wGrVXsad6ayQZVqvyUmTuTb1cmeegdE0y-IFzhLiPKU/edit?usp=sharing
- search for preferredLocale

After

Total:	62.62sec			Last	12.27sec			ReportID	71955477

method	total time spent	max	min	avg	time last call completed	calls made
Onyx:get	40.44sec	1.79sec	0.08ms	139.94ms	12.27sec	289

excel data: https://docs.google.com/spreadsheets/d/1cVe3eEnwYgwbH4DRV__g9_W8kQFpVhvTjDu3Igr6Ds0/edit?usp=sharing

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

iwiznia

Nice improvement!!

parasharrajat · 2021-07-27T14:22:24Z

When we update the locale, Will the app re render components. I don't think it will.

Julesssss · 2021-07-27T14:23:34Z

src/libs/translate.js


+function getPreferredLocale() {


NAB: For consistency, we're missing a function doc here -- even though its function is obvious.

Nice optimization!

If it has a return value it needs a doc but not necessarily a description.

Julesssss · 2021-07-27T14:24:41Z

When we update the locale, Will the app re render components. I don't think it will.

That's a good point. I wouldn't have thought so either.

iwiznia · 2021-07-27T15:03:44Z

When we update the locale, Will the app re render components. I don't think it will.

Ohhh... hmmm yeah, that kind of sucks.

kidroca · 2021-07-27T16:49:41Z

When we update the locale, Will the app re render components. I don't think it will.

When does that happen, isn't the app reloading when a new locale is selected?

iwiznia · 2021-07-27T16:53:09Z

When does that happen, isn't the app reloading when a new locale is selected?

You can test this by changing the language in the preferences (maybe you need to force being in the bet if that's not done by default in dev). And no, it does not reload, all components that are using withLocalize will re-render themselves.

kidroca · 2021-07-27T16:55:48Z

I've mentioned this at Slack, there's another way to implement the HOC and it will not suffer this problem: https://expensify.slack.com/archives/C01GTK53T8Q/p1627403864349200?thread_ts=1627393814.334100&cid=C01GTK53T8Q

The changes would be slightly more, but it might be an even faster if the HOC is implemented through React.createContext. As this will also save 300+ class instances of withLocalize

I'll try it out tomorrow and do another benchmark

marcaaron · 2021-07-27T18:17:29Z

Looks good so far and definitely up for more improvements. I'm adding a link back to the original issue in the description so there is some more context.

marcaaron · 2021-07-27T18:18:41Z

But I think maybe this needs a new issue?

kidroca · 2021-07-27T20:26:12Z

But I think maybe this needs a new issue?

Yeah, I'll probably have to spent some more time here, and then test/bench the changes on the remaining platforms.

I can open an issue tomorrow

marcaaron · 2021-07-27T20:52:32Z

👍 Sounds great!

kidroca · 2021-07-28T07:31:21Z

@marcaaron I've created a new issue: #4268 and updated the PR description with the link

kidroca · 2021-07-30T21:02:28Z

Updated implementation and posted new benchmarks here: #4268 (comment)

I think another possible optimization that might be applied at the moment is to drop the withOnyx call altogether like this:

class LocaleContextProvider

    componentDidMount() {
        this.connectionId = Onyx.connect({
            key: ONYXKEYS.NVP_PREFERRED_LOCALE,
            callback: (val) => {
                if (val) {
                    this.setState({preferredLocale: val});
                }
            },
        });
    }

    componentWillUnmount() {
        Onyx.disconnect(this.connectionId);
    }

This skips creating the withOnyx class component
I've tried it briefly it was slightly faster bringing "Last call ended time" to ~10.87s (otherwise it's ~11.05s)
I can try to capture more data about it next week, but it might be better to focus/find something more important

marcaaron

These changes look good to me and do make chat switching significantly faster. I can't really think of any reasons why we should not do this (other than people being unfamiliar with the Context API). Probably as a follow up we can add something to the documentation about how to approach using withOnyx when a component will be used a great number of times throughout the app or some list.

marcaaron · 2021-07-30T21:46:51Z

src/components/withLocalize.js

+    const WithLocalize = forwardRef((props, ref) => (
+        <LocaleContext.Consumer>
+            {/* eslint-disable-next-line react/jsx-props-no-spreading */}
+            { translateUtils => <WrappedComponent {...translateUtils} {...props} ref={ref} />}


NAB, Unnecessary space here.

marcaaron · 2021-07-30T21:56:46Z

This skips creating the withOnyx class component
I've tried it briefly it was slightly faster bringing "Last call ended time" to ~10.87s (otherwise it's ~11.05s)
I can try to capture more data about it next week, but it might be better to focus/find something more important

Yeah, I think there are probably better optimizations to focus on. This is already so great 🎉

OSBotify · 2021-07-30T22:28:57Z

🚀 Deployed to staging in version: 1.0.81-7🚀

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

isagoico · 2021-07-31T17:46:41Z

@kidroca Hello! Any QA tests needed for this PR?

kidroca · 2021-07-31T18:52:19Z

@isagoico No specific test steps other than basic regressions as translations are used everywhere.
Switching the app language should work

nkuoch · 2021-08-02T09:52:47Z

@kidroca this PR created this regression.
Seems like it was pointed out before the merge though. Not sure why we merged it regardless and what was decided about it though? cc @marcaaron

kidroca · 2021-08-02T13:24:28Z

@kidroca this PR created this regression.
Seems like it was pointed out before the merge though. Not sure why we merged it regardless and what was decided about it

The referenced comment was made for the logic in the initial PR, since then the code changed quite a lot
But debugging this I see that even though the Context rerenders it's children does not because they don't receive anything new

We can fix this by passing preferredLocale down. This will trigger a re-render in child components when the locale changes

I see a new ticket have been opened: #4359
Are we going to handle this as a continuation PR that fixes the regression?
Or should the other ticket have to be triaged and go through the full process of proposing and accepting a solution?

IMO, since it was tracked it to this PR, it would be easier and faster to address here by re-open the existing issue

iwiznia · 2021-08-02T13:30:39Z

Are we going to handle this as a continuation PR that fixes the regression?

IMO yes, because it's a feature that worked and was broken by this PR.

parasharrajat · 2021-08-02T13:49:10Z

@kidroca Please close that Issue when you submit a fix via the PR. That issue was created for tracking the problem.

kidroca · 2021-08-02T13:51:52Z

Shouldn't I be assigned to the new ticket as well then?

parasharrajat · 2021-08-02T13:56:05Z

Is that any advantage of assigning an issue? Asking in general.

iwiznia · 2021-08-02T13:59:49Z

It makes it easier to find things if they are assigned to the people that worked on them. It is also used in some of our internal tooling to for example assign reviewers for issues or PRs.

marcaaron · 2021-08-02T16:05:14Z

Not sure why we merged it regardless and what was decided about it though?

Oh, hmm, we merged it because the implementation was entirely different. In short, we didn't suspect the new one would have the same issue.

OSBotify · 2021-08-06T02:43:10Z

🚀 Deployed to production in version: 1.0.82-7🚀

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

iwiznia previously approved these changes Jul 27, 2021

View reviewed changes

Julesssss reviewed Jul 27, 2021

View reviewed changes

kidroca mentioned this pull request Jul 28, 2021

[Performance] Onyx usage in withLocalize adds a significant overhead #4268

Closed

kidroca mentioned this pull request Jul 29, 2021

[Hold for payment 2021-08-17] [Performance] Optimize AsyncStorage on Android #2667

Closed

feat: create LocaleContext and LocaleContextProvider

72800fc

kidroca dismissed iwiznia’s stale review via 72800fc July 30, 2021 20:02

kidroca force-pushed the kidroca/locale-perf-fix branch from 6bf4ff2 to 72800fc Compare July 30, 2021 20:02

kidroca marked this pull request as ready for review July 30, 2021 21:02

kidroca requested a review from a team as a code owner July 30, 2021 21:02

kidroca requested review from marcaaron and Julesssss and removed request for a team July 30, 2021 21:02

MelvinBot requested a review from aldo-expensify July 30, 2021 21:02

kidroca requested a review from iwiznia July 30, 2021 21:02

marcaaron approved these changes Jul 30, 2021

View reviewed changes

marcaaron merged commit 8a958f2 into Expensify:main Jul 30, 2021

OSBotify mentioned this pull request Jul 30, 2021

Deploy Checklist: Expensify.cash 2021-07-30 #4333

Closed

100 tasks

parasharrajat mentioned this pull request Aug 2, 2021

Desktop - Chat - Timezone information not updated after language change #4352

Closed

kidroca deleted the kidroca/locale-perf-fix branch August 2, 2021 13:25

kidroca mentioned this pull request Aug 2, 2021

Fix: Selecting a language does not update in real time #4366

Merged

5 tasks

aman-atg mentioned this pull request Aug 4, 2021

Timestring is not being properly set on Refreshing the tab when the Language is English #4404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf: remove Onyx usage from `withLocalize` #4253

Perf: remove Onyx usage from `withLocalize` #4253

kidroca commented Jul 27, 2021 •

edited

Loading

iwiznia left a comment

parasharrajat commented Jul 27, 2021

Julesssss Jul 27, 2021

marcaaron Jul 27, 2021

Julesssss commented Jul 27, 2021

iwiznia commented Jul 27, 2021

kidroca commented Jul 27, 2021

iwiznia commented Jul 27, 2021

kidroca commented Jul 27, 2021

marcaaron commented Jul 27, 2021

marcaaron commented Jul 27, 2021

kidroca commented Jul 27, 2021

marcaaron commented Jul 27, 2021

kidroca commented Jul 28, 2021

kidroca commented Jul 30, 2021 •

edited

Loading

marcaaron left a comment

marcaaron Jul 30, 2021

marcaaron commented Jul 30, 2021

OSBotify commented Jul 30, 2021

isagoico commented Jul 31, 2021

kidroca commented Jul 31, 2021

nkuoch commented Aug 2, 2021

kidroca commented Aug 2, 2021

iwiznia commented Aug 2, 2021

parasharrajat commented Aug 2, 2021 •

edited

Loading

kidroca commented Aug 2, 2021

parasharrajat commented Aug 2, 2021

iwiznia commented Aug 2, 2021

marcaaron commented Aug 2, 2021

OSBotify commented Aug 6, 2021

Perf: remove Onyx usage from withLocalize #4253

Perf: remove Onyx usage from withLocalize #4253

Conversation

kidroca commented Jul 27, 2021 • edited Loading

Details

Fixed Issues (Related To)

Tests

My Benchmarks

Before

After

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

iwiznia left a comment

Choose a reason for hiding this comment

parasharrajat commented Jul 27, 2021

Julesssss Jul 27, 2021

Choose a reason for hiding this comment

marcaaron Jul 27, 2021

Choose a reason for hiding this comment

Julesssss commented Jul 27, 2021

iwiznia commented Jul 27, 2021

kidroca commented Jul 27, 2021

iwiznia commented Jul 27, 2021

kidroca commented Jul 27, 2021

marcaaron commented Jul 27, 2021

marcaaron commented Jul 27, 2021

kidroca commented Jul 27, 2021

marcaaron commented Jul 27, 2021

kidroca commented Jul 28, 2021

kidroca commented Jul 30, 2021 • edited Loading

class LocaleContextProvider

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Jul 30, 2021

Choose a reason for hiding this comment

marcaaron commented Jul 30, 2021

OSBotify commented Jul 30, 2021

isagoico commented Jul 31, 2021

kidroca commented Jul 31, 2021

nkuoch commented Aug 2, 2021

kidroca commented Aug 2, 2021

iwiznia commented Aug 2, 2021

parasharrajat commented Aug 2, 2021 • edited Loading

kidroca commented Aug 2, 2021

parasharrajat commented Aug 2, 2021

iwiznia commented Aug 2, 2021

marcaaron commented Aug 2, 2021

OSBotify commented Aug 6, 2021

Perf: remove Onyx usage from `withLocalize` #4253

Perf: remove Onyx usage from `withLocalize` #4253

kidroca commented Jul 27, 2021 •

edited

Loading

kidroca commented Jul 30, 2021 •

edited

Loading

parasharrajat commented Aug 2, 2021 •

edited

Loading