From 559d90d0dafbac1d97a1e6f18062309831a25d51 Mon Sep 17 00:00:00 2001 From: Hyunyoung Song Date: Tue, 28 Apr 2015 15:06:45 -0700 Subject: WidgetPreviewLoader concurrency issue / Caching improvement 1) Concurrency issue: unused bitmap was not properly synchronized which caused concurrency issue. Hence, leading current widget tray implementation to not use it. (a.k.a. cancel(false)). Issue fixed and now using the unused bitmap pool. 2) Caching improvement: LoadedBitmap cache was a legacy support system for the old widget tray implementation. On our latest implementation, cache and recycled view is completely being managed by the recycler view. Hence removed. Change-Id: I843e6a286b676f283172f2c1ef5cbeed0a9fb17f --- src/com/android/launcher3/WidgetPreviewLoader.java | 65 +++++++--------------- src/com/android/launcher3/widget/WidgetCell.java | 22 ++++---- .../launcher3/widget/WidgetsContainerView.java | 5 -- .../launcher3/widget/WidgetsListAdapter.java | 12 +++- .../launcher3/widget/WidgetsRowViewHolder.java | 2 +- 5 files changed, 41 insertions(+), 65 deletions(-) diff --git a/src/com/android/launcher3/WidgetPreviewLoader.java b/src/com/android/launcher3/WidgetPreviewLoader.java index 5c3ed9272..412cbcd6d 100644 --- a/src/com/android/launcher3/WidgetPreviewLoader.java +++ b/src/com/android/launcher3/WidgetPreviewLoader.java @@ -34,7 +34,6 @@ import com.android.launcher3.util.ComponentKey; import com.android.launcher3.util.Thunk; import com.android.launcher3.widget.WidgetCell; -import java.lang.ref.WeakReference; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -51,8 +50,13 @@ public class WidgetPreviewLoader { private static final float WIDGET_PREVIEW_ICON_PADDING_PERCENTAGE = 0.25f; private final HashMap mPackageVersions = new HashMap<>(); - private final HashMap> mLoadedPreviews = new HashMap<>(); - private Set mUnusedBitmaps = Collections.newSetFromMap(new WeakHashMap()); + + /** + * Weak reference objects, do not prevent their referents from being made finalizable, + * finalized, and then reclaimed. + */ + private Set mUnusedBitmaps = + Collections.newSetFromMap(new WeakHashMap()); private final Context mContext; private final IconCache mIconCache; @@ -84,18 +88,9 @@ public class WidgetPreviewLoader { String size = previewWidth + "x" + previewHeight; WidgetCacheKey key = getObjectKey(o, size); - // Check if we have the preview loaded or not. - synchronized (mLoadedPreviews) { - WeakReference ref = mLoadedPreviews.get(key); - if (ref != null && ref.get() != null) { - immediateResult[0] = ref.get(); - return new PreviewLoadRequest(null, key); - } - } - PreviewLoadTask task = new PreviewLoadTask(key, o, previewWidth, previewHeight, caller); task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); - return new PreviewLoadRequest(task, key); + return new PreviewLoadRequest(task); } /** @@ -192,22 +187,6 @@ public class WidgetPreviewLoader { mPackageVersions.remove(packageName); } - synchronized (mLoadedPreviews) { - Set keysToRemove = new HashSet<>(); - for (WidgetCacheKey key : mLoadedPreviews.keySet()) { - if (key.componentName.getPackageName().equals(packageName) && key.user.equals(user)) { - keysToRemove.add(key); - } - } - - for (WidgetCacheKey key : keysToRemove) { - WeakReference req = mLoadedPreviews.remove(key); - if (req != null && req.get() != null) { - mUnusedBitmaps.add(req.get()); - } - } - } - try { mDb.getWritableDatabase().delete(CacheDb.TABLE_NAME, CacheDb.COLUMN_PACKAGE + " = ? AND " + CacheDb.COLUMN_USER + " = ?", @@ -549,26 +528,25 @@ public class WidgetPreviewLoader { public class PreviewLoadRequest { private final PreviewLoadTask mTask; - private final WidgetCacheKey mKey; - public PreviewLoadRequest(PreviewLoadTask task, WidgetCacheKey key) { + public PreviewLoadRequest(PreviewLoadTask task) { mTask = task; - mKey = key; } - public void cancel(boolean recycleImage) { + public void cleanup() { if (mTask != null) { mTask.cancel(true); } - if (recycleImage) { - synchronized(mLoadedPreviews) { - WeakReference result = mLoadedPreviews.remove(mKey); - if (result != null && result.get() != null) { - mUnusedBitmaps.add(result.get()); - } - } + if (mTask.mBitmap == null) { + return; + } + + // The preview is no longer bound to any view, move it to {@link WeakReference} list. + synchronized(mUnusedBitmaps) { + mUnusedBitmaps.add(mTask.mBitmap); } + mTask.mBitmap = null; } } @@ -579,6 +557,7 @@ public class WidgetPreviewLoader { private final int mPreviewHeight; private final int mPreviewWidth; private final WidgetCell mCaller; + private Bitmap mBitmap; PreviewLoadTask(WidgetCacheKey key, Object info, int previewWidth, int previewHeight, WidgetCell caller) { @@ -597,7 +576,6 @@ public class WidgetPreviewLoader { protected Bitmap doInBackground(Void... params) { Bitmap unusedBitmap = null; - // TODO(hyunyoungs): Figure out why this path causes concurrency issue. synchronized (mUnusedBitmaps) { // Check if we can use a bitmap for (Bitmap candidate : mUnusedBitmaps) { @@ -638,10 +616,7 @@ public class WidgetPreviewLoader { @Override protected void onPostExecute(Bitmap result) { - synchronized(mLoadedPreviews) { - mLoadedPreviews.put(mKey, new WeakReference(result)); - } - + mBitmap = result; mCaller.applyPreview(result); } } diff --git a/src/com/android/launcher3/widget/WidgetCell.java b/src/com/android/launcher3/widget/WidgetCell.java index 7c7d982de..2df170eff 100644 --- a/src/com/android/launcher3/widget/WidgetCell.java +++ b/src/com/android/launcher3/widget/WidgetCell.java @@ -94,18 +94,25 @@ public class WidgetCell extends LinearLayout implements OnLayoutChangeListener { mOriginalImagePadding.right = mWidgetImage.getPaddingRight(); mOriginalImagePadding.bottom = mWidgetImage.getPaddingBottom(); - // Ensure we are using the right text size - DeviceProfile profile = LauncherAppState.getInstance().getDynamicGrid().getDeviceProfile(); mWidgetName = ((TextView) findViewById(R.id.widget_name)); mWidgetDims = ((TextView) findViewById(R.id.widget_dims)); } - public void reset() { + /** + * Called to clear the view and free attached resources. (e.g., {@link Bitmap} + */ + public void clear() { + if (DEBUG) { + Log.d(TAG, "reset called on:" + mWidgetName.getText()); + } mWidgetImage.setImageDrawable(null); mWidgetName.setText(null); mWidgetDims.setText(null); - cancelLoader(false); + if (mActiveRequest != null) { + mActiveRequest.cleanup(); + mActiveRequest = null; + } } /** @@ -211,13 +218,6 @@ public class WidgetCell extends LinearLayout implements OnLayoutChangeListener { return Math.min(size[0], info.spanX * cellWidth); } - private void cancelLoader(boolean recycleImage) { - if (mActiveRequest != null) { - mActiveRequest.cancel(recycleImage); - mActiveRequest = null; - } - } - /** * Helper method to get the string info of the tag. */ diff --git a/src/com/android/launcher3/widget/WidgetsContainerView.java b/src/com/android/launcher3/widget/WidgetsContainerView.java index 27a3ea13d..8090c88a4 100644 --- a/src/com/android/launcher3/widget/WidgetsContainerView.java +++ b/src/com/android/launcher3/widget/WidgetsContainerView.java @@ -26,7 +26,6 @@ import android.support.v7.widget.LinearLayoutManager; import android.support.v7.widget.RecyclerView; import android.util.AttributeSet; import android.util.Log; -import android.view.MotionEvent; import android.view.View; import android.widget.FrameLayout; import android.widget.ImageView; @@ -61,9 +60,6 @@ public class WidgetsContainerView extends FrameLayout implements Insettable, private static final String TAG = "WidgetsContainerView"; private static final boolean DEBUG = false; - /* {@link RecyclerView} will keep following # of views in cache, before recycling. */ - private static final int WIDGET_CACHE_SIZE = 2; - private static final int SPRING_MODE_DELAY_MS = 150; /* Global instances that are used inside this container. */ @@ -119,7 +115,6 @@ public class WidgetsContainerView extends FrameLayout implements Insettable, mView = (RecyclerView) findViewById(R.id.widgets_list_view); mView.setAdapter(mAdapter); mView.setLayoutManager(new LinearLayoutManager(getContext())); - mView.setItemViewCacheSize(WIDGET_CACHE_SIZE); mPadding.set(getPaddingLeft(), getPaddingTop(), getPaddingRight(), getPaddingBottom()); diff --git a/src/com/android/launcher3/widget/WidgetsListAdapter.java b/src/com/android/launcher3/widget/WidgetsListAdapter.java index a5b2aff1b..051a3ab5e 100644 --- a/src/com/android/launcher3/widget/WidgetsListAdapter.java +++ b/src/com/android/launcher3/widget/WidgetsListAdapter.java @@ -120,7 +120,9 @@ public class WidgetsListAdapter extends Adapter { mIconCache.getTitleAndIconForApp(infoOut.packageName, UserHandleCompat.myUserHandle(), false /* useLowResIcon */, infoOut); } - ((TextView) holder.getContent().findViewById(R.id.section)).setText(infoOut.title); + + TextView tv = ((TextView) holder.getContent().findViewById(R.id.section)); + tv.setText(infoOut.title); ImageView iv = (ImageView) holder.getContent().findViewById(R.id.section_image); iv.setImageBitmap(infoOut.iconBitmap); @@ -149,7 +151,7 @@ public class WidgetsListAdapter extends Adapter { @Override public WidgetsRowViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { if (DEBUG) { - Log.v(TAG, String.format("\nonCreateViewHolder, [widget#=%d]", viewType)); + Log.v(TAG, "\nonCreateViewHolder"); } ViewGroup container = (ViewGroup) mLayoutInflater.inflate( @@ -159,11 +161,15 @@ public class WidgetsListAdapter extends Adapter { @Override public void onViewRecycled(WidgetsRowViewHolder holder) { + if (DEBUG) { + Log.v(TAG, String.format("onViewDetachedFromWindow, [pos=%d]", + holder.getAdapterPosition())); + } ViewGroup row = ((ViewGroup) holder.getContent().findViewById(R.id.widgets_cell_list)); for (int i = 0; i < row.getChildCount(); i++) { WidgetCell widget = (WidgetCell) row.getChildAt(i); - widget.reset(); + widget.clear(); } } diff --git a/src/com/android/launcher3/widget/WidgetsRowViewHolder.java b/src/com/android/launcher3/widget/WidgetsRowViewHolder.java index 99a192c89..249559ab9 100644 --- a/src/com/android/launcher3/widget/WidgetsRowViewHolder.java +++ b/src/com/android/launcher3/widget/WidgetsRowViewHolder.java @@ -25,7 +25,7 @@ public class WidgetsRowViewHolder extends ViewHolder { ViewGroup mContent; - public WidgetsRowViewHolder(ViewGroup v) { + public WidgetsRowViewHolder(ViewGroup v) { super(v); mContent = v; } -- cgit v1.2.3