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..4f8ec557cfa59c0a435d3ff66b87d496a1a4d2d9 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,17 @@ 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";
+  }
+}
+
 }  // namespace
 
 // The implementation of MemoryCoordinatorHandle. See memory_coordinator.mojom
@@ -113,7 +126,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 +143,7 @@ void MemoryCoordinatorImpl::Start() {
   notification_registrar_.Add(
       this, NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
       NotificationService::AllBrowserContextsAndSources());
-  last_state_change_ = base::TimeTicks::Now();

[haraken, 2017/02/28 11:33:45]

Why can we remove this?

[bashi, 2017/03/02 04:01:45]

I slightly changed logic here. Instead of initializing |last_state_change_|
here, I added last_state_change_.is_null() check in UpdateCurrentMemoryState().
This allows us to update memory condition from the beginning.

-  state_updater_->ScheduleUpdateState(base::TimeDelta());
+  condition_observer_->ScheduleUpdateCondition(base::TimeDelta());
 }
 
 void MemoryCoordinatorImpl::CreateHandle(
@@ -158,7 +173,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 +203,22 @@ void MemoryCoordinatorImpl::RecordMemoryPressure(
   // TODO(bashi): Record memory pressure level.
 }
 
-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));
+  // Resets |last_state_change_| so that UpdateCurrentMemoryState() to set
+  // memory state forcibly.
+  last_state_change_ = base::TimeTicks();
+  UpdateCurrentMemoryState(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,
@@ -226,7 +233,10 @@ void MemoryCoordinatorImpl::Observe(int type,
   if (iter == children().end())
     return;
   iter->second.is_visible = *Details<bool>(details).ptr();
-  auto new_state = GetGlobalMemoryState();
+  MemoryState new_state = MemoryState::NORMAL;
+  if (!iter->second.is_visible &&

[haraken, 2017/02/28 11:33:45]

Add a comment about what this condition is doing.

[bashi, 2017/03/02 04:01:45]

Added comments. Also changed state calculation to make it consistent with
UpdateConditionIfNeeded().

+      GetMemoryCondition() != MemoryCondition::NORMAL)
+    new_state = MemoryState::THROTTLED;
   SetChildMemoryState(iter->first, new_state);
 }
 
@@ -236,7 +246,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);
@@ -246,22 +256,54 @@ 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) {

[haraken, 2017/02/28 11:33:45]

Add more comments on these branches.

[bashi, 2017/03/02 04:01:45]

Done.

+    NotifyStateToChildren(MemoryState::NORMAL);
+    UpdateCurrentMemoryState(MemoryState::NORMAL);
+  }
+  if (prev_condition == MemoryCondition::NORMAL &&

[haraken, 2017/02/28 11:33:45]

else if

[bashi, 2017/03/02 04:01:45]

Done.

+      next_condition == MemoryCondition::WARNING) {
+    // Set THROTTLED state to background processes.
+    for (auto& iter : children()) {
+      if (!iter.second.is_visible) {
+        SetChildMemoryState(iter.first, MemoryState::THROTTLED);
+      }
+    }

[haraken, 2017/02/28 11:33:45]

Are you intentionally not calling UpdateCurrentMemoryState?

[bashi, 2017/03/02 04:01:45]

Yes (I wanted to make this CL simpler), but agreed, we would want to do:

- UpdateCurrentMemoryState(NORMAL)
- Set NORMAL to foreground renderers

Changed conditions/actions.

+    // Idea: Purge memory from background processes.
+  }
+  if (prev_condition != MemoryCondition::CRITICAL &&

[haraken, 2017/02/28 11:33:45]

else if

[bashi, 2017/03/02 04:01:45]

Done.

+      next_condition == MemoryCondition::CRITICAL) {
+    UpdateCurrentMemoryState(MemoryState::THROTTLED);
+    NotifyStateToChildren(MemoryState::THROTTLED);
+    // Idea: Start discarding tabs.
+  }
+}
+
+void MemoryCoordinatorImpl::UpdateCurrentMemoryState(
+    base::MemoryState memory_state) {
+  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;
+  current_state_ = memory_state;
+  NotifyStateToClients(memory_state);
 }
 
 void MemoryCoordinatorImpl::DiscardTab() {
@@ -309,13 +351,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 = 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 +386,15 @@ void MemoryCoordinatorImpl::CreateChildInfoMapEntry(
   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);
 }
 
 MemoryCoordinatorImpl::ChildInfo::ChildInfo() {}