From 3aed7fe724bf01a9ac5ba24cbc6e9b992551070f Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Fri, 17 Dec 2021 05:10:19 +0000 Subject: [PATCH] Revert "Reduce allocations on location delivery" This reverts commit 232a0456e7353eb38af89646181ff48e531774bf. Reason for revert: Possible root cause for b/210934408 Change-Id: I9d36516d62cbe57ecda1ff2a53b972ec9206351b --- .../provider/LocationProviderManager.java | 63 ++++--------------- 1 file changed, 11 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/location/provider/LocationProviderManager.java b/services/core/java/com/android/server/location/provider/LocationProviderManager.java index 737b653318d2b..155b61891d129 100644 --- a/services/core/java/com/android/server/location/provider/LocationProviderManager.java +++ b/services/core/java/com/android/server/location/provider/LocationProviderManager.java @@ -177,7 +177,7 @@ public class LocationProviderManager extends protected interface LocationTransport { void deliverOnLocationChanged(LocationResult locationResult, - @Nullable IRemoteCallback onCompleteCallback) throws Exception; + @Nullable Runnable onCompleteCallback) throws Exception; void deliverOnFlushComplete(int requestCode) throws Exception; } @@ -197,8 +197,9 @@ public class LocationProviderManager extends @Override public void deliverOnLocationChanged(LocationResult locationResult, - @Nullable IRemoteCallback onCompleteCallback) throws RemoteException { - mListener.onLocationChanged(locationResult.asList(), onCompleteCallback); + @Nullable Runnable onCompleteCallback) throws RemoteException { + mListener.onLocationChanged(locationResult.asList(), + SingleUseCallback.wrap(onCompleteCallback)); } @Override @@ -226,7 +227,7 @@ public class LocationProviderManager extends @Override public void deliverOnLocationChanged(LocationResult locationResult, - @Nullable IRemoteCallback onCompleteCallback) + @Nullable Runnable onCompleteCallback) throws PendingIntent.CanceledException { BroadcastOptions options = BroadcastOptions.makeBasic(); options.setDontSendToRestrictedApps(true); @@ -242,34 +243,20 @@ public class LocationProviderManager extends intent.putExtra(KEY_LOCATIONS, locationResult.asList().toArray(new Location[0])); } - PendingIntent.OnFinished onFinished = null; - // send() SHOULD only run the completion callback if it completes successfully. however, - // b/201299281 (which could not be fixed in the S timeframe) means that it's possible + // b/199464864 (which could not be fixed in the S timeframe) means that it's possible // for send() to throw an exception AND run the completion callback. if this happens, we // would over-release the wakelock... we take matters into our own hands to ensure that // the completion callback can only be run if send() completes successfully. this means // the completion callback may be run inline - but as we've never specified what thread // the callback is run on, this is fine. - GatedCallback gatedCallback; - if (onCompleteCallback != null) { - gatedCallback = new GatedCallback(() -> { - try { - onCompleteCallback.sendResult(null); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); - } - }); - onFinished = (pI, i, rC, rD, rE) -> gatedCallback.run(); - } else { - gatedCallback = new GatedCallback(null); - } + GatedCallback gatedCallback = new GatedCallback(onCompleteCallback); mPendingIntent.send( mContext, 0, intent, - onFinished, + (pI, i, rC, rD, rE) -> gatedCallback.run(), null, null, options.toBundle()); @@ -306,7 +293,7 @@ public class LocationProviderManager extends @Override public void deliverOnLocationChanged(@Nullable LocationResult locationResult, - @Nullable IRemoteCallback onCompleteCallback) + @Nullable Runnable onCompleteCallback) throws RemoteException { // ILocationCallback doesn't currently support completion callbacks Preconditions.checkState(onCompleteCallback == null); @@ -727,13 +714,6 @@ public class LocationProviderManager extends final PowerManager.WakeLock mWakeLock; - // b/206340085 - if we allocate a new wakelock releaser object for every delivery we - // increase the risk of resource starvation. if a client stops processing deliveries the - // system server binder allocation pool will be starved as we continue to queue up - // deliveries, each with a new allocation. in order to mitigate this, we use a single - // releaser object per registration rather than per delivery. - final ExternalWakeLockReleaser mWakeLockReleaser; - private volatile ProviderTransport mProviderTransport; private int mNumLocationsDelivered = 0; private long mExpirationRealtimeMs = Long.MAX_VALUE; @@ -747,7 +727,6 @@ public class LocationProviderManager extends .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, WAKELOCK_TAG); mWakeLock.setReferenceCounted(true); mWakeLock.setWorkSource(request.getWorkSource()); - mWakeLockReleaser = new ExternalWakeLockReleaser(identity, mWakeLock); } @Override @@ -964,7 +943,7 @@ public class LocationProviderManager extends } listener.deliverOnLocationChanged(deliverLocationResult, - mUseWakeLock ? mWakeLockReleaser : null); + mUseWakeLock ? mWakeLock::release : null); EVENT_LOG.logProviderDeliveredLocations(mName, locationResult.size(), getIdentity()); } @@ -2782,7 +2761,7 @@ public class LocationProviderManager extends @GuardedBy("this") private boolean mRun; - GatedCallback(@Nullable Runnable callback) { + GatedCallback(Runnable callback) { mCallback = callback; } @@ -2817,24 +2796,4 @@ public class LocationProviderManager extends } } } - - private static class ExternalWakeLockReleaser extends IRemoteCallback.Stub { - - private final CallerIdentity mIdentity; - private final PowerManager.WakeLock mWakeLock; - - ExternalWakeLockReleaser(CallerIdentity identity, PowerManager.WakeLock wakeLock) { - mIdentity = identity; - mWakeLock = Objects.requireNonNull(wakeLock); - } - - @Override - public void sendResult(Bundle data) { - try { - mWakeLock.release(); - } catch (RuntimeException e) { - Log.e(TAG, "wakelock over-released by " + mIdentity, e); - } - } - } }