search

hippware / rn-chat

MIT License
5 stars 0 forks source link

Battery Drain #1245

Closed mstidham closed 7 years ago

mstidham commented 7 years ago

Rz:

Issue: Battery feels hot and drains quickly.

Staging Baseline:

PROD Baseline:

southerneer commented 7 years ago

yeeeesh, this one's gonna be hairy.

  1. I would at least push this off until after the backend fixes go in for removing retracts and deleted bots from homestream. That'll help a fair bit by removing unnecessary network round-trips.
  2. The app will work a lot harder on continuous login/logout app open/close than it would in a less test-y situation.
    • We probably need to establish baselines of what constitutes "normal" usage.
    • We should also establish what constitutes an acceptable battery drain percentage during "normal" usage. Give us a target plz.
thescurry commented 7 years ago

@southerneer all fair points... we probably need to circle up as a group to discuss your 2nd point on batt baseline. @zavreb Lets discuss this one Friday or Monday.

southerneer commented 7 years ago

Another thought: the auth timeouts I was seeing around the time this ticket was created could also account for a spike in network activity. Collecting data over the course of a few days might be helpful too.

aksonov commented 7 years ago

Probably it is related to #1239 changes and should be solved by #1247

southerneer commented 7 years ago

@aksonov #1239 never got merged so it didn't make it into Staging or a codepush. Probably #1247 will help anyway, but it wouldn't be changing anything from that PR.

aksonov commented 7 years ago

What do you mean? I see this code in master so it was merged...

15 сент. 2017 г., в 21:01, Eric Kirkham notifications@github.com написал(а):

@aksonov #1239 never got merged so it didn't make it into Staging or a codepush. Probably #1247 will help anyway, but it wouldn't be changing anything from that PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aksonov commented 7 years ago

Okay, not 1239 but very cpu traffic intensive accumulateItems function..

15 сент. 2017 г., в 21:01, Eric Kirkham notifications@github.com написал(а):

@aksonov #1239 never got merged so it didn't make it into Staging or a codepush. Probably #1247 will help anyway, but it wouldn't be changing anything from that PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bengtan commented 7 years ago

Clarifying for QA:

There wasn't a specific change to the app to address this. However, some refactoring and potential improvements might have indirectly made this sufficiently better.

mstidham commented 7 years ago

This is still an issue on Staging. Staging Version: 1.47.0 (96) iPhone 7 Plus Version: 10.3.3

thescurry commented 7 years ago

@mstidham what version of the app/ios?

zavreb commented 7 years ago

Please hold, @mstidham is establishing a baseline with details.

zavreb commented 7 years ago

I provided some details @southerneer. @mstidham can you run a baseline with activity that only plays with HS. Message me on slack if you have questions.

mstidham commented 7 years ago

Staging Baseline:

Location setting: Always App Used: Only tr staging App Usage: 5 minutes % Lost: 4% (47% --> 43%) .8% Charging: No Brightness Level: 50% Activity: Scrolling on HS only PROD Baseline:

Location setting: Always App Used Only tr prod App Usage: 5 minutes % Lost: 1%, (42% --> 41%) 0.2% Charging: No Brightness Level: 50% Activity: Scrolling on HS only

irfirl commented 7 years ago

iPhone 6, iOS 11 Activity: staying on HS with minimal interaction Prod: 47% to 46% in 5 minutes (1%) Staging: 46% to 43% in 5 minutes (3%) Result: staging drains battery 3x faster than prod.

zavreb commented 7 years ago

Per @thescurry:

There’s still a major race condition going on in staging.

southerneer commented 7 years ago

In comparing the code between 1.45.4 (current prod version) and 1.47.3 (current Staging version) I don't see anything obviously off. The major differences are:

I did some energy profiling of each version of the app with my 5s and couldn't see any major differences. I'll try tomorrow with an iOS 11 test phone and maybe that will reveal something.

bengtan commented 7 years ago

What's our desired/expected outcome? In other words, what would be sufficient in order to consider this ticket resolved?

Apparently, 1.47.3-Staging takes up 3 times more power than 1.45.4-Prod. If we can get the Staging app to use about the same power as the Prod app, is that good enough?

Or do we think that even the Prod app uses too much power?

bengtan commented 7 years ago

Quoting @southerneer:

In comparing the code between 1.45.4 (current prod version) and 1.47.3 (current Staging version) I don't see anything obviously off.

...

I did some energy profiling of each version of the app with my 5s and couldn't see any major differences.

Those app versions are conflated with the server environments. It would be useful if we could get data points for the following two combinations:

1.45.4 (Staging) 1.47.3 or later (Prod)

That might help indicate whether there's a (indirect? tangential?) relationship with the server.

We can't test 1.47.3 (Prod) but we can try testing 1.45.4 (Staging). Someone wanna give it a try? (cc @zavreb, @mstidham).

bengtan commented 7 years ago

Here's the version history between 1.45.4 and 1.47.3.

Can someone [1] test 1.46.0 (Staging), followed by 1.46.1 (Staging) please?

[1] ie. Those who can reproduce the issue and have contributed metrics above.

thescurry commented 7 years ago

Isn't xcode also a diff between prod/staging currently?

bengtan commented 7 years ago

@thescurry:

That is true.

Eric had to upgrade to XCode 9 in order to release 1.47.0 or 1.47.1 (I'm not sure which but it's one of those).

Also, the whole 1.47.x series coincided with iOS 11.

It is possible that the change of IDE (ie. XCode) or the build environment (ie. iOS 11), or both, may be a cause of differences in battery/power consumption/performance. Very unlikely, and definitely not the first (or even the second or third) place to look, but possible.

bengtan commented 7 years ago

Actually, that would be interesting ... if we re-built 1.45.4 using XCode 9 and see if that somehow makes a difference ... but it would be one of the last places to look.

bengtan commented 7 years ago

Private message on Slack from @thescurry:

and to answer your question, the goal should be for us to shoot for prods battery performance.

Prod I think is reasonable.

we could make it better, but as a start... I like it.

mstidham commented 7 years ago

Staging Baseline: Location setting: Always App Used: Only tr staging App Usage: 5 minutes Charging: No Brightness Level: 50% Activity: Scrolling to End of Feed on HS then tapping tinyrobot to top of feed and repeat.

1.45.4 98% to 96% Loss of 2% 1.46.0 95% to 91% Loss of 4% 1.46.1 90% to 86% Loss of4% 1.47.0 85% to 81% Loss of 4%

bengtan commented 7 years ago

That's interesting/unusual. The only difference between 1.45.4 and 1.46.0 is: 

# 1.46.0 - 2017 September 14

* Firebase integration
* Onboarding w/Firebase: Verify your phone number screen (#1173)
* Onboarding w/Firebase: Enable a Country Code Library (#1177)
* Onboarding w/Firebase: Confirmation Code Screen (#1178)
* Onboarding w/Firebase: Resend Confirmation (#1179)

... which is weird.

@mstidham:

Could you please repeat your test with:

[1] Specifically, I'd like you to repeat 1.45.4 (Staging) when the battery percentage is lower than 90%. Just to see whether it's repeatable or a one-off anomaly.

mstidham commented 7 years ago

1.45.4 Staging 72% to 70% Loss of 2%

1.45.4 Production 69% to 67% Loss of 2% Production I did not do a clean install. I did do clean installs on all others.

bengtan commented 7 years ago

That's ... interesting. Seems like firebase is doing something.

Maybe it's partially contributing to the increased battery usage somehow ... unlikely though that may sound.

@mstidham:

Thanks for these measurements. It's another data point for @southerneer to consider.

southerneer commented 7 years ago

A couple things I noticed playing around with @thescurry 's phone yesterday:

I codepushed a change that should make the app clean up properly when backgrounded (check Staging-Eric) but @zavreb and @irfirl are still reporting significant battery drain while the app is open. Back to the drawing board...

southerneer commented 7 years ago

Another thing I'm noticing that I'm not sure was there before is xmpp disconnect errors like RNXMPPService: xmppStreamDidDisconnect:withError:. Probably unrelated because it doesn't seem to be affecting app performance...the XMPP connection closes as expected on the client side. cc @bengtan

southerneer commented 7 years ago

I've removed the FirebaseMessaging module and calls to react-native-mixpanel on the native side . Testing and profiling with my 5s saw better results initially but the phone was still chugging pretty hard after a bit...hard to tell if that's more a function of the old phone or our code. Testing on the white iPhone 6 saw a big improvement in energy usage. Pushing new version (1.48.0) now...

image

southerneer commented 7 years ago

Has anyone tested 1.45.4 with the current Staging backend setup (TestFlight -> tinyrobotStaging -> Previous Builds)? Most likely the battery drain is coming from front-end changes, but backend changes can also affect how the app uses network resources.

mstidham commented 7 years ago

@southerneer this is the results that I got 2 days ago.

image

bengtan commented 7 years ago

... which coincides with adding Firebase (which started from 1.46.0).

zavreb commented 7 years ago

@thescurry might have more specific details to provide with his new thermometer.

thescurry commented 7 years ago

Ok, here are my findings on "thermal load" on 3 different apps, (rz: keep in mind, scurry idled the phone with screen off for 5 min between takes):

Nextdoor: 88.4F startup/idle 87.7 with HS scroll to first lazy load 86.8 after 5 minutes of scrolling HS 88.4 in background for 1 min (phone screen turned off)

Instagram: 86.3F startup/idle 87.3 with HS scroll to first lazy load 86.6 after 5 minutes of scrolling HS 87.3 in background for 1 min (phone screen turned off)

tinyrobot: 90.1F startup/idle 91.9 with HS scroll to first lazy load 92.7 after 5 minutes of scrolling HS 91.8 in background for 1 min (phone screen turned off)

Clearly more thermal load with tinyrobot... not sure how useful this data is yet, but it clearly shows that we are indeed burning hotter than others with similar styled HS and user actions.

Also, for further clarity... I allowed the phone to cool back down for 5 min between each test. The tested starting temp for all three tests was ~86.1F

aksonov commented 7 years ago

@thescurry Thank for so interesting measurements.

Let's find version of tinyrobot that work OK (if it is possible). Is it 1.45.4? Probably we have to rollback and check what exact commit makes thing worse. Maybe Firebase, maybe new React Native version, etc.

If all tinyrobot versions have battery drain then it is different problem. Instagram doesn't actively use GPS like our app, as well as XMPP (parsing and network traffic takes additional CPU, also, as I said before, we are using many requests for home stream processing (separate request for every bot, two requests (one xmpp and one https) to get image, etc.). Also React Native could add more CPU processing.

I don't know nextdoor, but our app is much more complex comparing to instagram.

thescurry commented 7 years ago

Hello Pavel,

Yes, I was going to roll back to 1.45.4 and test, but I ran into the issue that twitter digits is now gone and I couldn't login. :( I would be happy to test this, but not certain how to proceed.

I had tested 1.45.4 previously and found it to run much cooler, with less battery drain (I would consider it acceptable), but that was before we could accurately measure temperature.

aksonov commented 7 years ago

What about bypass function for staging?

9 окт. 2017 г., в 20:24, Steve Curry notifications@github.com написал(а):

Hello Pavel,

Yes, I was going to roll back to 1.45.4 and test, but I ran into the issue that twitter digits is now gone and I couldn't login. :( I would be happy to test this, but not certain how to proceed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

zavreb commented 7 years ago

@mstidham is looking into this.

mstidham commented 7 years ago

No apps open, Brightness at 50%, Wifi-On, Bluetooth-On, Auto-Lock-Never, Low Power Mode-Off Testy Tester (191-9191)

Verify that user is showing online/connected. If user is showing offline send app to background (not a true background) and quickly bring it to foreground.

Leave HS open and scroll to bottom of feed and tap tinyrobot to be taken to the top of the feed for 5 minutes.

1.45.4 (91) drained 2% in 5 minutes 1.46.0 (93) drained 6% in 5 minutes

image

image

thescurry commented 7 years ago

Testing via the testytester account on staging with version v1.45.4 and Both starting at 86.1 degrees F. There are massive differences in the thermal signature between these two versions. Hopefully, this is helpful in diagnoses. I found HS scrolling to be one of the "hottest" moments of our apps thermal output. I'll see if I can find other "hot spots" within the app and if I do find them, I'll report them in a future ticket comment.

v1.46.0 tinyrobot: 90.7F startup/idle (from HS) 92.7 with HS scroll to first lazy load 101.1 after 5 minutes of scrolling HS (this did pop to the 103 range several times, but on average it was in the 101.1F range). 92.6 in background for 1 min (phone screen turned off)

v1.45.4 tinyrobot: 87.5F startup/idle (from HS) 87.6 with HS scroll to first lazy load 89.7 after 5 minutes of scrolling HS 88.8 in background for 1 min (phone screen turned off) 86.6 (killed app, turned off phone screen and waited 5 minutes)

Also:

@zavreb found this following article about measuring battery consumption. This would probably need to be executed by you @aksonov because it I believe it requires setup via xcode, dev tools, etc. Pavel could you please give this a try while working on diagnosing the battery drain issue?

https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/InstrumentsUserGuide/MeasuringEnergyImpact.html

bengtan commented 7 years ago

Something we could test for ... is whether firebase is using CPU to try to reach back to Google.

It's a complicated setup, but we could route a phone's wifi connection through a router, and then packet sniff the router. We wouldn't be able to see the contents of firebases's encrypted traffic, but we would be able to see if it was communicating (or trying to communicate) with the mothership, and how much traffic (or retries) it was doing.

Do you want to sanction such an investigation, @thescurry?

thescurry commented 7 years ago

@bengtan I'm open to every avenue that we have. We need this issue solved before submitting the app and this is really the one last major issue we have left to solve for before submitting. The more eyeballs we have looking at this particular issue, the better.

Maybe we could wire a laptop to the internet using a cable and use the built in wifi to host the phone and sniff traffic? :)

mstidham commented 7 years ago

Staging Version 1.53.1 The same test ran as above in comments and still got 6% battery drain in 5 minutes. 38% to 32% on battery.

image

aksonov commented 7 years ago

@mstidham As I wrote before HS loading/scrolling is very CPU intensive. 1.47 and later versions use more advances HS load that allow to load MORE items per minute. So could you just test another behaviour - 5 min of 'boarding' screen (screen after logout) for both 1.53.1 and earlier. And 5 min of HS screen without scrolling for both 1.53.1 and earlier. Or load equal number of events (not 'scrolling for 5 min!')

mstidham commented 7 years ago

Version 1.53.1 Boarding Screen 0% battery drain HS No Scroll (User not showing connected after clean install) 0% battery drain HS No Scroll (App from background, connected/online) 1% battery drain

Version 1.52.1 Boarding Screen 1% battery drain HS No Scroll 1% battery drain HS No Scroll (Connected/Online) 1% battery drain

I notice battery drain when bringing the app to the foreground and the HS refreshing/connecting (showing user as online). I have noticed a 2% drain in a matter of seconds as the HS refreshes, this is not consistent though.

zavreb commented 7 years ago

@thescurry thoughts? I've heard positive verbal remarks re: performance w/thermometer.

southerneer commented 7 years ago

It should also be noted that a "1% change" can be very misleading because we don't have hundredths or even tenths of a percent. For example, the app could go from 91.99% to 90.01% and that would appear as a 1% change to the iPhone user even though the "real" change was almost 2%. This would appear the same as going from 91.01% to 90.99%...a 1% change on the battery icon when the "real" change was just .02%. The best way around this is longer test runs where the minimum battery drain is 3% or more.

bengtan commented 7 years ago

Steve said to me that, after a while, the phone starts to heat up again. This is a different symptom because it heats up afterwards, not straight away.

Upon further investigation, this new symptom appears to be correlated with Steve appearing to be offline according to xmpp presence.

ie.

User shows to be Offline after initial install/load of the app #1324

thescurry commented 7 years ago

Yes, to further comment on what @bengtan said above: I start the app (phone usually starts out very close to 88.6F), wait a minute and then have someone check to see if I appear online (my presence). If my presence appears offline to others (no blue dot by my name), my phone heats up slowly (about 2 degrees per minute)... eventually getting very hot, above 101F while idle on the HS... this usually takes about 10-15 minutes, it's like the temp creeps upwards.

Now, if I open the app, wait a minute and then have someone check and my presence shows online... I don't see the same thermal event where the phone heats up. It's very strange, it's very inconsistent as well. With me showing online... I'm seeing temps of ~89F while idle on the HS. It makes it seem like this presence issue is causing the phone to heat up if users are online, yet showing an offline presence.

Hopefully, this info is helpful in some way. @aksonov I will likely be awake late tonight, sp if you have any questions, please ping me on slack... I'm happy to discuss further or test further.