search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

Android - Setting - Android Staging app has noticeable lag as compared to PROD #17427

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Open New Expensify Staging App.
  2. Tap on profile image.
  3. Go to different menus (from workspace -> to profile, from profile -> preferences and so on).

Expected Result:

Staging app should be faster or should not have this behaviour.

Actual Result:

Noticeable lag experienced when using Android Staging app. It looks like the app shakes/rumbles/jumps a little bit in every change menu event (or when tapping between options).

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.0.0

Reproducible in staging?: Yes

Reproducible in production?: No

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Staging video

https://user-images.githubusercontent.com/93399543/231911492-e7a9207b-0b8c-4ca6-acac-5f16af80feb1.mp4

Production video

https://user-images.githubusercontent.com/93399543/231911547-e312f962-38e0-4c73-a55f-082fb0833bcf.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa13495b950dd931
  • Upwork Job ID: 1657115257012432896
  • Last Price Increase: 2023-05-12
OSBotify commented 1 year ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
MelvinBot commented 1 year ago

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

francoisl commented 1 year ago

@kbecciv out of curiosity, were you testing on a High Traffic account?

I can kinda see the app feels a little less smooth in my normal account, but curious if you noticed any difference based on the type of account.

francoisl commented 1 year ago

Could be a red herring, but I wonder if it has to do with disabling hardware acceleration in https://github.com/Expensify/App/pull/16532/files.

kbecciv commented 1 year ago

@francoisl Happening with gmail and expensifail accounts in same way.

marcaaron commented 1 year ago

Maybe my eye is not well trained, but I can't tell the difference here at all 👴

francoisl commented 1 year ago

Yeah so I've been running some tests on my physical device (Pixel 6) yesterday and I couldn't notice a real difference either.

https://user-images.githubusercontent.com/2229301/232129539-d909ae0a-6f95-45c6-b655-1b20550e3bb6.mp4

https://user-images.githubusercontent.com/2229301/232129793-d6cb42b9-5df6-4d9e-9d4f-c9904865a955.mp4

I'm trying to run the same tests on a Pixel 3 AVD (since it has less power) but having trouble with Android Studio atm :/

francoisl commented 1 year ago

It's more noticeable on a Pixel 3 AVD, pretty sure it has to do with disabling hardware acceleration indeed.

https://user-images.githubusercontent.com/2229301/232146649-d1e05d48-a398-4952-9870-d236f44b6de3.mov

https://user-images.githubusercontent.com/2229301/232146579-2e4a1c35-982a-47e2-8ff7-708748efdb1f.mov

cc @ArekChr do you know of a way we could re-enable hardware acceleration in https://github.com/Expensify/App/pull/16532, but still get that fix to work? I'm thinking we'd somehow need to re-enable it at the application level, but disable it at the View level only (maybe?) for ImageView. I don't see a way to manage that directly from React Native in their docs, though.

marcaaron commented 1 year ago

Not sure what the latest is on this or if it should block the deploy? cc @mountiny as you are the NewDot deployer this week.

mountiny commented 1 year ago

Yeah, checking this one out! After reading though the comments I feel like this is not a real blocker. Seems like this is somehowe perceived on older Android devices (emulator) but newer onces seem alright.

Thats why I think this should not be a blocker and we should ahndle this with Callstack separately. @ArekChr is ooo due to injury so I will raise this in the channel.

Do you agree @francoisl and @marcaaron?

MelvinBot commented 1 year ago

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

mountiny commented 1 year ago

Given the agreement, I am removing the deploy blocker.

Posted in Slack, hopefully Callstack will get someone to investigate tomorrow.

fabioh8010 commented 1 year ago

Hi, I'm Fábio from Callstack - expert contributor group - I would like to take a look at this issue.

fabioh8010 commented 1 year ago

Update: Disabling hardware acceleration in the entire app will make app slower indeed, so we need another approach for that. Like @francoisl stated, it's possible to control hardware acceleration at the View level. With that in mind, I changed FastImageViewWithUrl.java inside react-native-fast-image library to render with the software layer using setLayerType(View.LAYER_TYPE_SOFTWARE, null). But we end up facing other drawing problem:

FastImageViewWithUrl not displayed because it is too large to fit into a software layer (or drawing cache), needs 17756280 bytes, only 9331200 available

So looks like disabling hardware acceleration only for the image won't work, at least without making any other modifications. I will continue investigation to see if there is some sort of possible combination that we can use to avoid these issues while keeping hardware acceleration enabled in the app.

MelvinBot commented 1 year ago

@fabioh8010, @NicMendonca, @marcaaron Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 1 year ago

Not overdue - pending investigation by @fabioh8010

fabioh8010 commented 1 year ago

Update: Friday was Upskilling Day on Callstack so I haven't work on issues that day.

No significant updates for today, still under investigations.

fabioh8010 commented 1 year ago

Update: I'm studying about this canvas size limit and I'm going to prepare some separate Android / RN projects to try to test different combinations of approaches (or even libraries) to see if I can come up with a solution that would allow keeping hardware acceleration enabled.

fabioh8010 commented 1 year ago

Update: I'll be OOO for some days and will return on May 4th, so I passed all my findings and investigations to Arek since he will come back next week and can help me or take over this issue.

Found some possible solutions and sent all context to him.

melvin-bot[bot] commented 1 year ago

@fabioh8010 @NicMendonca @marcaaron this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

ArekChr commented 1 year ago

Update: Today, I will start rewriting the image transformation process to mirror how it's done in Expo Images. The Expo Images library uses Glide, so a migration is possible. The current code is written in Kotlin and RNFI in Java. If I am successful, I plan to write a proposal for this solution.

--edit--

If we apply the transformation, the image will be displayed in lower resolution, keeping the image quality when zooming it. After that, we can enable hardware acceleration so that it will remove lags on low-end Android devices.

ArekChr commented 1 year ago

I did a quick research on expo-images has a lot of other dependencies from the expo, both the transform function uses these dependencies, and probably migrating this function might not be worth it. Investigating another solution

NicMendonca commented 1 year ago

Thanks @ArekChr for the update!

ArekChr commented 1 year ago

Proposal:

Problem:

The Android Staging app has a noticeable lag compared to PROD, resulting in a poor user experience.

Root Cause:

The root cause of the problem is that the hardwareAccelerated attribute is disabled.

Solution:

To address the problem, I propose fixing the react-native-fast-image framework and preventing the display of large images allowed to draw. Specifically, I suggest you implement a new image resizing transform that turns images to a maximum width/height in pixels. Transform will be applied to glide RequestOptions in a react-native-fast-image framework, limiting the maximum height for large images. The transformation will be performed before the image is mounted, which means Glide will resize images while building a Drawable view.

I'm still investigating what the limit is. In my current PoC, I handled display images with a maximum height of 8k pixels. If an image is bigger, I get an error: java.lang.RuntimeException: Canvas: trying to draw too large(xxxxxxxxxxbytes) bitmap. In the final solution, width and height will be automatically calculated based on the total amount of pixels of the original size.

Example of how transform will be applied:

            RequestBuilder<Drawable> builder =
                    requestManager
                            .load(imageSource == null ? null : imageSource.getSourceForLoad())
                            .apply(FastImageViewConverter
                                    .getOptions(context, imageSource, mSource)
                                    .placeholder(mDefaultSource)
                                    .fallback(mDefaultSource))
+                                   .transform(new ResizeTransformation(8000));

PoC: https://github.com/ArekChr/ExpoImageTest/blob/rnfi-transformation-8k-pixels/patches/react-native-fast-image%2B8.6.3.patch

Expected Result:

Hardware acceleration will bring back performance on low-end devices and applied Glide transform will not crash large images and will keep the high image quality limited to a maximum height/width possible to draw.

Alternative Solutions:

None were explored.

melvin-bot[bot] commented 1 year ago

@fabioh8010 @ArekChr @NicMendonca @marcaaron this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01aa13495b950dd931

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal)

aimane-chnaif commented 1 year ago

Proposal looks good to me

I'm still investigating what the limit is. In my current PoC, I handled display images with a maximum height of 8k pixels. If an image is bigger, I get an error: java.lang.RuntimeException: Canvas: trying to draw too large(xxxxxxxxxxbytes) bitmap. In the final solution, width and height will be automatically calculated based on the total amount of pixels of the original size.

@ArekChr do you have more updates on this?

ArekChr commented 1 year ago

@aimane-chnaif Working on that. I will send an update soon

Julesssss commented 1 year ago

FYI, I'm moving the tracking and resolution of the performance regression to this issue.

ArekChr commented 1 year ago

Update: I created PR for image resize transform. I made progress in the playground displaying large images, but I have a problem with displaying them properly in the Expensify app. The issue may be related to too complex calculation logic of image size or image pan zoom framework. Here is playground and results below:

https://github.com/Expensify/App/assets/24796318/e5d5b026-919f-4195-ae66-b97d0c222aba

Zrzut ekranu 2023-05-17 o 12 43 46

Edit: after some changes in image size calculation on the js side still can't achieve good quality

Zrzut ekranu 2023-05-17 o 12 51 45
marcaaron commented 1 year ago

Just a heads up, I'm going to be OOO from May 23rd to May 30th.

I think this one is kind of on the back burner as far as a WAQ issue goes. Still important, but there's not really anything I feel that I can materially do to make this go "faster" on my end for now.

@Julesssss it seems like you are working on possibly the same or a related issue in https://github.com/Expensify/App/issues/18963. Do you want to keep an eye on this one or can it be merged with the issue you are assigned? I haven't really had time to give this any attention and I'm a bit out of my depth when it comes to Android performance stuff.

Julesssss commented 1 year ago

Since I have reverted the PR which disabled hardware acceleration, we now just need a solution to the low-res large image issue that is common on Android. Let's keep that discussion on this issue.

Julesssss commented 1 year ago

To confirm, the performance regression was located and reverted. We are now working on the image resolution issue.