High Performance impact when using a lot of HOCs

[beheh]

In my app we've noticed a heavy overall performance regression after switching to i18next with react-i18next. In our configuration we're using client-side rendering with asynchronously supplied translation files and a key-based fallback (also using ICU).

After some investigating, it looks like on one page where we use a few nested HOCs, loading the last part of the translations results in a very bad behaviour from react-i18next:

Note how each of the HOCs separately reacts to the load completion, firing onI18nChanged which does a setState call (using new Date() to force a rerender).

Unfortunately, each of these single setState calls seems to result in a full rerender of our application, as React schedules and then commits the work. While rerendering isn't super expensive if we do it dozens of times this adds up to a lot of wasted time (which we actually see in a few extra seconds on page load!).
I think the main issue here is that each HOC (which wraps an I18n component around our component) registers itself with it's own callback that is executed separately and not batched in any way.

I don't exactly know how this could be handled differently, but there must be a more efficient way. Possibly we could take a look at Redux/Flux and see how they commit state changes that happen in a lot of places at once?

[beheh]

I was thinking of maybe just letting I18NextProvider rerender instead of having each HOC do it for itself, but that unfortunately means that any shouldComponentUpdate above a HOC'd component would block that child component from rerendering when the language changes or is loaded, so that seems to be quite a significant drawback.

[beheh]

It looks like setState should be asynchronous and batching by default. Possibly React is unable to do this due correctly due to the whole "defer until initialized" thing?

react-i18next/src/I18n.js

Lines 69 to 77 in 844e7dd

	const initialized = () => {
	// due to emitter removing issue in i18next we need to delay remove
	setTimeout(() => {
	this.i18n.off('initialized', initialized);
	}, 1000);
	ready();
	};
	this.i18n.on('initialized', initialized);

[jamuhl]

Thank you for the time you take to investigate that. Currently i got no ideal idea to solve this.

Eventual reducing the number of used hocs would help (what we do is using hocs on page level components and from there pass the t function down to components in children)

plus try changing bindI18n, bindStore for inner components -> but like you said might have the draw back of not rerender inner children deeper the tree using shouldComponentUpdate

[beheh]

I had another thought today: Instead of each I18n (HOC) component doing it's own state management using this new Date() call to force a rerender, they could just be updating themselves based on context changes. For example, whenever a translation loads, the t function could be replaced in the context, thus forcing a rerender.

Especially if react-i18next migrates to the new context API this would work well, as that API is no longer affected by shouldComponentUpdate in subtrees (and just works by updating the affected part of the component tree).

I'd like to avoid having to get rid of th HOCs, as they feel like a way for each of our components to translate themselves in isolation, as opposed to relying on passing t props all around the place.

[jamuhl]

hm...the t function gets already recreated on change: https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L101 and passed down in hoc: https://github.com/i18next/react-i18next/blob/master/src/translate.js#L72

[beheh]

It does get recreated but on a per-component basis. I was thinking of having a single version of the t function at the very top (I18nextProvider) that is updated there and only there (as the single source of truth).

[jamuhl]

Hm, the provider does only provide the i18n instance.

But i guess we might do some mode in the hoc to not use it's own t function but the one finding on context...

So there could be one hoc as direct child of the provider using as is and passing down t on context which is picked up by the others deeper down the tree.

But that will remove the support of having a default namespace set and loaded on the hocs deeper down....idk

[tavurth]

Even something as simple as the following would be helpful.

const CURRENT_TRANSLATIONS = {};

function translate() {} // stub
function checkForRenderUpdates() {} // stub

let CURRENT_TRANSLATE = translationId => translate(translationId)

function setTranslations(...translations) {
   const newTranslations = {};

   // check to see if translation or namespaces already loaded 
   // (or in the default namespace)
   check()

   for (let translationId in translations) {
       // merge namespaces
   }

   CURRENT_TRANSLATE = generateTranslator(newTranslations);

   checkForRenderUpdates();
   return newTranslations
}

function getTranslate() {
    return CURRENT_TRANSLATE;
}

function translate(translationId) {
    return getTranslate()(translationId);
}

translateHOC(Component) {
    render() {
        <Component t={getTranslate()} />
    }
}

export default { getTranslate, translate };

We've moved from react-intl to react-i18next and have found that when using namespaces our app slows down for 2+ seconds while re-rendering multiple translated components after namespace load completion.

At the moment I'm loading all translations in at the beginning as namespaces seem to have a huge performance overhead.

However, when later specifying a namespace, 'orders', or 'prices', react-i18next will load the translations for that namespace again from the server, even if they've been loaded into the default namespace.

This prevents depreciation of older components and forces one to disable all react-i18next namespaces, removing the main reason for the migration.

The affected components can be seen in this profile block, which takes 1 second for 15 components, of which the translations have already been loaded into the default namespace:

[jamuhl]

@tavurth the problem is home made -> sure it delays rendering when you're loading namespaces via xhr -> so you will need to decide which namespaces you want to have in a lazy load namespace -> eg. stuff you need only on special usecases -> everything else should be loaded on init - or after rendering of eg. dashboard.

For your own solution -> do not merge namespaces into one -> push them into the resource store using the API available: https://www.i18next.com/overview/api#resource-handling

There is also no difference in access speed: namespaces are just a nesting construct in the resource store -> lng.namespace.key.some.nested.structure.to.lookup

Every call to t does nothing than use the namespace given t('namespace:key') or using the defined defaultNS -> the hoc does nothing else as creating a t function that overrides that defaultNS

[tavurth]

Ah I see, so the proper depreciation workflow is as follows?

languages.forEach(lng =>
  loadNamespaces(lng).then(namespaces =>
    namespaces.forEach(ns => i18next.addResources(lng, ns, namespaces[ns]))
  )
);

[jamuhl]

depends loadNamespaces is referring to https://github.com/i18next/react-i18next/blob/master/src/loadNamespaces.js ?!? if so that already calls https://www.i18next.com/overview/api#loadnamespaces

if that is something custom returning translations -> then i would guess yes

[jeznag]

Does using render props obviate the performance issue reported here?

[jamuhl]

@jeznag @beheh i guess no: https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L102 seems to be a problem enforcing rerender on each component nested inside.

But like said - personally i never run into noticeable performance issues on our projects - and those are not small at all. But we do not have very deep component trees, mostly just container component -> some richer components -> pure components (so at max 2-3 translate hocs nested)

[jamuhl]

@beheh an isolated test case for reproducing this behaviour would be great...as i already plan for including the new context api in upcoming version this would be awesome to assert we do not get that again.

[beheh]

I've boostrapped a quick create-react-app. as I wasn't able to come up with a clean test case. It would probably involve writing a component that counts it's own renders. Check it out here: https://github.com/beheh/react-i18next-performance

To see the issue, clone the repo, and run yarn, and yarn start. You should see your browser open. Notice how the console logs 5 initial renders, and then, once the string has loaded (using CustomCallbackBackend in src/i18n.js), another whole 50 times. This just climbs and climbs as an app has lots of components. Play around with the number of components in src/App.js to get a feeling for how fast it climbs:

• 1 component renders 6 times post-load
• 2 components render 14 times post-load
• 3 components render 24 times post-load
• 4 components render 36 times post-load
• 5 components render 50 times post-load

[jamuhl]

@beheh is there a reason you can't use the wait option?

[jamuhl]

@beheh just got an idea:

the trigger for rerender is here: https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L98 the problem eg. on load without wait is all the hocs call the loadNamespace which each will trigger a onI18nChange on all the hocs

What you think if we check what event we got and eg. for loaded only trigger a reload if the last n triggers were not the same and the namespace are relevant for that hoc (based on given namespaces to load in this hoc)?

---edit---
But also this bring some risk...after what time passed we would accept a equal event having eg. loaded same namespace again?

Another option would be to just debounce setState there -> but that would add an additional delay.

[jamuhl]

@beheh Beside setting wait to true setting those event listeners like needed would be a solution too:

		react: {
			bindStore: false,
			bindI18n: 'languageChanged'
		},

i guess you do not rely on rerenders on loaded?!?

Beside that i guess the simplest solution would be a debounced rerender on those events.

[jamuhl]

@beheh guess i found an additional solution:

Checking the ready state i saw that all those events were triggered before the "ready" render: https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L62

so adding if (!this.state.ready) return; after https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L99 will remove the unneeded initial rerenders.

But this only works for initial rerenders - having a hoc that loads a different namespace (not yet loaded) all hocs would rerender too - so only option would be setting bindI18n as needed.

So my suggestion would be creating a breaking version with:

1. defaults for being performance optimized not getting started friendly:

bindStore: false,
bindI18n: 'languageChanged'

2. add an option to debounce the onI18nChange with default 50ms -> setting it to 0 will remove debouce

3. to not change state in onI18nChange while not ready

[beheh]

@jamuhl Thanks for your thoughts!

wait: false is not really an option for us. We run a data-heavy website, and whilst strings are all over the place to annotate the data, we don't care about them being available right away. If it ever becomes too jarring in places I assume we'll just do skeleton loading.

Regarding events, debouncing, I think react-i18next should try and avoid behaving too cleverly here. In the end there is some sort of state (the available languages and namespaces) stored in the i18next, that needs to be made available in a lot of places (every Trans component, every HOC...). The React solution here is to hoist this state up as a high as needed - for Translations usually toplevel - and then have it propagate downwards. The context API(s) are a very convenient way of doing that.

The issue here is that this state is passed as a reference to the (the i18n instance) which makes figuring out whether stuff has changed quite tricky, and which led to the current state of listening to events and emitting setState({updated: new Date()}), which in turn leads to React unfortunately not batching up all those updates, but actually treating those as discrete state changes with a whole amount of rerenders (possibly React's upcoming async mode will make this a non-issue in the future).

I think one way this could be solved is by passing more primitive data structures around the place. As an extreme example, imagine you were passing an object containing the various namespaces around via the context API. Each component could then inspect this state object and decide whether any namespace it cares about has changed before comitting to a rerender. Today a very good improvement would be already to reliably detect just whether any change has occurred and only update one state (in the context provider) instead of states scattered throughout the component tree.

And finally, thanks for the suggestion bailing out if ready is false! I'll try that out in the meantime.

[jamuhl]

@beheh having slept over it i guess we have this situation of render triggered

1. initial render (if not set wait: true) - state: not ready
2. bound to languageChange, loaded, ... render - state: not ready
3. render with state: ready

The main problem is only in 2) while having rerender for individual events is ok the problem are the rerender triggered by loaded event which gets called n times (for each loadNamespace once: https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L60)

So both:

• setting wait: true
• not binding to loaded
  would be valid solutions in regular cases.

The multiple call to loaded is caused because while we deduplicate rest calls on i18next side we call 'loaded' per queued loadNamespace: https://github.com/i18next/i18next/blob/master/src/BackendConnector.js#L101 (which normally is fine - only problem currently is in react using loadNamespace to lazy load those)

So my final suggestion would be:

1. to not change state in onI18nChange while not ready (default true - can be turned off via setting)

2. In i18next we change to trigger not per queued call but completed only once

[jamuhl]

@beheh Update:

1. react-i18next@7.8.0 to not change state in onI18nChange while not ready (omitBoundRerenders)

2. i18next@11.3.5 no longer triggers event loaded more than needed (once per done loading)

Please give feedback if the situation on your app is optimized with this changes.

[beheh]

Thanks for the releases. Running a quick check still shows the same issue with 2.5 seconds of rerendering:

Note how we see the two addResourceBundle calls at the beginning of the script that each seem to trigger some renders with onI18nChanged calls, followed by huge large forEach loop and a lot of emitted events, which each in turn calls another onI18nChanged triggering a full rerender again.

As we really want to get our page translated at this point we're considering to forgo the HOCs and switch to using the new Context API in our app to expose a t from getFixedT from top-level to all components that care about it, and only update that once when everything is loaded. I'm aware that's not applicable to all use cases (especially when components/t calls might be using various namespaces). Nontheless I'm happy to help out on this, as I'd like to see the "native" react-i18next functionality to become useable for larger apps.

[jamuhl]

@beheh saying "usable for larger apps" is a little to heavty

like stated before:

• set wait: true -> which large projects tend to do...as you need the translations to avoid flickering
• set bind like needed will help too

We use react-i18next on multiple large projects.

Anyway:

using addResourceBundle use the flag silent: https://github.com/i18next/i18next/blob/master/src/ResourceStore.js#L69 to avoid emitting events

or set bindStore: false

[jamuhl]

@beheh Why you're changed to use addResourceBundle and not let use the backend for loading?

[beheh]

Sorry, I didn't mean to make that sound like react-i18next is unusable. We've been very happy with the flexibility so far! We're just running into a few snags with our specific configuration, and I'd like help to get all of those resolved.

I haven't changed usage. This must be called by the backend. Is it possible it's called because we prepopulate ns with out namespaces?

I'll still try out bindStore: false.

[beheh]

Great, after adding bindStore and bindI18n this is looking a lot better for us!

I'll see how this perfoms in production and report back.

[beheh]

Looks great, I think this issue is resolved for us! I'd like to dive in a bit more at some point to understand the events more closely and why this was happening. Either way thanks for the help!

[jamuhl]

closing this for now - if still got some issues - let me know.