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 k9mail #450

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@yulin2
Contributor

yulin2 commented Feb 16, 2014

Hello developers of K-9 mail,

I'm a Ph.D. student and I'm doing research related to Android apps'
responsiveness. I found there are some cases in K9 that the database
is queried, or bitmap is generated in UI thread. Does this affect the
responsiveness of the app? An optimization could be extracting these
operations into AsyncTask.

For example, in AttachmentView.java, line 174 "getPreviewIcon()"
decodes a bitmap from a stream, which is a relative long
computation. Is it better to put them into a AsyncTask, so the UI
thread won't be blocked on decoding? I attach a sample patch
here.

Also, in MessageCompose.java, "actionCompose" method at line 412
invokes "Preferences.getPreferences" which create a "Storage" and
queries database. This also lead to a potential responsiveness
issue. Thus, I tried to move it into AsyncTask.

Similarly, MessageViewFragment.java invokes
"Preferences.getPreferences" at line 272. Note that I add a null check
"if(mAccount == null)" before every use of "mAccount", because there
are data races on "mAccount" after put line 272 into AsyncTask. I also
did the same transformation for ChooseFolder.java and
AccountSetupBasics.java.

There should be some other places that can be improved. I can't report
all of them at one time. However, what do you think about these
improvements? My thought is we can improve the responsiveness if we
try to avoid the access to IO/Database, generating bitmap in UI thread.

Thanks,
Yu

@pylerSM

This comment has been minimized.

Show comment
Hide comment
@pylerSM

pylerSM Feb 21, 2014

Contributor

Nice! This should be merged asap.

Contributor

pylerSM commented Feb 21, 2014

Nice! This should be merged asap.

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Feb 21, 2014

Contributor

Great! We're very glad to see our patch is useful. Thanks for your code review and confirmation.

Yu

Contributor

yulin2 commented Feb 21, 2014

Great! We're very glad to see our patch is useful. Thanks for your code review and confirmation.

Yu

@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Mar 1, 2014

Member

@yulin2: Thanks for looking into ways on how to improve K-9 Mail!

Generally, you're right that we should perform operations that access the storage only in the background. However, just extracting those operations without considering the whole component lifecycle will rarely lead to the desired results.
The right way to approach this for activities is to display a loading indicator and to load all necessary data in the background. Once that's done we can update the layout and start doing whatever the activity does.
The current code isn't very good at this. But in particular loading Account objects shouldn't be too problematic because most of the time they're already cached in memory.

About the format of the patch: You should put unrelated changes in separate commits. That way it's easier to review the changes and to pick only some of them.

I took the liberty of modifying your commit to only contain the unproblematic change:
fa7118d

Member

cketti commented Mar 1, 2014

@yulin2: Thanks for looking into ways on how to improve K-9 Mail!

Generally, you're right that we should perform operations that access the storage only in the background. However, just extracting those operations without considering the whole component lifecycle will rarely lead to the desired results.
The right way to approach this for activities is to display a loading indicator and to load all necessary data in the background. Once that's done we can update the layout and start doing whatever the activity does.
The current code isn't very good at this. But in particular loading Account objects shouldn't be too problematic because most of the time they're already cached in memory.

About the format of the patch: You should put unrelated changes in separate commits. That way it's easier to review the changes and to pick only some of them.

I took the liberty of modifying your commit to only contain the unproblematic change:
fa7118d

@cketti cketti closed this Mar 1, 2014

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