remove unnecessary re-renders to improve performance

[pietrop]

Is your Pull Request request related to another issue in this repository ?

This issue #159

Describe what the PR does
Moved header into a component with shouldComponentUopdate false to test avoid unecessary re-renders - test

State whether the PR is ready for review or whether it needs extra work

work in progress

Additional context

[pietrop]

Trying out why-did-you-update in /demo/app.js

See Common Fixing Scenarios

• Changes Are in Functions Only
• Value Did Not Change

[pietrop]

Rough draft of React performances optimisation notes from various docs and blogs can be found here

[pietrop]

To review this PR you need to run npm install as I’ve added react-visibility-sensor in package.json.

currentTime in TimedTextEditor


Updating on currentTime in TimedTextEditor might have a performance cost?

Or at the very lease it causing frequent re-renders


js
 if (nextProps.currentTime !== this.props.currentTime ) { return true; }




18ms + 17 to 24 re-renders calls

Vs
0.3ms + 17 to 24 re-renders calls if not updating TimedTextEditor on currentTime change (eg commenting out those 3 lines)

In current setup, draftJS Editor re-render should be separated from css injection in TimedTextEditor to use currentTime to show current word to at least avoid re-render DraftEditor on currentTime change.

react-visibility-sensor

Following up from this comment from @Laurian added VisibilitySensor in WrapperBlock, around the Speaker, Timecode, and draftJs Editor. This should avoid rendering any draftjs block unless it is visible

Downside of using VisibilitySensor is the text not in viewport is not rendered. Which means if you search with ctrl + f you it won’t show up in the results, so cannot search across the transcript.

However the performance boost should outweigh this, and a separate transcript wide search(and replace?) could be implemented at later stage if needed

Also not 100% sure how much of a performance boost react-visibility-sensor gives?

unnecessary re-renders

#159

Rough draft of React performances optimisation research notes from various docs and blogs can be found here

Used

• why-did-you-update lib
• React profiler tool
  
- [x] adding console.log in render method of components
• refactored logic of some of the components communications to improve performance
• use of shouldComponentUpdate
• use of PureComponent

---

Performance for over 1 hour

Tested with
#150

Seems better but would need testing on lower performance windows PC with clip and transcript over 1 hour (eg 2hours min)

Also important to note (as pointed out by @Laurian) performance for longer content, is improved is paragraph blocks being edited are smaller (eg few lines) as opposed to half of the view height of more.

But not conclusive for now that this PR addresses the longer files performance issue. It does seem to reduce unnecessary re-renders tho.

immutable js warnings


 why-did-you-update is commented out in /demo/app.js as it seems to slightly slows down performance when in development


if (process.env.NODE_ENV !== 'production') {
   const { whyDidYouUpdate } = require('why-did-you-update');
   whyDidYouUpdate(React, { exclude: [ /^HotKeysWrapper/ ] } );
}


However When using why-did-you-update to test unnecessary re-renders there’s a warning

immutable.js:4655 iterable.length has been deprecated, use iterable.size or iterable.count(). This warning will become a silent error in a future version. Error

As explained in this issue
facebookarchive/draft-js#950

DraftJS immutable JS dependency needs to be upgraded in DraftJs core library.

---

Still to do

• Move CustomEditor into its own file form TimedTextEditor
• Review CSS modules files are divided with concerns properly separated.
• Capture some kind of ADR or doc note with main takeaway from these performance tweaks

---

Also addressed, small fix for
#147

[pietrop]

Something else to think about for a separate PR is the idea of pagination.

Context: DraftJS use case at FB is for small comments areas and Facebook notes. So not sure it has been tested agains large quantities of text or Many paragraphs (link to issue) or that they will ever have a use case to optimise for that (?).

Another option would be to reduce the amount of content in Draft at any given time, only to what is visible.

Eg at a high level it could use a pagination approach, similar to what is described in this blog post (without Redux!) so that every time scrolling up or down, it moves/changes the paragraphs that are visible in the editor.

Eg the wrapper component keeps an array of paragraphs, and a range of indexes to the once visible in draftJs, then some logic handles swapping the current paragraphs... etc..

Done in this way pagination would be hidden from the users UI/UX, eg it wouldn't look like paginated text, but it could provide a considerable performance boost.

[sshniro]

Hi @pietrop is the paginating issue is still pending ? Shall I give it a try with React hooks and context ?

[pietrop]

Hi @sshniro,
That would be awesome, PR welcome, in the last comment I started flashing out a direction for a possible solution. You are welcome to try that or some other approach.

I am not sure hooks and context would necessarily help with the specifics tho, as the solution might entail working around the draftJs component specific performance issues, what did you have in mind?

[sshniro]

Aw sry my bad, I thought of handling the entire state out of draft , now only read the linked article. Ill play around with draft js and get back, thanks ! 👍

[pietrop]

Awesome! Looking forward to it!
Any questions feel free to reach out, happy to help

[pietrop]

Before

After

[jamesdools]

In this block, we have every single part of the state accounted for and checked, plus also one Prop (mediaUrl).

I checked there weren't any other special cases, but no it's looking at every single state property. So!

Instead of doing all that, we could write:

  shouldComponentUpdate = (nextProps, nextState) => {
     if (nextProps.mediaUrl !== this.props.mediaUrl) return true;

     return nextState !== this.state;
  }

[jamesdools]

I don't see any difference in performance or side-effects from that. If this is alright, I think we should do the same thing in the other places (as it's adding a lot of verbose and redundant code).

[pietrop]

Thanks @jamesdools added most of the changes in #175.

3588b26 and #180

Going to do a separate PR for the should component update tweaks against the develop branch, to make those changes a little clearer and easier to review.

Closing this for now, as Develop is up to date with it.

[pietrop]

addressed should component update in this standalone PR #182