Using the new MemoryPressureListener instead of the LowMemoryObserver when the enhanced memory mana…

base/chromeos/memory_pressure_observer_chromeos.cc

Index: base/chromeos/memory_pressure_observer_chromeos.cc
diff --git a/base/chromeos/memory_pressure_observer_chromeos.cc b/base/chromeos/memory_pressure_observer_chromeos.cc
index 22291da09ba9191054f26eb3c8bce70470917389..4abb963b18eb5ee18950808c7a9d498aba950051 100644
--- a/base/chromeos/memory_pressure_observer_chromeos.cc
+++ b/base/chromeos/memory_pressure_observer_chromeos.cc
@@ -4,6 +4,7 @@
 
 #include "base/chromeos/memory_pressure_observer_chromeos.h"
 
+#include "base/message_loop/message_loop.h"
 #include "base/process/process_metrics.h"
 #include "base/time/time.h"
 
@@ -11,15 +12,25 @@ namespace base {
 
 namespace {
 
-// The time between memory pressure checks.
+// The time between memory pressure checks. While under critical pressure, this
+// is also the timer to repeat cleanup attempts.
 const int kMemoryPressureIntervalInMS = 1000;
 
+// The cool down amount of events which should get skipped between moderate
+// pressure events. It is expressed in milliseconds divided by the timer
+// resolution.
+const int kModerateMemoryPressureCooldown = 10000 / kMemoryPressureIntervalInMS;

[James Cook, 2014/12/19 19:38:07]

nit: I would use two constants here, like "kModerateMemoryPressureCooldownMs =
10000" and then compute kModerateMemoryPressureCooldown.

[Mr4D (OOO till 08-26), 2014/12/19 23:34:41]

Done.

+
+// Threshold constants to emit pressure events.
+const int kMemoryPressureModerateThresholdPercent = 70;
+const int kMemoryPressureCriticalThresholdPercent = 90;
+
 // Converts free percent of memory into a memory pressure value.
 MemoryPressureListener::MemoryPressureLevel GetMemoryPressureLevelFromFillLevel(
     int memory_fill_level) {
-  if (memory_fill_level < 70)
+  if (memory_fill_level < kMemoryPressureModerateThresholdPercent)
     return MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE;
-  return memory_fill_level < 90 ?
+  return memory_fill_level < kMemoryPressureCriticalThresholdPercent ?
       MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE :
       MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL;
 }
@@ -66,7 +77,8 @@ int GetUsedMemoryInPercent() {
 
 MemoryPressureObserverChromeOS::MemoryPressureObserverChromeOS()
     : current_memory_pressure_level_(
-        MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE) {
+        MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE),
+      moderate_pressure_repeat_counter_(0)  {
   StartObserving();
 }
 
@@ -74,11 +86,18 @@ MemoryPressureObserverChromeOS::~MemoryPressureObserverChromeOS() {
   StopObserving();
 }
 
+void MemoryPressureObserverChromeOS::ScheduleEarlyCheck() {
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      Bind(&MemoryPressureObserverChromeOS::CheckMemoryPressure,
+           base::Unretained(this)));

[James Cook, 2014/12/19 19:38:07]

Just to double-check: |this| always outlives the MessageLoop? If not, does this
need a weakptr factory?

[Mr4D (OOO till 08-26), 2014/12/19 23:34:41]

Originally it is/was not, but since this is in base it is a good idea to make it
a weakptr factory to avoid any problems. Done.

+}
+
 void MemoryPressureObserverChromeOS::StartObserving() {
   timer_.Start(FROM_HERE,
-               base::TimeDelta::FromMilliseconds(kMemoryPressureIntervalInMS),
-               base::Bind(&MemoryPressureObserverChromeOS::CheckMemoryPressure,
-                          base::Unretained(this)));
+               TimeDelta::FromMilliseconds(kMemoryPressureIntervalInMS),
+               Bind(&MemoryPressureObserverChromeOS::CheckMemoryPressure,
+                    Unretained(this)));
 }
 
 void MemoryPressureObserverChromeOS::StopObserving() {
@@ -89,13 +108,31 @@ void MemoryPressureObserverChromeOS::StopObserving() {
 void MemoryPressureObserverChromeOS::CheckMemoryPressure() {
   MemoryPressureListener::MemoryPressureLevel old_pressure =
       current_memory_pressure_level_;
-  MemoryPressureListener::MemoryPressureLevel new_pressure =
+  current_memory_pressure_level_ =
       GetMemoryPressureLevelFromFillLevel(GetUsedMemoryInPercent());
-  if (old_pressure != new_pressure) {
-    current_memory_pressure_level_ = new_pressure;
-    // Everything but NONE will be sent to the listener.
-    if (new_pressure != MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE)
-      MemoryPressureListener::NotifyMemoryPressure(new_pressure);
+  switch (current_memory_pressure_level_) {

[James Cook, 2014/12/19 19:38:07]

I find the logic here hard to understand. Maybe this would be better as a series
of if()'s. In particular, I find it hard to think about whether this sends
multiple notifications if the memory pressure stays in the moderate range (I
don't think it does, but I was confused initially.)

Maybe it would make more sense to break it into

if (current == old) {
  // always notify for critical
  return;
}

// handle logic for changes

[Mr4D (OOO till 08-26), 2014/12/19 23:34:41]

Okay, Done. 
Note: The reason why I chose this however was that it a. shows better what is
done in which level and b. it forces a change when we add another state back in.

+    case MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:
+      // Moderate pressure changes are less frequently posted.
+      if (current_memory_pressure_level_ == old_pressure &&
+          ++moderate_pressure_repeat_counter_ <
+              kModerateMemoryPressureCooldown) {
+        return;
+      }
+      moderate_pressure_repeat_counter_ = 0;
+      // If we have just resolved the critical memory pressure, we ignore this
+      // call.
+      if (old_pressure ==
+          MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL)
+        return;
+      // Fallthrough.
+    case MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
+      // We notify the system with every check that we are (still) under
+      // pressure.
+      MemoryPressureListener::NotifyMemoryPressure(
+          current_memory_pressure_level_);
+      // Fallthrough.
+    case MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
+      break;
   }
 }

[James Cook, 2014/12/19 19:38:07]

I think this function is complex enough that it needs a little unit test.

[Mr4D (OOO till 08-26), 2014/12/20 00:12:45]

Done.