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

RichText: Do not run valueToEditableHTML on every render #12460

Merged
merged 1 commit into from Nov 30, 2018

Conversation

Projects
None yet
2 participants
@iseulde
Copy link
Member

iseulde commented Nov 30, 2018

Description

Currently we are calling valueToEditableHTML, which creates HTML from a RichText value, on every RichText render call, while this should only every be called once on every mount, to set the initial HTML in the TinyMCE component.

I think this removes about 3.5-4% of the work done on every key press. Note that it does not only affect key presses, but renders in general. So this function will be called a lot more than needed.

How has this been tested?

Screenshots

Types of changes

Checklist:

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

@iseulde iseulde added this to the 4.7 milestone Nov 30, 2018

@iseulde iseulde requested review from youknowriad , aduth and WordPress/gutenberg-core Nov 30, 2018

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Nov 30, 2018

Slightly related: #12402.

@iseulde iseulde changed the title Do not run valueToEditableHTML on every render RichText: Do not run valueToEditableHTML on every render Nov 30, 2018

@@ -874,7 +875,8 @@ export class RichText extends Component {
tagName={ Tagname }
onSetup={ this.onSetup }
style={ style }
defaultValue={ this.valueToEditableHTML( record ) }
record={ record }

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

mmm, wouldn't this have the same effect than before?

This is what I see:

  • the record value is created on every RichText render by the getRecord function.
  • the getRecord function returns a new object every time is called.
  • on every RichText render, the TinyMCE component will receive a new object reference for the record prop (although values may be the same, I haven't looked at what this function does), so it'll be re-rendered.

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

oh, I just saw that the TinyMCE component defines a shouldComponentUpdate that always returns false. So what this PR is trying to avoid is to actually call valueToEditableHTML in every RichText render, not re-renders of the child component.

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Right, valueToEditableHTML will only be called on mount there.

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

So, my understanding is that this would reduce the time it takes RichText to execute in every render/commit phase. I've tested this by:

  • Opened the Gutenberg demo.
  • In the first paragraph introduced the text "Howdy. ".

These are the results in master:

peek 2018-11-30 14-03-master-rich-text

These are the results in this branch:

peek 2018-11-30 13-50-branch-rich-text

Perhaps is difficult to observe in the GIFs, but it looks like RichText takes ~0,5ms to render in every keystroke in both master and this branch. I don't see at the moment which other tests I could do to effectively measure the reduction of the performance budget introduced by this branch.

Nevertheless, doing less work is always better! So, I think it's fine to merge this.

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

Actually, within RichText, the more expensive part seems to be the FormatEdit component.

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

A new record being created? Which function exactly?

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

I'm from mobile now.

This is from React Developer Tools' new profiler (to be found in the React tab). You can navigate every commit and components that were updated will be colored (bluish to yellow, yellow the ones that took the most time).

The getRecord method in RichText does:

getRecord(){
    const { formats, text } = this.formatToValue( this.props.value );
    const { start, end } = this.state;

    return { formats, text, start, end };
}

essentially returning a new object no matter whether formats, text, start, end values have been updated or not.

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Where does the result of getRecord get compared?

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Oh, I see now. Good catch.

This comment has been minimized.

@nosolosw

nosolosw Nov 30, 2018

Member

My understanding is that React does a shallow comparison of the props passed to the components to decide whether a particular component needs to be re-rendered. We have two mechanisms to avoid re-rendering: 1) always pass the same prop (in the case of arrays and objects, the same reference) or 2) hook into shouldComponentUpdate to do that process ourselves.

@nosolosw nosolosw self-requested a review Nov 30, 2018

@nosolosw
Copy link
Member

nosolosw left a comment

I see some e2e test failing, but they seem unrelated (may I have heard somewhere that they're now failing for every branch due to a Travis issue?).

This works properly and the code is fine, so giving you a 👍 dependant on e2e tests passing.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Nov 30, 2018

@nosolosw There are two render calls on every key press. In the picture below you see one selected, one a bit further on. Removing these calls speeds up the render call significantly. Overall it has a smaller impact on the whole work per key press though.

screen shot 2018-11-30 at 14 25 10

@iseulde iseulde force-pushed the fix/rich-text-render branch from 6d4e876 to 9250199 Nov 30, 2018

@iseulde iseulde force-pushed the fix/rich-text-render branch from 9250199 to a3326a4 Nov 30, 2018

@iseulde iseulde merged commit 4208c76 into master Nov 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the fix/rich-text-render branch Nov 30, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019

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