Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6284ee7e authored by Songchun Fan's avatar Songchun Fan Committed by Song Chun Fan
Browse files

[SettingsProvider] do not write under mLock 

persistSyncLocked() is called when holding the SettingsProvider lock.
It writes to disk synchronously, which can sometimes take long. This
blocks future reads that also tries to acquire the lock and causes SysUI
jankiness. This CL changes the synchronous write to async, just like any
other write operations in SettingsProvider.

BUG: 284452689
Test: atest com.android.providers.settings.SettingsStateTest

Change-Id: I318c4c6f358dbaa227cb53019db93f262ed10443
parent 779139dd

packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java

+13 −13
Original line number Original line Diff line number Diff line
@@ -3243,7 +3243,7 @@ public class SettingsProvider extends ContentProvider { 
            }

            }





            if (success && criticalSettings != null && criticalSettings.contains(name)) {

            if (success && criticalSettings != null && criticalSettings.contains(name)) {

                settingsState.persistSyncLocked();

                settingsState.persistSettingsLocked();

            }

            }





            if (forceNotify || success) {

            if (forceNotify || success) {
@@ -3294,7 +3294,7 @@ public class SettingsProvider extends ContentProvider { 
            }

            }





            if (success && criticalSettings != null && criticalSettings.contains(name)) {

            if (success && criticalSettings != null && criticalSettings.contains(name)) {

                settingsState.persistSyncLocked();

                settingsState.persistSettingsLocked();

            }

            }





            if (forceNotify || success) {

            if (forceNotify || success) {
@@ -3319,7 +3319,7 @@ public class SettingsProvider extends ContentProvider { 
            }

            }





            if (success && criticalSettings != null && criticalSettings.contains(name)) {

            if (success && criticalSettings != null && criticalSettings.contains(name)) {

                settingsState.persistSyncLocked();

                settingsState.persistSettingsLocked();

            }

            }





            if (forceNotify || success) {

            if (forceNotify || success) {
@@ -3385,7 +3385,7 @@ public class SettingsProvider extends ContentProvider { 
                            }

                            }

                        }

                        }

                        if (someSettingChanged) {

                        if (someSettingChanged) {

                            settingsState.persistSyncLocked();

                            settingsState.persistSettingsLocked();

                            success = true;

                            success = true;

                        }

                        }

                    }

                    }
@@ -3407,7 +3407,7 @@ public class SettingsProvider extends ContentProvider { 
                            }

                            }

                        }

                        }

                        if (someSettingChanged) {

                        if (someSettingChanged) {

                            settingsState.persistSyncLocked();

                            settingsState.persistSettingsLocked();

                            success = true;

                            success = true;

                        }

                        }

                    }

                    }
@@ -3435,7 +3435,7 @@ public class SettingsProvider extends ContentProvider { 
                            }

                            }

                        }

                        }

                        if (someSettingChanged) {

                        if (someSettingChanged) {

                            settingsState.persistSyncLocked();

                            settingsState.persistSettingsLocked();

                            success = true;

                            success = true;

                        }

                        }

                    }

                    }
@@ -3460,7 +3460,7 @@ public class SettingsProvider extends ContentProvider { 
                            logSettingChanged(userId, name, type, CHANGE_TYPE_DELETE);

                            logSettingChanged(userId, name, type, CHANGE_TYPE_DELETE);

                        }

                        }

                        if (someSettingChanged) {

                        if (someSettingChanged) {

                            settingsState.persistSyncLocked();

                            settingsState.persistSettingsLocked();

                            success = true;

                            success = true;

                        }

                        }

                    }

                    }
@@ -3559,7 +3559,7 @@ public class SettingsProvider extends ContentProvider { 
            ensureSettingsStateLocked(systemKey);

            ensureSettingsStateLocked(systemKey);

            SettingsState systemSettings = mSettingsStates.get(systemKey);

            SettingsState systemSettings = mSettingsStates.get(systemKey);

            migrateLegacySettingsLocked(systemSettings, database, TABLE_SYSTEM);

            migrateLegacySettingsLocked(systemSettings, database, TABLE_SYSTEM);

            systemSettings.persistSyncLocked();

            systemSettings.persistSettingsLocked();





            // Move over the secure settings.

            // Move over the secure settings.

            // Do this after System settings, since this is the first thing we check when deciding

            // Do this after System settings, since this is the first thing we check when deciding
@@ -3569,7 +3569,7 @@ public class SettingsProvider extends ContentProvider { 
            SettingsState secureSettings = mSettingsStates.get(secureKey);

            SettingsState secureSettings = mSettingsStates.get(secureKey);

            migrateLegacySettingsLocked(secureSettings, database, TABLE_SECURE);

            migrateLegacySettingsLocked(secureSettings, database, TABLE_SECURE);

            ensureSecureSettingAndroidIdSetLocked(secureSettings);

            ensureSecureSettingAndroidIdSetLocked(secureSettings);

            secureSettings.persistSyncLocked();

            secureSettings.persistSettingsLocked();





            // Move over the global settings if owner.

            // Move over the global settings if owner.

            // Do this last, since this is the first thing we check when deciding

            // Do this last, since this is the first thing we check when deciding
@@ -3585,7 +3585,7 @@ public class SettingsProvider extends ContentProvider { 
                            mSettingsCreationBuildId, null, true,

                            mSettingsCreationBuildId, null, true,

                            SettingsState.SYSTEM_PACKAGE_NAME);

                            SettingsState.SYSTEM_PACKAGE_NAME);

                }

                }

                globalSettings.persistSyncLocked();

                globalSettings.persistSettingsLocked();

            }

            }





            // Drop the database as now all is moved and persisted.

            // Drop the database as now all is moved and persisted.
@@ -4404,16 +4404,16 @@ public class SettingsProvider extends ContentProvider { 
                    if (userId == UserHandle.USER_SYSTEM) {

                    if (userId == UserHandle.USER_SYSTEM) {

                        SettingsState globalSettings = getGlobalSettingsLocked();

                        SettingsState globalSettings = getGlobalSettingsLocked();

                        ensureLegacyDefaultValueAndSystemSetUpdatedLocked(globalSettings, userId);

                        ensureLegacyDefaultValueAndSystemSetUpdatedLocked(globalSettings, userId);

                        globalSettings.persistSyncLocked();

                        globalSettings.persistSettingsLocked();

                    }

                    }





                    SettingsState secureSettings = getSecureSettingsLocked(mUserId);

                    SettingsState secureSettings = getSecureSettingsLocked(mUserId);

                    ensureLegacyDefaultValueAndSystemSetUpdatedLocked(secureSettings, userId);

                    ensureLegacyDefaultValueAndSystemSetUpdatedLocked(secureSettings, userId);

                    secureSettings.persistSyncLocked();

                    secureSettings.persistSettingsLocked();





                    SettingsState systemSettings = getSystemSettingsLocked(mUserId);

                    SettingsState systemSettings = getSystemSettingsLocked(mUserId);

                    ensureLegacyDefaultValueAndSystemSetUpdatedLocked(systemSettings, userId);

                    ensureLegacyDefaultValueAndSystemSetUpdatedLocked(systemSettings, userId);

                    systemSettings.persistSyncLocked();

                    systemSettings.persistSettingsLocked();





                    currentVersion = 146;

                    currentVersion = 146;

                }

                }

packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java

+20 −2
Original line number Original line Diff line number Diff line
@@ -72,6 +72,7 @@ import java.util.List; 
import java.util.Map;

import java.util.Map;

import java.util.Objects;

import java.util.Objects;

import java.util.Set;

import java.util.Set;

import java.util.concurrent.CountDownLatch;





/**

/**

 * This class contains the state for one type of settings. It is responsible

 * This class contains the state for one type of settings. It is responsible
@@ -589,9 +590,10 @@ final class SettingsState { 
    }

    }





    // The settings provider must hold its lock when calling here.

    // The settings provider must hold its lock when calling here.

    public void persistSyncLocked() {

    public void persistSettingsLocked() {

        mHandler.removeMessages(MyHandler.MSG_PERSIST_SETTINGS);

        mHandler.removeMessages(MyHandler.MSG_PERSIST_SETTINGS);

        doWriteState();

        // schedule a write operation right away

        mHandler.obtainMessage(MyHandler.MSG_PERSIST_SETTINGS).sendToTarget();

    }

    }





    // The settings provider must hold its lock when calling here.

    // The settings provider must hold its lock when calling here.
@@ -1725,4 +1727,20 @@ final class SettingsState { 
            return mPackageToMemoryUsage.getOrDefault(packageName, 0);

            return mPackageToMemoryUsage.getOrDefault(packageName, 0);

        }

        }

    }

    }



    /**

     * Allow tests to wait for the handler to finish handling all the remaining messages

     */

    @VisibleForTesting

    public void waitForHandler() {

        final CountDownLatch latch = new CountDownLatch(1);

        synchronized (mLock) {

            mHandler.post(latch::countDown);

        }

        try {

            latch.await();

        } catch (InterruptedException e) {

            // ignored

        }

    }

}
 
}

packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java

+33 −16
Original line number Original line Diff line number Diff line
@@ -76,6 +76,14 @@ public class SettingsStateTest extends AndroidTestCase { 
        mSettingsFile.delete();

        mSettingsFile.delete();

    }

    }





    @Override

    protected void tearDown() throws Exception {

        if (mSettingsFile != null) {

            mSettingsFile.delete();

        }

        super.tearDown();

    }



    public void testIsBinary() {

    public void testIsBinary() {

        assertFalse(SettingsState.isBinary(" abc 日本語"));

        assertFalse(SettingsState.isBinary(" abc 日本語"));
@@ -149,11 +157,10 @@ public class SettingsStateTest extends AndroidTestCase { 
     * Make sure settings can be written to a file and also can be read.

     * Make sure settings can be written to a file and also can be read.

     */

     */

    public void testReadWrite() {

    public void testReadWrite() {

        final File file = new File(getContext().getCacheDir(), "setting.xml");

        file.delete();

        final Object lock = new Object();

        final Object lock = new Object();





        final SettingsState ssWriter = new SettingsState(getContext(), lock, file, 1,

        assertFalse(mSettingsFile.exists());

        final SettingsState ssWriter = new SettingsState(getContext(), lock, mSettingsFile, 1,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        ssWriter.setVersionLocked(SettingsState.SETTINGS_VERSION_NEW_ENCODING);

        ssWriter.setVersionLocked(SettingsState.SETTINGS_VERSION_NEW_ENCODING);
@@ -162,11 +169,13 @@ public class SettingsStateTest extends AndroidTestCase { 
        ssWriter.insertSettingLocked("k3", null, null, false, "p2");

        ssWriter.insertSettingLocked("k3", null, null, false, "p2");

        ssWriter.insertSettingLocked("k4", CRAZY_STRING, null, false, "p3");

        ssWriter.insertSettingLocked("k4", CRAZY_STRING, null, false, "p3");

        synchronized (lock) {

        synchronized (lock) {

            ssWriter.persistSyncLocked();

            ssWriter.persistSettingsLocked();

        }

        }



        ssWriter.waitForHandler();

        final SettingsState ssReader = new SettingsState(getContext(), lock, file, 1,

        assertTrue(mSettingsFile.exists());

        final SettingsState ssReader = new SettingsState(getContext(), lock, mSettingsFile, 1,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {

        synchronized (lock) {

            assertEquals("\u0000", ssReader.getSettingLocked("k1").getValue());

            assertEquals("\u0000", ssReader.getSettingLocked("k1").getValue());

            assertEquals("abc", ssReader.getSettingLocked("k2").getValue());

            assertEquals("abc", ssReader.getSettingLocked("k2").getValue());
@@ -179,10 +188,8 @@ public class SettingsStateTest extends AndroidTestCase { 
     * In version 120, value "null" meant {code NULL}.

     * In version 120, value "null" meant {code NULL}.

     */

     */

    public void testUpgrade() throws Exception {

    public void testUpgrade() throws Exception {

        final File file = new File(getContext().getCacheDir(), "setting.xml");

        file.delete();

        final Object lock = new Object();

        final Object lock = new Object();

        final PrintStream os = new PrintStream(new FileOutputStream(file));

        final PrintStream os = new PrintStream(new FileOutputStream(mSettingsFile));

        os.print(

        os.print(

                "<?xml version='1.0' encoding='UTF-8' standalone='yes' ?>" +

                "<?xml version='1.0' encoding='UTF-8' standalone='yes' ?>" +

                        "<settings version=\"120\">" +

                        "<settings version=\"120\">" +
@@ -192,7 +199,7 @@ public class SettingsStateTest extends AndroidTestCase { 
                        "</settings>");

                        "</settings>");

        os.close();

        os.close();





        final SettingsState ss = new SettingsState(getContext(), lock, file, 1,

        final SettingsState ss = new SettingsState(getContext(), lock, mSettingsFile, 1,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        synchronized (lock) {

        synchronized (lock) {

            SettingsState.Setting s;

            SettingsState.Setting s;
@@ -213,7 +220,8 @@ public class SettingsStateTest extends AndroidTestCase { 
    public void testInitializeSetting_preserveFlagNotSet() {

    public void testInitializeSetting_preserveFlagNotSet() {

        SettingsState settingsWriter = getSettingStateObject();

        SettingsState settingsWriter = getSettingStateObject();

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.persistSyncLocked();

        settingsWriter.persistSettingsLocked();

        settingsWriter.waitForHandler();





        SettingsState settingsReader = getSettingStateObject();

        SettingsState settingsReader = getSettingStateObject();

        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());
@@ -223,7 +231,8 @@ public class SettingsStateTest extends AndroidTestCase { 
        SettingsState settingsWriter = getSettingStateObject();

        SettingsState settingsWriter = getSettingStateObject();

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, TEST_PACKAGE);

        settingsWriter.persistSyncLocked();

        settingsWriter.persistSettingsLocked();

        settingsWriter.waitForHandler();





        SettingsState settingsReader = getSettingStateObject();

        SettingsState settingsReader = getSettingStateObject();

        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());
@@ -234,7 +243,8 @@ public class SettingsStateTest extends AndroidTestCase { 
        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, false, TEST_PACKAGE,

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, false, TEST_PACKAGE,

                /* overrideableByRestore */ true);

                /* overrideableByRestore */ true);

        settingsWriter.persistSyncLocked();

        settingsWriter.persistSettingsLocked();

        settingsWriter.waitForHandler();





        SettingsState settingsReader = getSettingStateObject();

        SettingsState settingsReader = getSettingStateObject();

        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());
@@ -250,7 +260,8 @@ public class SettingsStateTest extends AndroidTestCase { 
        // already been set to true.

        // already been set to true.

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, false, TEST_PACKAGE,

        settingsWriter.insertSettingLocked(SETTING_NAME, "2", null, false, false, TEST_PACKAGE,

                /* overrideableByRestore */ true);

                /* overrideableByRestore */ true);

        settingsWriter.persistSyncLocked();

        settingsWriter.persistSettingsLocked();

        settingsWriter.waitForHandler();





        SettingsState settingsReader = getSettingStateObject();

        SettingsState settingsReader = getSettingStateObject();

        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());
@@ -481,8 +492,11 @@ public class SettingsStateTest extends AndroidTestCase { 
            settingsState.insertSettingLocked(

            settingsState.insertSettingLocked(

                    FLAG_NAME_1_STAGED, VALUE1, null, false, TEST_PACKAGE);

                    FLAG_NAME_1_STAGED, VALUE1, null, false, TEST_PACKAGE);

            settingsState.insertSettingLocked(FLAG_NAME_2, VALUE2, null, false, TEST_PACKAGE);

            settingsState.insertSettingLocked(FLAG_NAME_2, VALUE2, null, false, TEST_PACKAGE);

            settingsState.persistSyncLocked();

            settingsState.persistSettingsLocked();

        }

        settingsState.waitForHandler();





        synchronized (lock) {

            assertEquals(VALUE1, settingsState.getSettingLocked(FLAG_NAME_1_STAGED).getValue());

            assertEquals(VALUE1, settingsState.getSettingLocked(FLAG_NAME_1_STAGED).getValue());

            assertEquals(VALUE2, settingsState.getSettingLocked(FLAG_NAME_2).getValue());

            assertEquals(VALUE2, settingsState.getSettingLocked(FLAG_NAME_2).getValue());

        }

        }
@@ -522,7 +536,10 @@ public class SettingsStateTest extends AndroidTestCase { 
        synchronized (lock) {

        synchronized (lock) {

            settingsState.insertSettingLocked(INVALID_STAGED_FLAG_1,

            settingsState.insertSettingLocked(INVALID_STAGED_FLAG_1,

                    VALUE2, null, false, TEST_PACKAGE);

                    VALUE2, null, false, TEST_PACKAGE);

            settingsState.persistSyncLocked();

            settingsState.persistSettingsLocked();

        }

        settingsState.waitForHandler();

        synchronized (lock) {

            assertEquals(VALUE2, settingsState.getSettingLocked(INVALID_STAGED_FLAG_1).getValue());

            assertEquals(VALUE2, settingsState.getSettingLocked(INVALID_STAGED_FLAG_1).getValue());

        }

        }