From 83f8dbdaf1a0fa06b20b0bf3e485cd309b569950 Mon Sep 17 00:00:00 2001
From: Nicolo' Mazzucato <nicomazz@google.com>
Date: Mon, 13 Mar 2023 20:15:37 +0000
Subject: [PATCH] Use separate locks for BatteryController estimation and
 callbacks

Initially mFetchCallbacks was used as a lock for everything going on in the class.
From droidfooders traces we noticed that getEstimatedTimeRemainingString was blocking the main thread because an estimate update was going on the in background, even if it was only waiting to add a callback to a list.

 This was causing priority inversion (ui thread depending on the background thread with less priority), ending up blocking the ui thread for 150ms.

 With this cl there are 2 locks: one just for the callbacks list, and one for the estimate computations

Bug: 272677942
Test: BatteryControllerTest
Change-Id: I31cc862161377678f60b70f5870a5295bb2625fa
---
 .../statusbar/policy/BatteryControllerImpl.java        | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/BatteryControllerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/policy/BatteryControllerImpl.java
index 2ee52325ca4ab..654ba04eba7a7 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/BatteryControllerImpl.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/BatteryControllerImpl.java
@@ -57,6 +57,8 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 
+import javax.annotation.concurrent.GuardedBy;
+
 /**
  * Default implementation of a {@link BatteryController}. This controller monitors for battery
  * level change events that are broadcasted by the system.
@@ -94,7 +96,10 @@ public class BatteryControllerImpl extends BroadcastReceiver implements BatteryC
     private boolean mTestMode = false;
     @VisibleForTesting
     boolean mHasReceivedBattery = false;
+    @GuardedBy("mEstimateLock")
     private Estimate mEstimate;
+    private final Object mEstimateLock = new Object();
+
     private boolean mFetchingEstimate = false;
 
     // Use AtomicReference because we may request it from a different thread
@@ -321,7 +326,7 @@ public class BatteryControllerImpl extends BroadcastReceiver implements BatteryC
 
     @Nullable
     private String generateTimeRemainingString() {
-        synchronized (mFetchCallbacks) {
+        synchronized (mEstimateLock) {
             if (mEstimate == null) {
                 return null;
             }
@@ -340,7 +345,7 @@ public class BatteryControllerImpl extends BroadcastReceiver implements BatteryC
         mFetchingEstimate = true;
         mBgHandler.post(() -> {
             // Only fetch the estimate if they are enabled
-            synchronized (mFetchCallbacks) {
+            synchronized (mEstimateLock) {
                 mEstimate = null;
                 if (mEstimates.isHybridNotificationEnabled()) {
                     updateEstimate();
@@ -363,6 +368,7 @@ public class BatteryControllerImpl extends BroadcastReceiver implements BatteryC
     }
 
     @WorkerThread
+    @GuardedBy("mEstimateLock")
     private void updateEstimate() {
         Assert.isNotMainThread();
         // if the estimate has been cached we can just use that, otherwise get a new one and