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

Commit 1784919f authored by Xiang Wang's avatar Xiang Wang
Browse files

Stop global locking on session calls and UID state cache 

Instead we lock on each session for its own operations, and check
session internal state instead of the global UID cache so they
don't block on each other for reports. Only the UidObserver will
interrupt and iterate all sessions for updates, but only lock one
session at a time.

Bug: 304839503
Test: atest HintManagerServiceTest
Change-Id: If18af5440f1d36a96854fe48f1bf280dde403a83
parent 87256704

services/core/java/com/android/server/power/hint/HintManagerService.java

+42 −32
Original line number Diff line number Diff line
@@ -34,7 +34,7 @@ import android.os.RemoteException; 
import android.os.SystemProperties;

import android.util.ArrayMap;

import android.util.ArraySet;

import android.util.SparseArray;

import android.util.SparseIntArray;

import android.util.StatsEvent;



import com.android.internal.annotations.GuardedBy;
@@ -256,10 +256,11 @@ public final class HintManagerService extends SystemService { 


    @VisibleForTesting

    final class MyUidObserver extends UidObserver {

        private final SparseArray<Integer> mProcStatesCache = new SparseArray<>();



        private final Object mCacheLock = new Object();

        @GuardedBy("mCacheLock")

        private final SparseIntArray mProcStatesCache = new SparseIntArray();

        public boolean isUidForeground(int uid) {

            synchronized (mLock) {

            synchronized (mCacheLock) {

                return mProcStatesCache.get(uid, ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND)

                        <= ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND;

            }
@@ -268,6 +269,9 @@ public final class HintManagerService extends SystemService { 
        @Override

        public void onUidGone(int uid, boolean disabled) {

            FgThread.getHandler().post(() -> {

                synchronized (mCacheLock) {

                    mProcStatesCache.delete(uid);

                }

                synchronized (mLock) {

                    ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(uid);

                    if (tokenMap == null) {
@@ -280,7 +284,6 @@ public final class HintManagerService extends SystemService { 
                            sessionSet.valueAt(j).close();

                        }

                    }

                    mProcStatesCache.delete(uid);

                }

            });

        }
@@ -292,15 +295,18 @@ public final class HintManagerService extends SystemService { 
        @Override

        public void onUidStateChanged(int uid, int procState, long procStateSeq, int capability) {

            FgThread.getHandler().post(() -> {

                synchronized (mLock) {

                synchronized (mCacheLock) {

                    mProcStatesCache.put(uid, procState);

                }

                boolean shouldAllowUpdate = isUidForeground(uid);

                synchronized (mLock) {

                    ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(uid);

                    if (tokenMap == null) {

                        return;

                    }

                    for (ArraySet<AppHintSession> sessionSet : tokenMap.values()) {

                        for (AppHintSession s : sessionSet) {

                            s.onProcStateChanged();

                            s.onProcStateChanged(shouldAllowUpdate);

                        }

                    }

                }
@@ -429,10 +435,10 @@ public final class HintManagerService extends SystemService { 
            if (!DumpUtils.checkDumpPermission(getContext(), TAG, pw)) {

                return;

            }

            synchronized (mLock) {

            pw.println("HintSessionPreferredRate: " + mHintSessionPreferredRate);

            pw.println("HAL Support: " + isHalSupported());

            pw.println("Active Sessions:");

            synchronized (mLock) {

                for (int i = 0; i < mActiveSessions.size(); i++) {

                    pw.println("Uid " + mActiveSessions.keyAt(i).toString() + ":");

                    ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap =
@@ -476,7 +482,8 @@ public final class HintManagerService extends SystemService { 
            mHalSessionPtr = halSessionPtr;

            mTargetDurationNanos = durationNanos;

            mUpdateAllowed = true;

            updateHintAllowed();

            final boolean allowed = mUidObserver.isUidForeground(mUid);

            updateHintAllowed(allowed);

            try {

                token.linkToDeath(this, 0);

            } catch (RemoteException e) {
@@ -486,9 +493,8 @@ public final class HintManagerService extends SystemService { 
        }



        @VisibleForTesting

        boolean updateHintAllowed() {

            synchronized (mLock) {

                final boolean allowed = mUidObserver.isUidForeground(mUid);

        boolean updateHintAllowed(boolean allowed) {

            synchronized (this) {

                if (allowed && !mUpdateAllowed) resume();

                if (!allowed && mUpdateAllowed) pause();

                mUpdateAllowed = allowed;
@@ -498,8 +504,8 @@ public final class HintManagerService extends SystemService { 


        @Override

        public void updateTargetWorkDuration(long targetDurationNanos) {

            synchronized (mLock) {

                if (mHalSessionPtr == 0 || !updateHintAllowed()) {

            synchronized (this) {

                if (mHalSessionPtr == 0 || !mUpdateAllowed) {

                    return;

                }

                Preconditions.checkArgument(targetDurationNanos > 0, "Expected"
@@ -511,8 +517,8 @@ public final class HintManagerService extends SystemService { 


        @Override

        public void reportActualWorkDuration(long[] actualDurationNanos, long[] timeStampNanos) {

            synchronized (mLock) {

                if (mHalSessionPtr == 0 || !updateHintAllowed()) {

            synchronized (this) {

                if (mHalSessionPtr == 0 || !mUpdateAllowed) {

                    return;

                }

                Preconditions.checkArgument(actualDurationNanos.length != 0, "the count"
@@ -534,11 +540,13 @@ public final class HintManagerService extends SystemService { 
        /** TODO: consider monitor session threads and close session if any thread is dead. */

        @Override

        public void close() {

            synchronized (mLock) {

            synchronized (this) {

                if (mHalSessionPtr == 0) return;

                mNativeWrapper.halCloseHintSession(mHalSessionPtr);

                mHalSessionPtr = 0;

                mToken.unlinkToDeath(this, 0);

            }

            synchronized (mLock) {

                ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(mUid);

                if (tokenMap == null) {

                    Slogf.w(TAG, "UID %d is not present in active session map", mUid);
@@ -557,8 +565,8 @@ public final class HintManagerService extends SystemService { 


        @Override

        public void sendHint(@PerformanceHintManager.Session.Hint int hint) {

            synchronized (mLock) {

                if (mHalSessionPtr == 0 || !updateHintAllowed()) {

            synchronized (this) {

                if (mHalSessionPtr == 0 || !mUpdateAllowed) {

                    return;

                }

                Preconditions.checkArgument(hint >= 0, "the hint ID value should be"
@@ -568,7 +576,7 @@ public final class HintManagerService extends SystemService { 
        }



        public void setThreads(@NonNull int[] tids) {

            synchronized (mLock) {

            synchronized (this) {

                if (mHalSessionPtr == 0) {

                    return;

                }
@@ -588,7 +596,7 @@ public final class HintManagerService extends SystemService { 
                } finally {

                    Binder.restoreCallingIdentity(identity);

                }

                if (!updateHintAllowed()) {

                if (!mUpdateAllowed) {

                    Slogf.v(TAG, "update hint not allowed, storing tids.");

                    mNewThreadIds = tids;

                    return;
@@ -599,13 +607,15 @@ public final class HintManagerService extends SystemService { 
        }



        public int[] getThreadIds() {

            return mThreadIds;

            synchronized (this) {

                return Arrays.copyOf(mThreadIds, mThreadIds.length);

            }

        }



        @Override

        public void setMode(int mode, boolean enabled) {

            synchronized (mLock) {

                if (mHalSessionPtr == 0 || !updateHintAllowed()) {

            synchronized (this) {

                if (mHalSessionPtr == 0 || !mUpdateAllowed) {

                    return;

                }

                Preconditions.checkArgument(mode >= 0, "the mode Id value should be"
@@ -614,19 +624,19 @@ public final class HintManagerService extends SystemService { 
            }

        }



        private void onProcStateChanged() {

            updateHintAllowed();

        private void onProcStateChanged(boolean updateAllowed) {

            updateHintAllowed(updateAllowed);

        }



        private void pause() {

            synchronized (mLock) {

            synchronized (this) {

                if (mHalSessionPtr == 0) return;

                mNativeWrapper.halPauseHintSession(mHalSessionPtr);

            }

        }



        private void resume() {

            synchronized (mLock) {

            synchronized (this) {

                if (mHalSessionPtr == 0) return;

                mNativeWrapper.halResumeHintSession(mHalSessionPtr);

                if (mNewThreadIds != null) {
@@ -638,12 +648,12 @@ public final class HintManagerService extends SystemService { 
        }



        private void dump(PrintWriter pw, String prefix) {

            synchronized (mLock) {

            synchronized (this) {

                pw.println(prefix + "SessionPID: " + mPid);

                pw.println(prefix + "SessionUID: " + mUid);

                pw.println(prefix + "SessionTIDs: " + Arrays.toString(mThreadIds));

                pw.println(prefix + "SessionTargetDurationNanos: " + mTargetDurationNanos);

                pw.println(prefix + "SessionAllowed: " + updateHintAllowed());

                pw.println(prefix + "SessionAllowed: " + mUpdateAllowed);

            }

        }

services/core/java/com/android/server/power/hint/TEST_MAPPING

0 → 100644
+15 −0
Original line number Diff line number Diff line 
{

  "postsubmit": [

    {

      "name": "FrameworksServicesTests",

      "options": [

        {

          "include-filter": "com.android.server.power.hint"

        },

        {

          "exclude-annotation": "androidx.test.filters.FlakyTest"

        }

      ]

    }

  ]

}
 
 No newline at end of file

services/tests/servicestests/src/com/android/server/power/hint/HintManagerServiceTest.java

+227 −11
Original line number Diff line number Diff line
@@ -25,8 +25,6 @@ import static org.junit.Assert.assertNotEquals; 
import static org.junit.Assert.assertNotNull;

import static org.junit.Assert.assertThrows;

import static org.junit.Assert.assertTrue;

import static org.junit.Assume.assumeFalse;

import static org.junit.Assume.assumeTrue;

import static org.mockito.Mockito.any;

import static org.mockito.Mockito.anyBoolean;

import static org.mockito.Mockito.anyInt;
@@ -46,6 +44,7 @@ import android.os.IBinder; 
import android.os.IHintSession;

import android.os.PerformanceHintManager;

import android.os.Process;

import android.util.Log;



import com.android.server.FgThread;

import com.android.server.LocalServices;
@@ -58,7 +57,14 @@ import org.junit.Test; 
import org.mockito.Mock;

import org.mockito.MockitoAnnotations;



import java.util.ArrayList;

import java.util.Collections;

import java.util.List;

import java.util.concurrent.CountDownLatch;

import java.util.concurrent.TimeUnit;

import java.util.concurrent.atomic.AtomicInteger;

import java.util.concurrent.atomic.AtomicReference;

import java.util.concurrent.locks.LockSupport;



/**

 * Tests for {@link com.android.server.power.hint.HintManagerService}.
@@ -67,8 +73,11 @@ import java.util.concurrent.CountDownLatch; 
 *  atest FrameworksServicesTests:HintManagerServiceTest

 */

public class HintManagerServiceTest {

    private static final String TAG = "HintManagerServiceTest";



    private static final long DEFAULT_HINT_PREFERRED_RATE = 16666666L;

    private static final long DEFAULT_TARGET_DURATION = 16666666L;

    private static final long CONCURRENCY_TEST_DURATION_SEC = 10;

    private static final int UID = Process.myUid();

    private static final int TID = Process.myPid();

    private static final int TGID = Process.getThreadGroupLeader(TID);
@@ -103,6 +112,55 @@ public class HintManagerServiceTest { 
        LocalServices.addService(ActivityManagerInternal.class, mAmInternalMock);

    }



    static class NativeWrapperFake extends NativeWrapper {

        @Override

        public void halInit() {

        }



        @Override

        public long halGetHintSessionPreferredRate() {

            return 1;

        }



        @Override

        public long halCreateHintSession(int tgid, int uid, int[] tids, long durationNanos) {

            return 1;

        }



        @Override

        public void halPauseHintSession(long halPtr) {

        }



        @Override

        public void halResumeHintSession(long halPtr) {

        }



        @Override

        public void halCloseHintSession(long halPtr) {

        }



        @Override

        public void halUpdateTargetWorkDuration(long halPtr, long targetDurationNanos) {

        }



        @Override

        public void halReportActualWorkDuration(

                long halPtr, long[] actualDurationNanos, long[] timeStampNanos) {

        }



        @Override

        public void halSendHint(long halPtr, int hint) {

        }



        @Override

        public void halSetThreads(long halPtr, int[] tids) {

        }



        @Override

        public void halSetMode(long halPtr, int mode, boolean enabled) {

        }

    }



    private HintManagerService createService() {

        mService = new HintManagerService(mContext, new Injector() {

            NativeWrapper createNativeWrapper() {
@@ -112,6 +170,15 @@ public class HintManagerServiceTest { 
        return mService;

    }



    private HintManagerService createServiceWithFakeWrapper() {

        mService = new HintManagerService(mContext, new Injector() {

            NativeWrapper createNativeWrapper() {

                return new NativeWrapperFake();

            }

        });

        return mService;

    }



    @Test

    public void testInitializeService() {

        HintManagerService service = createService();
@@ -166,8 +233,7 @@ public class HintManagerServiceTest { 
            latch.countDown();

        });

        latch.await();



        assumeFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

        verify(mNativeWrapperMock, times(1)).halPauseHintSession(anyLong());



        // Set session to foreground and calling updateHintAllowed() would invoke resume();
@@ -181,7 +247,7 @@ public class HintManagerServiceTest { 
        });

        latch2.await();



        assumeTrue(a.updateHintAllowed());

        assertTrue(service.mUidObserver.isUidForeground(a.mUid));

        verify(mNativeWrapperMock, times(1)).halResumeHintSession(anyLong());

    }
@@ -254,7 +320,7 @@ public class HintManagerServiceTest { 
        });

        latch.await();



        assumeFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

        a.reportActualWorkDuration(DURATIONS_THREE, TIMESTAMPS_THREE);

        verify(mNativeWrapperMock, never()).halReportActualWorkDuration(anyLong(), any(), any());

    }
@@ -280,7 +346,7 @@ public class HintManagerServiceTest { 
        service.mUidObserver.onUidStateChanged(

                a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0);

        FgThread.getHandler().runWithScissors(() -> { }, 500);

        assertFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

        a.sendHint(PerformanceHintManager.Session.CPU_LOAD_RESET);

        verify(mNativeWrapperMock, never()).halSendHint(anyLong(), anyInt());

    }
@@ -303,7 +369,7 @@ public class HintManagerServiceTest { 
        });

        latch.await();



        assertFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

    }



    @Test
@@ -316,7 +382,7 @@ public class HintManagerServiceTest { 


        service.mUidObserver.onUidStateChanged(

                a.mUid, ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND, 0, 0);

        assertTrue(a.updateHintAllowed());

        assertTrue(service.mUidObserver.isUidForeground(a.mUid));

    }



    @Test
@@ -342,7 +408,7 @@ public class HintManagerServiceTest { 
        service.mUidObserver.onUidStateChanged(

                a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0);

        FgThread.getHandler().runWithScissors(() -> { }, 500);

        assertFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

        a.setThreads(SESSION_TIDS_A);

        verify(mNativeWrapperMock, never()).halSetThreads(anyLong(), any());

    }
@@ -372,9 +438,159 @@ public class HintManagerServiceTest { 
        service.mUidObserver.onUidStateChanged(

                a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0);

        FgThread.getHandler().runWithScissors(() -> { }, 500);

        assertFalse(a.updateHintAllowed());

        assertFalse(service.mUidObserver.isUidForeground(a.mUid));

        a.setMode(0, true);

        verify(mNativeWrapperMock, never()).halSetMode(anyLong(), anyInt(), anyBoolean());

    }



    // This test checks that concurrent operations from different threads on IHintService,

    // IHintSession and UidObserver will not cause data race or deadlock. Ideally we should also

    // check the output of threads' reportActualDuration performance to detect lock starvation

    // but the result is not stable, so it's better checked manually.

    @Test

    public void testConcurrency() throws Exception {

        HintManagerService service = createServiceWithFakeWrapper();

        // initialize session threads to run in parallel

        final int sessionCount = 10;

        // the signal that the main thread will send to session threads to check for run or exit

        AtomicReference<Boolean> shouldRun = new AtomicReference<>(true);

        // the signal for main test thread to wait for session threads and notifier thread to

        // finish and exit

        CountDownLatch latch = new CountDownLatch(sessionCount + 1);

        // list of exceptions with one per session thread or notifier thread

        List<AtomicReference<Exception>> execs = new ArrayList<>(sessionCount + 1);

        List<Thread> threads = new ArrayList<>(sessionCount + 1);

        for (int i = 0; i < sessionCount; i++) {

            final AtomicReference<Exception> exec = new AtomicReference<>();

            execs.add(exec);

            int j = i;

            Thread app = new Thread(() -> {

                try {

                    while (shouldRun.get()) {

                        runAppHintSession(service, j, shouldRun);

                    }

                } catch (Exception e) {

                    exec.set(e);

                } finally {

                    latch.countDown();

                }

            });

            threads.add(app);

        }



        // initialize a UID state notifier thread to run in parallel

        final AtomicReference<Exception> notifierExec = new AtomicReference<>();

        execs.add(notifierExec);

        Thread notifier = new Thread(() -> {

            try {

                long min = Long.MAX_VALUE;

                long max = Long.MIN_VALUE;

                long sum = 0;

                int count = 0;

                while (shouldRun.get()) {

                    long start = System.nanoTime();

                    service.mUidObserver.onUidStateChanged(UID,

                            ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND, 0, 0);

                    long elapsed = System.nanoTime() - start;

                    sum += elapsed;

                    count++;

                    min = Math.min(min, elapsed);

                    max = Math.max(max, elapsed);

                    LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(500));

                    service.mUidObserver.onUidStateChanged(UID,

                            ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0);

                    LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(500));

                }

                Log.d(TAG, "notifier thread min " + min + " max " + max + " avg " + sum / count);

                service.mUidObserver.onUidGone(UID, true);

            } catch (Exception e) {

                notifierExec.set(e);

            } finally {

                latch.countDown();

            }

        });

        threads.add(notifier);



        // start all the threads

        for (Thread thread : threads) {

            thread.start();

        }

        // keep the test running for a few seconds

        LockSupport.parkNanos(TimeUnit.SECONDS.toNanos(CONCURRENCY_TEST_DURATION_SEC));

        // send signal to stop all threads

        shouldRun.set(false);

        // wait for all threads to exit

        latch.await();

        // check if any thread throws exception

        for (AtomicReference<Exception> exec : execs) {

            if (exec.get() != null) {

                throw exec.get();

            }

        }

    }



    private void runAppHintSession(HintManagerService service, int logId,

            AtomicReference<Boolean> shouldRun) throws Exception {

        IBinder token = new Binder();

        AppHintSession a = (AppHintSession) service.getBinderServiceInstance()

                .createHintSession(token, SESSION_TIDS_A, DEFAULT_TARGET_DURATION);

        // we will start some threads and get their valid TIDs to update

        int threadCount = 3;

        // the list of TIDs

        int[] tids = new int[threadCount];

        // atomic index for each thread to set its TID in the list

        AtomicInteger k = new AtomicInteger(0);

        // signal for the session main thread to wait for child threads to finish updating TIDs

        CountDownLatch latch = new CountDownLatch(threadCount);

        // signal for the session main thread to notify child threads to exit

        CountDownLatch stopLatch = new CountDownLatch(1);

        for (int j = 0; j < threadCount; j++) {

            Thread thread = new Thread(() -> {

                try {

                    tids[k.getAndIncrement()] = android.os.Process.myTid();

                    latch.countDown();

                    stopLatch.await();

                } catch (InterruptedException e) {

                    throw new RuntimeException(e);

                }

            });

            thread.start();

        }

        latch.await();

        a.setThreads(tids);

        // we don't need the threads to exist after update

        stopLatch.countDown();

        a.updateTargetWorkDuration(5);

        // measure the time it takes in HintManagerService to run reportActualDuration

        long min = Long.MAX_VALUE;

        long max = Long.MIN_VALUE;

        long sum = 0;

        int count = 0;

        List<Long> values = new ArrayList<>();

        long testStart = System.nanoTime();

        // run report actual for 4-second per cycle

        while (shouldRun.get() && System.nanoTime() - testStart < TimeUnit.SECONDS.toNanos(

                Math.min(4, CONCURRENCY_TEST_DURATION_SEC))) {

            long start = System.nanoTime();

            a.reportActualWorkDuration(new long[]{5}, new long[]{start});

            long elapsed = System.nanoTime() - start;

            values.add(elapsed);

            LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(5));

            sum += elapsed;

            count++;

            min = Math.min(min, elapsed);

            max = Math.max(max, elapsed);

        }

        Collections.sort(values);

        if (!values.isEmpty()) {

            Log.d(TAG, "app thread " + logId + " min " + min + " max " + max

                    + " avg " + sum / count + " count " + count

                    + " 80th " + values.get((int) (values.size() * 0.8))

                    + " 90th " + values.get((int) (values.size() * 0.9))

                    + " 95th " + values.get((int) (values.size() * 0.95)));

        } else {

            Log.w(TAG, "No reportActualWorkDuration executed");

        }

        a.close();

    }

}