Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 6 tasks
kbecciv opened this issue Apr 14, 2023 · 35 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Apr 14, 2023

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

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

Bug6017051_Staging_android.mp4

Production video

Bug6017051_PROD_android.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
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Apr 14, 2023
@OSBotify
Copy link
Contributor

👋 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
Copy link

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

@francoisl
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Author

kbecciv commented Apr 14, 2023

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

@marcaaron
Copy link
Contributor

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

@francoisl
Copy link
Contributor

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

  • On main
screen-20230413-183937.mp4
  • With hardware acceleration re-enabled (basically remove lines 13-14 of this PR)
screen-20230413-184404.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
Copy link
Contributor

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

  • With hardware acceleration enabled
android_olderversion.mov
  • On current main - animations feel less smooth
android_main.mov

cc @ArekChr do you know of a way we could re-enable hardware acceleration in #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.

@melvin-bot melvin-bot bot added the Overdue label Apr 17, 2023
@marcaaron
Copy link
Contributor

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.

@melvin-bot melvin-bot bot removed the Overdue label Apr 17, 2023
@mountiny
Copy link
Contributor

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?

@mountiny mountiny added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 17, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@mountiny
Copy link
Contributor

Given the agreement, I am removing the deploy blocker.

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

@fabioh8010
Copy link
Contributor

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

@ArekChr
Copy link
Contributor

ArekChr commented May 8, 2023

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
Copy link
Contributor

Thanks @ArekChr for the update!

@ArekChr
Copy link
Contributor

ArekChr commented May 10, 2023

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
Copy link

melvin-bot bot commented May 12, 2023

@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 melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff Overdue labels May 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 12, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 12, 2023

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

@aimane-chnaif
Copy link
Contributor

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?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels May 12, 2023
@ArekChr
Copy link
Contributor

ArekChr commented May 15, 2023

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

@Julesssss
Copy link
Contributor

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

@ArekChr
Copy link
Contributor

ArekChr commented May 17, 2023

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:

Nagranie.z.ekranu.2023-05-17.o.12.03.22.mov
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

@melvin-bot melvin-bot bot added the Overdue label May 19, 2023
@marcaaron
Copy link
Contributor

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 #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.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels May 19, 2023
@Julesssss Julesssss self-assigned this May 22, 2023
@Julesssss
Copy link
Contributor

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.

@melvin-bot melvin-bot bot removed the Overdue label May 22, 2023
@Julesssss
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests