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

athena/resource_manager/resource_manager_impl.cc

Index: athena/resource_manager/resource_manager_impl.cc
diff --git a/athena/resource_manager/resource_manager_impl.cc b/athena/resource_manager/resource_manager_impl.cc
index 8025f751366c32f64de208ab264af1cd29645065..4a5761a0eab36bb1e3161425e77c0219ce84f36a 100644
--- a/athena/resource_manager/resource_manager_impl.cc
+++ b/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 CheckVisibilityStates();
+
+  // 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 ResourcesGotReleased();

[oshima, 2014/09/22 16:22:51]

OnResourceReleased is more common.

[Mr4D (OOO till 08-26), 2014/09/23 00:28:56]

Done.

[Even though I have to say that the On.. notion feels more like an external
event and not like an internal function]

+
+  // The memory pressure has increased, previously applied measures did not show
+  // effect and immediate action is required.
+  void MemoryPressureIncreased();

[oshima, 2014/09/22 16:22:51]

ditto. OnMemoryPressureIncreased

[Mr4D (OOO till 08-26), 2014/09/23 00:28:56]

Done. And ditto.

+
+  // 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.
@@ -241,6 +259,8 @@ void ResourceManagerImpl::OnWindowRemoved(aura::Window* removed_window,
 
 void ResourceManagerImpl::OnMemoryPressure(
       MemoryPressureObserver::MemoryPressure pressure) {
+  if (pressure > current_memory_pressure_)
+    MemoryPressureIncreased();
   current_memory_pressure_ = pressure;
   ManageResource();
 }
@@ -259,31 +279,19 @@ 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.
+  CheckVisibilityStates();
 
+  // 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::CheckVisibilityStates() {

[oshima, 2014/09/22 16:22:50]

how about UpdateVisibilityStates?

[Mr4D (OOO till 08-26), 2014/09/23 00:28:56]

Done.

   // 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
@@ -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++;
+          ResourcesGotReleased();
       }
     }
 
@@ -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) {
+    ResourcesGotReleased();
     unloadable_activities.back()->SetCurrentState(Activity::ACTIVITY_UNLOADED);
     return;
   } else if (current_memory_pressure_ == MEMORY_PRESSURE_CRITICAL) {
     if (oldest_media_activity) {
+      ResourcesGotReleased();
       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::ResourcesGotReleased() {
+  // 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::MemoryPressureIncreased() {
+  // By setting the timer to Now, the next call will immediately be performed.
+  next_resource_management_time_ = base::Time::Now();

[oshima, 2014/09/22 16:22:50]

Note that Now() may not always increase (that is, may go back), so the next
AllowedToUnloadActivty still may return false.

It's rare so this shouldn't be an issue, but just wanted to point out.

[Mr4D (OOO till 08-26), 2014/09/23 00:28:56]

Which would be ok! All which is required is that ResourcesGotReleased() is
setting it to a later time. So no problem!

[oshima, 2014/09/23 00:42:35]

Just in case you might have misunderstood my comment. Now() in
AllowedToUnloadActivity may return earlier time than Now() here, so the next
call may not execute immediately. (It's extremely rare so this won't be an
issue, as I said)

+}
+
+bool ResourceManagerImpl::AllowedToUnloadActivity() {
+  return current_memory_pressure_ != MEMORY_PRESSURE_LOW &&
+         base::Time::Now() >= next_resource_management_time_;
+}
+
 // static
 void ResourceManager::Create() {
   DCHECK(!instance);