chromeos: Set a least score to the currently focused tab so that it is not selected

chrome/browser/oom_priority_manager.cc

Index: chrome/browser/oom_priority_manager.cc
===================================================================
--- chrome/browser/oom_priority_manager.cc	(revision 101722)
+++ chrome/browser/oom_priority_manager.cc	(working copy)
@@ -20,6 +20,7 @@
 #include "chrome/common/chrome_constants.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/renderer_host/render_process_host.h"
+#include "content/browser/renderer_host/render_widget_host.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/browser/zygote_host_linux.h"
 #include "content/common/notification_service.h"
@@ -58,7 +59,8 @@
   return Singleton<OomPriorityManager>::get();
 }
 
-OomPriorityManager::OomPriorityManager() {
+OomPriorityManager::OomPriorityManager()
+  : last_used_score_(chrome::kLowestRendererOomScore) {
   renderer_stats_.reserve(32);  // 99% of users have < 30 tabs open
   registrar_.Add(this,
       content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
@@ -66,6 +68,9 @@
   registrar_.Add(this,
       content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
       NotificationService::AllSources());
+  registrar_.Add(this,
+      content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
+      NotificationService::AllSources());
 }
 
 OomPriorityManager::~OomPriorityManager() {
@@ -124,6 +129,16 @@
   return first.memory_used < second.memory_used;
 }
 
+void OomPriorityManager::AdjustTabScore(base::ProcessHandle pid) {
+  base::AutoLock pid_to_oom_score_autolock(pid_to_oom_score_lock_);
+  // Use a score 1 less than the last used score. A lesser value ensures

[James Cook, 2011/09/19 17:08:55]

I don't think this does what you want.  oom_score_adj is an adjustment to the
kernel's internal OOM score for a process - it gets added in.  The kernel's
portion is usually 100-200 points, depending on the process size, runtime, etc. 
So just adjusting by a single point won't necessarily change the order.

Would it work to just set the freshly-visible tab to kLowestRendererOomScore?

[amruthraj, 2011/09/19 17:17:05]

The problem with this approach is that when a user does a Ctrl-Tab-and-Hold,
thereby switching all tabs in very short time, all of them get set to 300. That
is the reason I chose the decrement approach.
Is it fine to have another segment in the scores, something like 300-400 for
tabs switching case and 400-1000 in DoAdjustOomPriorities case?

[Greg Spencer (Chromium), 2011/09/19 19:45:51]

I agree with James that this isn't really what you want.  The reason we
distribute the values over the range is to make sure that we provide the
strongest signal possible to the kernel about the order, but it is still a
"suggestion" as far as the kernel is concerned, and making them only one value
apart is basically setting them equal (e.g. 1% difference in memory use can
cause them to happen in a different order than desired).

As for the Ctrl-TAB-and-hold issue, I don't think they all get set to 300. 
Basically, it drops out the "last clicked" time from the sort, but doesn't
prevent the currently displayed tab or pinned tab or smaller tab from being
elevated in priority: they'll still get spread out along the range based on the
remaining criterion.

This is also an edge case: not many people will hold down ctrl-tab.  If we see
this being a problem, then we can add a timer before setting the last selection
time so that it won't get set unless you view the tab for longer than 500ms or
something (in which case the user really is visiting the tab).

[amruthraj, 2011/09/20 10:07:54]

Done.

+  // that the currently focused tab doesn't get killed.
+  int score = --last_used_score_;
+  ZygoteHost::GetInstance()->AdjustRendererOOMScore(
+      pid, score);
+  pid_to_oom_score_[pid] = score;
+}
+
 void OomPriorityManager::Observe(int type, const NotificationSource& source,
                                     const NotificationDetails& details) {
   base::ProcessHandle handle = 0;
@@ -135,6 +150,18 @@
       pid_to_oom_score_.erase(handle);
       break;
     }
+    case content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED: {

[James Cook, 2011/09/19 17:08:55]

Does this fire just when the widget becomes visible, or also when it goes
invisible?

[amruthraj, 2011/09/19 17:17:05]

It gets fired in invisible case too. But the if condition next line checks for
only visibility case.

[amruthraj, 2011/09/20 10:07:54]

Added a variable visible here.

+      if (*Details<bool>(details).ptr()) {
+        base::ProcessHandle pid =
+            Source<RenderWidgetHost>(source).ptr()->process()->GetHandle();
+        // Post a delayed task so that the tab switch time doesn't get effected.

[Greg Spencer (Chromium), 2011/09/19 19:45:51]

effected -> affected

[amruthraj, 2011/09/20 10:07:54]

Done.

+        BrowserThread::PostDelayedTask(

[Greg Spencer (Chromium), 2011/09/19 19:45:51]

This doesn't replace previous tasks in the queue, and for your ctrl-tab-and-hold
case (admittedly a corner), will queue up a bunch of adjustments at once.  I'd
suggest using a timer to trigger the adjustment, and just resetting the timer
here.

[amruthraj, 2011/09/20 10:07:54]

Done.

+            BrowserThread::FILE, FROM_HERE,
+            NewRunnableMethod(this, &OomPriorityManager::AdjustTabScore, pid),
+            200);

[James Cook, 2011/09/19 17:08:55]

200 should be a named constant

[amruthraj, 2011/09/20 10:07:54]

Done.

+      }
+      break;
+    }
     default:
       NOTREACHED() << L"Received unexpected notification";
       break;
@@ -231,22 +258,19 @@
   // (least likely to be killed) of the duplicates, so that a
   // particular renderer process takes on the oom_score_adj of the
   // least likely tab to be killed.
-  const int kPriorityRange = chrome::kHighestRendererOomScore -
-                             chrome::kLowestRendererOomScore;
-  float priority_increment =
-      static_cast<float>(kPriorityRange) / renderer_stats_.size();
-  float priority = chrome::kLowestRendererOomScore;
+  float priority_increment = -1;

[James Cook, 2011/09/19 17:08:55]

I think the increment here needs to be much larger, like it was before.

[amruthraj, 2011/09/20 10:07:54]

I reverted all the changes here.

+  int priority = chrome::kHighestRendererOomScore;
   std::set<base::ProcessHandle> already_seen;
   int score = 0;
   ProcessScoreMap::iterator it;
-  for (StatsList::iterator iterator = renderer_stats_.begin();
-       iterator != renderer_stats_.end(); ++iterator) {
+  for (StatsList::reverse_iterator iterator = renderer_stats_.rbegin();
+       iterator != renderer_stats_.rend(); ++iterator) {
     if (already_seen.find(iterator->renderer_handle) == already_seen.end()) {
       already_seen.insert(iterator->renderer_handle);
+      score = std::max(chrome::kLowestRendererOomScore, priority);
+      it = pid_to_oom_score_.find(iterator->renderer_handle);
       // If a process has the same score as the newly calculated value,
       // do not set it.
-      score = static_cast<int>(priority + 0.5f);
-      it = pid_to_oom_score_.find(iterator->renderer_handle);
       if (it == pid_to_oom_score_.end() || it->second != score) {
         ZygoteHost::GetInstance()->AdjustRendererOOMScore(
             iterator->renderer_handle, score);
@@ -255,6 +279,7 @@
       priority += priority_increment;
     }
   }
+  last_used_score_ = score;
 }
 
 }  // namespace browser