Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Dec 4, 2018

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.

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 reflecting the updated content, but
the onscreen component will display the new text immediately.

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.
@sgomes sgomes added [Type] Performance Related to performance efforts [Package] Editor /packages/editor User Experience (UX) labels Dec 4, 2018
@sgomes sgomes requested a review from youknowriad December 4, 2018 17:15
@ellatrix
Copy link
Member

ellatrix commented Dec 4, 2018

Why, considering the amount of performance improvements we have?

@sgomes
Copy link
Contributor Author

sgomes commented Dec 4, 2018

@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
Copy link
Member

ellatrix commented Dec 4, 2018

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
Copy link
Member

ellatrix commented Dec 4, 2018

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
Copy link
Member

ellatrix commented Dec 4, 2018

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

@sgomes
Copy link
Contributor Author

sgomes commented Dec 4, 2018

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
Copy link

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
Copy link
Member

ellatrix commented Dec 6, 2018 via email

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

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.

@gziolo gziolo closed this Feb 6, 2019
@gziolo gziolo deleted the perf/throttle-editor-user-input branch February 6, 2019 11:47
@gziolo gziolo added the [Status] Not Implemented Issue/PR we will (likely) not implement. label Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants