Fix Search Page Slow Issue (CMD+K) - 2nd, 3rd solution

[Looxor]

Details

1. src/pages/SearchPage.js

• added conditional render using didScreenTransitionEnd

Fixed Issues

$ #3601

Tests

1. While using e.cash use the cmd+k command
2. Type on the search field

QA Steps

1. While using e.cash use the cmd+k command
2. Type on the search field

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

expensify-fix-result.mp4

iOS

Android

I have read the CLA Document and I hereby sign the CLA.

[nickmurray47]

@Looxor I added a Work In Progress label until we get a demo account that we can test this PR with and so we don't merge this PR until we've fully tested yet.

[timszot]

@nickmurray47 @Looxor I tested this with my account! Here's a summary of what I found.

I used the dev tools performance profiler to test the CMD +K shortcut followed by typing a name (I used tim since that has multiple results in my account). I didn't notice any regression of performance when using this branch as a build source. I didn't notice an increase in any performance either... but I think this is safe to merge.

[nickmurray47]

Ok, I still think we should hold off on merging until we can definitely say that the PR does increase performance for the search page. @Looxor I also notice that this PR is pretty different from your original proposal of potentially cutting down on the number of renders. Can you include some details on why this change would improve performance?

I'm wondering for testing if we can include some kind of performance metric to quantify this. I will also work on getting you your own testing account. Thoughts?

[Looxor]

@nickmurray47, Thank you for your attention.

Regarding multiple rendering, I think that it's better to decrease multiple rendering, but as you mentioned before, it makes some regression issues. At the moment, I realized that it is related to too many components, and I tried to find other ways.

Regarding didScreenTransitionEnd, I found a more effective way to display an ActivityIndicator.
It was more effective to use the conditional rendering than using "visible" variable because a former way does not require to render any blank component if not necessary.

Thank you,

[nickmurray47]

@Looxor I also saw you posted in our open-source channel https://expensify.slack.com/archives/C01GTK53T8Q/p1625851107062100. Did you still need help mocking additional users?

[Looxor]

@nickmurray47 ,
And I want to continue to integrate 1st solution of mine, but stopped due to the regression problem.
Can you please provide me a demo user (and password) so that I can check the regression problem as you mentioned?

[Looxor]

@nickmurray47 , Yes, I still need them.

[nickmurray47]

Ok, @isagoico do you have a production QA account (expensifail) with a ton of chats on it that you could lend to an external contributor?

[isagoico]

@nickmurray47 Sure! You can try with applausetester+ig@applause.expensifail.com / 123456Hola and let me know if that one has enough chats, If it doesn't have enough I can try and find another one.

[nickmurray47]

Thank you @isagoico!

[Looxor]

@isagoico , Thank you so much. It works well.

[Looxor]

Regarding 3rd solution,

I just pushed the 3rd solution.

Looking up a component FullScreenLoadingIndicator, I noticed that there is a difference between "adjusting with height:100%" and "adjusting with {top: 0, bottom: 0}".

But, I also agree on that "adjusting with {left: 0, right: 0, top: 0, bottom: 0}" is a better solution than "width/height: 100%".
ex: https://codepen.io/wizardzines/pen/abNXEVy
article: https://stackoverflow.com/questions/15972192/css-positioning-to-fill-container-width-vs-left-right

So, I left the original code and put some additional code for our CMD+K case.
And I believe that the solution is working fast than before.

@timszot , @nickmurray47 ,
Could you all please check this carefully?

Thank you.

[parasharrajat]

Leaving my thoughts here. Looking at the changes, I am uncertain of any performance gain here. I would request you to pull Marc in for review as he is mostly looking into performance-related issues.

[Looxor]

Leaving my thoughts here. Looking at the changes, I am uncertain of any performance gain here. I would request you to pull Marc in for review as he is mostly looking into performance-related issues.

@parasharrajat

I made 2 screenshots for you to show the difference.

Untitled2.mp4

Untitled3.mp4

[parasharrajat]

It seems it more of a UI issue rather than a performance.

Edit:
So this suggests that the Absolute positioned element is slower than the flex equivalent, though all of this is handled by the CSS engine. Sorry, but it's hard to believe without any proof. But there is something really going on.

But if it makes a noticeable impact on affected users who first raised this issue then 👍

[Looxor]

Hi @isagoico,
At this point, can you please check this solution?
I pushed the 3rd solution, but now, I want to hear your opinion because other people can't notice the increase of the performance.

It seems it more of a UI issue rather than a performance.
Edit:
So this suggests that the Absolute positioned element is slower than the flex equivalent, though all of this is handled by the CSS engine. Sorry, but it's hard to believe without any proof. But there is something really going on.
But if it makes a noticeable impact on affected users who first raised this issue then 👍

[Looxor]

It seems it more of a UI issue rather than a performance.

Edit:
So this suggests that the Absolute positioned element is slower than the flex equivalent, though all of this is handled by the CSS engine. Sorry, but it's hard to believe without any proof. But there is something really going on.

But if it makes a noticeable impact on affected users who first raised this issue then 👍

@parasharrajat ,
Yes, that's right. The slaggy and slow transition of the search page, is due to UI issue than the performance-related issue.
So, I focused on UI issue than the performance issue.

[parasharrajat]

Sorry, @Looxor I currently don't have the bandwidth to test it.

[Looxor]

No problem. @parasharrajat , Thank you for your time.

[timszot]

@Looxor I did some more testing for you.
My performance tests were hitting CMD +K, once loaded type in test in the search field and wait for it to load the results.
Built from main:

Built from the proposed branch:

These proposed changes do cut down on performance costs, but I don't see a noticeable improvement when using CMD+K on my production account. @marcaaron @nickmurray47 I would like your thoughts here as well.

[marcaaron]

I don't see a noticeable improvement when using CMD+K on my production account

FWIW, it might be valuable to test locally on a production build. There is a lot of difference between dev and production JS. More info here -> https://reactjs.org/docs/optimizing-performance.html#use-the-production-build

[timszot]

FWIW, it might be valuable to test locally on a production build.

Sorry if this wasn't clear, all these tests were done with builds from dev, pointed to production.

[marcaaron]

Interesting. That seems to suggest that we are experiencing some kind of local dev API lag, but this PR is not touching any APIs. Is the speed difference consistently reproducible?

[timszot]

Is the speed difference consistently reproducible?

The actual time that it take to load the search and then populate the changes when you type in a value did not change from what I can tell. The only gains I saw was the rendering and scripting speeds. It was consistently faster.

[marcaaron]

Sorry I'm not following why these changes would make anything go faster. Can someone summarize?

[nickmurray47]

@timszot Thanks for the writeup and testing.

My understanding of why rendering would go faster is because instead of passing the didScreenTransitionEnd prop to the FullScreenLoadingIndicator component we only conditionally render it if true which means we no longer render a blank FullScreenLoadingIndicator.

It looks like the performance improvement is very marginal though so I don't think the PR is worth merging right now.

[marcaaron]

which means we no longer render a blank FullScreenLoadingIndicator

What impact does this have on performance? This is the original proposal accepted...

#3601 (comment)

How did we end up with this PR ?

[nickmurray47]

How did we end up with this PR?

I asked the same thing above. The original PR @Looxor submitted to address rendering caused several regressions and we ended up reverting it.

[nickmurray47]

@Looxor We are going to close this PR given the minimal performance improvement and how far off we got from the original proposal accepted.

[Looxor]

I want @nickmurray47 and @marcaaron to see this comment.
Thank you.