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

potential improvement for responsiveness in keepassdroid #43

Merged
merged 2 commits into from Mar 23, 2014

Conversation

Projects
None yet
4 participants
@yulin2
Contributor

yulin2 commented Feb 17, 2014

Hello developers of KeePassDroid,

I'm a Ph.D. student and I'm doing research related to Android apps'
responsiveness. KeePassFroid uses "ProgressTask" to execute long
operations in backgroud thread to improve responsiveness. However, I
found there are still some cases that the database is accessed/queried
in UI thread. Do they affect the performance/responsiveness of the
app?

For example, in FileSelectActivity.java, the "fileHistory.deleteFile"
is invoked at line 400 in "onContextItemSelected", and the DB is
eventually accessed. How about put them into AsyncTask? I attach a
sample patch here to show this. Note that after put it into AsyncTask,
there are data races on "fileHistory", since "fileHistory" is also used in
other threads at several places (e.g., line 250, line 337). Thus, the
"fileHistory.init()" can be invoked concurrently, we could add
synchronization on it. I put line 287, 288 into AsyncTask as well.

Similarly, I also transformed PasswordActivity.java and put
"getKeyFile" method (which access DB) into AsyncTask.

There may be some other places that can be improved. What do you think
about these improvements? My thought is we can improve the
responsiveness if we try to avoid the access to DB in UI thread.

Thanks,
Yu

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Mar 4, 2014

Contributor

Hello developers of keepassdroid,

I sent these patches two weeks ago. Do you have any comments on them? Will the refactorings improve the responsiveness of keepassdroid.

Thanks,
Yu

Contributor

yulin2 commented Mar 4, 2014

Hello developers of keepassdroid,

I sent these patches two weeks ago. Do you have any comments on them? Will the refactorings improve the responsiveness of keepassdroid.

Thanks,
Yu

@vceron

This comment has been minimized.

Show comment
Hide comment
@vceron

vceron Mar 5, 2014

maybe pinging @bpellin ?

vceron commented Mar 5, 2014

maybe pinging @bpellin ?

@brwolfgang

This comment has been minimized.

Show comment
Hide comment
@brwolfgang

brwolfgang Mar 5, 2014

It's sad to see a project dying with so much work to do. I was thinking about forking this and implement some necessary changes, such as visual modernization to Jellybean and up, but now that
I see what a trouble it can be to contribute to this project I'm moving out. Good luck @yulin2

brwolfgang commented Mar 5, 2014

It's sad to see a project dying with so much work to do. I was thinking about forking this and implement some necessary changes, such as visual modernization to Jellybean and up, but now that
I see what a trouble it can be to contribute to this project I'm moving out. Good luck @yulin2

@bpellin

This comment has been minimized.

Show comment
Hide comment
@bpellin

bpellin Mar 9, 2014

Owner

These operations aren't terribly long running, but moving them off the UI thread will improve responsiveness.

The Toasts need to be made on the UI thread. Also some of the code moved to the background task is manipulating the UI. I believe this also needs to be done on the UI thread.

Owner

bpellin commented Mar 9, 2014

These operations aren't terribly long running, but moving them off the UI thread will improve responsiveness.

The Toasts need to be made on the UI thread. Also some of the code moved to the background task is manipulating the UI. I believe this also needs to be done on the UI thread.

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Mar 11, 2014

Contributor

Thanks for the confirmation. In this patch, I don't put "Toasts" in doInBackground, "Toasts" is put in onPostExecute so it's still executed in UI thread.

Do you plan to merge this pull request some time?

Contributor

yulin2 commented Mar 11, 2014

Thanks for the confirmation. In this patch, I don't put "Toasts" in doInBackground, "Toasts" is put in onPostExecute so it's still executed in UI thread.

Do you plan to merge this pull request some time?

@bpellin

This comment has been minimized.

Show comment
Hide comment
@bpellin

bpellin Mar 22, 2014

Owner

I tested with these changes, and after selecting a file name I immediately get a crash when launching the PasswordActivity:
E/AndroidRuntime( 2323): at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2790)
E/AndroidRuntime( 2323): at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:2819)
E/AndroidRuntime( 2323): at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2266)
E/AndroidRuntime( 2323): at android.app.ActivityThread.access$600(ActivityThread.java:141)
E/AndroidRuntime( 2323): at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256)
E/AndroidRuntime( 2323): at android.os.Handler.dispatchMessage(Handler.java:99)
E/AndroidRuntime( 2323): at android.os.Looper.loop(Looper.java:137)
E/AndroidRuntime( 2323): at android.app.ActivityThread.main(ActivityThread.java:5103)
E/AndroidRuntime( 2323): at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime( 2323): at java.lang.reflect.Method.invoke(Method.java:525)
E/AndroidRuntime( 2323): at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
E/AndroidRuntime( 2323): at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
E/AndroidRuntime( 2323): at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime( 2323): Caused by: java.lang.NullPointerException
E/AndroidRuntime( 2323): at com.keepassdroid.PasswordActivity.onResume(PasswordActivity.java:167)
E/AndroidRuntime( 2323): at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1192)
E/AndroidRuntime( 2323): at android.app.Activity.performResume(Activity.java:5211)
E/AndroidRuntime( 2323): at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2780)
E/AndroidRuntime( 2323): ... 12 more
W/ActivityManager( 1112): Force finishing activity com.android.keepass/com.keepassdroid.PasswordActivity
W/ActivityManager( 1112): Force finishing activity com.android.keepass/com.keepassdroid.fileselect.FileSelectActivity

Owner

bpellin commented Mar 22, 2014

I tested with these changes, and after selecting a file name I immediately get a crash when launching the PasswordActivity:
E/AndroidRuntime( 2323): at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2790)
E/AndroidRuntime( 2323): at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:2819)
E/AndroidRuntime( 2323): at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2266)
E/AndroidRuntime( 2323): at android.app.ActivityThread.access$600(ActivityThread.java:141)
E/AndroidRuntime( 2323): at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256)
E/AndroidRuntime( 2323): at android.os.Handler.dispatchMessage(Handler.java:99)
E/AndroidRuntime( 2323): at android.os.Looper.loop(Looper.java:137)
E/AndroidRuntime( 2323): at android.app.ActivityThread.main(ActivityThread.java:5103)
E/AndroidRuntime( 2323): at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime( 2323): at java.lang.reflect.Method.invoke(Method.java:525)
E/AndroidRuntime( 2323): at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
E/AndroidRuntime( 2323): at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
E/AndroidRuntime( 2323): at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime( 2323): Caused by: java.lang.NullPointerException
E/AndroidRuntime( 2323): at com.keepassdroid.PasswordActivity.onResume(PasswordActivity.java:167)
E/AndroidRuntime( 2323): at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1192)
E/AndroidRuntime( 2323): at android.app.Activity.performResume(Activity.java:5211)
E/AndroidRuntime( 2323): at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2780)
E/AndroidRuntime( 2323): ... 12 more
W/ActivityManager( 1112): Force finishing activity com.android.keepass/com.keepassdroid.PasswordActivity
W/ActivityManager( 1112): Force finishing activity com.android.keepass/com.keepassdroid.fileselect.FileSelectActivity

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Mar 23, 2014

Contributor

The reason for this NullPointerException in onResume is: we should put "setContentView()"
in onCreate rather than AsyncTask.

This make sure the "TextView password" in onResume is inflated so that findViewById won't return null.

I updated the patch and tested it. I should work now.

Contributor

yulin2 commented Mar 23, 2014

The reason for this NullPointerException in onResume is: we should put "setContentView()"
in onCreate rather than AsyncTask.

This make sure the "TextView password" in onResume is inflated so that findViewById won't return null.

I updated the patch and tested it. I should work now.

@bpellin

This comment has been minimized.

Show comment
Hide comment
@bpellin

bpellin Mar 23, 2014

Owner

It looks good with your latest changes. I'll merge in your changes.

Owner

bpellin commented Mar 23, 2014

It looks good with your latest changes. I'll merge in your changes.

@bpellin bpellin merged commit 481838c into bpellin:master Mar 23, 2014

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Mar 23, 2014

Contributor

Great! Thanks for your confirmation:)

Contributor

yulin2 commented Mar 23, 2014

Great! Thanks for your confirmation:)

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