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

TextStorage in RCTText is now only set when it differs from old value. #6341

Closed
wants to merge 1 commit into from
Closed

TextStorage in RCTText is now only set when it differs from old value. #6341

wants to merge 1 commit into from

Conversation

bpoetzschke
Copy link
Contributor

Motivation
Multiple instances of Text inside a ListView 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 the RCTText element, will set more than 1,000 times and also the method setNeedsDisplay is called multiple times. This will causes huge memory problems and the app crashes.

With this commit I check in RCTText if the NSTextStorage differs from the old value. If yes then set it otherwise don't set the NSTextStorage. This will prevent to call setNeedsDisplay when not really needed.

Gist with sample app to show behavior can be found here: https://gist.github.com/bpoetzschke/28a17969c6aa54219e18

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @nicklockwood, @sahrens and @a2 to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 7, 2016
@@ -74,8 +74,10 @@ - (void)removeReactSubview:(UIView *)subview

- (void)setTextStorage:(NSTextStorage *)textStorage
{
_textStorage = textStorage;
[self setNeedsDisplay];
if (_textStorage!=textStorage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces

@ide
Copy link
Contributor

ide commented Mar 7, 2016

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.

@facebook-github-bot
Copy link
Contributor

@bpoetzschke updated the pull request.

To fix multiple draw calls with the same text storage we check here if the _textStorage has been changed.
@mroswald
Copy link
Contributor

If i understand correctly, changing RCTText.m::setTextStorage to this:

- (void)setTextStorage:(NSTextStorage *)textStorage
{
  if ( _textStorage != textStorage ) {
    _textStorage = textStorage;
    [self setNeedsDisplay];
  } else {
    NSLog(@"we saved cpu! text not changed: %@", _textStorage);
  }
}

should show you the savings. Here is a small sample app:

'use strict';
import React, {
  AppRegistry,
  Component,
  StyleSheet,
  ListView,
  Text,
  View
} from 'react-native';

class TextPerformance extends Component {
    constructor(props) {
        super(props);
        let randomGeneratedList = [];
        for(let i=0;i<10000;i++) {
            randomGeneratedList.push(Math.random() * 1000);
        }

        this._ds = new ListView.DataSource({
            rowHasChanged: (r1, r2) => r1 !== r2
        });
        this.state = {
            dataSource: this._ds.cloneWithRows(randomGeneratedList)
        };
    }

    static renderItem(item) {
        return (
            <Text>{item}</Text>
        );
    }

    render() {
        return (
            <View style={styles.container}>
                <ListView
                    dataSource={this.state.dataSource}
                    renderRow={TextPerformance.renderItem}
                />   
            </View>         
        );
    }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  }
});

AppRegistry.registerComponent('TextPerformance', () => TextPerformance);

@nicklockwood
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 1832d79 Mar 10, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants