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 032a91d8333f926264a57292e0982b267b85603f..5dd3ef92f018092ea0cd18622dc237d71a874467 100644
--- a/content/browser/memory/memory_coordinator_impl.cc
+++ b/content/browser/memory/memory_coordinator_impl.cc
@@ -166,12 +166,16 @@ MemoryCoordinatorImpl* MemoryCoordinatorImpl::GetInstance() {
                          MemoryCoordinatorImplSingletonTraits>::get();
 }
 
+const int kDefaultMinimumTransitionPeriodSeconds = 30;
+
 MemoryCoordinatorImpl::MemoryCoordinatorImpl(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     std::unique_ptr<MemoryMonitor> memory_monitor)
     : delegate_(GetContentClient()->browser()->GetMemoryCoordinatorDelegate()),
       memory_monitor_(std::move(memory_monitor)),
-      state_updater_(base::MakeUnique<MemoryStateUpdater>(this, task_runner)) {
+      state_updater_(base::MakeUnique<MemoryStateUpdater>(this, task_runner)),
+      minimum_state_transition_period_(base::TimeDelta::FromSeconds(
+          kDefaultMinimumTransitionPeriodSeconds)) {
   DCHECK(memory_monitor_.get());
   base::MemoryCoordinatorProxy::SetMemoryCoordinator(this);
 }
@@ -186,7 +190,7 @@ void MemoryCoordinatorImpl::Start() {
       this, NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
       NotificationService::AllBrowserContextsAndSources());
   last_state_change_ = base::TimeTicks::Now();
-  state_updater_->ScheduleUpdateState(base::TimeDelta());
+  state_updater_->ScheduleUpdateCondition(base::TimeDelta());
 }
 
 void MemoryCoordinatorImpl::CreateHandle(
@@ -216,7 +220,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)
@@ -227,8 +231,14 @@ bool MemoryCoordinatorImpl::SetChildMemoryState(int render_process_id,
       !CanSuspendRenderer(render_process_id))
     return false;
 
+  base::TimeTicks now = base::TimeTicks::Now();
+  // State should remain unchanged for a certain period of time.
+  if (now - iter->second.last_state_change < minimum_state_transition_period_)
+    return false;
+
   // Update the internal state and send the message.
   iter->second.memory_state = memory_state;
+  iter->second.last_state_change = now;
   iter->second.handle->child()->OnStateChange(ToMojomMemoryState(memory_state));
   return true;
 }
@@ -243,48 +253,37 @@ base::MemoryState MemoryCoordinatorImpl::GetChildMemoryState(
 
 void MemoryCoordinatorImpl::RecordMemoryPressure(
     base::MemoryPressureMonitor::MemoryPressureLevel level) {
-  DCHECK(GetGlobalMemoryState() != base::MemoryState::UNKNOWN);
-  int state = static_cast<int>(GetGlobalMemoryState());
+  // TODO(bashi): Update metrics.
+  int condition = static_cast<int>(GetMemoryCondtion());
   switch (level) {
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:
       UMA_HISTOGRAM_ENUMERATION(
-          "Memory.Coordinator.StateOnModerateNotificationReceived",
-          state, base::kMemoryStateMax);
+          "Memory.Coordinator.StateOnModerateNotificationReceived", condition,
+          base::kMemoryStateMax);
       break;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
       UMA_HISTOGRAM_ENUMERATION(
-          "Memory.Coordinator.StateOnCriticalNotificationReceived",
-          state, base::kMemoryStateMax);
+          "Memory.Coordinator.StateOnCriticalNotificationReceived", condition,
+          base::kMemoryStateMax);
       break;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
       NOTREACHED();
   }
 }
 
-base::MemoryState MemoryCoordinatorImpl::GetGlobalMemoryState() const {
-  return current_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 current_state_;
 }
 
 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));
+  current_state_ = 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);
+  state_updater_->ScheduleUpdateCondition(duration);
 }
 
 void MemoryCoordinatorImpl::Observe(int type,
@@ -299,7 +298,11 @@ void MemoryCoordinatorImpl::Observe(int type,
   if (iter == children().end())
     return;
   iter->second.is_visible = *Details<bool>(details).ptr();
-  auto new_state = GetGlobalMemoryState();
+  // TODO(bashi): Tentative.
+  MemoryState new_state = MemoryState::NORMAL;
+  if (!iter->second.is_visible &&
+      GetMemoryCondtion() != MemoryCondition::NORMAL)
+    new_state = MemoryState::THROTTLED;
   SetChildMemoryState(iter->first, new_state);

[bashi, 2017/02/27 06:53:58]

This basically means:

Foregrounded renderers: change to NORMAL if needed
Backgrounded renderers: change to THROTTLED when free memory is low

 }
 
@@ -309,7 +312,7 @@ base::MemoryState MemoryCoordinatorImpl::GetStateForProcess(
   if (handle == base::kNullProcessHandle)
     return MemoryState::UNKNOWN;
   if (handle == base::GetCurrentProcessHandle())
-    return GetCurrentMemoryState();
+    return current_state_;
 
   for (auto& iter : children()) {
     auto* render_process_host = GetRenderProcessHost(iter.first);
@@ -319,25 +322,60 @@ base::MemoryState MemoryCoordinatorImpl::GetStateForProcess(
   return MemoryState::UNKNOWN;
 }
 
-bool MemoryCoordinatorImpl::ChangeStateIfNeeded(base::MemoryState prev_state,
-                                                base::MemoryState next_state) {
+const char* MemoryConditionToString(MemoryCondition condition) {
+  switch (condition) {
+    case MemoryCondition::NORMAL:
+      return "normal";
+    case MemoryCondition::WARNING:
+      return "warning";
+    case MemoryCondition::CRITICAL:
+      return "critical";
+  }
+}
+
+void MemoryCoordinatorImpl::UpdateConditionIfNeeded(
+    MemoryCondition next_condition) {
   DCHECK(CalledOnValidThread());
-  if (prev_state == next_state)
-    return false;
+  if (memory_condition_ == next_condition)
+    return;
 
-  base::TimeTicks prev_last_state_change = last_state_change_;
-  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));
-  RecordStateChange(prev_state, next_state,

[bashi, 2017/02/27 06:53:58]

Most of UMAs recorded here weren't helpful. We will remove them.

-                    last_state_change_ - prev_last_state_change);
-  NotifyStateToClients();
-  NotifyStateToChildren();
-  return true;
+               "MemoryCoordinatorImpl::UpdateConditionIfNeeded", "prev",
+               MemoryConditionToString(prev_condition), "next",
+               MemoryConditionToString(next_condition));
+
+  // TODO(bashi): Record some metrics.
+
+  // TODO(bashi): Below actions are tentative. We might want to prioritize
+  // processes and handle them one-by-one.

[bashi, 2017/02/27 06:53:58]

I think we need to discuss what we should do here.

+  if (next_condition == MemoryCondition::NORMAL) {
+    NotifyStateToChildren(MemoryState::NORMAL);
+    UpdateCurrentMemoryState(MemoryState::NORMAL);
+  }
+  if (prev_condition == MemoryCondition::NORMAL &&
+      next_condition == MemoryCondition::WARNING) {
+    NotifyStateToChildren(MemoryState::THROTTLED);
+    // Idea: Purge memory from background processes.
+  }
+  if (prev_condition != MemoryCondition::CRITICAL &&
+      next_condition == MemoryCondition::CRITICAL) {
+    UpdateCurrentMemoryState(MemoryState::THROTTLED);
+    // Idea: Throttle memory allocation in foreground renderers.
+    // Idea: Start discarding tabs.
+  }
+}
+
+void MemoryCoordinatorImpl::UpdateCurrentMemoryState(
+    base::MemoryState memory_state) {
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (now - last_state_change_ < minimum_state_transition_period_)
+    return;
+  last_state_change_ = now;
+  current_state_ = memory_state;
+  NotifyStateToClients(memory_state);
 }
 
 void MemoryCoordinatorImpl::DiscardTab() {
@@ -385,13 +423,14 @@ 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();
+  auto new_state = GetMemoryCondtion() == 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;
@@ -415,20 +454,20 @@ void MemoryCoordinatorImpl::CreateChildInfoMapEntry(
   // corresponding renderer process is ready to communicate. Renderer processes
   // call AddChild() when they are ready.
   child_info.memory_state = MemoryState::NORMAL;
+  child_info.last_state_change = base::TimeTicks::Now();
   child_info.is_visible = true;
   child_info.handle = std::move(handle);
 }
 
-void MemoryCoordinatorImpl::NotifyStateToClients() {
-  auto state = GetCurrentMemoryState();
+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);
 }
 
 void MemoryCoordinatorImpl::RecordStateChange(MemoryState prev_state,