-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
TextStorage in RCTText is now only set when it differs from old value. #6341
TextStorage in RCTText is now only set when it differs from old value. #6341
Conversation
By analyzing the blame information on this pull request, we identified @nicklockwood, @sahrens and @a2 to be potential reviewers. |
@@ -74,8 +74,10 @@ - (void)removeReactSubview:(UIView *)subview | |||
|
|||
- (void)setTextStorage:(NSTextStorage *)textStorage | |||
{ | |||
_textStorage = textStorage; | |||
[self setNeedsDisplay]; | |||
if (_textStorage!=textStorage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces
This looks correct in isolation to me. cc @nicklockwood can you sanity check this? And @tadeuzagallo you might want to profile this to understand the performance improvement. |
@bpoetzschke updated the pull request. |
To fix multiple draw calls with the same text storage we check here if the _textStorage has been changed.
If i understand correctly, changing RCTText.m::setTextStorage to this:
should show you the savings. Here is a small sample app:
|
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to Phabricator to review. |
1832d79
Motivation
Multiple instances of
Text
inside aListView
is a bad idea for the performance of the app.When you create 1000 elements and you scroll through the list it is really slow and laggy because the
NSTextStorage
, which is the backbone of theRCTText
element, will set more than 1,000 times and also the methodsetNeedsDisplay
is called multiple times. This will causes huge memory problems and the app crashes.With this commit I check in
RCTText
if theNSTextStorage
differs from the old value. If yes then set it otherwise don't set theNSTextStorage
. This will prevent to callsetNeedsDisplay
when not really needed.Gist with sample app to show behavior can be found here: https://gist.github.com/bpoetzschke/28a17969c6aa54219e18