A visibile to invisible state change will also release memory and should...

A visibile to invisible state change will also release memory and should therefore wait as an unload does

A visibility change will also release memory and should therefore also stall an immediate unload.

Updated comments and cleaned up the ResourceManager function.

BUG=414918
TEST=unittest

Review URL: https://codereview.chromium.org/588823002

Cr-Commit-Position: refs/heads/master@{#296118}

athena/resource_manager/memory_pressure_notifier.h

@@ -28,12 +28,14 @@ class MemoryPressureObserver {
 
   // The reported memory pressure. Note: The value is intentionally abstracted
   // since the real amount of free memory is only estimated (due to e.g. zram).
+  // Note: The bigger the index of the pressure level, the more resources are
+  // in use.
   enum MemoryPressure {
-    MEMORY_PRESSURE_UNKNOWN,   // The memory pressure cannot be determined.
-    MEMORY_PRESSURE_LOW,       // Single call if memory fill level is below 50%.
-    MEMORY_PRESSURE_MODERATE,  // Polled for memory fill level of ~50 .. 75%.
-    MEMORY_PRESSURE_HIGH,      // Polled for memory fill level of ~75% .. 90%.
-    MEMORY_PRESSURE_CRITICAL,  // Polled for memory fill level of above ~90%.
+    MEMORY_PRESSURE_UNKNOWN = 0,   // The memory pressure cannot be determined.
+    MEMORY_PRESSURE_LOW,           // Single call if fill level is below 50%.
+    MEMORY_PRESSURE_MODERATE,      // Polled for fill level of ~50 .. 75%.
+    MEMORY_PRESSURE_HIGH,          // Polled for fill level of ~75% .. 90%.
+    MEMORY_PRESSURE_CRITICAL,      // Polled for fill level of above ~90%.
   };
   // The observer.
   virtual void OnMemoryPressure(MemoryPressure pressure) = 0;

athena/resource_manager/resource_manager_impl.cc

@@ -81,11 +81,29 @@ class ResourceManagerImpl : public ResourceManager,
   // Manage the resources for our activities.
   void ManageResource();
 
+  // Check that the visibility of activities is properly set.
+  void UpdateVisibilityStates();
+
+  // Check if activities can be unloaded to reduce memory pressure.
+  void TryToUnloadAnActivity();
+
   // Order our activity list to the order of activities of the stream.
   // TODO(skuhne): Once the ActivityManager is responsible to create this list
   // for us, we can remove this code here.
   void UpdateActivityOrder();
 
+  // Resources were released and a quiet period is needed before we release
+  // more since it takes a while to trickle through the system.
+  void OnResourcesReleased();
+
+  // The memory pressure has increased, previously applied measures did not show
+  // effect and immediate action is required.
+  void OnMemoryPressureIncreased();
+
+  // Returns true when the previous memory release was long enough ago to try
+  // unloading another activity.
+  bool AllowedToUnloadActivity();
+
   // The sorted (new(front) -> old(back)) activity list.
   // TODO(skuhne): Once the ActivityManager is responsible to create this list
   // for us, we can remove this code here.
@@ -115,7 +133,7 @@ class ResourceManagerImpl : public ResourceManager,
   bool in_overview_mode_;
 
   // True if we are in split view mode.
-  bool in_splitview_mode_;
+  bool in_split_view_mode_;
 
   // The last time the resource manager was called to release resources.
   // Avoid too aggressive resource de-allocation by enforcing a wait time of
@@ -145,7 +163,7 @@ ResourceManagerImpl::ResourceManagerImpl(ResourceManagerDelegate* delegate)
       queued_command_(false),
       activity_order_changed_(false),
       in_overview_mode_(false),
-      in_splitview_mode_(false),
+      in_split_view_mode_(false),
       next_resource_management_time_(base::Time::Now()),
       wait_time_for_resource_deallocation_(base::TimeDelta::FromMilliseconds(
           delegate_->MemoryPressureIntervalInMS())) {
@@ -205,7 +223,7 @@ void ResourceManagerImpl::OnOverviewModeExit() {
 
 void ResourceManagerImpl::OnSplitViewModeEnter() {
   // Re-apply the memory pressure to make sure enough items are visible.
-  in_splitview_mode_ = true;
+  in_split_view_mode_ = true;
   ManageResource();
 }
 
@@ -213,7 +231,7 @@ void ResourceManagerImpl::OnSplitViewModeEnter() {
 void ResourceManagerImpl::OnSplitViewModeExit() {
   // We don't do immediately something yet. The next ManageResource call will
   // come soon.
-  in_splitview_mode_ = false;
+  in_split_view_mode_ = false;
 }
 
 void ResourceManagerImpl::OnWindowStackingChanged() {
@@ -241,6 +259,8 @@ void ResourceManagerImpl::OnWindowRemoved(aura::Window* removed_window,
 
 void ResourceManagerImpl::OnMemoryPressure(
       MemoryPressureObserver::MemoryPressure pressure) {
+  if (pressure > current_memory_pressure_)
+    OnMemoryPressureIncreased();
   current_memory_pressure_ = pressure;
   ManageResource();
 }
@@ -259,38 +279,26 @@ void ResourceManagerImpl::ManageResource() {
     return;
   }
 
-  // TODO(skuhne): This algorithm needs to take all kinds of predictive analysis
-  // and running applications into account. For this first patch we only do a
-  // very simple "floating window" algorithm which is surely not good enough.
-  size_t max_running_activities = 5;
-  switch (current_memory_pressure_) {
-    case MEMORY_PRESSURE_UNKNOWN:
-      // If we do not know how much memory we have we assume that it must be a
-      // high consumption.
-      // Fallthrough.
-    case MEMORY_PRESSURE_HIGH:
-      max_running_activities = 5;
-      break;
-    case MEMORY_PRESSURE_CRITICAL:
-      max_running_activities = 0;
-      break;
-    case MEMORY_PRESSURE_MODERATE:
-      max_running_activities = 7;
-      break;
-    case MEMORY_PRESSURE_LOW:
-      // This doesn't really matter. We do not change anything but turning
-      // activities visible.
-      max_running_activities = 10000;
-      break;
-  }
+  // Check that the visibility of items is properly set. Note that this might
+  // already trigger a release of resources. If this happens,
+  // AllowedToUnloadActivity() will return false.
+  UpdateVisibilityStates();
 
+  // Since resource deallocation takes time, we avoid to release more resources
+  // in short succession. Note that we come here periodically and if one call
+  // is not triggering an unload, the next one will.
+  if (AllowedToUnloadActivity())
+    TryToUnloadAnActivity();
+}
+
+void ResourceManagerImpl::UpdateVisibilityStates() {
   // The first n activities should be treated as "visible", means they updated
   // in overview mode and will keep their layer resources for faster switch
   // times. Usually we use |kMaxVisibleActivities| items, but when the memory
   // pressure gets critical we only hold as many as are really visible.
   size_t max_activities = kMaxVisibleActivities;
   if (current_memory_pressure_ == MEMORY_PRESSURE_CRITICAL)
-    max_activities = 1 + (in_splitview_mode_ ? 1 : 0);
+    max_activities = in_split_view_mode_ ? 2 : 1;
 
   // Restart and / or bail if the order of activities changes due to our calls.
   activity_order_changed_ = false;
@@ -318,11 +326,10 @@ void ResourceManagerImpl::ManageResource() {
           (current_memory_pressure_ != MEMORY_PRESSURE_LOW ||
            visiblity_state == Activity::ACTIVITY_VISIBLE)) {
         activity->SetCurrentState(visiblity_state);
-        // If we turned an activity invisible, we should not at the same time
-        // throw an activity out of memory. Thus we grant one more invisible
-        // Activity in that case.
+        // If we turned an activity invisible, we are already releasing memory
+        // and can hold off releasing more for now.
         if (visiblity_state == Activity::ACTIVITY_INVISIBLE)
-          max_running_activities++;
+          OnResourcesReleased();
       }
     }
 
@@ -334,23 +341,33 @@ void ResourceManagerImpl::ManageResource() {
       ++index;
     }
   }
+}
 
-  // If there is only a low memory pressure, or our last call to release
-  // resources cannot have had any impact yet, we return.
-  // TODO(skuhne): The upper part of this function bumps up the state (to
-  // visible) and the lower part might unload one. Going forward this algorithm
-  // will change significantly and when it does we might want to break this into
-  // two separate pieces.
-  if (current_memory_pressure_ == MEMORY_PRESSURE_LOW ||
-      base::Time::Now() < next_resource_management_time_)
-    return;
-  // Do not release too many activities in short succession since it takes time
-  // to release resources. As such wait the memory pressure interval before the
-  // next call.
-  next_resource_management_time_ = base::Time::Now() +
-                                   wait_time_for_resource_deallocation_;
+void ResourceManagerImpl::TryToUnloadAnActivity() {
+  // TODO(skuhne): This algorithm needs to take all kinds of predictive analysis
+  // and running applications into account. For this first patch we only do a
+  // very simple "floating window" algorithm which is surely not good enough.
+  size_t max_running_activities = 5;
+  switch (current_memory_pressure_) {
+    case MEMORY_PRESSURE_UNKNOWN:
+      // If we do not know how much memory we have we assume that it must be a
+      // high consumption.
+      // Fallthrough.
+    case MEMORY_PRESSURE_HIGH:
+      max_running_activities = 5;
+      break;
+    case MEMORY_PRESSURE_CRITICAL:
+      max_running_activities = 0;
+      break;
+    case MEMORY_PRESSURE_MODERATE:
+      max_running_activities = 7;
+      break;
+    case MEMORY_PRESSURE_LOW:
+      NOTREACHED();
+      return;
+  }
 
-  // Check if/which activity we want to unload.
+  // Check if / which activity we want to unload.
   Activity* oldest_media_activity = NULL;
   std::vector<Activity*> unloadable_activities;
   for (std::vector<Activity*>::iterator it = activity_list_.begin();
@@ -369,10 +386,12 @@ void ResourceManagerImpl::ManageResource() {
   }
 
   if (unloadable_activities.size() > max_running_activities) {
+    OnResourcesReleased();
     unloadable_activities.back()->SetCurrentState(Activity::ACTIVITY_UNLOADED);
     return;
   } else if (current_memory_pressure_ == MEMORY_PRESSURE_CRITICAL) {
     if (oldest_media_activity) {
+      OnResourcesReleased();
       oldest_media_activity->SetCurrentState(Activity::ACTIVITY_UNLOADED);
       LOG(WARNING) << "Unloading item to releave critical memory pressure";
       return;
@@ -422,6 +441,24 @@ void ResourceManagerImpl::UpdateActivityOrder() {
   activity_order_changed_ = true;
 }
 
+void ResourceManagerImpl::OnResourcesReleased() {
+  // Do not release too many activities in short succession since it takes time
+  // to release resources. As such wait the memory pressure interval before the
+  // next call.
+  next_resource_management_time_ = base::Time::Now() +
+                                   wait_time_for_resource_deallocation_;
+}
+
+void ResourceManagerImpl::OnMemoryPressureIncreased() {
+  // By setting the timer to Now, the next call will immediately be performed.
+  next_resource_management_time_ = base::Time::Now();
+}
+
+bool ResourceManagerImpl::AllowedToUnloadActivity() {
+  return current_memory_pressure_ != MEMORY_PRESSURE_LOW &&
+         base::Time::Now() >= next_resource_management_time_;
+}
+
 // static
 void ResourceManager::Create() {
   DCHECK(!instance);

athena/resource_manager/resource_manager_unittest.cc

@@ -244,22 +244,35 @@ TEST_F(ResourceManagerTest, VisibilityChanges) {
   EXPECT_NE(Activity::ACTIVITY_INVISIBLE, app4->GetCurrentState());
 }
 
-// Make sure that an activity which got just demoted from visible to invisible,
+// Make sure that an activity which got just reduced from visible to invisible,
 // does not get thrown out of memory in the same step.
 TEST_F(ResourceManagerTest, NoUnloadFromVisible) {
+  // The timeout override in milliseconds.
+  const int kTimeoutOverrideInMs = 20;
+  // Tell the resource manager to wait for 20ms between calls.
+  ResourceManager::Get()->SetWaitTimeBetweenResourceManageCalls(
+      kTimeoutOverrideInMs);
+
   // Create a few dummy activities in the reverse order as we need them.
   TestActivity* app2 = CreateActivity("app2");
   TestActivity* app1 = CreateActivity("app1");
   app1->SetCurrentState(Activity::ACTIVITY_VISIBLE);
   app2->SetCurrentState(Activity::ACTIVITY_VISIBLE);
 
-  // Applying low resource pressure should turn one item ivisible.
+  // Applying low resource pressure should turn one item invisible.
   ResourceManager::Get()->SetMemoryPressureAndStopMonitoring(
       MemoryPressureObserver::MEMORY_PRESSURE_CRITICAL);
   EXPECT_EQ(Activity::ACTIVITY_VISIBLE, app1->GetCurrentState());
   EXPECT_EQ(Activity::ACTIVITY_INVISIBLE, app2->GetCurrentState());
 
-  // Applying low resource pressure again will unload it.
+  // Trying to apply the memory pressure again does not do anything.
+  ResourceManager::Get()->SetMemoryPressureAndStopMonitoring(
+      MemoryPressureObserver::MEMORY_PRESSURE_CRITICAL);
+  EXPECT_EQ(Activity::ACTIVITY_VISIBLE, app1->GetCurrentState());
+  EXPECT_EQ(Activity::ACTIVITY_INVISIBLE, app2->GetCurrentState());
+
+  // Waiting and applying the pressure again should unload it.
+  usleep(kTimeoutOverrideInMs * 1000);
   ResourceManager::Get()->SetMemoryPressureAndStopMonitoring(
       MemoryPressureObserver::MEMORY_PRESSURE_CRITICAL);
   EXPECT_EQ(Activity::ACTIVITY_VISIBLE, app1->GetCurrentState());