From 2fd8ce4933524dcd35d9cf94352ee2154a33f153 Mon Sep 17 00:00:00 2001
From: Dianne Hackborn <hackbod@google.com>
Date: Thu, 12 Jul 2018 14:51:54 -0700
Subject: [PATCH] Fix a few issues with procstats associations:

- Use the correct state when comparing the association
  with the process.  (Was causing warning spam as well.)

- Don't wtf if we call startAssociationIfNeeded() no a
  ServiceRecord that doesn't have a process...  that is normal
  when first bringing up a service.

Also add a switch to turn this all off, which for now is
leaving it on.

Test: manual
Bug: 111391911
Bug: 111302807
Bug: 111131998

Change-Id: I2d3f43fd99c7ade24c867ad80a728d898ab0e6c5
---
 .../internal/app/procstats/ProcessState.java  | 62 +++++++++----------
 .../internal/app/procstats/ProcessStats.java  |  7 ++-
 .../server/am/ActivityManagerService.java     |  3 +
 .../android/server/am/ConnectionRecord.java   | 10 +--
 .../server/am/ContentProviderConnection.java  | 10 +--
 .../server/am/ContentProviderRecord.java      | 38 +++++++-----
 .../com/android/server/am/ServiceRecord.java  | 18 +++---
 7 files changed, 81 insertions(+), 67 deletions(-)

diff --git a/core/java/com/android/internal/app/procstats/ProcessState.java b/core/java/com/android/internal/app/procstats/ProcessState.java
index dbf7c93dadd20..ad42288c25bb1 100644
--- a/core/java/com/android/internal/app/procstats/ProcessState.java
+++ b/core/java/com/android/internal/app/procstats/ProcessState.java
@@ -129,7 +129,7 @@ public final class ProcessState {
     private final PssTable mPssTable;
 
     private ProcessState mCommonProcess;
-    private int mCurState = STATE_NOTHING;
+    private int mCurCombinedState = STATE_NOTHING;
     private long mStartTime;
 
     private int mLastPssState = STATE_NOTHING;
@@ -180,7 +180,7 @@ public final class ProcessState {
         mPackage = pkg;
         mUid = uid;
         mVersion = vers;
-        mCurState = commonProcess.mCurState;
+        mCurCombinedState = commonProcess.mCurCombinedState;
         mStartTime = now;
         mDurations = new DurationsTable(commonProcess.mStats.mTableData);
         mPssTable = new PssTable(commonProcess.mStats.mTableData);
@@ -324,7 +324,7 @@ public final class ProcessState {
 
     public boolean isInUse() {
         return mActive || mNumActiveServices > 0 || mNumStartedServices > 0
-                || mCurState != STATE_NOTHING;
+                || mCurCombinedState != STATE_NOTHING;
     }
 
     public boolean isActive() {
@@ -333,7 +333,7 @@ public final class ProcessState {
 
     public boolean hasAnyData() {
         return !(mDurations.getKeyCount() == 0
-                && mCurState == STATE_NOTHING
+                && mCurCombinedState == STATE_NOTHING
                 && mPssTable.getKeyCount() == 0);
     }
 
@@ -355,7 +355,7 @@ public final class ProcessState {
         }
 
         // First update the common process.
-        mCommonProcess.setState(state, now);
+        mCommonProcess.setCombinedState(state, now);
 
         // If the common process is not multi-package, there is nothing else to do.
         if (!mCommonProcess.mMultiPackage) {
@@ -364,29 +364,29 @@ public final class ProcessState {
 
         if (pkgList != null) {
             for (int ip=pkgList.size()-1; ip>=0; ip--) {
-                pullFixedProc(pkgList, ip).setState(state, now);
+                pullFixedProc(pkgList, ip).setCombinedState(state, now);
             }
         }
     }
 
-    public void setState(int state, long now) {
+    public void setCombinedState(int state, long now) {
         ensureNotDead();
-        if (!mDead && (mCurState != state)) {
+        if (!mDead && (mCurCombinedState != state)) {
             //Slog.i(TAG, "Setting state in " + mName + "/" + mPackage + ": " + state);
             commitStateTime(now);
-            mCurState = state;
+            mCurCombinedState = state;
         }
     }
 
-    public int getState() {
-        return mCurState;
+    public int getCombinedState() {
+        return mCurCombinedState;
     }
 
     public void commitStateTime(long now) {
-        if (mCurState != STATE_NOTHING) {
+        if (mCurCombinedState != STATE_NOTHING) {
             long dur = now - mStartTime;
             if (dur > 0) {
-                mDurations.addDuration(mCurState, dur);
+                mDurations.addDuration(mCurCombinedState, dur);
             }
         }
         mStartTime = now;
@@ -434,8 +434,8 @@ public final class ProcessState {
             mCommonProcess.incStartedServices(memFactor, now, serviceName);
         }
         mNumStartedServices++;
-        if (mNumStartedServices == 1 && mCurState == STATE_NOTHING) {
-            setState(STATE_SERVICE_RESTARTING + (memFactor*STATE_COUNT), now);
+        if (mNumStartedServices == 1 && mCurCombinedState == STATE_NOTHING) {
+            setCombinedState(STATE_SERVICE_RESTARTING + (memFactor*STATE_COUNT), now);
         }
     }
 
@@ -450,8 +450,8 @@ public final class ProcessState {
             mCommonProcess.decStartedServices(memFactor, now, serviceName);
         }
         mNumStartedServices--;
-        if (mNumStartedServices == 0 && (mCurState%STATE_COUNT) == STATE_SERVICE_RESTARTING) {
-            setState(STATE_NOTHING, now);
+        if (mNumStartedServices == 0 && (mCurCombinedState %STATE_COUNT) == STATE_SERVICE_RESTARTING) {
+            setCombinedState(STATE_NOTHING, now);
         } else if (mNumStartedServices < 0) {
             Slog.wtfStack(TAG, "Proc started services underrun: pkg="
                     + mPackage + " uid=" + mUid + " name=" + mName);
@@ -485,16 +485,16 @@ public final class ProcessState {
                 break;
         }
         if (!always) {
-            if (mLastPssState == mCurState && SystemClock.uptimeMillis()
+            if (mLastPssState == mCurCombinedState && SystemClock.uptimeMillis()
                     < (mLastPssTime+(30*1000))) {
                 return;
             }
         }
-        mLastPssState = mCurState;
+        mLastPssState = mCurCombinedState;
         mLastPssTime = SystemClock.uptimeMillis();
-        if (mCurState != STATE_NOTHING) {
+        if (mCurCombinedState != STATE_NOTHING) {
             // First update the common process.
-            mCommonProcess.mPssTable.mergeStats(mCurState, 1, pss, pss, pss, uss, uss, uss,
+            mCommonProcess.mPssTable.mergeStats(mCurCombinedState, 1, pss, pss, pss, uss, uss, uss,
                     rss, rss, rss);
 
             // If the common process is not multi-package, there is nothing else to do.
@@ -504,7 +504,7 @@ public final class ProcessState {
 
             if (pkgList != null) {
                 for (int ip=pkgList.size()-1; ip>=0; ip--) {
-                    pullFixedProc(pkgList, ip).mPssTable.mergeStats(mCurState, 1,
+                    pullFixedProc(pkgList, ip).mPssTable.mergeStats(mCurCombinedState, 1,
                             pss, pss, pss, uss, uss, uss, rss, rss, rss);
                 }
             }
@@ -623,7 +623,7 @@ public final class ProcessState {
 
     public long getDuration(int state, long now) {
         long time = mDurations.getValueForId((byte)state);
-        if (mCurState == state) {
+        if (mCurCombinedState == state) {
             time += now - mStartTime;
         }
         return time;
@@ -728,7 +728,7 @@ public final class ProcessState {
             final int key = mDurations.getKeyAt(i);
             final int type = SparseMappingTable.getIdFromKey(key);
             long time = mDurations.getValue(key);
-            if (mCurState == type) {
+            if (mCurCombinedState == type) {
                 time += now - mStartTime;
             }
             final int procState = type % STATE_COUNT;
@@ -831,7 +831,7 @@ public final class ProcessState {
                     final int bucket = ((iscreen + imem) * STATE_COUNT) + procStates[ip];
                     long time = mDurations.getValueForId((byte)bucket);
                     String running = "";
-                    if (mCurState == bucket) {
+                    if (mCurCombinedState == bucket) {
                         running = " (running)";
                     }
                     if (time != 0) {
@@ -1181,14 +1181,14 @@ public final class ProcessState {
             final int key = mDurations.getKeyAt(i);
             final int type = SparseMappingTable.getIdFromKey(key);
             long time = mDurations.getValue(key);
-            if (mCurState == type) {
+            if (mCurCombinedState == type) {
                 didCurState = true;
                 time += now - mStartTime;
             }
             DumpUtils.printProcStateTagAndValue(pw, type, time);
         }
-        if (!didCurState && mCurState != STATE_NOTHING) {
-            DumpUtils.printProcStateTagAndValue(pw, mCurState, now - mStartTime);
+        if (!didCurState && mCurCombinedState != STATE_NOTHING) {
+            DumpUtils.printProcStateTagAndValue(pw, mCurCombinedState, now - mStartTime);
         }
     }
 
@@ -1255,14 +1255,14 @@ public final class ProcessState {
             final int key = mDurations.getKeyAt(i);
             final int type = SparseMappingTable.getIdFromKey(key);
             long time = mDurations.getValue(key);
-            if (mCurState == type) {
+            if (mCurCombinedState == type) {
                 didCurState = true;
                 time += now - mStartTime;
             }
             durationByState.put(type, time);
         }
-        if (!didCurState && mCurState != STATE_NOTHING) {
-            durationByState.put(mCurState, now - mStartTime);
+        if (!didCurState && mCurCombinedState != STATE_NOTHING) {
+            durationByState.put(mCurCombinedState, now - mStartTime);
         }
 
         for (int i=0; i<mPssTable.getKeyCount(); i++) {
diff --git a/core/java/com/android/internal/app/procstats/ProcessStats.java b/core/java/com/android/internal/app/procstats/ProcessStats.java
index 3cafa5e5ec205..15f140e2ea2d4 100644
--- a/core/java/com/android/internal/app/procstats/ProcessStats.java
+++ b/core/java/com/android/internal/app/procstats/ProcessStats.java
@@ -1387,14 +1387,15 @@ public final class ProcessStats implements Parcelable {
             } else {
                 final ProcessState proc = act.getAssociationState().getProcess();
                 if (proc != null) {
-                    if (act.mProcState == proc.getState()) {
+                    final int procState = proc.getCombinedState() % STATE_COUNT;
+                    if (act.mProcState == procState) {
                         act.startActive(now);
                     } else {
                         act.stopActive(now);
-                        if (act.mProcState < proc.getState()) {
+                        if (act.mProcState < procState) {
                             Slog.w(TAG, "Tracking association " + act + " whose proc state "
                                     + act.mProcState + " is better than process " + proc
-                                    + " proc state " + proc.getState());
+                                    + " proc state " + procState);
                         }
                     }
                 } else {
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index a230609825477..d8de7fc3c91d9 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -520,6 +520,9 @@ public class ActivityManagerService extends IActivityManager.Stub
     // Used to indicate that an app transition should be animated.
     static final boolean ANIMATE = true;
 
+    // If set, we will push process association information in to procstats.
+    static final boolean TRACK_PROCSTATS_ASSOCIATIONS = true;
+
     /**
      * Default value for {@link Settings.Global#NETWORK_ACCESS_TIMEOUT_MS}.
      */
diff --git a/services/core/java/com/android/server/am/ConnectionRecord.java b/services/core/java/com/android/server/am/ConnectionRecord.java
index 2cea4faba613f..fa8e6c4317701 100644
--- a/services/core/java/com/android/server/am/ConnectionRecord.java
+++ b/services/core/java/com/android/server/am/ConnectionRecord.java
@@ -108,10 +108,12 @@ final class ConnectionRecord {
     public void startAssociationIfNeeded() {
         // If we don't already have an active association, create one...  but only if this
         // is an association between two different processes.
-        if (association == null && (binding.service.appInfo.uid != clientUid
-                || !binding.service.processName.equals(clientProcessName))) {
-            ProcessStats.ProcessStateHolder holder = binding.service.app != null
-                    ? binding.service.app.pkgList.get(binding.service.name.getPackageName()) : null;
+        if (ActivityManagerService.TRACK_PROCSTATS_ASSOCIATIONS
+                && association == null && binding.service.app != null
+                && (binding.service.appInfo.uid != clientUid
+                        || !binding.service.processName.equals(clientProcessName))) {
+            ProcessStats.ProcessStateHolder holder = binding.service.app.pkgList.get(
+                    binding.service.name.getPackageName());
             if (holder == null) {
                 Slog.wtf(TAG_AM, "No package in referenced service "
                         + binding.service.name.toShortString() + ": proc=" + binding.service.app);
diff --git a/services/core/java/com/android/server/am/ContentProviderConnection.java b/services/core/java/com/android/server/am/ContentProviderConnection.java
index 36d3f4f619dec..f2d4f739c8bfb 100644
--- a/services/core/java/com/android/server/am/ContentProviderConnection.java
+++ b/services/core/java/com/android/server/am/ContentProviderConnection.java
@@ -55,10 +55,12 @@ public final class ContentProviderConnection extends Binder {
     public void startAssociationIfNeeded() {
         // If we don't already have an active association, create one...  but only if this
         // is an association between two different processes.
-        if (association == null && (provider.appInfo.uid != client.uid
-                || !provider.info.processName.equals(client.processName))) {
-            ProcessStats.ProcessStateHolder holder = provider.proc != null
-                    ? provider.proc.pkgList.get(provider.name.getPackageName()) : null;
+        if (ActivityManagerService.TRACK_PROCSTATS_ASSOCIATIONS
+                && association == null && provider.proc != null
+                && (provider.appInfo.uid != client.uid
+                        || !provider.info.processName.equals(client.processName))) {
+            ProcessStats.ProcessStateHolder holder = provider.proc.pkgList.get(
+                    provider.name.getPackageName());
             if (holder == null) {
                 Slog.wtf(TAG_AM, "No package in referenced provider "
                         + provider.name.toShortString() + ": proc=" + provider.proc);
diff --git a/services/core/java/com/android/server/am/ContentProviderRecord.java b/services/core/java/com/android/server/am/ContentProviderRecord.java
index 53783bebe7f19..c22966d631f6e 100644
--- a/services/core/java/com/android/server/am/ContentProviderRecord.java
+++ b/services/core/java/com/android/server/am/ContentProviderRecord.java
@@ -90,21 +90,23 @@ final class ContentProviderRecord implements ComponentName.WithComponentName {
 
     public void setProcess(ProcessRecord proc) {
         this.proc = proc;
-        for (int iconn = connections.size() - 1; iconn >= 0; iconn--) {
-            final ContentProviderConnection conn = connections.get(iconn);
-            if (proc != null) {
-                conn.startAssociationIfNeeded();
-            } else {
-                conn.stopAssociation();
-            }
-        }
-        if (externalProcessTokenToHandle != null) {
-            for (int iext = externalProcessTokenToHandle.size() - 1; iext >= 0; iext--) {
-                final ExternalProcessHandle handle = externalProcessTokenToHandle.valueAt(iext);
+        if (ActivityManagerService.TRACK_PROCSTATS_ASSOCIATIONS) {
+            for (int iconn = connections.size() - 1; iconn >= 0; iconn--) {
+                final ContentProviderConnection conn = connections.get(iconn);
                 if (proc != null) {
-                    handle.startAssociationIfNeeded(this);
+                    conn.startAssociationIfNeeded();
                 } else {
-                    handle.stopAssociation();
+                    conn.stopAssociation();
+                }
+            }
+            if (externalProcessTokenToHandle != null) {
+                for (int iext = externalProcessTokenToHandle.size() - 1; iext >= 0; iext--) {
+                    final ExternalProcessHandle handle = externalProcessTokenToHandle.valueAt(iext);
+                    if (proc != null) {
+                        handle.startAssociationIfNeeded(this);
+                    } else {
+                        handle.stopAssociation();
+                    }
                 }
             }
         }
@@ -287,10 +289,12 @@ final class ContentProviderRecord implements ComponentName.WithComponentName {
         public void startAssociationIfNeeded(ContentProviderRecord provider) {
             // If we don't already have an active association, create one...  but only if this
             // is an association between two different processes.
-            if (mAssociation == null && (provider.appInfo.uid != mOwningUid
-                    || !provider.info.processName.equals(mOwningProcessName))) {
-                ProcessStats.ProcessStateHolder holder = provider.proc != null
-                        ? provider.proc.pkgList.get(provider.name.getPackageName()) : null;
+            if (ActivityManagerService.TRACK_PROCSTATS_ASSOCIATIONS
+                    && mAssociation == null && provider.proc != null
+                    && (provider.appInfo.uid != mOwningUid
+                            || !provider.info.processName.equals(mOwningProcessName))) {
+                ProcessStats.ProcessStateHolder holder = provider.proc.pkgList.get(
+                        provider.name.getPackageName());
                 if (holder == null) {
                     Slog.wtf(TAG_AM, "No package in referenced provider "
                             + provider.name.toShortString() + ": proc=" + provider.proc);
diff --git a/services/core/java/com/android/server/am/ServiceRecord.java b/services/core/java/com/android/server/am/ServiceRecord.java
index f72b8010429b7..8f436206f4802 100644
--- a/services/core/java/com/android/server/am/ServiceRecord.java
+++ b/services/core/java/com/android/server/am/ServiceRecord.java
@@ -502,14 +502,16 @@ final class ServiceRecord extends Binder implements ComponentName.WithComponentN
 
     public void setProcess(ProcessRecord _proc) {
         app = _proc;
-        for (int conni=connections.size()-1; conni>=0; conni--) {
-            ArrayList<ConnectionRecord> cr = connections.valueAt(conni);
-            for (int i=0; i<cr.size(); i++) {
-                final ConnectionRecord conn = cr.get(i);
-                if (_proc != null) {
-                    conn.startAssociationIfNeeded();
-                } else {
-                    conn.stopAssociation();
+        if (ActivityManagerService.TRACK_PROCSTATS_ASSOCIATIONS) {
+            for (int conni = connections.size() - 1; conni >= 0; conni--) {
+                ArrayList<ConnectionRecord> cr = connections.valueAt(conni);
+                for (int i = 0; i < cr.size(); i++) {
+                    final ConnectionRecord conn = cr.get(i);
+                    if (_proc != null) {
+                        conn.startAssociationIfNeeded();
+                    } else {
+                        conn.stopAssociation();
+                    }
                 }
             }
         }