From abffa29b651d0bf75f63843244ea3ae364d3bb24 Mon Sep 17 00:00:00 2001 From: Tim Murray Date: Fri, 15 Jan 2021 13:18:37 -0800 Subject: UsageStatsService: rewrite locking to avoid contention post-unlock Various ActivityManager actions require calls to UsageStatsService.reportEventOrAddToQueue(), but that function can block for hundreds of milliseconds while UsageStatsService is initialized during post-unlock. However, the only important part of reportEventOrAddToQueue is the check to see if a user has been unlocked, at which point the action is enqueued on a Handler. Move mUserUnlockedStates to a CopyOnWriteArraySet, which should be updated very infrequently, and allow the common case of "the user is unlocked and an event should be added to the queue" to run concurrently with other UsageStatsService operations. Test: atest android.app.usage.cts.UsageStatsTest Test: atest android.app.usage.UsageStatsTest Test: atest UsageStatsDatabaseTest Bug: 161866124 Change-Id: I11743d78f20db5e5a9471544ae5a1c0ab492202d --- .../android/server/usage/UsageStatsService.java | 62 +++++++++++----------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/services/usage/java/com/android/server/usage/UsageStatsService.java b/services/usage/java/com/android/server/usage/UsageStatsService.java index 3815326b6886..0cb1255c6830 100644 --- a/services/usage/java/com/android/server/usage/UsageStatsService.java +++ b/services/usage/java/com/android/server/usage/UsageStatsService.java @@ -79,7 +79,6 @@ import android.util.ArraySet; import android.util.AtomicFile; import android.util.Slog; import android.util.SparseArray; -import android.util.SparseBooleanArray; import android.util.SparseIntArray; import com.android.internal.annotations.VisibleForTesting; @@ -111,6 +110,7 @@ import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Set; +import java.util.concurrent.CopyOnWriteArraySet; /** * A service that collects, aggregates, and persists application usage data. @@ -162,7 +162,7 @@ public class UsageStatsService extends SystemService implements ShortcutServiceInternal mShortcutServiceInternal; private final SparseArray mUserState = new SparseArray<>(); - private final SparseBooleanArray mUserUnlockedStates = new SparseBooleanArray(); + private final CopyOnWriteArraySet mUserUnlockedStates = new CopyOnWriteArraySet<>(); private final SparseIntArray mUidToKernelCounter = new SparseIntArray(); int mUsageSource; @@ -333,7 +333,7 @@ public class UsageStatsService extends SystemService implements synchronized (mLock) { // User was started but never unlocked so no need to report a user stopped event - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { persistPendingEventsLocked(userId); return; } @@ -346,7 +346,7 @@ public class UsageStatsService extends SystemService implements if (userService != null) { userService.userStopped(); } - mUserUnlockedStates.put(userId, false); + mUserUnlockedStates.remove(userId); mUserState.put(userId, null); // release the service (mainly for GC) } } @@ -360,6 +360,11 @@ public class UsageStatsService extends SystemService implements UsageStatsIdleService.scheduleUpdateMappingsJob(getContext()); } synchronized (mLock) { + // This should be safe to add this early. Other than reportEventOrAddToQueue, every + // other user grabs the lock before accessing + // mUserUnlockedStates. reportEventOrAddToQueue does not depend on anything other than + // mUserUnlockedStates, and the lock will protect the handler. + mUserUnlockedStates.add(userId); // Create a user unlocked event to report final Event unlockEvent = new Event(USER_UNLOCKED, SystemClock.elapsedRealtime()); unlockEvent.mPackage = Event.DEVICE_EVENT_PACKAGE_NAME; @@ -377,7 +382,6 @@ public class UsageStatsService extends SystemService implements initializeUserUsageStatsServiceLocked(userId, System.currentTimeMillis(), installedPackages); - mUserUnlockedStates.put(userId, true); final UserUsageStatsService userService = getUserUsageStatsServiceLocked(userId); if (userService == null) { Slog.i(TAG, "Attempted to unlock stopped or removed user " + userId); @@ -780,12 +784,11 @@ public class UsageStatsService extends SystemService implements } private void reportEventOrAddToQueue(int userId, Event event) { + if (mUserUnlockedStates.contains(userId)) { + mHandler.obtainMessage(MSG_REPORT_EVENT, userId, 0, event).sendToTarget(); + return; + } synchronized (mLock) { - if (mUserUnlockedStates.get(userId)) { - mHandler.obtainMessage(MSG_REPORT_EVENT, userId, 0, event).sendToTarget(); - return; - } - LinkedList events = mReportedEvents.get(userId); if (events == null) { events = new LinkedList<>(); @@ -823,7 +826,7 @@ public class UsageStatsService extends SystemService implements synchronized (mLock) { // This should never be called directly when the user is locked - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.wtf(TAG, "Failed to report event for locked user " + userId + " (" + event.mPackage + "/" + event.mClass + " eventType:" + event.mEventType @@ -1006,7 +1009,7 @@ public class UsageStatsService extends SystemService implements final int tokenRemoved; synchronized (mLock) { final long timeRemoved = System.currentTimeMillis(); - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { // If user is not unlocked and a package is removed for them, we will handle it // when the user service is initialized and package manager is queried. return; @@ -1030,7 +1033,7 @@ public class UsageStatsService extends SystemService implements */ private boolean pruneUninstalledPackagesData(int userId) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { return false; // user is no longer unlocked } @@ -1050,7 +1053,7 @@ public class UsageStatsService extends SystemService implements // fetch the installed packages outside the lock so it doesn't block package manager. final HashMap installedPkgs = getInstalledPackages(UserHandle.USER_SYSTEM); synchronized (mLock) { - if (!mUserUnlockedStates.get(UserHandle.USER_SYSTEM)) { + if (!mUserUnlockedStates.contains(UserHandle.USER_SYSTEM)) { return false; // user is no longer unlocked } @@ -1069,7 +1072,7 @@ public class UsageStatsService extends SystemService implements List queryUsageStats(int userId, int bucketType, long beginTime, long endTime, boolean obfuscateInstantApps) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.w(TAG, "Failed to query usage stats for locked user " + userId); return null; } @@ -1103,7 +1106,7 @@ public class UsageStatsService extends SystemService implements List queryConfigurationStats(int userId, int bucketType, long beginTime, long endTime) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.w(TAG, "Failed to query configuration stats for locked user " + userId); return null; } @@ -1122,7 +1125,7 @@ public class UsageStatsService extends SystemService implements List queryEventStats(int userId, int bucketType, long beginTime, long endTime) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.w(TAG, "Failed to query event stats for locked user " + userId); return null; } @@ -1140,7 +1143,7 @@ public class UsageStatsService extends SystemService implements */ UsageEvents queryEvents(int userId, long beginTime, long endTime, int flags) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.w(TAG, "Failed to query events for locked user " + userId); return null; } @@ -1159,7 +1162,7 @@ public class UsageStatsService extends SystemService implements UsageEvents queryEventsForPackage(int userId, long beginTime, long endTime, String packageName, boolean includeTaskRoot) { synchronized (mLock) { - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { Slog.w(TAG, "Failed to query package events for locked user " + userId); return null; } @@ -1203,7 +1206,7 @@ public class UsageStatsService extends SystemService implements final int userCount = mUserState.size(); for (int i = 0; i < userCount; i++) { final int userId = mUserState.keyAt(i); - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { persistPendingEventsLocked(userId); continue; } @@ -1261,7 +1264,7 @@ public class UsageStatsService extends SystemService implements final int numUsers = mUserState.size(); for (int user = 0; user < numUsers; user++) { final int userId = mUserState.keyAt(user); - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { continue; } ipw.println("user=" + userId); @@ -1288,7 +1291,7 @@ public class UsageStatsService extends SystemService implements final int numUsers = mUserState.size(); for (int user = 0; user < numUsers; user++) { final int userId = mUserState.keyAt(user); - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { continue; } ipw.println("user=" + userId); @@ -1344,7 +1347,7 @@ public class UsageStatsService extends SystemService implements idpw.printPair("user", userId); idpw.println(); idpw.increaseIndent(); - if (mUserUnlockedStates.get(userId)) { + if (mUserUnlockedStates.contains(userId)) { if (checkin) { mUserState.valueAt(i).checkin(idpw); } else { @@ -1382,7 +1385,7 @@ public class UsageStatsService extends SystemService implements ipw.println("the specified user does not exist."); return UserHandle.USER_NULL; } - if (!mUserUnlockedStates.get(userId)) { + if (!mUserUnlockedStates.contains(userId)) { ipw.println("the specified user is currently in a locked state."); return UserHandle.USER_NULL; } @@ -2250,12 +2253,11 @@ public class UsageStatsService extends SystemService implements @Override public byte[] getBackupPayload(int user, String key) { + if (!mUserUnlockedStates.contains(user)) { + Slog.w(TAG, "Failed to get backup payload for locked user " + user); + return null; + } synchronized (mLock) { - if (!mUserUnlockedStates.get(user)) { - Slog.w(TAG, "Failed to get backup payload for locked user " + user); - return null; - } - // Check to ensure that only user 0's data is b/r for now // Note: if backup and restore is enabled for users other than the system user, the // #onUserUnlocked logic, specifically when the update mappings job is scheduled via @@ -2275,7 +2277,7 @@ public class UsageStatsService extends SystemService implements @Override public void applyRestoredPayload(int user, String key, byte[] payload) { synchronized (mLock) { - if (!mUserUnlockedStates.get(user)) { + if (!mUserUnlockedStates.contains(user)) { Slog.w(TAG, "Failed to apply restored payload for locked user " + user); return; } -- cgit v1.2.3-59-g8ed1b From 89125443c97e955353f66761cd828f14ce20296c Mon Sep 17 00:00:00 2001 From: Tim Murray Date: Wed, 20 Jan 2021 08:40:17 -0800 Subject: NotificationManagerService: check cache before checking contacts In case a contact has been added to the cache before the future runs its work, check the cache before querying the contact. Test: atest com.android.server.notification.ValidateNotificationPeopleTest Bug: 161866124 Change-Id: I9c59a934952e4dc3cce1f99e905603cd357b9dbc --- .../notification/ValidateNotificationPeople.java | 47 +++++++++++++--------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/services/core/java/com/android/server/notification/ValidateNotificationPeople.java b/services/core/java/com/android/server/notification/ValidateNotificationPeople.java index f59934fd6810..d7bc3bb8af28 100644 --- a/services/core/java/com/android/server/notification/ValidateNotificationPeople.java +++ b/services/core/java/com/android/server/notification/ValidateNotificationPeople.java @@ -523,27 +523,39 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor { if (VERBOSE) Slog.i(TAG, "Executing: validation for: " + mKey); long timeStartMs = System.currentTimeMillis(); for (final String handle: mPendingLookups) { + final String cacheKey = getCacheKey(mContext.getUserId(), handle); LookupResult lookupResult = null; - final Uri uri = Uri.parse(handle); - if ("tel".equals(uri.getScheme())) { - if (DEBUG) Slog.d(TAG, "checking telephone URI: " + handle); - lookupResult = resolvePhoneContact(mContext, uri.getSchemeSpecificPart()); - } else if ("mailto".equals(uri.getScheme())) { - if (DEBUG) Slog.d(TAG, "checking mailto URI: " + handle); - lookupResult = resolveEmailContact(mContext, uri.getSchemeSpecificPart()); - } else if (handle.startsWith(Contacts.CONTENT_LOOKUP_URI.toString())) { - if (DEBUG) Slog.d(TAG, "checking lookup URI: " + handle); - lookupResult = searchContacts(mContext, uri); - } else { - lookupResult = new LookupResult(); // invalid person for the cache - if (!"name".equals(uri.getScheme())) { - Slog.w(TAG, "unsupported URI " + handle); + boolean cacheHit = false; + synchronized (mPeopleCache) { + lookupResult = mPeopleCache.get(cacheKey); + if (lookupResult != null && !lookupResult.isExpired()) { + // The name wasn't already added to the cache, no need to retry + cacheHit = true; + } + } + if (!cacheHit) { + final Uri uri = Uri.parse(handle); + if ("tel".equals(uri.getScheme())) { + if (DEBUG) Slog.d(TAG, "checking telephone URI: " + handle); + lookupResult = resolvePhoneContact(mContext, uri.getSchemeSpecificPart()); + } else if ("mailto".equals(uri.getScheme())) { + if (DEBUG) Slog.d(TAG, "checking mailto URI: " + handle); + lookupResult = resolveEmailContact(mContext, uri.getSchemeSpecificPart()); + } else if (handle.startsWith(Contacts.CONTENT_LOOKUP_URI.toString())) { + if (DEBUG) Slog.d(TAG, "checking lookup URI: " + handle); + lookupResult = searchContacts(mContext, uri); + } else { + lookupResult = new LookupResult(); // invalid person for the cache + if (!"name".equals(uri.getScheme())) { + Slog.w(TAG, "unsupported URI " + handle); + } } } if (lookupResult != null) { - synchronized (mPeopleCache) { - final String cacheKey = getCacheKey(mContext.getUserId(), handle); - mPeopleCache.put(cacheKey, lookupResult); + if (!cacheHit) { + synchronized (mPeopleCache) { + mPeopleCache.put(cacheKey, lookupResult); + } } if (DEBUG) { Slog.d(TAG, "lookup contactAffinity is " + lookupResult.getAffinity()); @@ -580,4 +592,3 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor { } } } - -- cgit v1.2.3-59-g8ed1b