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)

1 files changed, 60 insertions, 12 deletions

diff --git a/core/java/android/app/SharedPreferencesImpl.java b/core/java/android/app/SharedPreferencesImpl.java
index c1180e25a0d3..c5a8288b500f 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<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 @@ 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<String> keysModified;  // may be null
         public Set<OnSharedPreferenceChangeListener> 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);