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

put file access into background thread? #6

Closed
wants to merge 347 commits into
base: master
from

Conversation

7 participants
@yulin2

yulin2 commented Oct 17, 2014

Hi, I'm doing research on performance for Android apps. I found two event handlers access disk (read/write files) from UI thread, but Android docs suggest us to avoid blocking calls in UI thread. Do you think they may lead to any responsiveness issues?

I tried to refactoring by putting them into AsyncTask. Looking forward to see your comments.

andreynovikov and others added some commits Sep 27, 2012

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 30, 2014

I sent this pr several days ago. Any idea/comments?

yulin2 commented Oct 30, 2014

I sent this pr several days ago. Any idea/comments?

@palant

This comment has been minimized.

Show comment
Hide comment
@palant

palant Oct 30, 2014

Contributor

@rjeschke, could you have a look?

Contributor

palant commented Oct 30, 2014

@rjeschke, could you have a look?

@rjeschke

This comment has been minimized.

Show comment
Hide comment
@rjeschke

rjeschke Oct 31, 2014

Contributor

In this case, the responsiveness issues don't originate from the disc access, but you're right, Android rules are: no IO on the UI thread.

Your changes to AdblockPlus.java are okay, also you have some indentation issues (e.g. look at the do block starting in line 434)

The changes you did to Preferences.java could lead to a false negative detection for a rooted phone. AsyncTask is, as the name suggests, asynchronous, so you don't know when the action is performed or finished. This could lead to onResume called before copyAssets finished, which will cause a failure in the rooted-phone-detection (as iptables might not be available at this moment).

Solutions I see for this issue are:

  • blocking until the async task finishes (which is pretty much counterproductive to what we want to achieve)
  • delay the actions in onResume depending on if copyAssets is finished or not by using another async task
Contributor

rjeschke commented Oct 31, 2014

In this case, the responsiveness issues don't originate from the disc access, but you're right, Android rules are: no IO on the UI thread.

Your changes to AdblockPlus.java are okay, also you have some indentation issues (e.g. look at the do block starting in line 434)

The changes you did to Preferences.java could lead to a false negative detection for a rooted phone. AsyncTask is, as the name suggests, asynchronous, so you don't know when the action is performed or finished. This could lead to onResume called before copyAssets finished, which will cause a failure in the rooted-phone-detection (as iptables might not be available at this moment).

Solutions I see for this issue are:

  • blocking until the async task finishes (which is pretty much counterproductive to what we want to achieve)
  • delay the actions in onResume depending on if copyAssets is finished or not by using another async task
@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 31, 2014

Nice to hear from you. I fix the indentation in AdblockPlus.java. For Preferences.java, I didn't notice the problem you mentioned. Which line in onResume needs to synchronize with copyAssets?

The first solution you propose can work (just invoke AsyncTask.get() in onResume). I agree this solution may block onResume when you first load the AdblockPlus.

But I don't quite understand the second solution. It still needs to wait the AsyncTask to finish?

yulin2 commented Oct 31, 2014

Nice to hear from you. I fix the indentation in AdblockPlus.java. For Preferences.java, I didn't notice the problem you mentioned. Which line in onResume needs to synchronize with copyAssets?

The first solution you propose can work (just invoke AsyncTask.get() in onResume). I agree this solution may block onResume when you first load the AdblockPlus.

But I don't quite understand the second solution. It still needs to wait the AsyncTask to finish?

@rjeschke

This comment has been minimized.

Show comment
Hide comment
@rjeschke

rjeschke Oct 31, 2014

Contributor

The two points I specified were two different solutions.

If we choose point 1 (blocking), then there's no need to change the original code, as both solutions would block the UI thread (and not may block, but definitely block).

The second solution (delay/2nd async task) would be the clean solution as it makes the whole startup process asynchronous (though it does require a lot more code changes).

Contributor

rjeschke commented Oct 31, 2014

The two points I specified were two different solutions.

If we choose point 1 (blocking), then there's no need to change the original code, as both solutions would block the UI thread (and not may block, but definitely block).

The second solution (delay/2nd async task) would be the clean solution as it makes the whole startup process asynchronous (though it does require a lot more code changes).

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 31, 2014

For the first solution, we still get some concurrency between the async task and onStart. That's why I said may block. But I agree it's not good enough.

For the second solution, I still don't understand your thought based on your short description, so I can't do any change for it right now. How about I remove the changes in Preferences.java so you can merge the first refactoring?

yulin2 commented Oct 31, 2014

For the first solution, we still get some concurrency between the async task and onStart. That's why I said may block. But I agree it's not good enough.

For the second solution, I still don't understand your thought based on your short description, so I can't do any change for it right now. How about I remove the changes in Preferences.java so you can merge the first refactoring?

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 31, 2014

I remove the second async refactoring so you can merge the one in AdblockPlus.java. If you implement solution 2 for Preferences.java by any chance, let me know and I'm interested in how to do that:)

yulin2 commented Oct 31, 2014

I remove the second async refactoring so you can merge the one in AdblockPlus.java. If you implement solution 2 for Preferences.java by any chance, let me know and I'm interested in how to do that:)

@fhd fhd closed this Aug 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment