diff --git a/java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java index 88122792e..09d0ea210 100644 --- a/java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java +++ b/java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java @@ -87,9 +87,6 @@ public class ContactsBinaryDictionary extends ExpandableBinaryDictionary { mLocale = locale; mUseFirstLastBigrams = useFirstLastBigramsForLocale(locale); registerObserver(context); - - // Load the current binary dictionary from internal storage. If no binary dictionary exists, - // reloadDictionaryIfRequired will start a new thread to generate one asynchronously. reloadDictionaryIfRequired(); } diff --git a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java index 3eafcb245..3d6073350 100644 --- a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java +++ b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java @@ -26,7 +26,6 @@ import com.android.inputmethod.latin.makedict.FormatSpec; import com.android.inputmethod.latin.makedict.UnsupportedFormatException; import com.android.inputmethod.latin.makedict.WordProperty; import com.android.inputmethod.latin.SuggestedWords.SuggestedWordInfo; -import com.android.inputmethod.latin.utils.AsyncResultHolder; import com.android.inputmethod.latin.utils.CombinedFormatUtils; import com.android.inputmethod.latin.utils.ExecutorUtils; import com.android.inputmethod.latin.utils.FileUtils; @@ -40,6 +39,8 @@ import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantReadWriteLock; /** * Abstract base class for an expandable dictionary that can be created and updated dynamically @@ -59,7 +60,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { private static final boolean DBG_STRESS_TEST = false; private static final int TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS = 100; - private static final int TIMEOUT_FOR_READ_OPS_FOR_TESTS_IN_MILLISECONDS = 10000; private static final int DEFAULT_MAX_UNIGRAM_COUNT = 10000; private static final int DEFAULT_MAX_BIGRAM_COUNT = 10000; @@ -93,9 +93,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { /** Dictionary file */ private final File mDictFile; - private final AtomicBoolean mProcessingLargeTask; + /** Indicates whether a task for reloading the dictionary has been scheduled. */ + private final AtomicBoolean mIsReloading; + + /** Indicates whether the current dictionary needs to be reloaded. */ private boolean mNeedsToReload; + private final ReentrantReadWriteLock mLock; /* A extension for a binary dictionary file. */ protected static final String DICT_FILE_EXTENSION = ".dict"; @@ -144,8 +148,9 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { mLocale = locale; mDictFile = getDictFile(context, dictName, dictFile); mBinaryDictionary = null; - mProcessingLargeTask = new AtomicBoolean(); + mIsReloading = new AtomicBoolean(); mNeedsToReload = false; + mLock = new ReentrantReadWriteLock(); } public static File getDictFile(final Context context, final String dictName, @@ -159,12 +164,30 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { return dictFile != null ? dictFile.getName() : name + "." + locale.toString(); } + private void asyncExecuteTaskWithWriteLock(final Runnable task) { + asyncExecuteTaskWithLock(mLock.writeLock(), task); + } + + private void asyncExecuteTaskWithLock(final Lock lock, final Runnable task) { + ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + @Override + public void run() { + lock.lock(); + try { + task.run(); + } finally { + lock.unlock(); + } + } + }); + } + /** * Closes and cleans up the binary dictionary. */ @Override public void close() { - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { if (mBinaryDictionary != null) { @@ -188,6 +211,15 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { return attributeMap; } + private void removeBinaryDictionary() { + asyncExecuteTaskWithWriteLock(new Runnable() { + @Override + public void run() { + removeBinaryDictionaryLocked(); + } + }); + } + private void removeBinaryDictionaryLocked() { if (mBinaryDictionary != null) { mBinaryDictionary.close(); @@ -211,7 +243,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { } public void clear() { - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { removeBinaryDictionaryLocked(); @@ -224,13 +256,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { * Check whether GC is needed and run GC if required. */ protected void runGCIfRequired(final boolean mindsBlockByGC) { - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { if (mBinaryDictionary == null) { return; } - runGCAfterAllPrioritizedTasksIfRequiredLocked(mindsBlockByGC); + runGCIfRequiredLocked(mindsBlockByGC); } }); } @@ -241,25 +273,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { } } - private void runGCAfterAllPrioritizedTasksIfRequiredLocked(final boolean mindsBlockByGC) { - // needsToRunGC() have to be called with lock. - if (mBinaryDictionary.needsToRunGC(mindsBlockByGC)) { - if (setProcessingLargeTaskIfNot()) { - // Run GC after currently existing time sensitive operations. - ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() { - @Override - public void run() { - try { - mBinaryDictionary.flushWithGC(); - } finally { - mProcessingLargeTask.set(false); - } - } - }); - } - } - } - /** * Dynamically adds a word unigram to the dictionary. May overwrite an existing entry. */ @@ -267,13 +280,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { final String shortcutTarget, final int shortcutFreq, final boolean isNotAWord, final boolean isBlacklisted, final int timestamp) { reloadDictionaryIfRequired(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { if (mBinaryDictionary == null) { return; } - runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */); + runGCIfRequiredLocked(true /* mindsBlockByGC */); addWordDynamicallyLocked(word, frequency, shortcutTarget, shortcutFreq, isNotAWord, isBlacklisted, timestamp); } @@ -293,13 +306,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { public void addBigramDynamically(final String word0, final String word1, final int frequency, final int timestamp) { reloadDictionaryIfRequired(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { if (mBinaryDictionary == null) { return; } - runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */); + runGCIfRequiredLocked(true /* mindsBlockByGC */); addBigramDynamicallyLocked(word0, word1, frequency, timestamp); } }); @@ -315,13 +328,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { */ public void removeBigramDynamically(final String word0, final String word1) { reloadDictionaryIfRequired(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { if (mBinaryDictionary == null) { return; } - runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */); + runGCIfRequiredLocked(true /* mindsBlockByGC */); mBinaryDictionary.removeBigramWords(word0, word1); } }); @@ -338,14 +351,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { final ArrayList languageModelParams, final AddMultipleDictionaryEntriesCallback callback) { reloadDictionaryIfRequired(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { - if (mBinaryDictionary == null) { - return; - } - final boolean locked = setProcessingLargeTaskIfNot(); try { + if (mBinaryDictionary == null) { + return; + } mBinaryDictionary.addMultipleDictionaryEntries( languageModelParams.toArray( new LanguageModelParam[languageModelParams.size()])); @@ -353,9 +365,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { if (callback != null) { callback.onFinished(); } - if (locked) { - mProcessingLargeTask.set(false); - } } } }); @@ -367,29 +376,33 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { final boolean blockOffensiveWords, final int[] additionalFeaturesOptions, final int sessionId, final float[] inOutLanguageWeight) { reloadDictionaryIfRequired(); - if (processingLargeTask()) { - return null; - } - final AsyncResultHolder> holder = - new AsyncResultHolder>(); - ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() { - @Override - public void run() { + boolean lockAcquired = false; + try { + lockAcquired = mLock.readLock().tryLock( + TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS, TimeUnit.MILLISECONDS); + if (lockAcquired) { if (mBinaryDictionary == null) { - holder.set(null); - return; + return null; } - final ArrayList binarySuggestion = + final ArrayList suggestions = mBinaryDictionary.getSuggestionsWithSessionId(composer, prevWord, proximityInfo, blockOffensiveWords, additionalFeaturesOptions, sessionId, inOutLanguageWeight); - holder.set(binarySuggestion); if (mBinaryDictionary.isCorrupted()) { - removeBinaryDictionaryLocked(); + Log.i(TAG, "Dictionary (" + mDictName +") is corrupted. " + + "Remove and regenerate it."); + removeBinaryDictionary(); } + return suggestions; } - }); - return holder.get(null, TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS); + } catch (final InterruptedException e) { + Log.e(TAG, "Interrupted tryLock() in getSuggestionsWithSessionId().", e); + } finally { + if (lockAcquired) { + mLock.readLock().unlock(); + } + } + return null; } @Override @@ -404,17 +417,24 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { @Override public boolean isValidWord(final String word) { reloadDictionaryIfRequired(); - if (processingLargeTask()) { - return false; - } - final AsyncResultHolder holder = new AsyncResultHolder(); - ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() { - @Override - public void run() { - holder.set(isValidWordLocked(word)); + boolean lockAcquired = false; + try { + lockAcquired = mLock.readLock().tryLock( + TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS, TimeUnit.MILLISECONDS); + if (lockAcquired) { + if (mBinaryDictionary == null) { + return false; + } + return isValidWordLocked(word); } - }); - return holder.get(false, TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS); + } catch (final InterruptedException e) { + Log.e(TAG, "Interrupted tryLock() in isValidWord().", e); + } finally { + if (lockAcquired) { + mLock.readLock().unlock(); + } + } + return false; } protected boolean isValidWordLocked(final String word) { @@ -484,13 +504,14 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { } /** - * Reloads the dictionary if required. + * Load the current binary dictionary from internal storage. If the dictionary file doesn't + * exists or needs to be regenerated, the new dictionary file will be asynchronously generated. + * However, the dictionary itself is accessible even before the new dictionary file is actually + * generated. It may return a null result for getSuggestions() in that case by design. */ public final void reloadDictionaryIfRequired() { if (!isReloadRequired()) return; - if (setProcessingLargeTaskIfNot()) { - reloadDictionary(); - } + asyncReloadDictionary(); } /** @@ -500,52 +521,39 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { return mBinaryDictionary == null || mNeedsToReload; } - private boolean processingLargeTask() { - return mProcessingLargeTask.get(); - } - - // Returns whether the dictionary is being used for a large task. If true, we should not use - // this dictionary for latency sensitive operations. - private boolean setProcessingLargeTaskIfNot() { - return mProcessingLargeTask.compareAndSet(false /* expect */ , true /* update */); - } - /** - * Reloads the dictionary. Access is controlled on a per dictionary file basis and supports - * concurrent calls from multiple instances that share the same dictionary file. + * Reloads the dictionary. Access is controlled on a per dictionary file basis. */ - private final void reloadDictionary() { - // Ensure that only one thread attempts to read or write to the shared binary dictionary - // file at the same time. - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { - @Override - public void run() { - try { - if (!dictionaryFileExists() || haveContentsChanged()) { - // If the shared dictionary file does not exist or is out of date and - // contents have been updated, the first instance that acquires the lock - // will generate a new one - createNewDictionaryLocked(); - } else if (mBinaryDictionary == null) { - // Otherwise, if the local dictionary is older than the shared dictionary, - // load the shared dictionary. - loadBinaryDictionaryLocked(); + private final void asyncReloadDictionary() { + if (mIsReloading.compareAndSet(false, true)) { + mNeedsToReload = false; + asyncExecuteTaskWithWriteLock(new Runnable() { + @Override + public void run() { + try { + if (!dictionaryFileExists() || haveContentsChanged()) { + // If the shared dictionary file does not exist or contents have been + // updated, generate a new one. + createNewDictionaryLocked(); + } else if (mBinaryDictionary == null) { + // Otherwise, load the existing dictionary. + loadBinaryDictionaryLocked(); + } + if (mBinaryDictionary != null && !(isValidDictionaryLocked() + // TODO: remove the check below + && matchesExpectedBinaryDictFormatVersionForThisType( + mBinaryDictionary.getFormatVersion()))) { + // Binary dictionary or its format version is not valid. Regenerate + // the dictionary file. writeBinaryDictionary will remove the + // existing files if appropriate. + createNewDictionaryLocked(); + } + } finally { + mIsReloading.set(false); } - mNeedsToReload = false; - if (mBinaryDictionary != null && !(isValidDictionaryLocked() - // TODO: remove the check below - && matchesExpectedBinaryDictFormatVersionForThisType( - mBinaryDictionary.getFormatVersion()))) { - // Binary dictionary or its format version is not valid. Regenerate - // the dictionary file. writeBinaryDictionary will remove the - // existing files if appropriate. - createNewDictionaryLocked(); - } - } finally { - mProcessingLargeTask.set(false); } - } - }); + }); + } } // TODO: cache the file's existence so that we avoid doing a disk access each time. @@ -557,7 +565,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { * Flush binary dictionary to dictionary file. */ public void asyncFlushBinaryDictionary() { - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithWriteLock(new Runnable() { @Override public void run() { flushDictionaryLocked(); @@ -568,16 +576,15 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { // TODO: Implement BinaryDictionary.isInDictionary(). @UsedForTesting public boolean isInUnderlyingBinaryDictionaryForTests(final String word) { - final AsyncResultHolder holder = new AsyncResultHolder(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { - @Override - public void run() { - if (mDictType == Dictionary.TYPE_USER_HISTORY) { - holder.set(mBinaryDictionary.isValidWord(word)); - } + mLock.readLock().lock(); + try { + if (mBinaryDictionary != null && mDictType == Dictionary.TYPE_USER_HISTORY) { + return mBinaryDictionary.isValidWord(word); } - }); - return holder.get(false, TIMEOUT_FOR_READ_OPS_FOR_TESTS_IN_MILLISECONDS); + return false; + } finally { + mLock.readLock().unlock(); + } } @UsedForTesting @@ -599,7 +606,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { @UsedForTesting public void dumpAllWordsForDebug() { reloadDictionaryIfRequired(); - ExecutorUtils.getExecutor(mDictName).execute(new Runnable() { + asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() { @Override public void run() { Log.d(TAG, "Dump dictionary: " + mDictName); diff --git a/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java b/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java index 4399ba04f..f2d7b76b2 100644 --- a/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java +++ b/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java @@ -264,6 +264,7 @@ public class UserHistoryDictionaryTests extends AndroidTestCase { for (final String word : words) { UserHistoryDictionary.addToDictionary(dict, prevWord, word, true, mCurrentTime); prevWord = word; + dict.waitAllTasksForTests(); assertTrue(dict.isInUnderlyingBinaryDictionaryForTests(word)); } forcePassingShortTime();