Skip to content
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.

Sign up for GitHub

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

fix: algorithm concurrency #761

Merged
merged 4 commits into from
Jul 7, 2020
Merged

fix: algorithm concurrency #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b599514
Lock and unlock local algorithm reference
jeffdgr8 Jul 2, 2020
b12541b
Use thread pools
jeffdgr8 Jul 2, 2020
d0f12f8
Remove comments
jeffdgr8 Jul 4, 2020
ee6d12d
Make algorithm variables final
jeffdgr8 Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 38 additions & 25 deletions library/src/main/java/com/google/maps/android/clustering/ClusterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,17 @@ public void setAlgorithm(Algorithm<T> algorithm) {
public void setAlgorithm(ScreenBasedAlgorithm<T> algorithm) {
algorithm.lock();
try {
if (mAlgorithm != null) {
algorithm.addItems(mAlgorithm.getItems());
}

final Algorithm<T> oldAlgorithm = getAlgorithm();
mAlgorithm = algorithm;

if (oldAlgorithm != null) {
oldAlgorithm.lock();
try {
algorithm.addItems(oldAlgorithm.getItems());
} finally {
oldAlgorithm.unlock();
}
}
} finally {
algorithm.unlock();
}
Expand Down Expand Up @@ -156,11 +162,12 @@ public Algorithm<T> getAlgorithm() {
* {@link #cluster()} for the map to be cleared.
*/
public void clearItems() {
mAlgorithm.lock();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
mAlgorithm.clearItems();
algorithm.clearItems();
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand All @@ -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<T> items) {
mAlgorithm.lock();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.addItems(items);
return algorithm.addItems(items);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand All @@ -186,11 +194,12 @@ public boolean addItems(Collection<T> items) {
* @return true if the cluster manager contents changed as a result of the call
*/
public boolean addItem(T myItem) {
mAlgorithm.lock();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.addItem(myItem);
return algorithm.addItem(myItem);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand All @@ -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<T> items) {
mAlgorithm.lock();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.removeItems(items);
return algorithm.removeItems(items);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand All @@ -216,11 +226,12 @@ public boolean removeItems(Collection<T> 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();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.removeItem(item);
return algorithm.removeItem(item);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand All @@ -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();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.updateItem(item);
return algorithm.updateItem(item);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand Down Expand Up @@ -294,11 +306,12 @@ public void onInfoWindowClick(Marker marker) {
private class ClusterTask extends AsyncTask<Float, Void, Set<? extends Cluster<T>>> {
@Override
protected Set<? extends Cluster<T>> doInBackground(Float... zoom) {
mAlgorithm.lock();
final Algorithm<T> algorithm = getAlgorithm();
algorithm.lock();
try {
return mAlgorithm.getClusters(zoom[0]);
return algorithm.getClusters(zoom[0]);
} finally {
mAlgorithm.unlock();
algorithm.unlock();
}
}

Expand Down
9 changes: 5 additions & 4 deletions ...y/src/main/java/com/google/maps/android/clustering/algo/PreCachingAlgorithmDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -35,6 +37,7 @@ public class PreCachingAlgorithmDecorator<T extends ClusterItem> extends Abstrac
// TODO: evaluate maxSize parameter for LruCache.
private final LruCache<Integer, Set<? extends Cluster<T>>> mCache = new LruCache<Integer, Set<? extends Cluster<T>>>(5);
private final ReadWriteLock mCacheLock = new ReentrantReadWriteLock();
private final Executor mExecutor = Executors.newCachedThreadPool();

public PreCachingAlgorithmDecorator(Algorithm<T> algorithm) {
mAlgorithm = algorithm;
Expand Down Expand Up @@ -101,12 +104,10 @@ public Set<? extends Cluster<T>> getClusters(float zoom) {
Set<? extends Cluster<T>> 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)
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;
}
Expand Down
6 changes: 4 additions & 2 deletions library/src/main/java/com/google/maps/android/clustering/view/DefaultClusterRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -79,6 +81,7 @@ public class DefaultClusterRenderer<T extends ClusterItem> implements ClusterRen
private final ClusterManager<T> mClusterManager;
private final float mDensity;
private boolean mAnimate;
private final Executor mExecutor = Executors.newSingleThreadExecutor();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this should be single threaded vs. a larger thread pool? Making this single threaded changes the current behavior/performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as noted by @barbeau #601 (comment) when reviewing the original PR, the implementation is effectively single threaded already, which is why I chose to use a single thread executor. There's only ever one render task in flight and potentially one queued up to run next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! That wasn't immediately obvious to me so this is definitely a nice improvement 👍


private static final int[] BUCKETS = {10, 20, 50, 100, 200, 500, 1000};
private ShapeDrawable mColoredCircleBackground;
Expand Down Expand Up @@ -331,8 +334,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<? extends Cluster<T>> clusters) {
Expand Down