Skip to content

RichText: Do not run valueToEditableHTML on every render #12460

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.

Sign up for GitHub

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

Merged
ellatrix merged 1 commit into from
Nov 30, 2018
Merged

RichText: Do not run valueToEditableHTML on every render #12460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export class RichText extends Component {
this.valueToFormat = this.valueToFormat.bind( this );
this.setRef = this.setRef.bind( this );
this.isActive = this.isActive.bind( this );
this.valueToEditableHTML = this.valueToEditableHTML.bind( this );

this.formatToValue = memize( this.formatToValue.bind( this ), { size: 1 } );

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

@oandregal oandregal Nov 30, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oandregal oandregal Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ellatrix ellatrix Nov 30, 2018

Choose a reason for hiding this comment

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

Right, valueToEditableHTML will only be called on mount there.

Copy link
Member

@oandregal oandregal Nov 30, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oandregal oandregal Nov 30, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ellatrix ellatrix Nov 30, 2018

Choose a reason for hiding this comment

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

A new record being created? Which function exactly?

Copy link
Member

@oandregal oandregal Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ellatrix ellatrix Nov 30, 2018

Choose a reason for hiding this comment

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

Where does the result of getRecord get compared?

Copy link
Member Author

@ellatrix ellatrix Nov 30, 2018

Choose a reason for hiding this comment

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

Oh, I see now. Good catch.

Copy link
Member

@oandregal oandregal Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

valueToEditableHTML={ this.valueToEditableHTML }
isPlaceholderVisible={ isPlaceholderVisible }
aria-label={ placeholder }
aria-autocomplete="list"
Expand Down
5 changes: 3 additions & 2 deletions packages/editor/src/components/rich-text/tinymce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,8 @@ export default class TinyMCE extends Component {
const {
tagName = 'div',
style,
defaultValue,
record,
valueToEditableHTML,
className,
isPlaceholderVisible,
onPaste,
Expand Down Expand Up @@ -372,7 +373,7 @@ export default class TinyMCE extends Component {
ref: this.bindEditorNode,
style,
suppressContentEditableWarning: true,
dangerouslySetInnerHTML: { __html: defaultValue },
dangerouslySetInnerHTML: { __html: valueToEditableHTML( record ) },
onPaste,
onInput,
onFocus: this.onFocus,
Expand Down