Refactor ARC OOM handler code

chrome/browser/memory/tab_manager_delegate_chromeos.cc

Index: chrome/browser/memory/tab_manager_delegate_chromeos.cc
diff --git a/chrome/browser/memory/tab_manager_delegate_chromeos.cc b/chrome/browser/memory/tab_manager_delegate_chromeos.cc
index 08c658b78b0017b46e20c3477d171205cb0c940b..1c77920013ede10eb6bd6c3dcd7cf1b0c9d23d37 100644
--- a/chrome/browser/memory/tab_manager_delegate_chromeos.cc
+++ b/chrome/browser/memory/tab_manager_delegate_chromeos.cc
@@ -36,7 +36,6 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/arc_service_manager.h"
-#include "components/arc/common/process.mojom.h"
 #include "components/device_event_log/device_event_log.h"
 #include "components/exo/shell_surface.h"
 #include "content/public/browser/browser_thread.h"
@@ -86,12 +85,17 @@ bool IsArcMemoryManagementEnabled() {
 
 void OnSetOomScoreAdj(bool success, const std::string& output) {
   VLOG(2) << "OnSetOomScoreAdj " << success << " " << output;
-  if (!success || output != "")
-    LOG(WARNING) << "Set OOM score error: " << output;
+  if (!success)
+    LOG(ERROR) << "Set OOM score error: " << output;
+  else if (!output.empty())
+    LOG(WARNING) << "Set OOM score: " << output;
 }
 
 }  // namespace
 
+// static
+const int TabManagerDelegate::kLowestOomScore = -1000;
+
 std::ostream& operator<<(std::ostream& os, const ProcessType& type) {
   switch (type) {
     case ProcessType::FOCUSED_TAB:
@@ -155,10 +159,8 @@ ProcessType TabManagerDelegate::Candidate::GetProcessTypeInternal() const {
   if (app()) {
     if (app()->is_focused())
       return ProcessType::FOCUSED_APP;
-    if (app()->process_state() <=
-        arc::mojom::ProcessState::IMPORTANT_FOREGROUND) {
+    if (app()->IsUserVisible())
       return ProcessType::VISIBLE_APP;

[cylee1, 2017/05/10 16:31:55]

Let's rename it, it's no longer "visible app" but something like "important app"
or "non-killable app".

[Yusuke Sato, 2017/05/10 18:29:43]

Done.

-    }
     return ProcessType::BACKGROUND_APP;
   }
   if (tab()) {
@@ -531,19 +533,7 @@ TabManagerDelegate::GetSortedCandidates(
     candidates.emplace_back(&tab);
   }
 
-  // A special process on Android side which serves as a dummy "focused" app
-  // when the focused window is a Chrome side window (i.e., all Android
-  // processes are running in the background). We don't want to kill it anyway.
-  static constexpr char kArcInBackgroundDummyprocess[] =
-      "org.chromium.arc.home";
-
   for (const auto& app : arc_processes) {
-    // Skip persistent android processes since they should never be killed here.
-    // Neither do we set their OOM scores so their score remains minimum.
-    if (app.process_state() <= arc::mojom::ProcessState::PERSISTENT_UI ||
-        app.process_name() == kArcInBackgroundDummyprocess) {
-      continue;
-    }
     candidates.emplace_back(&app);
   }
 
@@ -609,12 +599,11 @@ void TabManagerDelegate::LowMemoryKillImpl(
                       << " KB";
     if (target_memory_to_free_kb <= 0)
       break;
-    // Never kill selected tab or Android foreground app, regardless whether
-    // they're in the active window. Since the user experience would be bad.
+    // Never kill selected tab, foreground app, and visible apps regardless of
+    // whether they're in the active window. Since the user experience would be
+    // bad.
     ProcessType process_type = it->process_type();
-    if (process_type == ProcessType::VISIBLE_APP ||
-        process_type == ProcessType::FOCUSED_APP ||
-        process_type == ProcessType::FOCUSED_TAB) {
+    if (process_type <= ProcessType::VISIBLE_APP) {
       MEMORY_LOG(ERROR) << "Skipped killing " << *it;
       continue;
     }
@@ -660,8 +649,20 @@ void TabManagerDelegate::LowMemoryKillImpl(
 void TabManagerDelegate::AdjustOomPrioritiesImpl(
     const TabStatsList& tab_list,
     const std::vector<arc::ArcProcess>& arc_processes) {
+  std::vector<TabManagerDelegate::Candidate> candidates;
+  std::vector<TabManagerDelegate::Candidate> apps_non_killable;
+
   // Least important first.
-  const auto candidates = GetSortedCandidates(tab_list, arc_processes);
+  auto all_candidates = GetSortedCandidates(tab_list, arc_processes);
+  for (auto& candidate : all_candidates) {

[cylee1, 2017/05/10 16:31:55]

const auto ?

[Yusuke Sato, 2017/05/10 18:29:43]

This is non-const for the std::move calls.

[cylee1, 2017/05/11 08:59:53]

Acknowledged.

+    if (!candidate.app() || candidate.app()->IsKernelKillable()) {
+      // Add tabs and killable apps to |candidates|.
+      candidates.emplace_back(std::move(candidate));
+    } else {
+      // Add non-killable apps to |apps_non_killable|.
+      apps_non_killable.emplace_back(std::move(candidate));
+    }
+  }
 
   // Now we assign priorities based on the sorted list. We're assigning
   // priorities in the range of kLowestRendererOomScore to
@@ -691,6 +692,10 @@ void TabManagerDelegate::AdjustOomPrioritiesImpl(
 
   ProcessScoreMap new_map;
 
+  // Make the apps non-killable.
+  if (!apps_non_killable.empty())
+    SetOomScore(apps_non_killable, kLowestOomScore, &new_map);
+
   // Higher priority part.
   DistributeOomScoreInRange(candidates.begin(), lower_priority_part,
                             chrome::kLowestRendererOomScore, range_middle,
@@ -698,9 +703,18 @@ void TabManagerDelegate::AdjustOomPrioritiesImpl(
   // Lower priority part.
   DistributeOomScoreInRange(lower_priority_part, candidates.end(), range_middle,
                             chrome::kHighestRendererOomScore, &new_map);
+
   oom_score_map_.swap(new_map);
 }
 
+void TabManagerDelegate::SetOomScore(
+    const std::vector<TabManagerDelegate::Candidate>& candidates,
+    int score,
+    ProcessScoreMap* new_map) {
+  DistributeOomScoreInRange(candidates.begin(), candidates.end(), score - 1,

[cylee1, 2017/05/10 16:31:55]

I'm confused: why is it score -1 ? 

[Yusuke Sato, 2017/05/10 18:29:43]

This is because of the rounding code at L737. Added a comment.

[cylee1, 2017/05/11 08:59:53]

Ah it's because of the round down of "negative" numbers. I didn't handle it
nicely then.
How about using math functions like "round()" at L737 to handle it better for
both positive and negative values ?

Even better, we can avoid floating point arithmetic by using something like
range / num + (index < (range % num))
trick ?  Just some random thought. 

[Yusuke Sato, 2017/05/11 18:50:16]

Done.

Let me use round as this seems easier to read for everyone.

+                            score - 1, new_map);
+}
+
 void TabManagerDelegate::DistributeOomScoreInRange(
     std::vector<TabManagerDelegate::Candidate>::const_iterator begin,
     std::vector<TabManagerDelegate::Candidate>::const_iterator end,