Drop the global memory state from memory coordinator

content/browser/memory/memory_coordinator_impl.cc

Index: content/browser/memory/memory_coordinator_impl.cc
diff --git a/content/browser/memory/memory_coordinator_impl.cc b/content/browser/memory/memory_coordinator_impl.cc
index 49ea5500830287960f921f7398508c836039081b..36a153f66c24759dcf1e238d01103a16a67aaa85 100644
--- a/content/browser/memory/memory_coordinator_impl.cc
+++ b/content/browser/memory/memory_coordinator_impl.cc
@@ -10,8 +10,8 @@
 #include "base/process/process_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
+#include "content/browser/memory/memory_condition_observer.h"
 #include "content/browser/memory/memory_monitor.h"
-#include "content/browser/memory/memory_state_updater.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ -24,6 +24,8 @@ namespace content {
 
 namespace {
 
+const int kDefaultMinimumTransitionPeriodSeconds = 30;
+
 mojom::MemoryState ToMojomMemoryState(base::MemoryState state) {
   switch (state) {
     case base::MemoryState::UNKNOWN:
@@ -40,6 +42,19 @@ mojom::MemoryState ToMojomMemoryState(base::MemoryState state) {
   }
 }
 
+const char* MemoryConditionToString(MemoryCondition condition) {
+  switch (condition) {
+    case MemoryCondition::NORMAL:
+      return "normal";
+    case MemoryCondition::WARNING:
+      return "warning";
+    case MemoryCondition::CRITICAL:
+      return "critical";
+  }
+  NOTREACHED();
+  return "N/A";
+}
+
 }  // namespace
 
 // The implementation of MemoryCoordinatorHandle. See memory_coordinator.mojom
@@ -113,7 +128,10 @@ MemoryCoordinatorImpl::MemoryCoordinatorImpl(
     std::unique_ptr<MemoryMonitor> memory_monitor)
     : delegate_(GetContentClient()->browser()->GetMemoryCoordinatorDelegate()),
       memory_monitor_(std::move(memory_monitor)),
-      state_updater_(base::MakeUnique<MemoryStateUpdater>(this, task_runner)) {
+      condition_observer_(
+          base::MakeUnique<MemoryConditionObserver>(this, task_runner)),
+      minimum_state_transition_period_(base::TimeDelta::FromSeconds(
+          kDefaultMinimumTransitionPeriodSeconds)) {
   DCHECK(memory_monitor_.get());
   base::MemoryCoordinatorProxy::SetMemoryCoordinator(this);
 }
@@ -127,8 +145,7 @@ void MemoryCoordinatorImpl::Start() {
   notification_registrar_.Add(
       this, NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
       NotificationService::AllBrowserContextsAndSources());
-  last_state_change_ = base::TimeTicks::Now();
-  state_updater_->ScheduleUpdateState(base::TimeDelta());
+  condition_observer_->ScheduleUpdateCondition(base::TimeDelta());
 }
 
 void MemoryCoordinatorImpl::CreateHandle(
@@ -158,7 +175,7 @@ bool MemoryCoordinatorImpl::SetChildMemoryState(int render_process_id,
   if (!iter->second.handle->child().is_bound())
     return false;
 
-  memory_state = OverrideGlobalState(memory_state, iter->second);
+  memory_state = OverrideState(memory_state, iter->second);
 
   // A nop doesn't need to be sent, but is considered successful.
   if (iter->second.memory_state == memory_state)
@@ -188,30 +205,26 @@ void MemoryCoordinatorImpl::RecordMemoryPressure(
   // TODO(bashi): Record memory pressure level.
 }
 
-base::MemoryState MemoryCoordinatorImpl::GetGlobalMemoryState() const {
-  return current_state_;
+base::MemoryState MemoryCoordinatorImpl::GetBrowserMemoryState() const {
+  return browser_memory_state_;
 }
 
 base::MemoryState MemoryCoordinatorImpl::GetCurrentMemoryState() const {
-  // SUSPENDED state may not make sense to the browser process. Use THROTTLED
-  // instead when the global state is SUSPENDED.
-  // TODO(bashi): Maybe worth considering another state for the browser.
-  return current_state_ == MemoryState::SUSPENDED ? MemoryState::THROTTLED
-                                                  : current_state_;
+  return GetBrowserMemoryState();
 }
 
 void MemoryCoordinatorImpl::SetCurrentMemoryStateForTesting(
     base::MemoryState memory_state) {
-  // This changes the current state temporariy for testing. The state will be
-  // updated 1 minute later.
-  ForceSetGlobalState(memory_state, base::TimeDelta::FromMinutes(1));
+  // Resets |last_state_change_| so that NotifyStateToBrowser() to set
+  // memory state forcibly.
+  last_state_change_ = base::TimeTicks();
+  NotifyStateToBrowser(memory_state);
 }
 
-void MemoryCoordinatorImpl::ForceSetGlobalState(base::MemoryState new_state,
-                                                base::TimeDelta duration) {
-  DCHECK(new_state != MemoryState::UNKNOWN);
-  ChangeStateIfNeeded(current_state_, new_state);
-  state_updater_->ScheduleUpdateState(duration);
+void MemoryCoordinatorImpl::ForceSetMemoryCondition(MemoryCondition condition,
+                                                    base::TimeDelta duration) {
+  UpdateConditionIfNeeded(condition);
+  condition_observer_->ScheduleUpdateCondition(duration);
 }
 
 void MemoryCoordinatorImpl::Observe(int type,
@@ -225,8 +238,21 @@ void MemoryCoordinatorImpl::Observe(int type,
   auto iter = children().find(process->GetID());
   if (iter == children().end())
     return;
+
   iter->second.is_visible = *Details<bool>(details).ptr();
-  auto new_state = GetGlobalMemoryState();
+  // The current heuristics for state calculation:
+  // - Foregrounded renderers: THROTTLED when condition is CRITICAL, otherwise
+  //   NORMAL.
+  // - Backgrounded renderers: THROTTLED when condition is WARNING/CRITICAL,
+  //   otherwise NORMAL.
+  MemoryState new_state = MemoryState::NORMAL;
+  MemoryCondition condition = GetMemoryCondition();
+  if (condition == MemoryCondition::WARNING) {
+    new_state =
+        iter->second.is_visible ? MemoryState::NORMAL : MemoryState::THROTTLED;
+  } else if (condition == MemoryCondition::CRITICAL) {
+    new_state = MemoryState::THROTTLED;
+  }
   SetChildMemoryState(iter->first, new_state);
 }
 
@@ -236,7 +262,7 @@ base::MemoryState MemoryCoordinatorImpl::GetStateForProcess(
   if (handle == base::kNullProcessHandle)
     return MemoryState::UNKNOWN;
   if (handle == base::GetCurrentProcessHandle())
-    return GetCurrentMemoryState();
+    return browser_memory_state_;
 
   for (auto& iter : children()) {
     auto* render_process_host = GetRenderProcessHost(iter.first);
@@ -246,22 +272,43 @@ base::MemoryState MemoryCoordinatorImpl::GetStateForProcess(
   return MemoryState::UNKNOWN;
 }
 
-bool MemoryCoordinatorImpl::ChangeStateIfNeeded(base::MemoryState prev_state,
-                                                base::MemoryState next_state) {
+void MemoryCoordinatorImpl::UpdateConditionIfNeeded(
+    MemoryCondition next_condition) {
   DCHECK(CalledOnValidThread());
-  if (prev_state == next_state)
-    return false;
+  if (memory_condition_ == next_condition)
+    return;
 
-  last_state_change_ = base::TimeTicks::Now();
-  current_state_ = next_state;
+  MemoryCondition prev_condition = memory_condition_;
+  memory_condition_ = next_condition;
 
   TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("memory-infra"),
-               "MemoryCoordinatorImpl::ChangeStateIfNeeded", "prev",
-               MemoryStateToString(prev_state), "next",
-               MemoryStateToString(next_state));
-  NotifyStateToClients();
-  NotifyStateToChildren();
-  return true;
+               "MemoryCoordinatorImpl::UpdateConditionIfNeeded", "prev",
+               MemoryConditionToString(prev_condition), "next",
+               MemoryConditionToString(next_condition));
+
+  // TODO(bashi): Following actions are tentative. We might want to prioritize
+  // processes and handle them one-by-one.
+
+  if (next_condition == MemoryCondition::NORMAL) {
+    // Set NORMAL state to all clients/processes.
+    NotifyStateToBrowser(MemoryState::NORMAL);
+    NotifyStateToChildren(MemoryState::NORMAL);
+  } else if (next_condition == MemoryCondition::WARNING) {
+    // Set NORMAL state to foreground proceses and clients in the browser
+    // process. Set THROTTLED state to background processes.
+    NotifyStateToBrowser(MemoryState::NORMAL);
+    for (auto& iter : children()) {
+      auto state =
+          iter.second.is_visible ? MemoryState::NORMAL : MemoryState::THROTTLED;
+      SetChildMemoryState(iter.first, state);
+    }
+    // Idea: Purge memory from background processes.
+  } else if (next_condition == MemoryCondition::CRITICAL) {
+    // Set THROTTLED state to all clients/processes.
+    NotifyStateToBrowser(MemoryState::THROTTLED);
+    NotifyStateToChildren(MemoryState::THROTTLED);
+    // Idea: Start discarding tabs.
+  }
 }
 
 void MemoryCoordinatorImpl::DiscardTab() {
@@ -308,14 +355,16 @@ bool MemoryCoordinatorImpl::CanSuspendRenderer(int render_process_id) {
 }
 
 void MemoryCoordinatorImpl::OnChildAdded(int render_process_id) {
-  // Populate the global state as an initial state of a newly created process.
-  auto new_state = GetGlobalMemoryState();
+  // Populate an initial state of a newly created process, assuming it's
+  // foregrounded.

[haraken, 2017/03/03 02:08:00]

You don't need to fix this in this CL but this assumption may not be correct.
e.g., a case where users restore many tabs at the same time.

Either way, it looks better to share the following code with
MemoryCoordinatorImpl::Observe.

[bashi, 2017/03/03 02:41:32]

Agreed. Added TODO.

+  auto new_state = GetMemoryCondition() == MemoryCondition::CRITICAL
+                       ? MemoryState::THROTTLED
+                       : MemoryState::NORMAL;
   SetChildMemoryState(render_process_id, new_state);
 }
 
-base::MemoryState MemoryCoordinatorImpl::OverrideGlobalState(
-    MemoryState memory_state,
-    const ChildInfo& child) {
+base::MemoryState MemoryCoordinatorImpl::OverrideState(MemoryState memory_state,
+                                                       const ChildInfo& child) {
   // We don't suspend foreground renderers. Throttle them instead.
   if (child.is_visible && memory_state == MemoryState::SUSPENDED)
     return MemoryState::THROTTLED;
@@ -343,16 +392,30 @@ void MemoryCoordinatorImpl::CreateChildInfoMapEntry(
   child_info.handle = std::move(handle);
 }
 
-void MemoryCoordinatorImpl::NotifyStateToClients() {
-  auto state = GetCurrentMemoryState();
+void MemoryCoordinatorImpl::NotifyStateToBrowser(
+    base::MemoryState memory_state) {
+  if (memory_state == browser_memory_state_)

[bashi, 2017/03/02 08:59:31]

unittests catched the bug. I should have added this line.

+    return;
+
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (!last_state_change_.is_null() &&
+      (now - last_state_change_ < minimum_state_transition_period_))
+    return;
+
+  last_state_change_ = now;
+  browser_memory_state_ = memory_state;
+  NotifyStateToClients(memory_state);
+}
+
+void MemoryCoordinatorImpl::NotifyStateToClients(MemoryState state) {
   base::MemoryCoordinatorClientRegistry::GetInstance()->Notify(state);
 }
 
-void MemoryCoordinatorImpl::NotifyStateToChildren() {
+void MemoryCoordinatorImpl::NotifyStateToChildren(MemoryState state) {
   // It's OK to call SetChildMemoryState() unconditionally because it checks
   // whether this state transition is valid.
   for (auto& iter : children())
-    SetChildMemoryState(iter.first, current_state_);
+    SetChildMemoryState(iter.first, state);
 }
 
 MemoryCoordinatorImpl::ChildInfo::ChildInfo() {}