Closed m-natarajan closed 8 months ago
Job added to Upwork: https://www.upwork.com/jobs/~01dd3351f4b07d85c0
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External
)
Triggered auto assignment to @laurenreidexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
: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:
Triggered auto assignment to @marcaaron (Engineering
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
Trying to repro now.
It seems like a possible memory leak or something. I do notice things get slightly more janky. But doesn't seem like a blocker if the only way to reproduce it is to go to this particular page and background the app.
@hoangzinh, @laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Asking here to get another repro of this
@laurenreidexpensify , snaggin' from you since I have an iPhone and am able to reproduce, at least the part where the scroll isn't smooth.
https://github.com/Expensify/App/assets/22508304/62851b28-8abf-4f93-93fe-848af93c40ba
Also... reporting back that I didn't experience any heat issues or noticeable battery drain.
Waiting for proposals
@hoangzinh, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Seeking proposals on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1709046872674669
Upwork job price has been updated to $1000
Thanks for posting in #expensify-open-source @hoangzinh Doubling price to get eyes and proposals.
Does only iOS have this problem?Does Android work smoothly?
📣 @wustwg! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: wg940412478@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01eec1faf4fd4ceb3c?viewMode=1
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
I also feel a little laggy on my Android device. But @m-natarajan could you confirm if this issue also happens on Android devices? Thanks
Contributor details Your Expensify account email: poppies.great_0r@icloud.com Upwork Profile Link: https://www.upwork.com/freelancers/~0107f7a8dc488440a0
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
Performance degradation occurs when viewing the Troubleshoot screen on iOS.
The Troubleshoot screen is presented using transparentModal
. This causes the About screen's animation to continue to play as the Troubleshoot screen overlays the About screen.
Update the presentation type for the Troubleshoot screen to modal
. This will prevent rendering the About screen while the Troubleshoot screen is displayed.
Tested playing/pausing animations based on useFocusEffect
, but this introduces stutter when an animation is resumed.
It might be worth optimizing the Coin.lottie
(569KB) and Desk.lottie
(165KB) animations. Apart from ExpensifyLounge.lottie
(313KB), all other animations are <100KB.
Performance and heating issues on the Troubleshoot page if we put the app into background state then back to foreground.
[!NOTE] The issue is only happening on native devices because the navigation is stacking the screens (in memory). I wasn't able to reproduce on Android, only on iOS.
This happens because on Native when we navigate to a new screen, the previous screen remains in the navigation stack. Each new screen is added (pushed) onto the top of the stack, and when the user navigates back, the current screen is removed (popped), revealing the previous screen.
The problem in our case is that some of these pages contain Lottie animations and because the screens are kept in memory as we navigate (ex. from About to Troubleshoot), both pages animations never unmount when transitioning the app back from background to foreground - which increases the load on the UI thread significantly.
We can handle this by creating a native platform specific hook called useAppState
which we will use within the Lottie/index.tsx
component.
Which we will add to the Lottie component like so:
// ...existing logic
const appState = useAppState();
// If the image fails to load or app is in background state, we'll just render an empty view
// using the fallback in case of a Lottie error or to prevent memory leaks.
if (isError || appState.isBackground) {
return <View style={aspectRatioStyle} />;
}
// ...existing logic
By doing this we unmount the animations when the app goes into background, and mount them again when the app is foregrounded - this way we take the load off the UI thread and get rid of both performance and heating issues.
@hoangzinh @mallenexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@tamachai @ikevin127 Thanks for proposals everyone. My MAC has a problem when connect to an external device so I can't verify proposals on my ios device atm, please bear with me. I will sort it out soon.
@tamachai @ikevin127 Thanks for proposals everyone. Both proposals look great, especially @ikevin127 with details root cause. I have a few concerns about solutions:
@tamachai: Update the presentation type for the Troubleshoot screen to modal
I'm worried about this approach potentially causing regression bugs. We must have reasons of using transparentModal
. There is a PR change to use card
presentation but has been reverted https://github.com/Expensify/App/pull/37305.
About your alternative solutions, could you elaborate more about playing/pausing animations based on useFocusEffect
? Thanks
@hoangzinh With the solution I proposed that is the trade-off since we're re-mounting the animation.
This load delay also happens first time you open the app and navigate to one of the screens where an animation exists - so it makes sense it would also exist when bringing the app back from background - which means all processes restart / re-mount.
I can look into more but for now just assume this is as good as it gets considering the RCA.
@ikevin127 instead of based on app state, could you test & compare with useIsFocused
hook? (hide/display based on isFocused state)
@tamachai @ikevin127 Thanks for proposals everyone. Both proposals look great, especially @ikevin127 with details root cause. I have a few concerns about solutions:
- @tamachai:
Update the presentation type for the Troubleshoot screen to modal
I'm worried about this approach potentially causing regression bugs. We must have reasons of usingtransparentModal
. There is a PR change to usecard
presentation but has been reverted revert: native stack #37305. About your alternative solutions, could you elaborate more aboutplaying/pausing animations based on useFocusEffect
? Thanks- @ikevin127 When I watch your recordings, when the app is foregrounded, the animation doesn't load in a short period. Is there any way we can avoid it? Thanks
card
uses OS defined animations. Whereas modal
and transparentModel
use the same animations as each other but have distinct settings to control the visibility of the header and previous screen. It's best to avoid transparent modal unless the app needs to render the screen below the current, such as when overlaying modals on iPad or using effects like blur. For reference, here's the view hierarchy with transparentModal
vs modal
.
transparentModal:
modal:
When I experimented playing/pausing the animation via useFocusEffect
, a stutter was introduced during view transition, as the animation begins to play.
Issue not reproducible during KI retests. (First week)
@tamachai if we use modal
, will it keep the previous state if we navigate back? For example, previously we had an expectation here https://github.com/Expensify/App/issues/36077
playing/pausing the animation via
useFocusEffect
Could you elaborate on how would you playing/pausing
?
Issue not reproducible during KI retests. (First week)
@mallenexpensify my iPhone is broken so I couldn't verify atm. Could you help to check if we can still reproduce this issue in the latest version? Thanks.
Reproducible for me on 1.4.47. It might not be clear in the vid but the scrolling seemed or felt jumpy
https://github.com/Expensify/App/assets/22508304/77e8a3a8-163c-4cac-ae5b-e708562017d4
@tamachai if we use
modal
, will it keep the previous state if we navigate back? For example, previously we had an expectation here #36077
Yes, it should behave the same way. It doesn’t actually alter the view stack. An issue would occur if the modal wasn’t full screen, but this isn’t the case. Maybe we can look at only applying this to the settings and troubleshooting screens and then verify there are no regressions.
playing/pausing the animation via
useFocusEffect
Could you elaborate on how would you
playing/pausing
?
LottieView exposes a ref prop that can be used to control the view via its imperative API. Using useFocusEffect we can call play/pause, depending on if the screen hosting the Lottie view is focused. The issue here is that starting playback introduces stutter. This stutter is especially noticeable when swiping back to the previous screen.
@ikevin127 @tamachai could you guys try to use useFocusEffect
to hide/show lotte view and share recordings results here? So we can know which approach gives better results. Thanks
@hoangzinh I tried that but for my proposal it didn't work since (IMO) our issue has to be tackled based on the AppState change (background/foreground) and not whether the screen with animation is focused or not.
As explained by the other proposal's author, because the way our navigation works currently - there's no page focus change when the app goes into background and comes back to the foreground, hence we cannot use that to tackle the issue as you suggested.
@ikevin127 Thanks for testing. I did think that, at least, we can avoid Lotte animation on hidden pages.
@ikevin127 @tamachai could you guys try to use
useFocusEffect
to hide/show lotte view and share recordings results here? So we can know which approach gives better results. Thanks
Sure thing. I’ll try to put something together today.
Has anyone confirmed if there is battery drain or if performance is affected. Wonder if this purely aesthetic. Asking because I'm wanting to assign this too a VIP/Wave project.
I can confirm that when the UI thread gets really heavy upon resuming app from background to foreground -> if waiting for 3+ minutes the phone heat increases gradually w/ every minute which also drains battery faster than usual.
Thanks @ikevin127 , added to #vip-vsb
@hoangzinh @mallenexpensify 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!
if waiting for 3+ minutes the phone heat increases gradually w/ every minute which also drains battery faster than usual
Hi @ikevin127 your approach won't fix this case, will it? Because your approach is only applied when the app is changed state from background to foreground 🤔
@hoangzinh It does fix the issue - both heating and heavy animation problems were gone when I tested my solution.
Again, as explained in my proposal's RCA - the lottie animations never stop when the app goes into the background, causing memory leak - the issue occurs because when the app reopens (from background state), we are presented with all that in-memory build-up - which causes our heavy UI / overheating issues.
What my solution does is un-mount the animation as soon as the app goes into the background and re-mounts it when the app is brought back to the foreground.
The only tradeoff of the solution being that there's a bit of delay until animation is rendered, depending on animation size - which we already have currently when first navigating to any screen with Lottie animation.
@tamachai @ikevin127 Thanks for coming with me on this issue. I appreciate it. After playing around with solutions, @ikevin127 proposal looks good to me. The drawback of his solution is there Lottie view is not displayed a second when the app goes to forgeground. But I think it does not really matter because eventually it's displayed perfectly. Moreover, if we can combine useIsFocused
and useAppState
hooks, I think it would improve performance here. I.e here
if (isError || !isFocused || appState.isBackground) {
return <View style={aspectRatioStyle} />;
}
Link to @ikevin127 proposal https://github.com/Expensify/App/issues/36645#issuecomment-1971281335 🎀👀🎀 C+ reviewed
Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Thank you all for your help with this bug. I have a follow-up question regarding @ikevin127's analysis of the issue.
The Lottie animation doesn't unmount or stop when the app moves to the background or when the screen (i.e., the stack) is hidden behind another during navigation.
I have two questions:
react-freeze
. Do we know why this isn't resolving the performance issue when hidden stack screens have Lottie animations?@hayata-suenaga Here's my take:
Are we certain that the Lottie library doesn't manage the animation state when the app moves to the background?
TLDR: The problem lies in the way our screens are stacked when transitioning the app to background state and back, not an issue with the Lottie library.
Lottie does manage animation in terms of app state by keeping record of the animation timeline and resuming, according to this lottie-react-native
merged PR - but other than that there's no un-mount / re-mount functionality for edge cases like ours (navigation stack transparentModal
+ app state change with Lottie animation ongoing).
We use react-freeze. Do we know why this isn't resolving the performance issue when hidden stack screens have Lottie animations?
As mentioned in our docs here:
react-freeze
allows us to increase performance by avoiding unnecessary re-renders of screens that aren’t visible to the user anyway.
This only prevents re-renders but since in our edge case there's no re-rendering happening, but only animation related behaviour (UI thread) - which does not help when bringing app back from background to the foreground and processes resume.
Also, in our case our react-freeze
wrapper only applies to screens that are not focussed, but when the Troubleshoot page goes into the background - the screen remains focused, bringing the problem back to the app state transition.
This is why we need the proposed solution which un-mounts and re-mounts the animation component in order to actually fix this performance issue.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Issue was found when executing #35306 Version Number: 1.4.42.0 Reproducible in staging?: y Reproducible in production?: New feature 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 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:
Action Performed:
Expected Result:
Swiping should be smooth. The phone shouldn't heat up or crash.
Actual Result:
Low performance when I foreground the app. The phone heats up if I stay on the page. It even crashed once. The issue disappears if I navigate back and to the "Troubleshoot" again.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/00fb81f1-f0b8-4114-890f-a6ca371e338d
View all open jobs on GitHub
Upwork Automation - Do Not Edit