1644202 - [Translate] Typing in the editor has poor performance

Status: RESOLVED FIXED

(Webtools Graveyard :: Pontoon, defect, P3)

Description

[Itiel]

This isn't something new (started about a year ago, I think), I just got used to it at some point.
Essentially typing in the editor takes 3-4 seconds for the text to fill up, and that's when I'm even not fast typing.

A profile taken on a i7-6700 machine with 16GB RAM and a 860 Pro SSD, when typing and having the slowness:
https://share.firefox.dev/2YglkW6

Seeing that I don't always see this happening, maybe having a very long list of visible strings may have something do to with it. I'm viewing all of the missing translations for Firefox (Hebrew locale) and scrolling all the way to the bottom (so about ~760 strings atm). Then I can really feel the slowness when typing in one of the strings.

Comment 1

[Matjaz Horvat [:mathjazz]]

CC Adrian, because this is related to his Editor work.

Comment 2

[Francesco Lodolo [:flod]]

I never noticed any issue while using Pontoon, but this link is really slow for me too (provided on Matrix by Itiel).

Scroll a few times in the list of strings, then try typing
https://pontoon.mozilla.org/he/firefox/all-resources/?status=missing&string=208083

Comment 3

[Adrian Gaudebert]

I've identified the source of the slowness of the editor: it comes from our usage of the unsavedchanges module. Whenever we change the content of the editor, we update unsavedchanges. Whenever that changes, we re-render almost every component on the page. That means that on each key stroke in the editor, a full render is triggered, which leads to really poor performance.

We can solve this by changing the way we look at unsavedchanges data. I'm going to solve this problem as part of bug 1646010.

Comment 4

[Matjaz Horvat [:mathjazz]]

As part of bug 1646010 we landed some performance improvements.

Itiel, do you notice any difference in responsiveness?

Comment 5

[Itiel]

Yes, a great deal. Thanks!
The only remaining junkiness I notice is (as usual, when the scroll list is very long) the transition from the placeholder to a typed text (in the first letter), or from any typed text to the placeholder (e.g. when you type just "a" and then remove it, or when you Ctrl+A and delete all text).
Everything inbetween is snappy.

Comment 6

[Matjaz Horvat [:mathjazz]]

Happy to hear!

I'll keep the bug open to address the remaining issue.

Comment 7

[Julen Ruiz Aizpuru]

Adrian, could you please assign the bug to me? I'd like to fix the remaining issues.

Comment 8

[Julen Ruiz Aizpuru]

The only remaining junkiness I notice is (as usual, when the scroll list is very long) the transition from the placeholder to a typed text (in the first letter), or from any typed text to the placeholder (e.g. when you type just "a" and then remove it, or when you Ctrl+A and delete all text).
Everything inbetween is snappy.

In addition to this, I also experience sluggishness when switching tabs from Pontoon and/or coming back (OS X's beachball shows up frequently).

I have captured and analyzed some profile snapshots (using production builds, and with browser extensions disabled), both with the browser's built-in and React devtools' profilers, and it indeed displays some redundant activity in two different phases at least:

1. When users click anywhere
2. When the contents of the editing textarea are changed from/to initial values.

Clicking anywhere

Every element making use of react-clickoutsde via the handleClickOutside handler function forcedfully re-renders. While arguably some of the re-renders could be seen as not causing harm, the reality is everything adds up, and in some cases the re-renders also have other side-effects that delay things even more (e.g. updateFiltersFromURLParams in FiltersPanel takes ~3ms on every click in my machine).

The affected components could be made 'pure', i.e. make them shallow-compare props and state to determine if they should re-render. Another option is reworking the existing implementation of components relying on click-outside behavior so that the click-outside-specific behavior only activates when popover/modal/layered elements are rendered. I'm more fond of this latter solution because it avoids the extra renders by design, and not by the virtue of implementing a framework escape hatch.

Changing textarea contents from/to initial values

This is related to the unsavedchanges module, and more specifically its state's exist and ignored members being updated and read as unsavedChangesExist and unsavedChangesIgnored from the redux store.

This is causing re-renders even if the value of the state isn't being used for rendering purposes (they are used for parameters in a callback). Reading such values directly from the store at invoke time should suffice, i.e. without any component-level subscriptions.

More redundant re-renders

Apart from these conflictive phases, more component re-renders happen when typing takes place — this is easy to observe when enabling the Highlight updates when components render react-devtools option. These affect the Helpers components (Machinery / Other Locales), so probably some hook is retrieving extra data that forces re-renders.

Another redundant re-render happens at 2 minute intervals, when /user-data/ is retrieved. Even if no data changes, the entire app is re-rendered.

Comment 9

[Julen Ruiz Aizpuru]

As I start to work on these specific problems I see the code changes will be significant in some cases, so I'll file separate blocker bugs for the three points that I detailed in my last comment. That way we'll be able to better track each change's impact.

Comment 10

[Adrian Gaudebert]

Thanks for taking care of these issues Julen! Feel free to assign me for reviews (and do not hesitate to ping me on matrix, as I might not pay attention to github notifications… ).

Comment 11

[Julen Ruiz Aizpuru]

With the dependent bugs fixed, and while there's still room for improvement (as indicated in my last PR), the state of things might already be good enough to close this bug. Nonetheless, it'd be great to verify this with the people who has seen considerable delays (Itiel, Jordi) if this is now working well and/or to which extent.

Matjaz, when could we see the latest changes live in production so that everyone can verify on their end?

Comment 12

[Matjaz Horvat [:mathjazz]]

Good point, Julen - I was also wondering whether we should close the bug now. We don't deploy to prod on Fridays, so changes are expected to go live on Monday.

In the meantime, I've pushed the latest master to stage:
https://mozilla-pontoon-staging.herokuapp.com/projects/firefox/all-resources/

Itiel, could you take it for a spin?

Comment 13

[Itiel]

I tested (in Saturday) both the staging server and the normal Pontoon server and both seem to work well.

Comment 14

[Matjaz Horvat [:mathjazz]]

Thanks, Itiel!

And well done, Julen!

Resolving as FIXED. 🎉