From 0b03f13cabec84d2d841fde47ce9fec0d531b6a1 Mon Sep 17 00:00:00 2001
From: Dan Zivkovic <zivkovic@google.com>
Date: Tue, 17 Feb 2015 15:12:05 -0800
Subject: [PATCH] Sanitize the usage of executors.
There should be 1 executor each for static and dynamic language models.
This prevents too many dynamic LM updates from running in parallel,
competing for resources.
Change-Id: I8ec439e0ea2d92fba275bc20a0b8c9193346a0c1
---
.../latin/ContactsContentObserver.java | 41 ++++-----
.../latin/DictionaryFacilitatorImpl.java | 2 +-
.../latin/ExpandableBinaryDictionary.java | 13 ++-
.../spellcheck/UserDictionaryLookup.java | 23 ++---
.../latin/utils/ExecutorUtils.java | 88 ++++++++++++++-----
.../latin/utils/ExecutorUtilsTests.java | 5 +-
6 files changed, 102 insertions(+), 70 deletions(-)
diff --git a/java/src/com/android/inputmethod/latin/ContactsContentObserver.java b/java/src/com/android/inputmethod/latin/ContactsContentObserver.java
index 019d17d56e..b23226dce0 100644
--- a/java/src/com/android/inputmethod/latin/ContactsContentObserver.java
+++ b/java/src/com/android/inputmethod/latin/ContactsContentObserver.java
@@ -23,22 +23,21 @@ import android.os.SystemClock;
import android.provider.ContactsContract.Contacts;
import android.util.Log;
-import com.android.inputmethod.annotations.UsedForTesting;
import com.android.inputmethod.latin.ContactsManager.ContactsChangedListener;
import com.android.inputmethod.latin.utils.ExecutorUtils;
import java.util.ArrayList;
-import java.util.concurrent.ExecutorService;
/**
- * A content observer that listens to updates to content provider {@link Contacts.CONTENT_URI}.
+ * A content observer that listens to updates to content provider {@link Contacts#CONTENT_URI}.
*/
// TODO:add test
-public class ContactsContentObserver {
+public class ContactsContentObserver implements Runnable {
private static final String TAG = ContactsContentObserver.class.getSimpleName();
private static final boolean DEBUG = false;
- private ContentObserver mObserver;
+ private ContentObserver mContentObserver;
+ private ContactsChangedListener mContactsChangedListener;
private final Context mContext;
private final ContactsManager mManager;
@@ -52,29 +51,27 @@ public class ContactsContentObserver {
if (DEBUG) {
Log.d(TAG, "Registered Contacts Content Observer");
}
- mObserver = new ContentObserver(null /* handler */) {
+ mContactsChangedListener = listener;
+ mContentObserver = new ContentObserver(null /* handler */) {
@Override
public void onChange(boolean self) {
- getBgExecutor().execute(new Runnable() {
- @Override
- public void run() {
- if (haveContentsChanged()) {
- if (DEBUG) {
- Log.d(TAG, "Contacts have changed; notifying listeners");
- }
- listener.onContactsChange();
- }
- }
- });
+ // TODO(zivkovic): Limit the queue to 1 instance of ContactsContentObserver.
+ ExecutorUtils.getExecutorForDynamicLanguageModelUpdate()
+ .execute(ContactsContentObserver.this);
}
};
final ContentResolver contentResolver = mContext.getContentResolver();
- contentResolver.registerContentObserver(Contacts.CONTENT_URI, true, mObserver);
+ contentResolver.registerContentObserver(Contacts.CONTENT_URI, true, mContentObserver);
}
- @UsedForTesting
- private ExecutorService getBgExecutor() {
- return ExecutorUtils.getExecutor("Check Contacts");
+ @Override
+ public void run() {
+ if (haveContentsChanged()) {
+ if (DEBUG) {
+ Log.d(TAG, "Contacts have changed; notifying listeners");
+ }
+ mContactsChangedListener.onContactsChange();
+ }
}
private boolean haveContentsChanged() {
@@ -105,6 +102,6 @@ public class ContactsContentObserver {
}
public void unregister() {
- mContext.getContentResolver().unregisterContentObserver(mObserver);
+ mContext.getContentResolver().unregisterContentObserver(mContentObserver);
}
}
diff --git a/java/src/com/android/inputmethod/latin/DictionaryFacilitatorImpl.java b/java/src/com/android/inputmethod/latin/DictionaryFacilitatorImpl.java
index 3d76751ce8..31e9d03053 100644
--- a/java/src/com/android/inputmethod/latin/DictionaryFacilitatorImpl.java
+++ b/java/src/com/android/inputmethod/latin/DictionaryFacilitatorImpl.java
@@ -443,7 +443,7 @@ public class DictionaryFacilitatorImpl implements DictionaryFacilitator {
final Locale[] locales, final DictionaryInitializationListener listener) {
final CountDownLatch latchForWaitingLoadingMainDictionary = new CountDownLatch(1);
mLatchForWaitingLoadingMainDictionaries = latchForWaitingLoadingMainDictionary;
- ExecutorUtils.getExecutor("InitializeBinaryDictionary").execute(new Runnable() {
+ ExecutorUtils.getExecutorForStaticLanguageModelUpdate().execute(new Runnable() {
@Override
public void run() {
doReloadUninitializedMainDictionaries(
diff --git a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java
index 8c780027be..064d79b3c3 100644
--- a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java
+++ b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java
@@ -164,12 +164,11 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
}
private void asyncExecuteTaskWithWriteLock(final Runnable task) {
- asyncExecuteTaskWithLock(mLock.writeLock(), mDictName /* executorName */, task);
+ asyncExecuteTaskWithLock(mLock.writeLock(), task);
}
- private static void asyncExecuteTaskWithLock(final Lock lock, final String executorName,
- final Runnable task) {
- ExecutorUtils.getExecutor(executorName).execute(new Runnable() {
+ private static void asyncExecuteTaskWithLock(final Lock lock, final Runnable task) {
+ ExecutorUtils.getExecutorForDynamicLanguageModelUpdate().execute(new Runnable() {
@Override
public void run() {
lock.lock();
@@ -663,7 +662,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
final String dictName = mDictName;
final File dictFile = mDictFile;
final AsyncResultHolder<DictionaryStats> result = new AsyncResultHolder<>();
- asyncExecuteTaskWithLock(mLock.readLock(), dictName /* executorName */, new Runnable() {
+ asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() {
@Override
public void run() {
final BinaryDictionary binaryDictionary = getBinaryDictionary();
@@ -714,7 +713,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
reloadDictionaryIfRequired();
final String tag = TAG;
final String dictName = mDictName;
- asyncExecuteTaskWithLock(mLock.readLock(), "dumpAllWordsForDebug", new Runnable() {
+ asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() {
@Override
public void run() {
Log.d(tag, "Dump dictionary: " + dictName + " for " + mLocale);
@@ -752,7 +751,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
public WordProperty[] getWordPropertiesForSyncing() {
reloadDictionaryIfRequired();
final AsyncResultHolder<WordProperty[]> result = new AsyncResultHolder<>();
- asyncExecuteTaskWithLock(mLock.readLock(), "sync-read", new Runnable() {
+ asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() {
@Override
public void run() {
final ArrayList<WordProperty> wordPropertyList = new ArrayList<>();
diff --git a/java/src/com/android/inputmethod/latin/spellcheck/UserDictionaryLookup.java b/java/src/com/android/inputmethod/latin/spellcheck/UserDictionaryLookup.java
index baff8f0664..19620511de 100644
--- a/java/src/com/android/inputmethod/latin/spellcheck/UserDictionaryLookup.java
+++ b/java/src/com/android/inputmethod/latin/spellcheck/UserDictionaryLookup.java
@@ -26,14 +26,13 @@ import android.util.Log;
import com.android.inputmethod.annotations.UsedForTesting;
import com.android.inputmethod.latin.common.LocaleUtils;
+import com.android.inputmethod.latin.utils.ExecutorUtils;
import java.io.Closeable;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Locale;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
@@ -82,12 +81,6 @@ public class UserDictionaryLookup implements Closeable {
private final ContentResolver mResolver;
- /**
- * Executor on which to perform the initial load and subsequent reloads (after a delay).
- */
- private final ScheduledExecutorService mLoadExecutor =
- Executors.newSingleThreadScheduledExecutor();
-
/**
* Runnable that calls loadUserDictionary().
*/
@@ -150,7 +143,8 @@ public class UserDictionaryLookup implements Closeable {
}
// Schedule a new reload after RELOAD_DELAY_MS.
- mReloadFuture = mLoadExecutor.schedule(mLoader, RELOAD_DELAY_MS, TimeUnit.MILLISECONDS);
+ mReloadFuture = ExecutorUtils.getExecutorForDynamicLanguageModelUpdate().schedule(
+ mLoader, RELOAD_DELAY_MS, TimeUnit.MILLISECONDS);
}
}
private final ContentObserver mObserver = new UserDictionaryContentObserver();
@@ -192,7 +186,8 @@ public class UserDictionaryLookup implements Closeable {
// Schedule the initial load to run immediately. It's possible that the first call to
// isValidWord occurs before the dictionary has actually loaded, so it should not
// assume that the dictionary has been loaded.
- mLoadExecutor.schedule(mLoader, 0, TimeUnit.MILLISECONDS);
+ ExecutorUtils.getExecutorForDynamicLanguageModelUpdate().schedule(
+ mLoader, 0, TimeUnit.MILLISECONDS);
// Register the observer to be notified on changes to the UserDictionary and all individual
// items.
@@ -236,9 +231,6 @@ public class UserDictionaryLookup implements Closeable {
Log.d(TAG, "Close called (no pun intended), cleaning up executor and observer");
}
if (mIsClosed.compareAndSet(false, true)) {
- // Shut down the load executor.
- mLoadExecutor.shutdown();
-
// Unregister the content observer.
mResolver.unregisterContentObserver(mObserver);
}
@@ -342,8 +334,7 @@ public class UserDictionaryLookup implements Closeable {
if (DEBUG) {
Log.d(TAG, "Loading UserDictionary");
}
- HashMap<String, ArrayList<Locale>> dictWords =
- new HashMap<String, ArrayList<Locale>>();
+ HashMap<String, ArrayList<Locale>> dictWords = new HashMap<>();
// Load the UserDictionary. Request that items be returned in the default sort order
// for UserDictionary, which is by frequency.
Cursor cursor = mResolver.query(UserDictionary.Words.CONTENT_URI,
@@ -413,7 +404,7 @@ public class UserDictionaryLookup implements Closeable {
Log.d(TAG, "Word [" + dictWord +
"] not seen for other locales, creating new entry");
}
- dictLocales = new ArrayList<Locale>();
+ dictLocales = new ArrayList<>();
dictWords.put(dictWord, dictLocales);
}
// Append the locale to the list of locales this word is in.
diff --git a/java/src/com/android/inputmethod/latin/utils/ExecutorUtils.java b/java/src/com/android/inputmethod/latin/utils/ExecutorUtils.java
index 50be16072e..c533a6273f 100644
--- a/java/src/com/android/inputmethod/latin/utils/ExecutorUtils.java
+++ b/java/src/com/android/inputmethod/latin/utils/ExecutorUtils.java
@@ -16,10 +16,12 @@
package com.android.inputmethod.latin.utils;
+import android.util.Log;
+
import com.android.inputmethod.annotations.UsedForTesting;
+import java.lang.Thread.UncaughtExceptionHandler;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
@@ -28,33 +30,49 @@ import java.util.concurrent.ThreadFactory;
* Utilities to manage executors.
*/
public class ExecutorUtils {
- static final ConcurrentHashMap<String, ScheduledExecutorService> sExecutorMap =
+
+ private static final String STATIC_LANGUAGE_MODEL_UPDATE = "StaticLanguageModelUpdate";
+ private static final String DYNAMIC_LANGUAGE_MODEL_UPDATE = "DynamicLanguageModelUpdate";
+
+ private static final ConcurrentHashMap<String, ScheduledExecutorService> sExecutorMap =
new ConcurrentHashMap<>();
- private static class ThreadFactoryWithId implements ThreadFactory {
- private final String mId;
+ @UsedForTesting
+ private static ScheduledExecutorService sExecutorServiceForTests;
- public ThreadFactoryWithId(final String id) {
- mId = id;
- }
+ @UsedForTesting
+ public static void setExecutorServiceForTests(
+ final ScheduledExecutorService executorServiceForTests) {
+ sExecutorServiceForTests = executorServiceForTests;
+ }
- @Override
- public Thread newThread(final Runnable r) {
- return new Thread(r, "Executor - " + mId);
- }
+ /**
+ * @return scheduled executor service used to update static language models
+ */
+ public static ScheduledExecutorService getExecutorForStaticLanguageModelUpdate() {
+ return getExecutor(STATIC_LANGUAGE_MODEL_UPDATE);
+ }
+
+ /**
+ * @return scheduled executor service used to update dynamic language models
+ */
+ public static ScheduledExecutorService getExecutorForDynamicLanguageModelUpdate() {
+ return getExecutor(DYNAMIC_LANGUAGE_MODEL_UPDATE);
}
/**
* Gets the executor for the given id.
*/
- public static ScheduledExecutorService getExecutor(final String id) {
+ private static ScheduledExecutorService getExecutor(final String id) {
+ if (sExecutorServiceForTests != null) {
+ return sExecutorServiceForTests;
+ }
ScheduledExecutorService executor = sExecutorMap.get(id);
if (executor == null) {
synchronized (sExecutorMap) {
executor = sExecutorMap.get(id);
if (executor == null) {
- executor = Executors.newSingleThreadScheduledExecutor(
- new ThreadFactoryWithId(id));
+ executor = Executors.newSingleThreadScheduledExecutor(new ExecutorFactory(id));
sExecutorMap.put(id, executor);
}
}
@@ -69,14 +87,42 @@ public class ExecutorUtils {
public static void shutdownAllExecutors() {
synchronized (sExecutorMap) {
for (final ScheduledExecutorService executor : sExecutorMap.values()) {
- executor.execute(new Runnable() {
- @Override
- public void run() {
- executor.shutdown();
- sExecutorMap.remove(executor);
- }
- });
+ executor.execute(new ExecutorShutdown(executor));
}
+ sExecutorMap.clear();
+ }
+ }
+
+ private static class ExecutorFactory implements ThreadFactory {
+ private final String mThreadName;
+
+ public ExecutorFactory(final String threadName) {
+ mThreadName = threadName;
+ }
+
+ @Override
+ public Thread newThread(final Runnable runnable) {
+ Thread thread = new Thread(runnable, mThreadName);
+ thread.setUncaughtExceptionHandler(new UncaughtExceptionHandler() {
+ @Override
+ public void uncaughtException(Thread thread, Throwable ex) {
+ Log.w(mThreadName + "-" + runnable.getClass().getSimpleName(), ex);
+ }
+ });
+ return thread;
+ }
+ }
+
+ private static class ExecutorShutdown implements Runnable {
+ private final ScheduledExecutorService mExecutor;
+
+ public ExecutorShutdown(final ScheduledExecutorService executor) {
+ mExecutor = executor;
+ }
+
+ @Override
+ public void run() {
+ mExecutor.shutdown();
}
}
}
diff --git a/tests/src/com/android/inputmethod/latin/utils/ExecutorUtilsTests.java b/tests/src/com/android/inputmethod/latin/utils/ExecutorUtilsTests.java
index ae2623d12d..3b1e43ed8e 100644
--- a/tests/src/com/android/inputmethod/latin/utils/ExecutorUtilsTests.java
+++ b/tests/src/com/android/inputmethod/latin/utils/ExecutorUtilsTests.java
@@ -25,18 +25,17 @@ import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
/**
- * Unit tests for ExecutorUtils.
+ * Unit tests for {@link ExecutorUtils}.
*/
@MediumTest
public class ExecutorUtilsTests extends AndroidTestCase {
private static final String TAG = ExecutorUtilsTests.class.getSimpleName();
- private static final String TEST_EXECUTOR_ID = "test";
private static final int NUM_OF_TASKS = 10;
private static final int DELAY_FOR_WAITING_TASKS_MILLISECONDS = 500;
public void testExecute() {
- final ExecutorService executor = ExecutorUtils.getExecutor(TEST_EXECUTOR_ID);
+ final ExecutorService executor = ExecutorUtils.getExecutorForDynamicLanguageModelUpdate();
final AtomicInteger v = new AtomicInteger(0);
for (int i = 0; i < NUM_OF_TASKS; ++i) {
executor.execute(new Runnable() {
--
GitLab