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

Commit 296b7e9b authored by Christopher Tate's avatar Christopher Tate
Browse files

Make isAppBad() lock-free 

We trade off increased cost of (rare) mutations of the set of "bad" apps
in order to make the core lookup "is this specific app bad?" lock-free.
Locking discipline turned out to be a dominant cost in a very hot code
path within the running system, even uncontended.

Bug: 175339368
Test: tbd
Change-Id: I8803a177d46b01b86897f1e8d0966eb2452849fd
parent d4c2928f

core/java/com/android/internal/app/ProcessMap.java

+3 −1
Original line number Diff line number Diff line
@@ -62,4 +62,6 @@ public class ProcessMap<E> { 
    public void clear() {

        mMap.clear();

    }



    public void putAll(ProcessMap<E> other) { mMap.putAll(other.mMap); }

}

services/core/java/com/android/server/am/AppErrors.java

+93 −79
Original line number Diff line number Diff line
@@ -52,7 +52,6 @@ import android.util.SparseArray; 
import android.util.TimeUtils;

import android.util.proto.ProtoOutputStream;



import com.android.internal.annotations.GuardedBy;

import com.android.internal.app.ProcessMap;

import com.android.internal.logging.MetricsLogger;

import com.android.internal.logging.nano.MetricsProto;
@@ -106,12 +105,18 @@ class AppErrors { 
     * later restarted (hopefully due to some user action).  The value is the

     * time it was added to the list.

     *

     * Access is synchronized on the container object itself, and no other

     * locks may be acquired while holding that one.

     * Read access is UNLOCKED, and must either be based on a single lookup

     * call on the current mBadProcesses instance, or a local copy of that

     * reference must be made and the local copy treated as the source of

     * truth.  Mutations are performed by synchronizing on mBadProcessLock,

     * cloning the existing mBadProcesses instance, performing the mutation,

     * then changing the volatile "live" mBadProcesses reference to point to the

     * mutated version.  These operations are very rare compared to lookups:

     * we intentionally trade additional cost for mutations for eliminating

     * lock operations from the simple lookup cases.

     */

    @GuardedBy("mBadProcesses")

    private final ProcessMap<BadProcessInfo> mBadProcesses = new ProcessMap<>();



    private volatile ProcessMap<BadProcessInfo> mBadProcesses = new ProcessMap<>();

    private final Object mBadProcessLock = new Object();



    AppErrors(Context context, ActivityManagerService service, PackageWatchdog watchdog) {

        context.assertRuntimeOverlayThemable();
@@ -128,14 +133,14 @@ class AppErrors { 
        mProcessCrashTimesPersistent.clear();

        mProcessCrashShowDialogTimes.clear();

        mProcessCrashCounts.clear();

        synchronized (mBadProcesses) {

            mBadProcesses.clear();

        synchronized (mBadProcessLock) {

            mBadProcesses = new ProcessMap<>();

        }

    }



    void dumpDebug(ProtoOutputStream proto, long fieldId, String dumpPackage) {

        synchronized (mBadProcesses) {

            if (mProcessCrashTimes.getMap().isEmpty() && mBadProcesses.getMap().isEmpty()) {

        final ProcessMap<BadProcessInfo> badProcesses = mBadProcesses;

        if (mProcessCrashTimes.getMap().isEmpty() && badProcesses.getMap().isEmpty()) {

            return;

        }
@@ -171,8 +176,8 @@ class AppErrors { 


        }



            if (!mBadProcesses.getMap().isEmpty()) {

                final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = mBadProcesses.getMap();

        if (!badProcesses.getMap().isEmpty()) {

            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = badProcesses.getMap();

            final int processCount = pmap.size();

            for (int ip = 0; ip < processCount; ip++) {

                final long btoken = proto.start(AppErrorsProto.BAD_PROCESSES);
@@ -203,7 +208,6 @@ class AppErrors { 


        proto.end(token);

    }

    }



    boolean dumpLocked(FileDescriptor fd, PrintWriter pw, boolean needSep, String dumpPackage) {

        final long now = SystemClock.uptimeMillis();
@@ -267,9 +271,10 @@ class AppErrors { 
            }

        }



        if (!mBadProcesses.getMap().isEmpty()) {

        final ProcessMap<BadProcessInfo> badProcesses = mBadProcesses;

        if (!badProcesses.getMap().isEmpty()) {

            boolean printed = false;

            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = mBadProcesses.getMap();

            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = badProcesses.getMap();

            final int processCount = pmap.size();

            for (int ip = 0; ip < processCount; ip++) {

                final String pname = pmap.keyAt(ip);
@@ -322,14 +327,25 @@ class AppErrors { 
    }



    boolean isBadProcess(final String processName, final int uid) {

        synchronized (mBadProcesses) {

        // NO LOCKING for the simple lookup

        return mBadProcesses.get(processName, uid) != null;

    }

    }



    void clearBadProcess(final String processName, final int uid) {

        synchronized (mBadProcesses) {

            mBadProcesses.remove(processName, uid);

        synchronized (mBadProcessLock) {

            final ProcessMap<BadProcessInfo> badProcesses = new ProcessMap<>();

            badProcesses.putAll(mBadProcesses);

            badProcesses.remove(processName, uid);

            mBadProcesses = badProcesses;

        }

    }



    void markBadProcess(final String processName, final int uid, BadProcessInfo info) {

        synchronized (mBadProcessLock) {

            final ProcessMap<BadProcessInfo> badProcesses = new ProcessMap<>();

            badProcesses.putAll(mBadProcesses);

            badProcesses.put(processName, uid, info);

            mBadProcesses = badProcesses;

        }

    }
@@ -812,11 +828,9 @@ class AppErrors { 
                        app.processName);

                if (!app.isolated) {

                    // XXX We don't have a way to mark isolated processes

                    // as bad, since they don't have a peristent identity.

                    synchronized (mBadProcesses) {

                        mBadProcesses.put(app.processName, app.uid,

                    // as bad, since they don't have a persistent identity.

                    markBadProcess(app.processName, app.uid,

                            new BadProcessInfo(now, shortMsg, longMsg, stackTrace));

                    }

                    mProcessCrashTimes.remove(app.processName, app.uid);

                    mProcessCrashCounts.remove(app.processName, app.uid);

                }