TabManagerDelegate: Better prioritize ARC processes.

chrome/browser/memory/tab_manager_delegate_chromeos.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_MEMORY_TAB_MANAGER_DELEGATE_CHROMEOS_H_
 #define CHROME_BROWSER_MEMORY_TAB_MANAGER_DELEGATE_CHROMEOS_H_
 
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/containers/hash_tables.h"
 #include "base/gtest_prod_util.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/process/process.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/arc/arc_process.h"
 #include "chrome/browser/memory/tab_manager.h"
 #include "chrome/browser/memory/tab_stats.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "components/arc/arc_bridge_service.h"
+#include "components/arc/common/process.mojom.h"
 #include "components/arc/instance_holder.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "ui/wm/public/activation_change_observer.h"
 
 namespace memory {
 
-// The importance of tabs/apps. The lower the value, the more likely a process
-// is to be selected on memory pressure. The values is additive, for example,
-// one tab could be of value (CHROME_NORMAL | CHROME_PINNED).
-// TODO(cylee): Refactor this CL so the prioritize logic is unified in
-// TabManager.
-enum ProcessPriority {
-  // Processes on Android side which generally don't have an app window, and
-  // possibly be auto relaunched if killed.
-  ANDROID_BACKGROUND = 1,
-  ANDROID_SERVICE = 1 << 1,
-  ANDROID_CANT_SAVE_STATE = 1 << 2,
-  ANDROID_PERCEPTIBLE = 1 << 3,
-  ANDROID_VISIBLE = 1 << 4,
-  ANDROID_TOP_SLEEPING = 1 << 5,
-  ANDROID_FOREGROUND_SERVICE = 1 << 6,
-  ANDROID_FOREGROUND = 1 << 7,
-  // A chrome window can be one of the 3 exclusive types below:
-  // internal page, normal page, or chrome app.
-  CHROME_INTERNAL = 1 << 8,
-  CHROME_NORMAL = 1 << 9,
-  CHROME_APP = 1 << 10,
-  // An android app which is on top of screen from Android's point of view,
-  // but the app window is inactive from Chrome OS's point of view.
-  // Give it a higher priority then normal chrome tab since it could not be
-  // reloaded if killed.
-  ANDROID_TOP_INACTIVE = CHROME_APP,
-  // A chrome tab could have following 3 additional attributes
-  // (not exclusive).
-  CHROME_PINNED = 1 << 11,
-  CHROME_MEDIA = 1 << 12,
-  CHROME_CANT_SAVE_STATE = 1 << 13,
-  // The highest level of priority. Either a selected tab or an Android app in
-  // an active window. In theory there should be at most one process with this
-  // priority at a time, but at the time of writing Chrome couldn't get Android
-  // window stack info yet so there may be multiple Android apps be token as on
-  // top of screen for now.
-  CHROME_SELECTED = 1 << 14,
-  ANDROID_TOP = CHROME_SELECTED,
+// Possible types of Apps/Tabs processes. From most important to least
+// important.
+enum ProcessType {

[Yusuke Sato, 2016/07/15 00:55:05]

enum class

?

[cylee1, 2016/07/15 17:31:13]

Yes I can make it so, at the expense of overloading operator<< because I cout <<
process_type somewhere. 
Or I'll encounter the error
http://stackoverflow.com/questions/11421432/how-can-i-output-the-value-of-an-...
which I need to use underlying_type to resolve otherwise.

[Luis Héctor Chávez, 2016/07/15 18:10:06]

I think it's fine to overload operator<<. You are already doing it for Candidate
and it will improve readability of logs.

[cylee1, 2016/07/20 17:57:53]

ok agree.

+  // There can be only one process having process type "FOCUSED_APP"
+  // or "FOCUSED_TAB" in the system at any give time (i.e., The focused window
+  // could be either a chrome window or an Android app. But not both.
+  FOCUSED_APP = 1,
+  FOCUSED_TAB = FOCUSED_APP,
 
-  ANDROID_PERSISTENT = 1 << 30,
-  ANDROID_NON_EXISTS = 0x7FFFFFFF
+  VISIBLE_APP = 2,
+  BACKGROUND_TAB = 3,
+  BACKGROUND_APP = 4,
+  UNKNOWN_TYPE = 5,
 };
 
 // The Chrome OS TabManagerDelegate is responsible for keeping the
 // renderers' scores up to date in /proc/<pid>/oom_score_adj.
 //
 // Note that AdjustOomPriorities will be called on the UI thread by
 // TabManager, but the actual work will take place on the file thread
 // (see implementation of AdjustOomPriorities).
 class TabManagerDelegate
     : public arc::InstanceHolder<arc::mojom::ProcessInstance>::Observer,
@@ skipping 46 matching lines @@
 
   // Kills a tab. Returns true if the tab is killed successfully.
   // Virtual for unit testing.
   virtual bool KillTab(int64_t tab_id);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, CandidatesSorted);
   FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, KillMultipleProcesses);
   FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, SetOomScoreAdj);
 
-  struct Candidate;
+  class Candidate;
   class FocusedProcess;
   class UmaReporter;
 
   friend std::ostream& operator<<(std::ostream& out,
                                   const Candidate& candidate);
 
   // content::NotificationObserver:
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
   // Pair to hold child process host id and ProcessHandle.
   typedef std::pair<int, base::ProcessHandle> ProcessInfo;
 
   // Cache OOM scores in memory.
   typedef base::hash_map<base::ProcessHandle, int> ProcessScoreMap;
 
-  // Get the list of candidates to kill, sorted by reversed importance.
+  // Get the list of candidates to kill, sorted by importance descendingly.

[Luis Héctor Chávez, 2016/07/15 01:29:24]

nit: descending importance

[cylee1, 2016/07/15 17:31:13]

Done.

   static std::vector<Candidate> GetSortedCandidates(
       const TabStatsList& tab_list,
       const std::vector<arc::ArcProcess>& arc_processes);
 
   // Posts AdjustFocusedTabScore task to the file thread.
   void OnFocusTabScoreAdjustmentTimeout();
 
   // Kills a process after getting all info of tabs and apps.
   void LowMemoryKillImpl(const TabStatsList& tab_list,
                          const std::vector<arc::ArcProcess>& arc_processes);
@@ skipping 17 matching lines @@
   void SetOomScoreAdjForApp(int nspid, int score);
 
   // Sets oom_score_adj for a list of tabs on the file thread.
   void SetOomScoreAdjForTabsOnFileThread(
       const std::vector<std::pair<base::ProcessHandle, int>>& entries);
 
   // Sets OOM score for processes in the range [|rbegin|, |rend|) to integers
   // distributed evenly in [|range_begin|, |range_end|).
   // The new score is set in |new_map|.
   void DistributeOomScoreInRange(
-      std::vector<TabManagerDelegate::Candidate>::reverse_iterator rbegin,
-      std::vector<TabManagerDelegate::Candidate>::reverse_iterator rend,
+      std::vector<TabManagerDelegate::Candidate>::const_iterator begin,
+      std::vector<TabManagerDelegate::Candidate>::const_iterator end,
       int range_begin,
       int range_end,
       ProcessScoreMap* new_map);
 
   // Initiates an oom priority adjustment.
   void ScheduleEarlyOomPrioritiesAdjustment();
 
   // Holds a reference to the owning TabManager.
   const base::WeakPtr<TabManager> tab_manager_;
 
@@ skipping 24 matching lines @@
   // Reports UMA histograms.
   std::unique_ptr<UmaReporter> uma_;
 
   // Weak pointer factory used for posting tasks to other threads.
   base::WeakPtrFactory<TabManagerDelegate> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TabManagerDelegate);
 };
 
 // On ARC enabled machines, either a tab or an app could be a possible
-// victim of low memory kill process. This is a helper struct which holds a
+// victim of low memory kill process. This is a helper class which holds a
 // pointer to an app or a tab (but not both) to facilitate prioritizing the
 // victims.
-struct TabManagerDelegate::Candidate {
-  Candidate(const TabStats* _tab, int _priority)
-      : tab(_tab), priority(_priority), is_arc_app(false) {}
-  Candidate(const arc::ArcProcess* _app, int _priority)
-      : app(_app), priority(_priority), is_arc_app(true) {}
+class TabManagerDelegate::Candidate {
+ public:
+  explicit Candidate(const TabStats* tab)
+      : tab_(tab), app_(nullptr), process_type_(GetProcessType()) {
+  }

[Yusuke Sato, 2016/07/15 00:55:05]

DCHECK(tab_)?

[cylee1, 2016/07/15 17:31:13]

Done.

+  explicit Candidate(const arc::ArcProcess* app)
+      : tab_(nullptr), app_(app), process_type_(GetProcessType()) {
+  }

[Yusuke Sato, 2016/07/15 00:55:05]

DCHECK(app_)?

[cylee1, 2016/07/15 17:31:13]

Done.

 
-  bool operator<(const Candidate& rhs) const { return priority < rhs.priority; }
+  // Higher priority first.
+  bool operator<(const Candidate& rhs) const {

[Yusuke Sato, 2016/07/15 00:55:05]

Can you move the implementation to .cc? The implementation is not longer
one-liner.

[cylee1, 2016/07/15 17:31:13]

Done.

+    if (process_type() != rhs.process_type())
+      return process_type() < rhs.process_type();
+    if (app() && rhs.app())
+      // Sort by (process_state, last_activity_time) pair.
+      // Smaller process_state value means higher priority as defined in
+      // Android.
+      // Larger last_activity_time means more recently used.
+      return std::make_pair(app()->process_state(),

[Yusuke Sato, 2016/07/15 00:55:05]

What about adding CompareAppStats method to the app class (just like line 239?)

[cylee1, 2016/07/15 17:31:13]

Moved to arc_process.cc . 

+                            -app()->last_activity_time()) <
+          std::make_pair(rhs.app()->process_state(),
+                         -rhs.app()->last_activity_time());
+    if (tab() && rhs.tab())
+      return TabManager::CompareTabStats(*tab(), *rhs.tab());
+    // Impossible case. If app and tab are mixed in one process type, favor
+    // apps.
+    NOTREACHED() << "Undefined comparison between apps and tabs";
+    return app();
+  }
 
-  union {
-    const TabStats* tab;
-    const arc::ArcProcess* app;
-  };
-  int priority;
-  bool is_arc_app;
+
+  const TabStats* tab() const { return tab_; }
+  const arc::ArcProcess* app() const { return app_; }
+  ProcessType process_type() const { return process_type_; }
+
+ private:
+  ProcessType GetProcessType() {

[Yusuke Sato, 2016/07/15 00:55:05]

* This can be const.
* Or better, can you make this 'static ProcessType GetProcessType(const
TabStats* tab, const arc::ArcProcess* app);'? Having process_type() and
GetProcessType() with the same signature is very confusing to me.

[cylee1, 2016/07/15 17:31:13]

Can I just use a const ? Passing all its member so another static function
doesn't look scalable to me.
(Imagine if process type depends on other members in the future)

[Luis Héctor Chávez, 2016/07/15 18:10:06]

Can you at the very least call this GetProcessTypeInternal()? I'll defer the
final decision on this to yusukes@.

[Yusuke Sato, 2016/07/19 01:31:36]

Yeah, please rename it then.

[cylee1, 2016/07/20 17:57:53]

Done.

[cylee1, 2016/07/20 17:57:53]

Done.

+    if (app()) {
+      if (app()->is_focused())
+        return ProcessType::FOCUSED_APP;
+      if (app()->process_state() == arc::mojom::ProcessState::TOP)
+        return ProcessType::VISIBLE_APP;
+      return ProcessType::BACKGROUND_APP;
+    } else if (tab()) {
+      if (tab()->is_selected)
+        return ProcessType::FOCUSED_TAB;
+      return ProcessType::BACKGROUND_TAB;
+    }
+    NOTREACHED() << "Unexpected process type";
+    return ProcessType::UNKNOWN_TYPE;
+  }
+
+  const TabStats* tab_;
+  const arc::ArcProcess* app_;
+  ProcessType process_type_;

[Yusuke Sato, 2016/07/15 00:55:05]

can be const too.

[cylee1, 2016/07/15 17:31:13]

Because this class must allow copy and assign (used in STL vector, see below),
it should not be defined const. 
http://stackoverflow.com/questions/15030972/type-non-static-const-member-cant...

 };

[Yusuke Sato, 2016/07/15 00:55:05]

DISALLOW_COPY_AND_ASSIGN is missing.

[cylee1, 2016/07/15 17:31:13]

This is used in a STL vector so it needs to allow copy and assign

[Luis Héctor Chávez, 2016/07/15 18:10:06]

non-copyable classes can be used in STL vectors. The only caveat is that you are
using std::sort(), which needs an explicit move-assignment constructor. If you
add

Candidate(Candidate&&) = default;
Candidate& operator=(Candidate&& other) {
  // This should go in the .cc.
  tab_ = std::move(other.tab_);
  app_ = std::move(other.app_);
  process_type_ = std::move(other.process_type_);
  return *this;
}

you should be fine. Due to the |process_type_| assignment, it indeed cannot be
const.

[cylee1, 2016/07/20 17:57:53]

OK you're right. 
But do I need move() here given those members are primitive types??
  tab_ = std::move(other.tab_); 
could be just 
  tab_ = other.tab_ ;
?

After thinking it again, I don't get the point of making the class move-only. I
mean conceptually it makes sense to be copiable. It's unlike type like uniq_ptr
which needs to manage ownership/ Also it's not more efficiency because the
suggested move assignment basically just does what the default copy assignment
does. How about just make it copiable ?

[cylee1, 2016/07/22 22:18:43]

I decided to drop the DISALLOW_COPY_AND_ASSIGN line and make the class copiable.
Let me know if you object to this change. I can fix it in another CL.

[Luis Héctor Chávez, 2016/07/22 22:30:31]

It should be slightly more efficient to make it non-copyable since you can do
more performant swaps in std::sort. What was your motivation to drop it? The
style guide suggests that most classes should _not_ be copiable:
https://chromium-cpp.appspot.com/

[cylee1, 2016/07/22 23:14:40]

Per IM discussion, reverted back to move assignment for now.
But to be honest I still doubt about the in-place swap part . How can be a swap
be done in-place by move semantic?
Isn't you still need something like 
tmp = std::move(a);
a = std::move(b);
b = std::move(tmp); 
? 
I can understand implementing move by swap though (and it's a common practice)

 
 // A thin wrapper over library process_metric.h to get memory status so unit
 // test get a chance to mock out.
 class TabManagerDelegate::MemoryStat {
  public:
   MemoryStat() {}
   virtual ~MemoryStat() {}
 
   // Returns target size of memory to free given current memory pressure and
   // pre-configured low memory margin.
   virtual int TargetMemoryToFreeKB();
 
   // Returns estimated memory to be freed if the process |pid| is killed.
   virtual int EstimatedMemoryFreedKB(base::ProcessHandle pid);
 
  private:
   // Returns the low memory margin system config. Low memory condition is
   // reported if available memory is under the number.
   static int LowMemoryMarginKB();
 
   // Reads in an integer.
   static int ReadIntFromFile(const char* file_name, int default_val);
 };
 
 }  // namespace memory
 
 #endif  // CHROME_BROWSER_MEMORY_TAB_MANAGER_DELEGATE_CHROMEOS_H_