improved speed for fetching app information #597
Conversation
backported fix for AndlyticsProject#512 to master
bumped version for minor release
backported fix for AndlyticsProject#562 to master
backported OAuth authenticator
Add link to alpha/beta versions and note that nightly builds are out of date
Merged dev and bumped to 2.6
added back full changelog
…le apps by fetching the app information in parallel rather than one app at a time.
|
How does this perform on a slow connection? |
| @@ -62,6 +62,9 @@ | |||
|
|
|||
| private ResponseHandler<String> responseHandler = HttpClientFactory.createResponseHandler(); | |||
|
|
|||
| // This is only used for synchronising fetchAppInfosAndStatistics() | |||
| private volatile int fetchAppInfoCounter; | |||
There was a problem hiding this comment.
Maybe use a java.util.concurrent.CountDownLatch instead of this plus the lock
There was a problem hiding this comment.
Thanks, I didn't know that existed.
|
Side point @nelenkov I know before we had a mess of throws 20 different exceptions, but maybe we should have at least a throws |
| final Object lock = new Object(); | ||
| fetchAppInfoCounter = 0; | ||
|
|
||
| for (final AppInfo app : apps) { |
There was a problem hiding this comment.
This could get silly for users with 20+, let alone those with 100+ apps (don't ask me why they have that many!), tbh even 10 threads in your case is quite high.
Probably need a ThreadPoolExecutor to keep the number down to a sensible amount (say 4/5 max, less on a slow connection).
There was a problem hiding this comment.
We could make the number of apps fetched at a time configurable, and manage it with a ThreadPoolExecutor.
That way users can control how many are updated at a time based on their own connection if it's a problem.
|
I've made some comments. In summary, fetching everything one at a time as we currently do isn't great, but it cannot be solved by simply running everything at once as it doesn't scale. It may be that we should ditch the concept of fetching all all info at once (except in the background syncs). And just update a few apps at once, this needs some thinking about. |
|
Oh and finally, any pull requests should be made against the |
Actually, that's the reason why I implemented it. I spend lots of time waiting for the whole thing to update because of my poor 2G connection (in Australia...). It performs quite well on a slow connection because it's only a small amount of data and the majority of the time I'm just waiting for data to be returned.
Hmm, I think your idea about a ThreadPoolExecutor will work better than breaking up the fetches into different intervals of the day. My take is that a user would check the stats for all their apps once or twice a day for an overview of what's happening. Updating it at different times would be somewhat confusing and unecessarily complicate things.
Sorry about that. Is there a way to change it or do I have to create another pull request? Thanks for taking the time to make some comments (and provide advice!). I'll fix some of those up now. |
- wrapped code inside of a try/finally block to prevent errors holding locks forever
|
Before going further with this, the app should be profiled to see where it takes the most time. If have proof that it's network, then you might try to optimize it. With most mobile connections the problem is latency -- it takes a long time to reach the server, getting data after that is usually more or less OK. So sending multiple requests in parallel might improve that a bit, but the number should be limited. Also, multithreading is hard :) Probably better to use Android tools (Handlers) to synchronize results. As for fetching everything at once, getting details only after select a particular app might help people with lots of app, so it's worth looking into. I'd like the mode to be selectable though, as I actually usually refresh manually and have background sync turned off. Maybe I should give it try though :) |
To enforce this we need to make all exceptions checked again, not sure if it's worth it. When running in a different thread, you generally need to catch all exceptions anyway, so it wont' help much here. Say you catch DevConsoleException, but not Exception, you'll still have a problem when something else goes wrong. |
I've tested this on both mobile connection and WiFi at work/home. There is a definite improvement in update times when doing it in parallel.
I understand your caution because this is my first commit towards the project and the patch affects core functionality. I am only working on this because it is a pain for me to wait up to a minute to fetch the updates one by one. I've already switched it to use a CountDownLatch and put the unlock in a "finally" block (which is working nicely).
As for someone with lots of apps, why would it be helpful to update only the apps they click into? I could definitely make this a user-selectable option though. One option keeps fetches it one-by-one as it currently does. The second option allows the app to make multiple (or all) requests at once.
I also have background sync off and refresh manually, but I use Andlytics to see how my all apps are going once or twice a day. It would be bothersome to click onto each of my apps to update the stats. |
|
By 'proof' I mean actual logs with actual timings, so we can see connecting took say 100 ms, fetching took 50 ms, parsing took 20ms, etc. As for threading, if you use Android messages you mostly don't need to bother with synchronization, as Handlers guarantee that everything is delivered to the same thread. |
|
Hmm, how would you go about getting those logs? Or would Log.debug() calls be sufficient? I'll submit the new code with the CountDownLatch fix. Personally I think it's simple enough to understand as the Handler code looks unnecessarily complicated (but I'll wait for the code review) |
Conflicts: AndroidManifest.xml res/raw/changelog.html src/com/github/andlyticsproject/console/v2/DevConsoleV2.java
My main issue with Andlytics is that it takes up to 30-45s to request information for my 10+ apps as it does this one by one.
fetchAppInfosAndStatistics() now runs in parallel rather than one app at a time.
With this patch it takes me about 10-15s.
Sorry for the space removals, it's an Eclipse setting.