Performance improvement suggestion #148

Closed
andrew659 opened this Issue Jul 1, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@andrew659

Dear developers,

I am a big fan of FBReaderJ, and recently I am writing a static code analysis tool to conduct performance analysis for Android apps. I found several violations of "view holder" patterns in FBReaderJ's code. These violations could affect the ListView scrolling performance.

Currently in FBReaderJ, in some list adapters the getView() method works likes this

public View getView(int position, View convertView, ViewGroup parent) {
if(convertView == null) {
convertView = mInflater.inflate(R.layout.listItem, null);
}
((TextView) convertView.findViewById(R.id.text)).setText(DATA[position]);

return convertView;
}

When the users scroll a list view, this implementation avoids inflations for each view (by using the recycled convertView), which saves CPU cycles and RAM. However, the method still invokes findViewById() every time when it is called. Android documentation says that findViewById is an expensive call, it recursively traverses a view tree to find a view matching the give ID. Google developers actually suggested a better way to implement getView(). It works like this:

We define a ViewHolder class with the field: TextView text . Then the getView() can be implemented like this:

public View getView(int position, View convertView, ViewGroup parent) {
ViewHolder holder;
if(convertView == null){
//we have no recycled views to use, then build new one
convertView = mInflater.inflate(R.layout.listItem, null);
holder = new ViewHolder();
holder.text = (TextView) convertView.getViewById(R.id.text);
convertView.setTag(holder)
} else {
//use the recycled view to improve performance
holder = (ViewHolder) convertView.getTag();
}
holder.text.setText(DATA[position]);

return convertView;
}

This avoids calling findViewById frequently and will improve performance when the list contains many items or on low end devices.

We found the violations of view holder pattern in these classes:
org.geometerplus.android.fbreader.CancelActivity.java
org.geometerplus.android.fbreader.network.NetworkLibraryAdapter.java
org.geometerplus.android.fbreader.style.StyleListActivity.java
org.geometerplus.android.fbreader.TOCActivity.java
org.geometerplus.android.fbreader.library.LibraryTreeAdapter.java
org.geometerplus.android.fbreader.BookmarksActivity.java

you may find more useful information in thees references:
view holder pattern: http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/
http://developer.android.com/training/improving-layouts/smooth-scrolling.html
http://www.youtube.com/watch?v=wDBM6wVEO70

In the last Google IO video, we find that the current implementation of getView() in FBReaderJ is a right way, but not a fast way. The video actually provides three ways: a slow way, a right way and a fast way.

Looking forward to your reply and hope I can help improve FBReaderJ :)

@Boidkan

This comment has been minimized.

Show comment
Hide comment
@Boidkan

Boidkan Sep 28, 2013

Looked through this as a part of a openhatch.org project at my university. I realized that this inefficiency is still found in the first three files mentioned. I will be looking at this in my spare time and contributing and bring this up at the open source club at my school.

Boidkan commented Sep 28, 2013

Looked through this as a part of a openhatch.org project at my university. I realized that this inefficiency is still found in the first three files mentioned. I will be looking at this in my spare time and contributing and bring this up at the open source club at my school.

@geometer

This comment has been minimized.

Show comment
Hide comment
@geometer

geometer Sep 28, 2013

Owner

I'm not sure I understand your idea. Do you mean findViewById is not efficient?

Owner

geometer commented Sep 28, 2013

I'm not sure I understand your idea. Do you mean findViewById is not efficient?

@andrew659

This comment has been minimized.

Show comment
Hide comment
@andrew659

andrew659 Sep 29, 2013

findViewById is a heavy API and should not called frequently. This is why Google suggested the "view holder" pattern. In the pattern, we can reuse some inflated list items, so it is possible to record the references to the items' inner view objects located by calling findViewById(). Later, when the old list item (convertView in the code) is reused, the call to findViewById() can be avoided.

Detailed explanations can be found at
http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/
http://www.youtube.com/watch?v=wDBM6wVEO70

findViewById is a heavy API and should not called frequently. This is why Google suggested the "view holder" pattern. In the pattern, we can reuse some inflated list items, so it is possible to record the references to the items' inner view objects located by calling findViewById(). Later, when the old list item (convertView in the code) is reused, the call to findViewById() can be avoided.

Detailed explanations can be found at
http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/
http://www.youtube.com/watch?v=wDBM6wVEO70

@geometer

This comment has been minimized.

Show comment
Hide comment
@geometer

geometer Sep 29, 2013

Owner

I see, thank you!

Owner

geometer commented Sep 29, 2013

I see, thank you!

@geometer

This comment has been minimized.

Show comment
Hide comment
@geometer

geometer Oct 7, 2013

Owner

FIxed in 1.9

Owner

geometer commented Oct 7, 2013

FIxed in 1.9

@geometer geometer closed this Oct 7, 2013

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