From b59951457c7f4b52fec264f8d631e31858f45764 Mon Sep 17 00:00:00 2001 From: Jeff Lockhart Date: Thu, 2 Jul 2020 02:30:42 -0600 Subject: [PATCH 1/4] Lock and unlock local algorithm reference Ensure methods operate on the same algorithm reference during lock, operation, and unlock. Ensure algorithm reference is not replaced while in use. --- .../android/clustering/ClusterManager.java | 63 +++++++++++-------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java b/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java index 27f1fe2dc..655cea834 100644 --- a/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java +++ b/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java @@ -123,11 +123,17 @@ public void setAlgorithm(Algorithm algorithm) { public void setAlgorithm(ScreenBasedAlgorithm algorithm) { algorithm.lock(); try { - if (mAlgorithm != null) { - algorithm.addItems(mAlgorithm.getItems()); - } - + Algorithm oldAlgorithm = getAlgorithm(); mAlgorithm = algorithm; + + if (oldAlgorithm != null) { + oldAlgorithm.lock(); + try { + algorithm.addItems(oldAlgorithm.getItems()); + } finally { + oldAlgorithm.unlock(); + } + } } finally { algorithm.unlock(); } @@ -156,11 +162,12 @@ public Algorithm getAlgorithm() { * {@link #cluster()} for the map to be cleared. */ public void clearItems() { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - mAlgorithm.clearItems(); + algorithm.clearItems(); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -171,11 +178,12 @@ public void clearItems() { * @return true if the cluster manager contents changed as a result of the call */ public boolean addItems(Collection items) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.addItems(items); + return algorithm.addItems(items); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -186,11 +194,12 @@ public boolean addItems(Collection items) { * @return true if the cluster manager contents changed as a result of the call */ public boolean addItem(T myItem) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.addItem(myItem); + return algorithm.addItem(myItem); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -201,11 +210,12 @@ public boolean addItem(T myItem) { * @return true if the cluster manager contents changed as a result of the call */ public boolean removeItems(Collection items) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.removeItems(items); + return algorithm.removeItems(items); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -216,11 +226,12 @@ public boolean removeItems(Collection items) { * @return true if the item was removed from the cluster manager as a result of this call */ public boolean removeItem(T item) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.removeItem(item); + return algorithm.removeItem(item); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -232,11 +243,12 @@ public boolean removeItem(T item) { * contained within the cluster manager and the cluster manager contents are unchanged */ public boolean updateItem(T item) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.updateItem(item); + return algorithm.updateItem(item); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } @@ -294,11 +306,12 @@ public void onInfoWindowClick(Marker marker) { private class ClusterTask extends AsyncTask>> { @Override protected Set> doInBackground(Float... zoom) { - mAlgorithm.lock(); + Algorithm algorithm = getAlgorithm(); + algorithm.lock(); try { - return mAlgorithm.getClusters(zoom[0]); + return algorithm.getClusters(zoom[0]); } finally { - mAlgorithm.unlock(); + algorithm.unlock(); } } From b12541b77d54b792332ad52a59efaed2b370ceab Mon Sep 17 00:00:00 2001 From: Jeff Lockhart Date: Thu, 2 Jul 2020 02:34:15 -0600 Subject: [PATCH 2/4] Use thread pools --- .../clustering/algo/PreCachingAlgorithmDecorator.java | 7 +++++-- .../android/clustering/view/DefaultClusterRenderer.java | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java b/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java index a39106a68..89ade3dc0 100644 --- a/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java +++ b/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java @@ -23,6 +23,8 @@ import java.util.Collection; import java.util.Set; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -35,6 +37,7 @@ public class PreCachingAlgorithmDecorator extends Abstrac // TODO: evaluate maxSize parameter for LruCache. private final LruCache>> mCache = new LruCache>>(5); private final ReadWriteLock mCacheLock = new ReentrantReadWriteLock(); + private final Executor mExecutor = Executors.newCachedThreadPool(); public PreCachingAlgorithmDecorator(Algorithm algorithm) { mAlgorithm = algorithm; @@ -102,11 +105,11 @@ public Set> getClusters(float zoom) { // TODO: Check if requests are already in-flight. if (mCache.get(discreteZoom + 1) == null) { // It seems this cannot use a thread pool due to thread locking issues (#660) - new Thread(new PrecacheRunnable(discreteZoom + 1)).start(); + mExecutor.execute(new PrecacheRunnable(discreteZoom + 1)); } if (mCache.get(discreteZoom - 1) == null) { // It seems this cannot use a thread pool due to thread locking issues (#660) - new Thread(new PrecacheRunnable(discreteZoom - 1)).start(); + mExecutor.execute(new PrecacheRunnable(discreteZoom - 1)); } return results; } diff --git a/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java b/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java index c9cbabaee..fe026c9af 100644 --- a/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java +++ b/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java @@ -66,6 +66,8 @@ import java.util.Queue; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; import java.util.concurrent.locks.Condition; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; @@ -79,6 +81,7 @@ public class DefaultClusterRenderer implements ClusterRen private final ClusterManager mClusterManager; private final float mDensity; private boolean mAnimate; + private final Executor mExecutor = Executors.newSingleThreadExecutor(); private static final int[] BUCKETS = {10, 20, 50, 100, 200, 500, 1000}; private ShapeDrawable mColoredCircleBackground; @@ -332,7 +335,7 @@ public void run() { renderTask.setProjection(projection); renderTask.setMapZoom(mMap.getCameraPosition().zoom); // It seems this cannot use a thread pool due to thread locking issues (#660) - new Thread(renderTask).start(); + mExecutor.execute(renderTask); } public void queue(Set> clusters) { From d0f12f825c2f2330befd3ac5d83e887e7a144f4a Mon Sep 17 00:00:00 2001 From: Jeff Lockhart Date: Fri, 3 Jul 2020 20:17:48 -0600 Subject: [PATCH 3/4] Remove comments --- .../android/clustering/algo/PreCachingAlgorithmDecorator.java | 2 -- .../maps/android/clustering/view/DefaultClusterRenderer.java | 1 - 2 files changed, 3 deletions(-) diff --git a/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java b/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java index 89ade3dc0..00757e19a 100644 --- a/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java +++ b/library/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java @@ -104,11 +104,9 @@ public Set> getClusters(float zoom) { Set> results = getClustersInternal(discreteZoom); // TODO: Check if requests are already in-flight. if (mCache.get(discreteZoom + 1) == null) { - // It seems this cannot use a thread pool due to thread locking issues (#660) mExecutor.execute(new PrecacheRunnable(discreteZoom + 1)); } if (mCache.get(discreteZoom - 1) == null) { - // It seems this cannot use a thread pool due to thread locking issues (#660) mExecutor.execute(new PrecacheRunnable(discreteZoom - 1)); } return results; diff --git a/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java b/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java index fe026c9af..b56683c78 100644 --- a/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java +++ b/library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java @@ -334,7 +334,6 @@ public void run() { }); renderTask.setProjection(projection); renderTask.setMapZoom(mMap.getCameraPosition().zoom); - // It seems this cannot use a thread pool due to thread locking issues (#660) mExecutor.execute(renderTask); } From ee6d12d402591ba5daf53812dc576ae4c4c1aaee Mon Sep 17 00:00:00 2001 From: Jeff Lockhart Date: Mon, 6 Jul 2020 15:57:05 -0600 Subject: [PATCH 4/4] Make algorithm variables final --- .../maps/android/clustering/ClusterManager.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java b/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java index 655cea834..0685ee56a 100644 --- a/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java +++ b/library/src/main/java/com/google/maps/android/clustering/ClusterManager.java @@ -123,7 +123,7 @@ public void setAlgorithm(Algorithm algorithm) { public void setAlgorithm(ScreenBasedAlgorithm algorithm) { algorithm.lock(); try { - Algorithm oldAlgorithm = getAlgorithm(); + final Algorithm oldAlgorithm = getAlgorithm(); mAlgorithm = algorithm; if (oldAlgorithm != null) { @@ -162,7 +162,7 @@ public Algorithm getAlgorithm() { * {@link #cluster()} for the map to be cleared. */ public void clearItems() { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { algorithm.clearItems(); @@ -178,7 +178,7 @@ public void clearItems() { * @return true if the cluster manager contents changed as a result of the call */ public boolean addItems(Collection items) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.addItems(items); @@ -194,7 +194,7 @@ public boolean addItems(Collection items) { * @return true if the cluster manager contents changed as a result of the call */ public boolean addItem(T myItem) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.addItem(myItem); @@ -210,7 +210,7 @@ public boolean addItem(T myItem) { * @return true if the cluster manager contents changed as a result of the call */ public boolean removeItems(Collection items) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.removeItems(items); @@ -226,7 +226,7 @@ public boolean removeItems(Collection items) { * @return true if the item was removed from the cluster manager as a result of this call */ public boolean removeItem(T item) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.removeItem(item); @@ -243,7 +243,7 @@ public boolean removeItem(T item) { * contained within the cluster manager and the cluster manager contents are unchanged */ public boolean updateItem(T item) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.updateItem(item); @@ -306,7 +306,7 @@ public void onInfoWindowClick(Marker marker) { private class ClusterTask extends AsyncTask>> { @Override protected Set> doInBackground(Float... zoom) { - Algorithm algorithm = getAlgorithm(); + final Algorithm algorithm = getAlgorithm(); algorithm.lock(); try { return algorithm.getClusters(zoom[0]);