Optimize the first rendering of the report main view and the report actions view #498

tgolen · 2020-09-15T22:50:49Z

This fixes #443 I hope. I wasn't able to reproduce the problem locally, so maybe one of you are able to?

This PR adds an HOC which will allow a component to render a collection in batches. In the case of the report main view, the batches are based off of first the visible reports, then 5 seconds later, the rest of the reports. On the report actions view, the first batch is 100 comments, then 5 seconds later, the rest of the comments.

Tests

From a cold boot of the app, sign in, and search with the chatpicker
Verify it is very very fast to do so

tgolen · 2020-09-15T22:54:47Z

src/components/withBatchedRendering.js

+
+                    // If the batchCallback returned false, then the props to get the items
+                    // from didn't exist from Ion yet
+                    if (items === false) {


This will go away once #482 is merged

marcaaron

Thanks for taking a crack at this @tgolen. It took me a while to wrap my head around how withBatchedRendering works, but I appreciate what you’ve done here so far.

Now, my instinct is that this HOC may point to a deeper problem somewhere else in the code. And I wonder if we really need it at all. However, I haven’t investigated performance issues yet so I won't share any wild guesses right now. And I won't block this solution if it's the glue that gets us from A to B.

I did spot a few problems that we should address and had a few suggestions for how to improve things.

marcaaron · 2020-09-15T23:19:32Z

src/page/home/MainView.js

 };

-class MainView extends React.Component {
+// This is a PureComponent because since this method is connected to an Ion collection,


because since

I think you maybe mean "component" not "method"?

marcaaron · 2020-09-16T00:36:13Z

src/page/home/MainView.js

+// it has setState() called on it anytime a single report in the collection changes. When
+// reports are first fetched, this component is rendered n times (where n is the number of reports).
+// By switching to a PureComponent, it will only re-render if the props change which is
+// much more performant.


this component is rendered n times (where n is the number of reports)

Might be reading this wrong, but I think this PR should make this statement no longer true? We will get all the reports then set the entire collection to withIon state before the component renders. So, we should expect it to only render once.

By switching to a PureComponent, it will only re-render if the props change which is much more performant.

Haven't used PureComponent much, but based on what I've read in the docs my understanding is that it just does a shallow comparison for all props and state to see if the component needs to re-render. Lmk if there's some other difference you see. It's still unclear to me why this would be more performant after reading this comment, but I am curious to learn more.

Might be reading this wrong, but I think this PR should make this statement no longer true?

I don't think it does, no. I checked out that branch, and I added a console.log('render') to the render method in MainView.js and this is what happens on page load (ignore the network errors):

I then changed that component to be a PureComponent (still on that same branch) and this is what happens:

Lmk if there's some other difference you see

No, there are no other differences, that's exactly what PureComponents do and that's what we want it to do. The only thing this component needs to do is render/show/hide the ReportViews for every report. It doesn't need any information from the reports collection other than the reportID, which is never going to change after first render. The only other thing it uses is this.props.match.params.reportID, and since those props are updated via the router, it will re-render the component when necessary.

It's still unclear to me why this would be more performant after reading this comment, but I am curious to learn more.

It's the difference between rendering all the ReportViews 52 times (literally) or 1 time, so there is a huge performance gain.

It doesn't need any information from the reports collection other than the reportID, which is never going to change after first render

Cool. That explanation helps! Would be good to include something about the reportIDs in the comment.

So if a new report appears in the props (e.g. if Pusher handles a new report comment for a report not yet in your list) then it should appear because it's a new key on this.props.reports, but other updates to existing reports should not trigger the re-render?

Yeah, that's the idea. Let me be sure to check that pusher case 👍

marcaaron · 2020-09-16T01:11:41Z

src/page/home/MainView.js

        return (
            <>
-                {_.map(this.props.reports, report => (
+                {_.map(this.props.itemsToRender, report => report.reportName && (


Just wondering here... are we rendering all these reports because after they are loaded it makes for a better overall performance when switching between them?

Yes, exactly.

Any ideas about why performance degrades when switching reports?

Yea, it's because all the comments have to be inserted into the DOM when reports are switched. Since the process to do DOM insertion is expensive, if we can do it in a way that doesn't impact the user, then it's a better experience.

Makes sense. So, if there are less comments to insert into the DOM (or whatever the equivalent is on mobile) then it would be faster to switch between chats. But we have instead decided to "front-load" that work. And this PR is basically prioritizing that work by delaying some of the "front-loaded" parts.

Yeah, that's correct.

marcaaron · 2020-09-16T01:40:17Z

src/page/home/report/ReportActionsView.js


    // Array of report actions for this report
-    reportActions: PropTypes.PropTypes.objectOf(PropTypes.shape(ReportActionPropTypes)),
+    itemsToRender: PropTypes.PropTypes.arrayOf(PropTypes.shape(ReportActionPropTypes)),


Could we possibly use a dynamic prop name here? It's an interesting idea to use this HOC as a prop filtering middleware of sorts... but I wonder if we need to lose the prop name as it makes it more difficult to follow the rest of the code.

That's a good idea. I'll look into that to see if I can keep the original prop name intact.

So I looked into this, and I actually don't want to use the original prop name (because that will overwrite the data that Ion passes to the component). I like having a separate prop for what withIon adds and withBatchedRendering adds. I have a little idea to improve this to make it more intuitive.

marcaaron · 2020-09-16T01:47:15Z

src/components/withBatchedRendering.js

+    return component.displayName || component.name || 'Component';
+}
+
+export default function (batchCallback, batchesToRender = 2, batchRenderDelay = 5000) {


Random thought... what if batchCallback returned an array where each item was a batch that we want to render + a render delay?

e.g.

(props) => { return [ {items: _.filter(props.something, someFilter), delay: 0}, {items: _.filter(props.something, someOtherFilter), delay: 2000}, ]; }

I think this makes it slightly easier to visualize the "batching" that's happening and also give us more control.

Yeah, that's a really cool idea. I like how that looks.

src/components/withBatchedRendering.js

marcaaron · 2020-09-16T02:29:57Z

src/components/withBatchedRendering.js

+
+                    setTimeout(() => {
+                        this.setState({
+                            itemsToRender: items,


Only update itemsToRender when a batch runs? What about when the props update? Don't we want to continue to set the latest props once all the filtering is done?

marcaaron · 2020-09-16T02:36:12Z

src/page/home/MainView.js


-class MainView extends React.Component {
+// This is a PureComponent because since this method is connected to an Ion collection,
+// it has setState() called on it anytime a single report in the collection changes. When


We are calling setState() on withIon not here, but I got your meaning

This reverts commit 3f77ea9.

quinthar · 2020-09-16T20:41:31Z

When you say "first 100 comments" -- can you actually prioritize the "last 100"?

…

On Tue, Sep 15, 2020, 6:51 PM Tim Golen ***@***.***> wrote: This fixes #443 <#443> I hope. I wasn't able to reproduce the problem locally, so maybe one of you are able to? This PR adds an HOC which will allow a component to render a collection in batches. In the case of the report main view, the batches are based off of first the visible reports, then 5 seconds later, the rest of the reports. On the report actions view, the first batch is 100 comments, then 5 seconds later, the rest of the comments. Tests 1. From a cold boot of the app, sign in, and search with the chatpicker 2. Verify it is very very fast to do so ------------------------------ You can view, comment on, or merge this pull request online at: #498 Commit Summary - Optimize how many reports render at one time - Add some comments to explain the pure component - Optimize the rendering of report actions - Add HOC for batched rendering File Changes - *A* src/components/withBatchedRendering.js <https://github.com/Expensify/ReactNativeChat/pull/498/files#diff-cf2df8cc07d2f2021e82db90331a4ab6> (69) - *M* src/page/home/MainView.js <https://github.com/Expensify/ReactNativeChat/pull/498/files#diff-0ef12fb26b941afa1462ce918baade5f> (59) - *M* src/page/home/report/ReportActionsView.js <https://github.com/Expensify/ReactNativeChat/pull/498/files#diff-c91277fc49b844dae6d2386aee8aa2f7> (32) Patch Links: - https://github.com/Expensify/ReactNativeChat/pull/498.patch - https://github.com/Expensify/ReactNativeChat/pull/498.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#498>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUVFDFELANQ4OD3RGU3SF7VVRANCNFSM4RN4JQ3Q> .

…actNativeChat into tgolen-first-render

marcaaron · 2020-09-16T20:28:26Z

src/components/withBatchedRendering.js

+                _.each(batches(this.props), (batch) => {
+                    setTimeout(() => {
+                        this.setState({
+                            itemsToRender: batch.items,


I think we need to listen for changes to whatever prop we are filtering with this HOC in componentDidUpdate() and then set it to state or else we will never get the updated props?

Try leaving a comment on a report. It won't work at all and I think this is the reason why.

Ah, I'll look into that, yeah. I wasn't done with the most recent changes I wanted to make so there are still a few things I haven't tested yet. THanks!

src/page/home/MainView.js

marcaaron · 2020-09-16T20:56:20Z

src/page/home/MainView.js

        return (
            <>
-                {_.map(this.props.reports, report => (
+                {_.map(this.props.itemsToRender, report => report.reportName && (


Makes sense. So, if there are less comments to insert into the DOM (or whatever the equivalent is on mobile) then it would be faster to switch between chats. But we have instead decided to "front-load" that work. And this PR is basically prioritizing that work by delaying some of the "front-loaded" parts.

marcaaron · 2020-09-16T21:03:29Z

src/page/home/report/ReportActionsView.js

+        const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber');
+        return [
+            {items: _.chain(sortedReportActions).last(100).indexBy('reportID').value(), delay: 0},
+            {items: _.indexBy(sortedReportActions, 'reportID'), delay: 7000},


sequenceNumber right? not reportID?

tgolen · 2020-09-16T21:12:42Z

@quinthar RE: the 100 comment, yes, it does prioritize the most recent 100 report actions.

tgolen · 2020-09-17T16:43:43Z

@marcaaron OK, I've updated it so that it works with componentDidUpdate(). However, this does lead to a slight problem. Envision this scenario:

Report action batch 1 is rendered (100 comments)
User posts a new comment
Report action batch 2 is rendered (the rest of the comments from step 1)
User's comment from step 2 disappears because it's overwritten

There are a couple of solutions I guess.

Change the implementation of componentDidMount() and find a way to do it so that the batches aren't "prescheduled" (for lack of a better term)
Decrease the delay of the second batch to make the window small enough that this problem isn't likely to happen
Only do batched rendering in MainView.js to begin with, which won't have this problem

Thoughts?

…actNativeChat into tgolen-first-render

marcaaron · 2020-09-17T17:09:05Z

Ah hmm, yeah one way around that would be to do something like this:

return [
   {batchMethod: (props) => {...}, delay: 0},
   {batchMethod: (props) => {...}, delay: 10000}
];

So rather than making batches a function you make each batch have a function that will refer to latest props

marcaaron · 2020-10-15T01:45:58Z

Gonna take this over for Tim and pick up where we left off.

…actNativeChat into tgolen-first-render

marcaaron · 2020-10-15T02:50:43Z

Removing myself as reviewer since I added a few commits to get this cleaned up - @chiragsalian @roryabraham would you all mind taking a look at this?

marcaaron · 2020-10-15T02:52:54Z

src/page/home/sidebar/ChatSwitcherView.js

                    }}
                    onChangeText={this.updateSearch}
-                    onClearButtonClick={this.reset}
+                    onClearButtonClick={() => this.reset()}


This was a random bug I found that was passing a Class as a param to this.reset() didn't seem to be breaking anything but was popping up in console.logs for proptypes errors.

roryabraham · 2020-10-15T16:39:35Z

src/components/withBatchedRendering.js

+ * @param {object} component
+ * @returns {string}
+ */
+function getDisplayName(component) {


IIRC this function is the same for all our HOC's, can we DRY this up and make it a separate utility?

Yeah I was thinking of doing this but just left it since the proposed changes are kind of complicated and I prefer to have fewer file changes per PR if possible.

Alright, it's NAB for me because this function is super simple

roryabraham · 2020-10-15T17:00:53Z

src/page/home/report/ReportActionsView.js

-    },
-})(ReportActionsView);
+export default compose(
+    withIon({


Just wondering, does it matter the order in which these HOC's are composed? I think it's right the way you have it here, but I'm still curious if having withIon be wrapped by withBatchedRendering would break withIon?

Just kinda thinking out loud here, but it seems like withBatchedRendering temporarily withholds certain props from being passed to the wrapped component. If that wrapped component were withIon, I'm wondering if withIon's componentDidMount would still set up all the Ion subscriptions correctly

Yeah the order does matter, but I think even if you have withBatchedRendering followed by withIon that nothing would inherently be wrong with that. We are just not likely to ever do it. withBatchedRendering is a just a dumb middleware that takes props and delays passing them down to the wrapped component. Whether that component is wrapped in withIon or not doesn't shouldn't really make a difference.

I could see the argument that if you used withBatchedRendering in front of a withIon that was using the function version of defining an Ion key like

{key: props => props.someValueThatWithBatchedRenderingIsTemporarilyWitholding}

Then maybe this initial subscription wouldn't work out in componentDidMount()

But in that case, it's a possible indication that we are holding something wrong.

Even if that did happen we would retry to subscribe pretty much the next time the component updates...

https://github.com/Expensify/ReactNativeChat/blob/43a1888714c33f5bc96c6f7e59f99cf92bbc16ac/src/components/withIon.js#L53-L58

roryabraham

Overall looks good, just the one DRY comment for getDisplayName for now - the other is NAB and more just a question.

roryabraham · 2020-10-15T17:09:50Z

src/components/withBatchedRendering.js

+                });
+            }
+
+            componentDidUpdate(prevProps) {


Also, if I'm being honest I'm confused about why we need this function and why this.props[propNameToBatch] is a list.

Ok hmm I'm trying to think of a better way to put this...

Right now I have

We need this to allow the flow of props down from a parent component to work normally after all the batches have finished rendering

But maybe the part you're missing is that...

We are effectively replacing the property coming in from props with the one set in state in this HOC

We explicitly remove the property from this.props in the render() so the props passed by a parent are filtered through this component

If we don't listen for when that property changes and set it to state we'll just remove it and the wrapped component's props will never update

Okay, I think I get it now

roryabraham

Changing this to an approval, but I'll let @chiragsalian have a look too.

chiragsalian

Looks great to me and works really well on web and mobile too. Left two minor comments. Approved anyway since they are mostly NABs.

chiragsalian · 2020-10-15T20:03:46Z

src/components/withBatchedRendering.js

+             * Cancels all the timers
+             */
+            cancelBatchTimers() {
+                _.each(this.timers, timerID => clearTimeout(timerID));


NAB, feel weird to have this single line method. Its only used in one spot and already has a comment above explaining what it does so it feels like this _.each(this.timers, timerID => clearTimeout(timerID)); could directly be used here. Not sure if you placed is separately for readability but the comment already helps with that.

Yeah that makes sense I was going to trigger this from two spots at one point then changed my mind.

chiragsalian · 2020-10-15T20:08:28Z

src/pages/home/sidebar/ChatSwitcherView.js

                    }}
                    onChangeText={this.updateSearch}
-                    onClearButtonClick={this.reset}
+                    onClearButtonClick={() => this.reset()}


Why this change, is it by accident?

This was to fix a random error I found. not a bug or related to these changes. one of the annoying prop-types kind of errors.

;

chiragsalian

LGTM 👍

AndrewGable · 2020-10-20T21:18:28Z

FYI - Conflicts

marcaaron · 2020-10-20T21:34:49Z

Ok, so I know we got pretty far with this one. But the original issue is now closed and we aren't experiencing the problem linked there (slow start up times for iOS apps). So we should probably leave this idea for a rainy day. As interesting as this solution is, it doesn't solve any problem we are currently experiencing. If anyone disagrees we can reopen.

tgolen added 4 commits September 15, 2020 11:13

Optimize how many reports render at one time

845460d

Add some comments to explain the pure component

f447382

Optimize the rendering of report actions

fbab50d

Add HOC for batched rendering

8fa9a37

tgolen requested review from AndrewGable and marcaaron September 15, 2020 22:50

tgolen self-assigned this Sep 15, 2020

tgolen commented Sep 15, 2020

View reviewed changes

tgolen and others added 3 commits September 15, 2020 16:56

Merge branch 'master' into tgolen-first-render

4457014

Remove initial rendering logic

8ed7f31

Merge remote-tracking branch 'origin' into tgolen-first-render

ceb3c90

marcaaron requested changes Sep 16, 2020

View reviewed changes

tgolen added 10 commits September 16, 2020 07:53

Merge branch 'master' into tgolen-first-render

9c5cea5

Improve comment

8160cec

Remove an extra view

5891973

Improve the batch definitions

901585a

Improve props

f6672dd

Rename comments to indicate where props are coming from

4f1a229

Add some comments for how the HOC is used

c0a3ded

Refactor URL storage in Ion a little

3f77ea9

Revert "Refactor URL storage in Ion a little"

6293c58

This reverts commit 3f77ea9.

Fix indexing

41fa8a1

Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…

9d9a2f8

…actNativeChat into tgolen-first-render

marcaaron requested changes Sep 16, 2020

View reviewed changes

Have batch rendering work on normal component updates

e0c2063

Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…

fe7ddf9

…actNativeChat into tgolen-first-render

marcaaron added 8 commits October 14, 2020 18:46

Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…

1ac8155

…actNativeChat into tgolen-first-render

fix conflicts

ae0406a

fix conflicts

f344c2b

clean up and add some comments

34a0fa6

remove some changes to clear up diff a bit

7fc19f5

fix another diff

c4e22d4

more clean up

9806698

fix style

0559a57

marcaaron requested review from chiragsalian, marcaaron and roryabraham and removed request for marcaaron October 15, 2020 02:49

marcaaron reviewed Oct 15, 2020

View reviewed changes

roryabraham reviewed Oct 15, 2020

View reviewed changes

roryabraham requested changes Oct 15, 2020

View reviewed changes

roryabraham reviewed Oct 15, 2020

View reviewed changes

roryabraham previously approved these changes Oct 15, 2020

View reviewed changes

fix conflicts

2f718e3

marcaaron dismissed roryabraham’s stale review via 2f718e3 October 15, 2020 18:32

fix bad merge

1230443

chiragsalian previously approved these changes Oct 15, 2020

View reviewed changes

just use single line instead of method;

5faccd6

;

marcaaron dismissed chiragsalian’s stale review via 5faccd6 October 15, 2020 20:55

chiragsalian approved these changes Oct 15, 2020

View reviewed changes

marcaaron closed this Oct 20, 2020

Optimize the first rendering of the report main view and the report actions view #498

Optimize the first rendering of the report main view and the report actions view #498

Uh oh!

Conversation

tgolen commented Sep 15, 2020

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quinthar commented Sep 16, 2020 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgolen commented Sep 16, 2020

Uh oh!

tgolen commented Sep 17, 2020

Uh oh!

marcaaron commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Oct 15, 2020

Uh oh!

marcaaron commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marcaaron commented Sep 17, 2020 •

edited

Loading

marcaaron Oct 15, 2020 •

edited

Loading