[Performance] Update withWindowDimensions HOC to use a Context Provider internally

[kidroca]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

withWindowDimensions can be improved through a refactor that won't change existing usages and behavior

Currently State:

• on a chat with 3 messages I have 35 withWindowDimensions HOCs mounted
• on a chat with 20 messages I have 101 withWindowDimensions HOCs mounted
• adding an attachment would raise the count by 4-5
• adding a plain text would raise the count by 2

All of these usages would create a separate class component that:

• have to be instantiated
• run it's lifecycle methods
• add its own subscription to Dimensions
• store another instance of the same callback due to binding: this.onDimensionChange = this.onDimensionChange.bind(this);
• has its own state copy that will be the same with all other withWindowDimensions instances

The main problem with this HOC is not that onDimensionChange is called often - it's not called at all during init. But initializing a chat with many messages could be choking. On mWeb showing/hiding the keyboard would trigger all the onDimensionsChange listeners, because the viewport do change. This might be something that is causing extra effort combined with the autofocus making the keyboard appear

This functionality can be split in two:

DimensionsContextProvider

A component that will wrap the App the way the SafeAreaProvider does here
It will have the class logic currently defined in withWindowDimensions. The difference is it will only mount and subscribe to Dimension events once and will provide any changes to context consumers. When dimensions change the context state will change and all consumers will update the way they currently do. the memory involved doesn't change as usages grow e.g. O(1) vs O(n)

withWindowDimensions as a DimensionContextConsumer

The HOC will become just a stateless function that passes contextual value to the wrapped component e.g.:

export default function withWindowDimensions(WrappedComponent) {
  const HOC_Wrapper = (props) => (
     <DimensionContext.Consumer>
       {(dimensionProps) => <WrappedComponent {...props} {...dimensionProps} />}
     </DimensionContext.Consumer>
  );

  HOC_Wrapper.displayName = `withWindowDimensions(${getComponentDisplayName(WrappedComponent)})`;
  return HOC_Wrapper;
}

Add some throttle or debounce

Wrapping the event handler with throttle or debounce from underscore, should further improve resize performance

---

This is a branch setup with some logging that you can use to play around: kidroca@e2126bc

Originally posted here: #2051 (comment)

• this (probably) won't solve the issue but will be a step in the right direction

Expected Result:

Using the withWindowDimensions should not increase memory usage or subscribe unnecessary listeners

Actual Result:

Each withWindowDimensions usage creates a new component with own lifecycle and own subscription to Dimension events

Action Performed:

N/A

Workaround:

No workaround needed. This is a code organization and performance improvement

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.0.17-3
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation\

withWindowDimensionsSample.mp4

Expensify/Expensify Issue URL: N/A

[kidroca]

Proposal

I've included everything in the ticket description.

I think, with the explanation above, this is something anyone should be able to do. But since I brought it up I would like to work on this or at least take some credit 😉

[marcaaron]

Great summary here and thanks for putting this together!

I think we are just missing something about what the practical impacts of such a change would be. Is there a way to apply this solution to a measurable outcome or some specific problem? e.g. I see mention that this should be addressed due to memory usage issues. But I'm not sure I'm aware of any issues directly related to memory usage.

So,

• It logically sounds like a good improvement
• It's not clear whether this solves any particular problems we are experiencing now

If this issue was accompanied by something like "we are consuming x memory and this change will reduce memory consumption by y amount which will make the app run z seconds faster" then I think we could start right away.

[kidroca]

At the time I didn't know a lot about your workflow and that I should present hard evidence of a problem

Looking at the implementation and usages throughout the code I just know it isn't optimal and provide explanation on that.
It's not just memory, everything is executed (n) times as well - addEventListener, onDimensionChange these will be called simultaneously for all the withWindowDimension class instances. For my sample chats of 15-20 messages there are ~100 instances. Switch between chats would execute these calls again and again. My proposal will bring the class instances to just 1, regardless of message count and the number of withWindowDimension wrappings.

To provide hard evidence and metrics for an issue like that could cost a lot of extra time and efforts. And with no guarantee that I'll get a job only makes it harder to justify the labor. I try to provide a good explanation and then you can decide is it worth investigating further.

[kidroca]

I've played around trying to gather enough information to prove this.
I couldn't track conclusive results from memory usage - 100 class instances would be about 64kb, that might seem small but the total component instances in the app are around 120kb of memory

I did manage to find Performance issue when onDimensionChange is triggered

onDimensionChange current performance

Here's a 4.5s sample where I've made 4 window resizes

Note the 1800ms scripting time

1200ms of the 1800ms (62%) can be traced to calls to onDimensionChange

A single change is around 300ms on my desktop workstation. On a mobile device it would take more time, and onDimensionChange would be called each time the Keyboard appears or disappears on mWeb - might be related to the Safari focusing issue because when an input is focused - the keyboard appears #2051 (no longer valid for switching chats but for new chat and search)

Here's the full profile that you can load inside chrome dev tools
Profile-20210421T155523.zip

onDimensionChange after suggested updates

Here's the same sample when withWindowDimension is updated so that it's a single class

4.5s sample where I've made 4 window resizes

Profile-20210421T173821.zip

---

Summary

I wasn't planning on working on the "after" metrics but I tried using the hook useWindowDimensions to quickly check if there was an improvement:

withWinodwDimensions using a hook

import {useWindowDimensions} from 'react-native';

export default function (WrappedComponent) {
    function withWindowDimensions(props) {
        const window = useWindowDimensions();
        const isSmallScreenWidth = window.width <= variables.mobileResponsiveWidthBreakpoint;

        return (
            <WrappedComponent
                windowHeight={window.height}
                windowWidth={window.width}
                isSmallScreenWidth={isSmallScreenWidth}
                {...props}
            />
        );
    }

    withWindowDimensions.displayName = `withWindowDimensions(${getComponentDisplayName(WrappedComponent)})`;
    return withWindowDimensions;
}

The code became shorter but performance did not improve - this is because internally the hook will create a new listener per each component that uses the hook so again 100 listeners - 100 callbacks

So to prove that my idea is an improvement I had to implement the HOC through context

This research also hints that it might not be a good idea to have that many components wrapped with the same HOC, and instead we can try to pass props from a Parent component.
The current case is a lot of the items in the chat need the window dimensions. They can receive them from the ReportActionView instead of each individually hooking to a HOC - this is just food for thought

[marcaaron]

To provide hard evidence and metrics for an issue like that could cost a lot of extra time and efforts. And with no guarantee that I'll get a job only makes it harder to justify the labor. I try to provide a good explanation and then you can decide is it worth investigating further.

I understand completely - I can keep looking into this and assign the issue to you if we decide to move forward on it. From our end, it's also hard to justify solving problems when we can't easily evaluate if they are worth fixing.

Thanks for the profiles as well. Since they are measuring the impact of a screen adjustment they seem to prove that resizing a window would happen faster if we adopted the suggestion. But not yet sure if this is a case that we should optimize yet.

[kidroca]

Thanks for the profiles as well. Since they are measuring the impact of a screen adjustment they seem to prove that resizing a window would happen faster if we adopted the suggestion. But not yet sure if this is a case that we should optimize yet.

Yes but not exactly. It's not that resizing happens slow but reacting to resizes would cost a lot of computing which would affect other stuff happening during that time. Since this is something that would not happen often I can see it not being a priority, but have in mind the mobile web case where when you focus the compose field the keyboard will open and the chat would start re-adjusting while all the onDimensionChange callbacks are fired

This reviews there's a significant relation between the number of instances and performance, it was easier to measure the effects or resizing.

[marcaaron]

but have in mind the mobile web case where when you focus the compose field the keyboard will open and the chat would start re-adjusting while all the onDimensionChange callbacks are fired

To clarify, opening the software keyboard will trigger a Dimensions 'change' event? On iOS simulator + debugging in Safari this doesn't happen for me. But maybe you're testing some other case that I haven't.

[kidroca]

To clarify, opening the software keyboard will trigger a Dimensions 'change' event? On iOS simulator + debugging in Safari this doesn't happen for me. But maybe you're testing some other case that I haven't.

Sorry I thought it did on iOS too. It turns out it's only on Android.

Maybe there's nothing that much to gain after all. I cannot trace anything significant back to withWindowDimensions when chats are switched

[kidroca]

Another case where onDimensionChange happen are orientation changes

This issue already deals with this by adding debounce #2180

Even with debounce added there's still a lot of JS scripting time as each HOC would capture its own debounced handler as explained here: #2326 (comment) - "a single change is around 300ms on my desktop workstation" - this is still true, because a single change would still trigger all the debounced handlers (100+) instead of just 1 handler

---

Now that I have a chat with 300+ messages I can try to capture more data regarding other negative effects - e.g. time to load / memory, but I'll wait for chat load time related tickets to close first

[parasharrajat]

I agree this point make some sense to me denouncing will add extra layer of timeouts. Normally current implementation should not hurt the app even if it adds extra computation. But mostly what I was concerned is the animations, When it comes to animations running on single thread, all these async tasks are a problem.

[marcaaron]

Closing this for now. @kidroca Feel free to reopen if you decide to follow up based on this and add the AutoAssignerTriage label to have this considered.

Now that I have a chat with 300+ messages I can try to capture more data regarding other negative effects - e.g. time to load / memory, but I'll wait for chat load time related tickets to close first

[marcaaron]

@parasharrajat please feel free to reopen this issue if you have additional metrics to share. Seems like the last time this was discussed we decided it would not be worth it. But maybe you can find something more concrete.