From 0b03f13cabec84d2d841fde47ce9fec0d531b6a1 Mon Sep 17 00:00:00 2001 From: Dan Zivkovic 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 019d17d56..b23226dce 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 3d76751ce..31e9d0305 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 8c780027b..064d79b3c 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 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 result = new AsyncResultHolder<>(); - asyncExecuteTaskWithLock(mLock.readLock(), "sync-read", new Runnable() { + asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() { @Override public void run() { final ArrayList 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 baff8f066..19620511d 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> dictWords = - new HashMap>(); + HashMap> 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(); + 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 50be16072..c533a6273 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 sExecutorMap = + + private static final String STATIC_LANGUAGE_MODEL_UPDATE = "StaticLanguageModelUpdate"; + private static final String DYNAMIC_LANGUAGE_MODEL_UPDATE = "DynamicLanguageModelUpdate"; + + private static final ConcurrentHashMap 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 ae2623d12..3b1e43ed8 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() {