SharedPreferences$Editor.startCommit()
Adds a fire-and-forget save method (startCommit) to the
SharedPreferences.Editor, which is the way most people use it anyway.
This commit adds the implementation. The previous commit added the
interface and docs:
previous change: Idf9934b445da1fb72b79f0192218b47c0a7f5a34
git commit: edf32d01316bd3432c023f17747461b08ae36375
In addition, this change:
-- adds a generic "runPendingWorkFinishers" mechanism to
ActivityThread to wait on async operations that are still
in flight and use it for this.
-- ties runPendingWorkFinishers into Activity.onPause,
BroadcastReceiver, and Service.
-- makes sSharedPreferences keyed on name, not File, to avoid
unnnecessary allocations
-- documents and guarantees what thread
OnSharedPreferenceChangeListener callbacks run on
-- makes a few things in frameworks/base use startCommit(), notably
Preference.java (which was ignoring the return value anyway)
Change-Id: I1c8db60ad45643226fe6d246d3e513eeb7bd0ebd
diff --git a/core/java/android/app/ContextImpl.java b/core/java/android/app/ContextImpl.java
index 7b35e7f..0cd1f3a 100644
--- a/core/java/android/app/ContextImpl.java
+++ b/core/java/android/app/ContextImpl.java
@@ -119,9 +119,12 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Set;
import java.util.WeakHashMap;
-import java.util.Map.Entry;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.atomic.AtomicBoolean;
class ReceiverRestrictedContext extends ContextWrapper {
ReceiverRestrictedContext(Context base) {
@@ -170,8 +173,8 @@
private static ThrottleManager sThrottleManager;
private static WifiManager sWifiManager;
private static LocationManager sLocationManager;
- private static final HashMap<File, SharedPreferencesImpl> sSharedPrefs =
- new HashMap<File, SharedPreferencesImpl>();
+ private static final HashMap<String, SharedPreferencesImpl> sSharedPrefs =
+ new HashMap<String, SharedPreferencesImpl>();
private AudioManager mAudioManager;
/*package*/ LoadedApk mPackageInfo;
@@ -210,7 +213,7 @@
private File mCacheDir;
private File mExternalFilesDir;
private File mExternalCacheDir;
-
+
private static long sInstanceCount = 0;
private static final String[] EMPTY_FILE_LIST = {};
@@ -335,15 +338,15 @@
@Override
public SharedPreferences getSharedPreferences(String name, int mode) {
SharedPreferencesImpl sp;
- File f = getSharedPrefsFile(name);
synchronized (sSharedPrefs) {
- sp = sSharedPrefs.get(f);
+ sp = sSharedPrefs.get(name);
if (sp != null && !sp.hasFileChanged()) {
//Log.i(TAG, "Returning existing prefs " + name + ": " + sp);
return sp;
}
}
-
+ File f = getSharedPrefsFile(name);
+
FileInputStream str = null;
File backup = makeBackupFile(f);
if (backup.exists()) {
@@ -355,7 +358,7 @@
if (f.exists() && !f.canRead()) {
Log.w(TAG, "Attempt to read preferences file " + f + " without permission");
}
-
+
Map map = null;
if (f.exists() && f.canRead()) {
try {
@@ -376,10 +379,10 @@
//Log.i(TAG, "Updating existing prefs " + name + " " + sp + ": " + map);
sp.replace(map);
} else {
- sp = sSharedPrefs.get(f);
+ sp = sSharedPrefs.get(name);
if (sp == null) {
sp = new SharedPreferencesImpl(f, mode, map);
- sSharedPrefs.put(f, sp);
+ sSharedPrefs.put(name, sp);
}
}
return sp;
@@ -2698,10 +2701,12 @@
private final File mFile;
private final File mBackupFile;
private final int mMode;
- private Map mMap;
- private final FileStatus mFileStatus = new FileStatus();
- private long mTimestamp;
+ private Map<String, Object> mMap; // guarded by 'this'
+ private long mTimestamp; // guarded by 'this'
+ private int mDiskWritesInFlight = 0; // guarded by 'this'
+
+ private final Object mWritingToDiskLock = new Object();
private static final Object mContent = new Object();
private WeakHashMap<OnSharedPreferenceChangeListener, Object> mListeners;
@@ -2710,22 +2715,24 @@
mFile = file;
mBackupFile = makeBackupFile(file);
mMode = mode;
- mMap = initialContents != null ? initialContents : new HashMap();
- if (FileUtils.getFileStatus(file.getPath(), mFileStatus)) {
- mTimestamp = mFileStatus.mtime;
+ mMap = initialContents != null ? initialContents : new HashMap<String, Object>();
+ FileStatus stat = new FileStatus();
+ if (FileUtils.getFileStatus(file.getPath(), stat)) {
+ mTimestamp = stat.mtime;
}
mListeners = new WeakHashMap<OnSharedPreferenceChangeListener, Object>();
}
public boolean hasFileChanged() {
+ FileStatus stat = new FileStatus();
+ if (!FileUtils.getFileStatus(mFile.getPath(), stat)) {
+ return true;
+ }
synchronized (this) {
- if (!FileUtils.getFileStatus(mFile.getPath(), mFileStatus)) {
- return true;
- }
- return mTimestamp != mFileStatus.mtime;
+ return mTimestamp != stat.mtime;
}
}
-
+
public void replace(Map newContents) {
if (newContents != null) {
synchronized (this) {
@@ -2733,7 +2740,7 @@
}
}
}
-
+
public void registerOnSharedPreferenceChangeListener(OnSharedPreferenceChangeListener listener) {
synchronized(this) {
mListeners.put(listener, mContent);
@@ -2749,7 +2756,7 @@
public Map<String, ?> getAll() {
synchronized(this) {
//noinspection unchecked
- return new HashMap(mMap);
+ return new HashMap<String, Object>(mMap);
}
}
@@ -2768,7 +2775,7 @@
}
public long getLong(String key, long defValue) {
synchronized (this) {
- Long v = (Long) mMap.get(key);
+ Long v = (Long)mMap.get(key);
return v != null ? v : defValue;
}
}
@@ -2791,10 +2798,31 @@
}
}
+ public Editor edit() {
+ return new EditorImpl();
+ }
+
+ // Return value from EditorImpl#commitToMemory()
+ private static class MemoryCommitResult {
+ public boolean changesMade; // any keys different?
+ public List<String> keysModified; // may be null
+ public Set<OnSharedPreferenceChangeListener> listeners; // may be null
+ public Map<?, ?> mapToWriteToDisk;
+ public final CountDownLatch writtenToDiskLatch = new CountDownLatch(1);
+ public volatile boolean writeToDiskResult = false;
+
+ public void setDiskWriteResult(boolean result) {
+ writeToDiskResult = result;
+ writtenToDiskLatch.countDown();
+ }
+ }
+
public final class EditorImpl implements Editor {
private final Map<String, Object> mModified = Maps.newHashMap();
private boolean mClear = false;
+ private AtomicBoolean mCommitInFlight = new AtomicBoolean(false);
+
public Editor putString(String key, String value) {
synchronized (this) {
mModified.put(key, value);
@@ -2841,30 +2869,67 @@
}
public void startCommit() {
- // TODO: implement
- commit();
+ if (!mCommitInFlight.compareAndSet(false, true)) {
+ throw new IllegalStateException("can't call startCommit() twice");
+ }
+
+ final MemoryCommitResult mcr = commitToMemory();
+ final Runnable awaitCommit = new Runnable() {
+ public void run() {
+ try {
+ mcr.writtenToDiskLatch.await();
+ } catch (InterruptedException ignored) {
+ }
+ }
+ };
+
+ QueuedWork.add(awaitCommit);
+
+ Runnable postWriteRunnable = new Runnable() {
+ public void run() {
+ awaitCommit.run();
+ mCommitInFlight.set(false);
+ QueuedWork.remove(awaitCommit);
+ }
+ };
+
+ SharedPreferencesImpl.this.enqueueDiskWrite(mcr, postWriteRunnable);
+
+ // Okay to notify the listeners before it's hit disk
+ // because the listeners should always get the same
+ // SharedPreferences instance back, which has the
+ // changes reflected in memory.
+ notifyListeners(mcr);
}
- public boolean commit() {
- boolean returnValue;
-
- boolean hasListeners;
- boolean changesMade = false;
- List<String> keysModified = null;
- Set<OnSharedPreferenceChangeListener> listeners = null;
-
+ // Returns true if any changes were made
+ private MemoryCommitResult commitToMemory() {
+ MemoryCommitResult mcr = new MemoryCommitResult();
synchronized (SharedPreferencesImpl.this) {
- hasListeners = mListeners.size() > 0;
+ // We optimistically don't make a deep copy until
+ // a memory commit comes in when we're already
+ // writing to disk.
+ if (mDiskWritesInFlight > 0) {
+ // We can't modify our mMap as a currently
+ // in-flight write owns it. Clone it before
+ // modifying it.
+ // noinspection unchecked
+ mMap = new HashMap<String, Object>(mMap);
+ }
+ mcr.mapToWriteToDisk = mMap;
+ mDiskWritesInFlight++;
+
+ boolean hasListeners = mListeners.size() > 0;
if (hasListeners) {
- keysModified = new ArrayList<String>();
- listeners =
- new HashSet<OnSharedPreferenceChangeListener>(mListeners.keySet());
+ mcr.keysModified = new ArrayList<String>();
+ mcr.listeners =
+ new HashSet<OnSharedPreferenceChangeListener>(mListeners.keySet());
}
synchronized (this) {
if (mClear) {
if (!mMap.isEmpty()) {
- changesMade = true;
+ mcr.changesMade = true;
mMap.clear();
}
mClear = false;
@@ -2874,53 +2939,122 @@
String k = e.getKey();
Object v = e.getValue();
if (v == this) { // magic value for a removal mutation
- if (mMap.containsKey(k)) {
- mMap.remove(k);
- changesMade = true;
+ if (!mMap.containsKey(k)) {
+ continue;
}
+ mMap.remove(k);
} else {
boolean isSame = false;
if (mMap.containsKey(k)) {
Object existingValue = mMap.get(k);
- isSame = existingValue != null && existingValue.equals(v);
+ if (existingValue != null && existingValue.equals(v)) {
+ continue;
+ }
}
- if (!isSame) {
- mMap.put(k, v);
- changesMade = true;
- }
+ mMap.put(k, v);
}
+ mcr.changesMade = true;
if (hasListeners) {
- keysModified.add(k);
+ mcr.keysModified.add(k);
}
}
mModified.clear();
}
-
- returnValue = writeFileLocked(changesMade);
}
+ return mcr;
+ }
- if (hasListeners) {
- for (int i = keysModified.size() - 1; i >= 0; i--) {
- final String key = keysModified.get(i);
- for (OnSharedPreferenceChangeListener listener : listeners) {
+ public boolean commit() {
+ MemoryCommitResult mcr = commitToMemory();
+ SharedPreferencesImpl.this.enqueueDiskWrite(
+ mcr, null /* sync write on this thread okay */);
+ try {
+ mcr.writtenToDiskLatch.await();
+ } catch (InterruptedException e) {
+ return false;
+ }
+ notifyListeners(mcr);
+ return mcr.writeToDiskResult;
+ }
+
+ private void notifyListeners(final MemoryCommitResult mcr) {
+ if (mcr.listeners == null || mcr.keysModified == null ||
+ mcr.keysModified.size() == 0) {
+ return;
+ }
+ if (Looper.myLooper() == Looper.getMainLooper()) {
+ for (int i = mcr.keysModified.size() - 1; i >= 0; i--) {
+ final String key = mcr.keysModified.get(i);
+ for (OnSharedPreferenceChangeListener listener : mcr.listeners) {
if (listener != null) {
listener.onSharedPreferenceChanged(SharedPreferencesImpl.this, key);
}
}
}
+ } else {
+ // Run this function on the main thread.
+ ActivityThread.sMainThreadHandler.post(new Runnable() {
+ public void run() {
+ notifyListeners(mcr);
+ }
+ });
}
-
- return returnValue;
}
}
- public Editor edit() {
- return new EditorImpl();
+ /**
+ * Enqueue an already-committed-to-memory result to be written
+ * to disk.
+ *
+ * They will be written to disk one-at-a-time in the order
+ * that they're enqueued.
+ *
+ * @param postWriteRunnable if non-null, we're being called
+ * from startCommit() and this is the runnable to run after
+ * the write proceeds. if null (from a regular commit()),
+ * then we're allowed to do this disk write on the main
+ * thread (which in addition to reducing allocations and
+ * creating a background thread, this has the advantage that
+ * we catch them in userdebug StrictMode reports to convert
+ * them where possible to startCommit...)
+ */
+ private void enqueueDiskWrite(final MemoryCommitResult mcr,
+ final Runnable postWriteRunnable) {
+ final Runnable writeToDiskRunnable = new Runnable() {
+ public void run() {
+ synchronized (mWritingToDiskLock) {
+ writeToFile(mcr);
+ }
+ synchronized (SharedPreferencesImpl.this) {
+ mDiskWritesInFlight--;
+ }
+ if (postWriteRunnable != null) {
+ postWriteRunnable.run();
+ }
+ }
+ };
+
+ final boolean isFromSyncCommit = (postWriteRunnable == null);
+
+ // Typical #commit() path with fewer allocations, doing a write on
+ // the current thread.
+ if (isFromSyncCommit) {
+ boolean wasEmpty = false;
+ synchronized (SharedPreferencesImpl.this) {
+ wasEmpty = mDiskWritesInFlight == 1;
+ }
+ if (wasEmpty) {
+ writeToDiskRunnable.run();
+ return;
+ }
+ }
+
+ QueuedWork.singleThreadExecutor().execute(writeToDiskRunnable);
}
- private FileOutputStream createFileOutputStream(File file) {
+ private static FileOutputStream createFileOutputStream(File file) {
FileOutputStream str = null;
try {
str = new FileOutputStream(file);
@@ -2943,49 +3077,56 @@
return str;
}
- private boolean writeFileLocked(boolean changesMade) {
+ // Note: must hold mWritingToDiskLock
+ private void writeToFile(MemoryCommitResult mcr) {
// Rename the current file so it may be used as a backup during the next read
if (mFile.exists()) {
- if (!changesMade) {
+ 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.
- return true;
+ mcr.setDiskWriteResult(true);
+ return;
}
if (!mBackupFile.exists()) {
if (!mFile.renameTo(mBackupFile)) {
Log.e(TAG, "Couldn't rename file " + mFile
+ " to backup file " + mBackupFile);
- return false;
+ mcr.setDiskWriteResult(false);
+ return;
}
} else {
mFile.delete();
}
}
-
+
// Attempt to write the file, delete the backup and return true as atomically as
// possible. If any exception occurs, delete the new file; next time we will restore
// from the backup.
try {
FileOutputStream str = createFileOutputStream(mFile);
if (str == null) {
- return false;
+ mcr.setDiskWriteResult(false);
+ return;
}
- XmlUtils.writeMapXml(mMap, str);
+ XmlUtils.writeMapXml(mcr.mapToWriteToDisk, str);
str.close();
setFilePermissionsFromMode(mFile.getPath(), mMode, 0);
- if (FileUtils.getFileStatus(mFile.getPath(), mFileStatus)) {
- mTimestamp = mFileStatus.mtime;
+ FileStatus stat = new FileStatus();
+ if (FileUtils.getFileStatus(mFile.getPath(), stat)) {
+ synchronized (this) {
+ mTimestamp = stat.mtime;
+ }
}
-
// Writing was successful, delete the backup file if there is one.
mBackupFile.delete();
- return true;
+ mcr.setDiskWriteResult(true);
+ return;
} catch (XmlPullParserException e) {
- Log.w(TAG, "writeFileLocked: Got exception:", e);
+ Log.w(TAG, "writeToFile: Got exception:", e);
} catch (IOException e) {
- Log.w(TAG, "writeFileLocked: Got exception:", e);
+ Log.w(TAG, "writeToFile: Got exception:", e);
}
// Clean up an unsuccessfully written file
if (mFile.exists()) {
@@ -2993,7 +3134,7 @@
Log.e(TAG, "Couldn't clean up partially-written file " + mFile);
}
}
- return false;
+ mcr.setDiskWriteResult(false);
}
}
}