Try: Reduce impact of text input on data store by throttling changes.

[sgomes]

Note: This is a trial PR where I am experimenting with what I believe is a good approach to reducing input lag when typing.

It appears to work correctly, but it may need extra work before it's ready to merge. It also may not be capitalizing on every opportunity for improvement.

This is mostly an early PR for early feedback on the approach.

Description

Currently, when typing, every keystroke immediately sends a synchronous update to the Redux data store. This has a negative impact on performance in larger documents.

This PR throttles the dispatching of changes to the data store, while updating the text input component immediately. The end result is that there can be a small delay between a user typing
text and the block details in the data store reflecting the updated content, but the onscreen component will display the new text with no delay.

This improves responsiveness and reduces the overall amount of CPU work performed by the application if the user types faster than the throttling period, by effectively batching multiple changes together.

An alternative approach would be to use debouncing instead of throttling. That would produce more consistent performance while typing and reduce the amount of work even further, but would increase time between updates to an arbitrarily large number, depending on how long the user can maintain a fast rate of typing. This may or may not be acceptable depending on project goals; some domain-specific knowledge would be useful in picking the right approach here.

How has this been tested?

Unit tests and ad-hoc usage testing. The change only affects text input, so all the ad-hoc testing was done by inputting text and analysing how often the updates were triggered, whether they were triggered correctly, and whether the resulting final state was valid.

In terms of performance testing, it's difficult to measure accurately, but a frame-by-frame inspection of continuous typing seems to indicate that there are now laggy periods every 1 second with regular visual updates in between, rather than large intervals without any visual updates, as previously. As expected, this PR doesn't appear to change how long it takes to respond to an input event, but only how often the application has to do so.

Types of changes

Performance improvement.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

@youknowriad: I added you as the only reviewer, but please feel free to add anyone else you think may be relevant.

[ellatrix]

Why, considering the amount of performance improvements we have?

[sgomes]

@iseulde I'm mostly looking for feedback, this is in no way something I see as being mandatory. It depends on how far the other performance fixes get the project, taking into account slower CPU devices, of course.

[ellatrix]

I measured the performance of some unmerged performance PRs (https://github.com/WordPress/gutenberg/tree/try/combined-performance-improvments) with the 4.6 release, and key presses are on average 3.4x faster. This is excluding your state tree splitting PR.

[ellatrix]

The measurement was done with #10418 (comment) and 8 key presses in the first paragraph. I measured both the key press event and the selection change event. The browser is Chrome, no CPU slowdown, React production. Average before: 79.32ms. Average after: 23.62ms.

[ellatrix]

And I think with a bit more effort, we can do even better.

[sgomes]

Great, those are some excellent improvements indeed! If that produces sufficient performance for your baseline devices, then I completely agree: the less code complexity, the better.

After chatting with @youknowriad, I am now aware of previous discussions in the team regarding throttling and the concerns around data consistency, and I completely understand. It's a useful technique but it has to be implemented carefully. I'll keep the PR open as a reference for now, if anyone wants to take a look, but feel free to close at any point.

[SchneiderSam]

The measurement was done with #10418 (comment) and 8 key presses in the first paragraph. I measured both the key press event and the selection change event. The browser is Chrome, no CPU slowdown, React production. Average before: 79.32ms. Average after: 23.62ms.

My content is not comparable to this content. If we speak of long content, then I think that this content is representative. Not mine. And yes, I also have professional articles with about +6000 words.

Please use the content of @florianbrinkmann, not mine. I think it's more meaningful. Thanks for working on this. You are super!!!

[ellatrix]

Thanks, I’ll test with that content too.

…

On Thu, 6 Dec 2018 at 01:56, SchneiderSam ***@***.***> wrote: The measurement was done with #10418 (comment) <#10418 (comment)> and 8 key presses in the first paragraph. I measured *both* the key press event and the selection change event. The browser is Chrome, no CPU slowdown, React production. Average before: 79.32ms. Average after: 23.62ms. My content is not comparable to this content <#11782 (comment)>. If we speak of long content, then I think that this content <#11782 (comment)> is representative. Not mine. And yes, I also have professional articles with about +6000 words. Please use the content of @florianbrinkmann <https://github.com/florianbrinkmann>, not mine. I think it's more meaningful. Thanks for working on this. You are super!!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12592 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEfg603z_I9xbAPAwZte0LBXE8A2EhHdks5u2GslgaJpZM4ZBHtr> .

[gziolo]

I'll keep the PR open as a reference for now, if anyone wants to take a look, but feel free to close at any point.

I think we were using this technique in the past and it had its own set of issues. Thank you @sgomes for spending your time investigating another approach to this topic. I trust the judgment of @youknowriad shared in your comment and I can only confirm that this is something that was discussed several times in the past. It's nice to still have this PR as a future reference. I'm closing this PR as won't fix.