ManojNimbalkar / bitcoin-wallet

Automatically exported from code.google.com/p/bitcoin-wallet
0 stars 0 forks source link

AutosyncService runs in a separate process #117

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Although my phone has been unplugged for hours and there's no user-visible UI 
showing, AutosyncService has not stopped.

Original issue reported on code.google.com by mh.in.en...@gmail.com on 23 Aug 2012 at 12:21

GoogleCodeExporter commented 9 years ago
This service is actually never stopped. It is meant to listen to the various 
events that start or stop autosyncing (but doesn't sync itself).

Original comment by andreas....@gmail.com on 23 Aug 2012 at 12:31

GoogleCodeExporter commented 9 years ago
You're listening for when the phone is plugged in, right?

Original comment by mh.in.en...@gmail.com on 23 Aug 2012 at 2:01

GoogleCodeExporter commented 9 years ago
Looking at the code, that's indeed what you do.

From 
http://developer.android.com/reference/android/content/Intent.html#ACTION_POWER_
CONNECTED

"Broadcast Action: External power has been connected to the device. This is 
intended for applications that wish to register specifically to this 
notification. Unlike ACTION_BATTERY_CHANGED, applications will be woken for 
this and so do not have to stay active to receive this notification."

It's not cool to run a service all the time, you just suck up system resources 
(btw, this came up in a Google-internal discussion of why apps so often run 
background services). What's the reason you can't have the service be started 
by this intent, run the sync, and stop itself when the sync is over?

Original comment by mh.in.en...@gmail.com on 23 Aug 2012 at 2:04

GoogleCodeExporter commented 9 years ago
The only resource this uses is memory. There is no thread running or something 
like this.

If you look at AutosyncService.check(), the logic is a bit more complex than 
what you describe. State is being kept (a stopped service looses state). A 
prefs item is being checked and listened to. There is some exponential backing 
off logic implemented. A WIFI lock is aquired. I expect there will be more 
logic in future.

I first tried to use just a BroadcastReceiver for the autosync logic but did 
not succeed because it does not have a complete lifecycle.

Original comment by andreas....@gmail.com on 23 Aug 2012 at 2:30

GoogleCodeExporter commented 9 years ago
Well, but memory is the most constrained resource on a phone.

In the case where shouldRunning == false, why can't you do a service.stop() at 
that point? The pref will be loaded from disk next time there's a charging 
event.

I don't see why acquiring a wifi lock or doing backed-off retries requires the 
service to live forever. When there's no work to do, it can stop itself and 
free up the memory for other apps. Otherwise just installing the app will make 
the whole phone run slower and that seems to encourage users to uninstall it.

Original comment by mh.in.en...@gmail.com on 23 Aug 2012 at 2:34

GoogleCodeExporter commented 9 years ago
One of the reasons is that I want to listen for the preference change, and a 
stopped service can't to that. Honestly, we are talking about one instance of a 
class with 9 fields. Do you really think this will be a problem?

Original comment by andreas....@gmail.com on 23 Aug 2012 at 2:47

GoogleCodeExporter commented 9 years ago
The preference can't change if the app isn't running, so why does the service 
need to be always-on to listen to that? You could just bind to it, if you need 
that, but I don't understand why you need to run check() when the pref changes. 
If the app is open and in the foreground then it's already tried/trying to sync.

A process (any process) has a significant amount of fixed overhead that is paid 
regardless of what Java classes are instantiated.

The AutoSyncService process, on my phone, is consuming 5.6mb of RAM - for 
nothing. I have a Galaxy Nexus which is very high end by any measure (it has a 
gig of RAM), so it's not a big problem for me because I still have a lot of 
free RAM that can be used to cache background processes and speed up 
multi-tasking. However many phones, especially cheaper/older/lower-end phones 
popular in poorer countries like China, don't have so much RAM:

http://en.wikipedia.org/wiki/Comparison_of_Android_devices

512 or 256mb of RAM is common. Most of it goes on foreground processes and the 
operating system itself. In such an environment, 5mb of RAM can make the 
difference between "switching apps is instantaneous" and "switching apps takes 
several seconds". So yes, app devs need to have a good reason to keep a service 
alive in the background. 

Original comment by mh.in.en...@gmail.com on 23 Aug 2012 at 2:55

GoogleCodeExporter commented 9 years ago
I'll reopen this issue on the basis that the service should not run in its own 
process. That is not what's intended.

Original comment by andreas....@gmail.com on 23 Aug 2012 at 3:26

GoogleCodeExporter commented 9 years ago
Hmmm, taken from http://developer.android.com/reference/android/app/Service.html

A Service is not a separate process.

A Service is not a thread.

Where do you see that AutosyncService is using an own Process that is using 5.6 
MB of RAM?

Original comment by andreas....@gmail.com on 23 Aug 2012 at 3:30

GoogleCodeExporter commented 9 years ago
btw. 256 MB RAM devices all run on Android 2.2 and below, which is unsupported 
by BitCoinJ anyway. Most 2.3+ devices run on at least 768 MB of RAM.

Original comment by andreas....@gmail.com on 23 Aug 2012 at 3:34

GoogleCodeExporter commented 9 years ago
In the "Running" section of the apps section of settings. If you look at the 
Moto Droid phones, they all run 2.3 (have been upgraded) and have only 512mb of 
RAM (which is shared for everything). More to the point, if a user has only a 
few apps like yours running, that's an entire foreground processes worth of RAM 
that gets burned. Some high profile apps are notoriously bad at this (like 
Facebook!) which basically slows down the entire operating system and makes 
phones artificially more expensive.

OK, so here's how it works.

A Service is a Java object, it's basically an abstraction. It receives messages 
from the system and has a lifecycle.

But ultimately a Service is just code. It's just a class that receives method 
calls. For it to do anything it must live inside a process (all code lives 
inside a process). If you have a service that is running, it is keeping a 
process alive, period, and that means a garbage collected heap, some Dalvik 
overhead, etc. A process is itself just a container for threads. Every process 
must have at least one thread - no operating system lets you have a process 
which doesn't have any threads.

What's actually happening behind the scenes is something like this:

class SomeInternalAndroidClass {
  public static void main(String args) {
    while (true) {
      message = waitForMessageFromSystem();
      if (message.isForService()) {
        Service service = findServiceForMessage(message);
        service.doSomething(message);
      }
    }
  }
}

Obviously that's not what the code really looks like (it's based on loopers and 
the Android RPC system), but you get the idea. If your service isn't doing 
anything, the thread is sitting inside "waitForMessageFromSystem" and taking up 
memory for the process heap, the thread stack, kernel control data structures 
and so on.

When you call stop(), that main loop notices there are no more services or 
activities left in the system and the process goes away (or rather, can go away 
- the system chooses when to get rid of it based on what the use is doing). The 
RAM can then be used to keep around processes that the user might switch back 
to. But if you never call stop, it won't do that.

Original comment by mh.in.en...@gmail.com on 23 Aug 2012 at 3:39

GoogleCodeExporter commented 9 years ago
I see, so the main problem is the service keeps the apps main process alive.

Moving AutosyncService back into AutosyncReceiver - which I would actually 
prefer - is not possible because of the WifiLock that has to be held onto.

AutosyncService could probably stop itself in the "else" branch in check(), but 
would then miss the prefs change. You are right in that this might be redundant 
anyway, but on the other hand a service should not assume anything about the 
other components. Also, this will solve one one half of the problem, because 
the service will still stay running at least until unplugged.

btw. I think developers assume that what they are doing is ok if its not worse 
than the Google Apps. Maps is always using up 20 MB in 2 processes and 3 
services...

Original comment by andreas....@gmail.com on 23 Aug 2012 at 4:23

GoogleCodeExporter commented 9 years ago
Will move WifiLock over to BlockchainService as a first step. This is where is 
belongs anyway. Was just afraid putting it there because it only applies to 
autosync, not to user-requested sync.

Original comment by andreas....@gmail.com on 23 Aug 2012 at 5:16

GoogleCodeExporter commented 9 years ago
Arrrgh, Android does not allow checking the power state from a receiver. There 
broadcasts are really the ugliest part of Android.

Original comment by andreas....@gmail.com on 23 Aug 2012 at 9:12

GoogleCodeExporter commented 9 years ago
Why would the service stay running until unplugged? The state change broadcast 
is a one-time thing, right?

Are you sure you can't check power state?

http://developer.android.com/training/monitoring-device-state/battery-monitoring
.html

It looks like the following magic incantation will get the data, albeit in a 
weird intentified format:

IntentFilter ifilter = new IntentFilter(Intent.ACTION_BATTERY_CHANGED);
Intent batteryStatus = context.registerReceiver(null, ifilter);

The Google apps are written by regular developers who don't have any special 
access or special knowledge, so their quality does vary unfortunately. Maps is 
a bit of a special case because I believe it actually replaces the system 
location service with a bit of custom magic: a better example would be 
comparing the G+ app with the Facebook app. G+ has services but it's very good 
at ensuring they don't hang around forever.

Original comment by mh.in.en...@gmail.com on 24 Aug 2012 at 11:29

GoogleCodeExporter commented 9 years ago
I currently do the following:

// determine initial power connected state
final Intent batteryChanged = context.registerReceiver(null, new 
IntentFilter(Intent.ACTION_BATTERY_CHANGED));
final int batteryStatus = 
batteryChanged.getIntExtra(BatteryManager.EXTRA_STATUS, -1);
final boolean isPowerConnected = batteryStatus == 
BatteryManager.BATTERY_STATUS_CHARGING || batteryStatus == 
BatteryManager.BATTERY_STATUS_FULL;

If I try that in a receiver, it complains about not being able to register are 
receiver in a receiver, which is understandable. Unfortunately, I do not want 
to register a receiver (argument is null).

Original comment by andreas....@gmail.com on 24 Aug 2012 at 5:37

GoogleCodeExporter commented 9 years ago
Oh, suck. That sounds like a bug in the framework. I'll make sure Diane sees 
this and hopefully it'll get fixed - maybe there's a workaround too.

Original comment by mh.in.en...@gmail.com on 24 Aug 2012 at 6:19

GoogleCodeExporter commented 9 years ago
FYI:
http://stackoverflow.com/questions/3691579/getting-battery-level-at-android-widg
et

Widgets are similar in that are also implemented using a BroadcastReceiver.

I'll try the getApplicationContext() workaround described in the stackoverflow.

Original comment by andreas....@gmail.com on 25 Aug 2012 at 7:56

GoogleCodeExporter commented 9 years ago
Hey, did you ever get a chance to try the getApplicationContext() workaround?

Original comment by mh.in.en...@gmail.com on 6 Dec 2012 at 3:00

GoogleCodeExporter commented 9 years ago
Yes, the workaround seems to work and AutosyncService has been migrated back 
into an AutosyncReceiver, thus not keeping a process running.

This is released in version 2.22, if I read my git history right.

Thanks for your input on this issue!

Original comment by andreas....@gmail.com on 6 Dec 2012 at 3:25