From acee2d53b47c4f17981d64fe5b738a0f44f83fcb Mon Sep 17 00:00:00 2001 From: Silvia Vinyes Date: Fri, 24 Apr 2020 14:22:14 +0100 Subject: [PATCH] Fix app compaction bug and add tests. Fixing b/154317004 App Compaction has a heuristic that ensures the delta in memory between two full compactions is high enough. The map that keeps track of previous compactions is missing some entries. There is a map that keeps entries with previously compacted processes. When trying to compact again one of those processes, the old entry is removed and a new one inserted. When a compaction was not made because of additional conditions / heuristics, the entry is removed but no new entry is inserted again - as no new compaction occurred. We need to make sure old entries are kept if the current compaction is not successful and if it is successful, the map order should reflect it was the most recent insertion. Test: atest CachedAppOptimizerTest Bug: 154317004 Change-Id: I0d4a364928fee162289098c4d4ea0005781bccfd --- .../android/server/am/CachedAppOptimizer.java | 73 +++-- .../server/am/CachedAppOptimizerTest.java | 249 +++++++++++++++++- 2 files changed, 291 insertions(+), 31 deletions(-) diff --git a/services/core/java/com/android/server/am/CachedAppOptimizer.java b/services/core/java/com/android/server/am/CachedAppOptimizer.java index f9d204fa008e2..43e3a04ad0329 100644 --- a/services/core/java/com/android/server/am/CachedAppOptimizer.java +++ b/services/core/java/com/android/server/am/CachedAppOptimizer.java @@ -45,6 +45,7 @@ import com.android.server.ServiceThread; import java.io.FileOutputStream; import java.io.FileReader; +import java.io.IOException; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; @@ -114,6 +115,14 @@ public final class CachedAppOptimizer { } private PropertyChangedCallbackForTest mTestCallback; + // This interface is for functions related to the Process object that need a different + // implementation in the tests as we are not creating real processes when testing compaction. + @VisibleForTesting + interface ProcessDependencies { + long[] getRss(int pid); + void performCompaction(String action, int pid) throws IOException; + } + // Handler constants. static final int COMPACT_PROCESS_SOME = 1; static final int COMPACT_PROCESS_FULL = 2; @@ -215,13 +224,16 @@ public final class CachedAppOptimizer { @VisibleForTesting final Set mProcStateThrottle; // Handler on which compaction runs. - private Handler mCompactionHandler; + @VisibleForTesting + Handler mCompactionHandler; private Handler mFreezeHandler; // Maps process ID to last compaction statistics for processes that we've fully compacted. Used // when evaluating throttles that we only consider for "full" compaction, so we don't store - // data for "some" compactions. - private Map mLastCompactionStats = + // data for "some" compactions. Uses LinkedHashMap to ensure insertion order is kept and + // facilitate removal of the oldest entry. + @VisibleForTesting + LinkedHashMap mLastCompactionStats = new LinkedHashMap() { @Override protected boolean removeEldestEntry(Map.Entry eldest) { @@ -233,17 +245,20 @@ public final class CachedAppOptimizer { private int mFullCompactionCount; private int mPersistentCompactionCount; private int mBfgsCompactionCount; + private final ProcessDependencies mProcessDependencies; public CachedAppOptimizer(ActivityManagerService am) { - mAm = am; - mCachedAppOptimizerThread = new ServiceThread("CachedAppOptimizerThread", - THREAD_PRIORITY_FOREGROUND, true); - mProcStateThrottle = new HashSet<>(); + this(am, null, new DefaultProcessDependencies()); } @VisibleForTesting - CachedAppOptimizer(ActivityManagerService am, PropertyChangedCallbackForTest callback) { - this(am); + CachedAppOptimizer(ActivityManagerService am, PropertyChangedCallbackForTest callback, + ProcessDependencies processDependencies) { + mAm = am; + mCachedAppOptimizerThread = new ServiceThread("CachedAppOptimizerThread", + THREAD_PRIORITY_FOREGROUND, true); + mProcStateThrottle = new HashSet<>(); + mProcessDependencies = processDependencies; mTestCallback = callback; } @@ -659,7 +674,8 @@ public final class CachedAppOptimizer { } } - private static final class LastCompactionStats { + @VisibleForTesting + static final class LastCompactionStats { private final long[] mRssAfterCompaction; LastCompactionStats(long[] rss) { @@ -712,9 +728,7 @@ public final class CachedAppOptimizer { lastCompactAction = proc.lastCompactAction; lastCompactTime = proc.lastCompactTime; - // remove rather than get so that insertion order will be updated when we - // put the post-compaction stats back into the map. - lastCompactionStats = mLastCompactionStats.remove(pid); + lastCompactionStats = mLastCompactionStats.get(pid); } if (pid == 0) { @@ -806,7 +820,7 @@ public final class CachedAppOptimizer { return; } - long[] rssBefore = Process.getRss(pid); + long[] rssBefore = mProcessDependencies.getRss(pid); long anonRssBefore = rssBefore[2]; if (rssBefore[0] == 0 && rssBefore[1] == 0 && rssBefore[2] == 0 @@ -863,16 +877,13 @@ public final class CachedAppOptimizer { default: break; } - try { Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, "Compact " + ((pendingAction == COMPACT_PROCESS_SOME) ? "some" : "full") + ": " + name); long zramFreeKbBefore = Debug.getZramFreeKb(); - FileOutputStream fos = new FileOutputStream("/proc/" + pid + "/reclaim"); - fos.write(action.getBytes()); - fos.close(); - long[] rssAfter = Process.getRss(pid); + mProcessDependencies.performCompaction(action, pid); + long[] rssAfter = mProcessDependencies.getRss(pid); long end = SystemClock.uptimeMillis(); long time = end - start; long zramFreeKbAfter = Debug.getZramFreeKb(); @@ -882,7 +893,6 @@ public final class CachedAppOptimizer { rssAfter[2] - rssBefore[2], rssAfter[3] - rssBefore[3], time, lastCompactAction, lastCompactTime, lastOomAdj, procState, zramFreeKbBefore, zramFreeKbAfter - zramFreeKbBefore); - // Note that as above not taking mPhenoTypeFlagLock here to avoid locking // on every single compaction for a flag that will seldom change and the // impact of reading the wrong value here is low. @@ -894,14 +904,14 @@ public final class CachedAppOptimizer { lastOomAdj, ActivityManager.processStateAmToProto(procState), zramFreeKbBefore, zramFreeKbAfter); } - synchronized (mAm) { proc.lastCompactTime = end; proc.lastCompactAction = pendingAction; } - if (action.equals(COMPACT_ACTION_FULL) || action.equals(COMPACT_ACTION_ANON)) { + // Remove entry and insert again to update insertion order. + mLastCompactionStats.remove(pid); mLastCompactionStats.put(pid, new LastCompactionStats(rssAfter)); } } catch (Exception e) { @@ -1018,4 +1028,23 @@ public final class CachedAppOptimizer { } } } + + /** + * Default implementation for ProcessDependencies, public vor visibility to OomAdjuster class. + */ + private static final class DefaultProcessDependencies implements ProcessDependencies { + // Get memory RSS from process. + @Override + public long[] getRss(int pid) { + return Process.getRss(pid); + } + + // Compact process. + @Override + public void performCompaction(String action, int pid) throws IOException { + try (FileOutputStream fos = new FileOutputStream("/proc/" + pid + "/reclaim")) { + fos.write(action.getBytes()); + } + } + } } diff --git a/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java b/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java index e5ec1f76c5546..96a44a46bbafb 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java @@ -16,14 +16,23 @@ package com.android.server.am; +import static android.app.ActivityManager.PROCESS_STATE_BOUND_FOREGROUND_SERVICE; + import static com.android.server.am.ActivityManagerService.Injector; import static com.android.server.am.CachedAppOptimizer.compactActionIntToString; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; + +import android.content.ComponentName; import android.content.Context; +import android.content.pm.ApplicationInfo; +import android.content.pm.PackageManagerInternal; import android.os.Handler; import android.os.HandlerThread; +import android.os.MessageQueue; import android.os.Process; import android.platform.test.annotations.Presubmit; import android.provider.DeviceConfig; @@ -31,9 +40,11 @@ import android.text.TextUtils; import androidx.test.platform.app.InstrumentationRegistry; +import com.android.server.LocalServices; import com.android.server.ServiceThread; import com.android.server.appop.AppOpsService; import com.android.server.testables.TestableDeviceConfig; +import com.android.server.wm.ActivityTaskManagerService; import org.junit.After; import org.junit.Assume; @@ -45,6 +56,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import java.io.File; +import java.io.IOException; import java.util.HashSet; import java.util.Set; import java.util.concurrent.CountDownLatch; @@ -68,25 +80,36 @@ public final class CachedAppOptimizerTest { private HandlerThread mHandlerThread; private Handler mHandler; private CountDownLatch mCountDown; + private ActivityManagerService mAms; + private Context mContext; + private TestInjector mInjector; + private TestProcessDependencies mProcessDependencies; + + @Mock + private PackageManagerInternal mPackageManagerInt; @Rule - public TestableDeviceConfig.TestableDeviceConfigRule + public final TestableDeviceConfig.TestableDeviceConfigRule mDeviceConfigRule = new TestableDeviceConfig.TestableDeviceConfigRule(); + @Rule + public final ApplicationExitInfoTest.ServiceThreadRule + mServiceThreadRule = new ApplicationExitInfoTest.ServiceThreadRule(); @Before public void setUp() { mHandlerThread = new HandlerThread(""); mHandlerThread.start(); mHandler = new Handler(mHandlerThread.getLooper()); - mThread = new ServiceThread("TestServiceThread", Process.THREAD_PRIORITY_DEFAULT, true /* allowIo */); mThread.start(); - - ActivityManagerService ams = new ActivityManagerService( - new TestInjector(InstrumentationRegistry.getInstrumentation().getContext()), - mThread); - mCachedAppOptimizerUnderTest = new CachedAppOptimizer(ams, + mContext = InstrumentationRegistry.getInstrumentation().getContext(); + mInjector = new TestInjector(mContext); + mAms = new ActivityManagerService( + new TestInjector(mContext), mServiceThreadRule.getThread()); + doReturn(new ComponentName("", "")).when(mPackageManagerInt).getSystemUiServiceComponent(); + mProcessDependencies = new TestProcessDependencies(); + mCachedAppOptimizerUnderTest = new CachedAppOptimizer(mAms, new CachedAppOptimizer.PropertyChangedCallbackForTest() { @Override public void onPropertyChanged() { @@ -94,7 +117,9 @@ public final class CachedAppOptimizerTest { mCountDown.countDown(); } } - }); + }, mProcessDependencies); + LocalServices.removeServiceForTest(PackageManagerInternal.class); + LocalServices.addService(PackageManagerInternal.class, mPackageManagerInt); } @After @@ -104,6 +129,19 @@ public final class CachedAppOptimizerTest { mCountDown = null; } + private ProcessRecord makeProcessRecord(int pid, int uid, int packageUid, String processName, + String packageName) { + ApplicationInfo ai = new ApplicationInfo(); + ai.packageName = packageName; + ProcessRecord app = new ProcessRecord(mAms, ai, processName, uid); + app.pid = pid; + app.info.uid = packageUid; + // Exact value does not mater, it can be any state for which compaction is allowed. + app.setProcState = PROCESS_STATE_BOUND_FOREGROUND_SERVICE; + app.setAdj = 905; + return app; + } + @Test public void init_setsDefaults() { mCachedAppOptimizerUnderTest.init(); @@ -197,7 +235,7 @@ public final class CachedAppOptimizerTest { CachedAppOptimizer.DEFAULT_USE_FREEZER); DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ACTIVITY_MANAGER, CachedAppOptimizer.KEY_USE_FREEZER, CachedAppOptimizer.DEFAULT_USE_FREEZER - ? "false" : "true" , false); + ? "false" : "true", false); // Then calling init will read and set that flag. mCachedAppOptimizerUnderTest.init(); @@ -790,6 +828,174 @@ public final class CachedAppOptimizerTest { .containsExactlyElementsIn(expected); } + @Test + public void processWithDeltaRSSTooSmall_notFullCompacted() throws Exception { + // Initialize CachedAppOptimizer and set flags to (1) enable compaction, (2) set RSS + // throttle to 12000. + mCachedAppOptimizerUnderTest.init(); + setFlag(CachedAppOptimizer.KEY_USE_COMPACTION, "true", true); + setFlag(CachedAppOptimizer.KEY_COMPACT_FULL_DELTA_RSS_THROTTLE_KB, "12000", false); + initActivityManagerService(); + + // Simulate RSS anon memory larger than throttle. + long[] rssBefore1 = + new long[]{/*totalRSS*/ 10000, /*fileRSS*/ 10000, /*anonRSS*/ 12000, /*swap*/ + 10000}; + long[] rssAfter1 = + new long[]{/*totalRSS*/ 9000, /*fileRSS*/ 9000, /*anonRSS*/ 11000, /*swap*/9000}; + // Delta between rssAfter1 and rssBefore2 is below threshold (500). + long[] rssBefore2 = + new long[]{/*totalRSS*/ 9500, /*fileRSS*/ 9500, /*anonRSS*/ 11500, /*swap*/9500}; + long[] rssAfter2 = + new long[]{/*totalRSS*/ 8000, /*fileRSS*/ 8000, /*anonRSS*/ 9000, /*swap*/8000}; + // Delta between rssAfter1 and rssBefore3 is above threshold (13000). + long[] rssBefore3 = + new long[]{/*totalRSS*/ 10000, /*fileRSS*/ 18000, /*anonRSS*/ 13000, /*swap*/ 7000}; + long[] rssAfter3 = + new long[]{/*totalRSS*/ 10000, /*fileRSS*/ 11000, /*anonRSS*/ 10000, /*swap*/ 6000}; + long[] valuesAfter = {}; + // Process that passes properties. + int pid = 1; + ProcessRecord processRecord = makeProcessRecord(pid, 2, 3, "p1", "app1"); + + // GIVEN we simulate RSS memory before above thresholds and it is the first time 'p1' is + // compacted. + mProcessDependencies.setRss(rssBefore1); + mProcessDependencies.setRssAfterCompaction(rssAfter1); // + // WHEN we try to run compaction + mCachedAppOptimizerUnderTest.compactAppFull(processRecord); + waitForHandler(); + // THEN process IS compacted. + assertThat(mCachedAppOptimizerUnderTest.mLastCompactionStats.get(pid)).isNotNull(); + valuesAfter = mCachedAppOptimizerUnderTest.mLastCompactionStats.get( + pid).getRssAfterCompaction(); + assertThat(valuesAfter).isEqualTo(rssAfter1); + + // WHEN delta is below threshold (500). + mProcessDependencies.setRss(rssBefore2); + mProcessDependencies.setRssAfterCompaction(rssAfter2); + // This is to avoid throttle of compacting too soon. + processRecord.lastCompactTime = processRecord.lastCompactTime - 10_000; + // WHEN we try to run compaction. + mCachedAppOptimizerUnderTest.compactAppFull(processRecord); + waitForHandler(); + // THEN process IS NOT compacted - values after compaction for process 1 should remain the + // same as from the last compaction. + assertThat(mCachedAppOptimizerUnderTest.mLastCompactionStats.get(pid)).isNotNull(); + valuesAfter = mCachedAppOptimizerUnderTest.mLastCompactionStats.get( + pid).getRssAfterCompaction(); + assertThat(valuesAfter).isEqualTo(rssAfter1); + + // WHEN delta is above threshold (13000). + mProcessDependencies.setRss(rssBefore3); + mProcessDependencies.setRssAfterCompaction(rssAfter3); + // This is to avoid throttle of compacting too soon. + processRecord.lastCompactTime = processRecord.lastCompactTime - 10_000; + // WHEN we try to run compaction + mCachedAppOptimizerUnderTest.compactAppFull(processRecord); + waitForHandler(); + // THEN process IS compacted - values after compaction for process 1 should be updated. + assertThat(mCachedAppOptimizerUnderTest.mLastCompactionStats.get(pid)).isNotNull(); + valuesAfter = mCachedAppOptimizerUnderTest.mLastCompactionStats.get( + pid).getRssAfterCompaction(); + assertThat(valuesAfter).isEqualTo(rssAfter3); + + } + + @Test + public void processWithAnonRSSTooSmall_notFullCompacted() throws Exception { + // Initialize CachedAppOptimizer and set flags to (1) enable compaction, (2) set RSS + // throttle to 8000. + mCachedAppOptimizerUnderTest.init(); + setFlag(CachedAppOptimizer.KEY_USE_COMPACTION, "true", true); + setFlag(CachedAppOptimizer.KEY_COMPACT_FULL_RSS_THROTTLE_KB, "8000", false); + initActivityManagerService(); + + // Simulate RSS anon memory larger than throttle. + long[] rssBelowThreshold = + new long[]{/*Total RSS*/ 10000, /*File RSS*/ 10000, /*Anon RSS*/ 7000, /*Swap*/ + 10000}; + long[] rssBelowThresholdAfter = + new long[]{/*Total RSS*/ 9000, /*File RSS*/ 7000, /*Anon RSS*/ 4000, /*Swap*/ + 8000}; + long[] rssAboveThreshold = + new long[]{/*Total RSS*/ 10000, /*File RSS*/ 10000, /*Anon RSS*/ 9000, /*Swap*/ + 10000}; + long[] rssAboveThresholdAfter = + new long[]{/*Total RSS*/ 8000, /*File RSS*/ 9000, /*Anon RSS*/ 6000, /*Swap*/5000}; + // Process that passes properties. + int pid = 1; + ProcessRecord processRecord = + makeProcessRecord(pid, 2, 3, "p1", + "app1"); + + // GIVEN we simulate RSS memory before below threshold. + mProcessDependencies.setRss(rssBelowThreshold); + mProcessDependencies.setRssAfterCompaction(rssBelowThresholdAfter); + // WHEN we try to run compaction + mCachedAppOptimizerUnderTest.compactAppFull(processRecord); + waitForHandler(); + // THEN process IS NOT compacted. + assertThat(mCachedAppOptimizerUnderTest.mLastCompactionStats.get(pid)).isNull(); + + // GIVEN we simulate RSS memory before above threshold. + mProcessDependencies.setRss(rssAboveThreshold); + mProcessDependencies.setRssAfterCompaction(rssAboveThresholdAfter); + // WHEN we try to run compaction + mCachedAppOptimizerUnderTest.compactAppFull(processRecord); + waitForHandler(); + // THEN process IS compacted. + assertThat(mCachedAppOptimizerUnderTest.mLastCompactionStats.get(pid)).isNotNull(); + long[] valuesAfter = mCachedAppOptimizerUnderTest.mLastCompactionStats.get( + pid).getRssAfterCompaction(); + assertThat(valuesAfter).isEqualTo(rssAboveThresholdAfter); + } + + + private void setFlag(String key, String value, boolean defaultValue) throws Exception { + mCountDown = new CountDownLatch(1); + DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ACTIVITY_MANAGER, key, value, defaultValue); + assertThat(mCountDown.await(5, TimeUnit.SECONDS)).isTrue(); + } + + private void waitForHandler() { + Idle idle = new Idle(); + mCachedAppOptimizerUnderTest.mCompactionHandler.getLooper().getQueue().addIdleHandler(idle); + mCachedAppOptimizerUnderTest.mCompactionHandler.post(() -> { }); + idle.waitForIdle(); + } + + private void initActivityManagerService() { + mAms = new ActivityManagerService(mInjector, mServiceThreadRule.getThread()); + mAms.mActivityTaskManager = new ActivityTaskManagerService(mContext); + mAms.mActivityTaskManager.initialize(null, null, mContext.getMainLooper()); + mAms.mAtmInternal = spy(mAms.mActivityTaskManager.getAtmInternal()); + mAms.mPackageManagerInt = mPackageManagerInt; + } + + private static final class Idle implements MessageQueue.IdleHandler { + private boolean mIdle; + + @Override + public boolean queueIdle() { + synchronized (this) { + mIdle = true; + notifyAll(); + } + return false; + } + + public synchronized void waitForIdle() { + while (!mIdle) { + try { + // Wait with a timeout of 10s. + wait(10000); + } catch (InterruptedException e) { + } + } + } + } + private class TestInjector extends Injector { TestInjector(Context context) { @@ -806,4 +1012,29 @@ public final class CachedAppOptimizerTest { return mHandler; } } + + // Test implementation for ProcessDependencies. + private static final class TestProcessDependencies + implements CachedAppOptimizer.ProcessDependencies { + private long[] mRss; + private long[] mRssAfterCompaction; + + @Override + public long[] getRss(int pid) { + return mRss; + } + + @Override + public void performCompaction(String action, int pid) throws IOException { + mRss = mRssAfterCompaction; + } + + public void setRss(long[] newValues) { + mRss = newValues; + } + + public void setRssAfterCompaction(long[] newValues) { + mRssAfterCompaction = newValues; + } + } }