From d15c4f1da58de847ebdecdfade96d21ba8128929 Mon Sep 17 00:00:00 2001 From: "Philip P. Moltmann" Date: Tue, 13 Dec 2016 16:23:21 -0800 Subject: [PATCH] Only persist last Shared Preferences state If multiple async shared preferences writes are queued, all but the last one can be ignored as they will be overwritten by the last one anyway. For commit() we need to make sure that we have at least persisted the state of the commit. Generation counts are 64 bit, hence they never overflow. Test: Produced a lot of SharedPreferences.Editor.apply and did not see excessive writes anymore, ran SharedPreferences CTS tests Bug: 33385963 Change-Id: I3968ed4b71befee6eeb90bea1666a0bb646544f6 (cherry picked from commit 31d6889f4c89dd8498e2095f9d8a3c39fbd17c86) --- .../android/app/SharedPreferencesImpl.java | 72 +++++++++++++++---- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/core/java/android/app/SharedPreferencesImpl.java b/core/java/android/app/SharedPreferencesImpl.java index c1180e25a0d37..c5a8288b500fc 100644 --- a/core/java/android/app/SharedPreferencesImpl.java +++ b/core/java/android/app/SharedPreferencesImpl.java @@ -26,6 +26,8 @@ import android.system.StructStat; import android.util.Log; import com.google.android.collect.Maps; + +import com.android.internal.annotations.GuardedBy; import com.android.internal.util.XmlUtils; import dalvik.system.BlockGuard; @@ -72,6 +74,14 @@ final class SharedPreferencesImpl implements SharedPreferences { private final WeakHashMap mListeners = new WeakHashMap(); + /** Current memory state (always increasing) */ + @GuardedBy("this") + private long mCurrentMemoryStateGeneration; + + /** Latest memory state that was committed to disk */ + @GuardedBy("mWritingToDiskLock") + private long mDiskStateGeneration; + SharedPreferencesImpl(File file, int mode) { mFile = file; mBackupFile = makeBackupFile(file); @@ -289,7 +299,7 @@ final class SharedPreferencesImpl implements SharedPreferences { // Return value from EditorImpl#commitToMemory() private static class MemoryCommitResult { - public boolean changesMade; // any keys different? + public long memoryStateGeneration; public List keysModified; // may be null public Set listeners; // may be null public Map mapToWriteToDisk; @@ -412,9 +422,11 @@ final class SharedPreferencesImpl implements SharedPreferences { } synchronized (this) { + boolean changesMade = false; + if (mClear) { if (!mMap.isEmpty()) { - mcr.changesMade = true; + changesMade = true; mMap.clear(); } mClear = false; @@ -441,13 +453,19 @@ final class SharedPreferencesImpl implements SharedPreferences { mMap.put(k, v); } - mcr.changesMade = true; + changesMade = true; if (hasListeners) { mcr.keysModified.add(k); } } mModified.clear(); + + if (changesMade) { + mCurrentMemoryStateGeneration++; + } + + mcr.memoryStateGeneration = mCurrentMemoryStateGeneration; } } return mcr; @@ -509,10 +527,12 @@ final class SharedPreferencesImpl implements SharedPreferences { */ private void enqueueDiskWrite(final MemoryCommitResult mcr, final Runnable postWriteRunnable) { + final boolean isFromSyncCommit = (postWriteRunnable == null); + final Runnable writeToDiskRunnable = new Runnable() { public void run() { synchronized (mWritingToDiskLock) { - writeToFile(mcr); + writeToFile(mcr, isFromSyncCommit); } synchronized (SharedPreferencesImpl.this) { mDiskWritesInFlight--; @@ -523,8 +543,6 @@ final class SharedPreferencesImpl implements SharedPreferences { } }; - final boolean isFromSyncCommit = (postWriteRunnable == null); - // Typical #commit() path with fewer allocations, doing a write on // the current thread. if (isFromSyncCommit) { @@ -538,6 +556,10 @@ final class SharedPreferencesImpl implements SharedPreferences { } } + if (DEBUG) { + Log.d(TAG, "added " + mcr.memoryStateGeneration + " -> " + mFile.getName()); + } + QueuedWork.singleThreadExecutor().execute(writeToDiskRunnable); } @@ -565,17 +587,34 @@ final class SharedPreferencesImpl implements SharedPreferences { } // Note: must hold mWritingToDiskLock - private void writeToFile(MemoryCommitResult mcr) { + private void writeToFile(MemoryCommitResult mcr, boolean isFromSyncCommit) { // Rename the current file so it may be used as a backup during the next read if (mFile.exists()) { - if (!mcr.changesMade) { - // If the file already exists, but no changes were - // made to the underlying map, it's wasteful to - // re-write the file. Return as if we wrote it - // out. + boolean needsWrite = false; + + if (isFromSyncCommit) { + // Only need to write if the disk state is older than this commit + if (mDiskStateGeneration < mcr.memoryStateGeneration) { + needsWrite = true; + } + } else { + synchronized (this) { + // No need to persist intermediate states. Just wait for the latest state to be + // persisted. + if (mCurrentMemoryStateGeneration == mcr.memoryStateGeneration) { + needsWrite = true; + } + } + } + + if (!needsWrite) { + if (DEBUG) { + Log.d(TAG, "skipped " + mcr.memoryStateGeneration + " -> " + mFile.getName()); + } mcr.setDiskWriteResult(true); return; } + if (!mBackupFile.exists()) { if (!mFile.renameTo(mBackupFile)) { Log.e(TAG, "Couldn't rename file " + mFile @@ -599,6 +638,11 @@ final class SharedPreferencesImpl implements SharedPreferences { } XmlUtils.writeMapXml(mcr.mapToWriteToDisk, str); FileUtils.sync(str); + + if (DEBUG) { + Log.d(TAG, "wrote " + mcr.memoryStateGeneration + " -> " + mFile.getName()); + } + str.close(); ContextImpl.setFilePermissionsFromMode(mFile.getPath(), mMode, 0); try { @@ -612,7 +656,11 @@ final class SharedPreferencesImpl implements SharedPreferences { } // Writing was successful, delete the backup file if there is one. mBackupFile.delete(); + + mDiskStateGeneration = mcr.memoryStateGeneration; + mcr.setDiskWriteResult(true); + return; } catch (XmlPullParserException e) { Log.w(TAG, "writeToFile: Got exception:", e);