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

Speed improvements for Input Example #2280

Closed

Conversation

SmilinBrian
Copy link
Contributor

I am finding the Input Tester example very useful while diagnosing issues with composition input. However, once you type for a while things slow down to a crawl as the huge table of logged events gets re-rendered.

Optimize by:

  • Store JSX row components instead of the data needed to build each row.
  • Limit to showing only the 150 most recent events.

PR includes a change that removes the event listeners when the example is unmounted. While it can sometimes be handy to have input events sent to the console log while typing in other examples, it occasionally throws exceptions on selection events, so it's safer to stop listening.

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn prettier.)
  • The relevant examples still work. (Run examples with yarn watch.)

Otherwise we keep logging events in all the other Slate Examples, and sometimes hit exceptions on selection change events.
Store JSX row components instead of the data needed to build each row.
Limit to showing only the 150 most recent events.
@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #2280 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2280   +/-   ##
=======================================
  Coverage   83.59%   83.59%           
=======================================
  Files          42       42           
  Lines        4103     4103           
=======================================
  Hits         3430     3430           
  Misses        673      673

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53fd59b...7ff3871. Read the comment docs.

@ericedem
Copy link
Contributor

@SmilinBrian Won't limiting the amount of events tracked prevent the repeat column from working correctly?

Playing locally, one easy win for performance is to change Event from a functional component to a PureComponent which prevents it from ever getting rerendered (I think this will have the same performance as storing the JSX locally?). Though this would break with the sliding window approach you are using.

Just an idea, but another improvement might be to use pagination to limit how many items get rendered at a time. Though that is going to start complicating the example.

@SmilinBrian
Copy link
Contributor Author

Re: the repeat column: Key events arrive with the repeat flag, it's not based on any examination of "what came before".

Re: rendering and optimization: Yes, I considered those approaches, but I think PureComponent would not solve it based on the way the ArrayValue renders, and agree that pagination/windowing would make the example too complicated. Using the "Highlight Updates" flag in React Dev Tools the event list appears to update much less than before the change.

@ianstormtaylor
Copy link
Owner

Storing the pre-instantiated JSX feels weird to me. Can we just limit the list to 50 or 100 and call it a day?

@SmilinBrian
Copy link
Contributor Author

I think limiting it to 50 would solve the sluggishness and leave the code more obvious. Meanwhile, if you somehow need more than 50 messages, this example is also dumping everything to the console log.
I'll amend the PR.

@ianstormtaylor
Copy link
Owner

As of #3093 (which was just merged), I believe this issue is no longer applicable, because a lot has changed. I'm going through and closing out any potential issues that are not out of date with the overhaul. Thanks for understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants