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)
(cherry picked from commit d15c4f1da58de847ebdecdfade96d21ba8128929)
diff --git a/core/java/android/app/SharedPreferencesImpl.java b/core/java/android/app/SharedPreferencesImpl.java
index c1180e2..c5a8288 100644
--- a/core/java/android/app/SharedPreferencesImpl.java
+++ b/core/java/android/app/SharedPreferencesImpl.java
@@ -26,6 +26,8 @@
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 @@
private final WeakHashMap<OnSharedPreferenceChangeListener, Object> mListeners =
new WeakHashMap<OnSharedPreferenceChangeListener, Object>();
+ /** 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 @@
// Return value from EditorImpl#commitToMemory()
private static class MemoryCommitResult {
- public boolean changesMade; // any keys different?
+ public long memoryStateGeneration;
public List<String> keysModified; // may be null
public Set<OnSharedPreferenceChangeListener> listeners; // may be null
public Map<?, ?> mapToWriteToDisk;
@@ -412,9 +422,11 @@
}
synchronized (this) {
+ boolean changesMade = false;
+
if (mClear) {
if (!mMap.isEmpty()) {
- mcr.changesMade = true;
+ changesMade = true;
mMap.clear();
}
mClear = false;
@@ -441,13 +453,19 @@
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 @@
*/
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 boolean isFromSyncCommit = (postWriteRunnable == null);
-
// Typical #commit() path with fewer allocations, doing a write on
// the current thread.
if (isFromSyncCommit) {
@@ -538,6 +556,10 @@
}
}
+ if (DEBUG) {
+ Log.d(TAG, "added " + mcr.memoryStateGeneration + " -> " + mFile.getName());
+ }
+
QueuedWork.singleThreadExecutor().execute(writeToDiskRunnable);
}
@@ -565,17 +587,34 @@
}
// 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 @@
}
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 @@
}
// 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);
