Android systrace: Optimize memory dumps.

systrace/atrace_helper/jni/process_info.h

Index: systrace/atrace_helper/jni/process_info.h
diff --git a/systrace/atrace_helper/jni/process_info.h b/systrace/atrace_helper/jni/process_info.h
index ff05b334dd9d5efc1a6b584367c1834d1d903e47..64766fbb75e25e0663641ef268ea115b159e99ee 100644
--- a/systrace/atrace_helper/jni/process_info.h
+++ b/systrace/atrace_helper/jni/process_info.h
@@ -5,67 +5,91 @@
 #ifndef PROCESS_INFO_H_
 #define PROCESS_INFO_H_
 
+#include <functional>
 #include <map>
 #include <memory>
+#include <string>
 
 #include "process_memory_stats.h"
 
-// Reads various process stats and details from /proc/pid/.
-class ProcessInfo {
+// Contains time-indefinite process information from multiple procfs dumps.
+class PersistentProcessInfo {
  public:
   struct ThreadInfo {
-    char name[128] = {};
+    int tid;
+    std::string name;

[Primiano Tucci (use gerrit), 2017/06/22 07:55:52]

isn't this going to create all sort of (slower) heap allocations an movements?
Or are you using this class in a copiable/movable structure?

[kraynov, 2017/06/22 09:19:45]

It takes O(n_processes) to allocate all the strings which is quite small and
roughly constant overhead for ~hundreds of processes with ~tens of threads. In
case of ProcessInfo name field is using to query std::set which would be
inconvenient with char*. For consistency and simplicity I'd prefer to keep
std::string in these structures.

   };
-  using ThreadInfoMap = std::map<int, std::unique_ptr<ThreadInfo>>;
+  using ThreadMap = std::map<int, std::unique_ptr<ThreadInfo>>;
+
+  struct ProcessInfo {
+    int pid;
+    bool in_kernel;
+    bool is_app;
+    std::string name;
+    std::string exe;
+    ThreadMap threads;
+  };
+  using ProcessMap = std::map<int, std::unique_ptr<ProcessInfo>>;
 
-  // Returns true if |pid| is a process (|pid| == TGID), false if it's just a
-  // thread of another process, or if |pid| doesn't exist at all.
-  static bool IsProcess(int pid);
+ private:
+  bool UpdateProcessInfo(int pid);
 
-  explicit ProcessInfo(int pid);
+  ProcessMap processes_;
+  friend class ProcessDumpManager;
+};
 
-  bool ReadProcessName();
-  bool ReadThreadNames();
-  bool ReadOOMStats();
-  bool ReadPageFaultsAndCPUTimeStats();
+// Contains process information captured at specific moment.
+class InstantProcessInfo {
+ public:
+  struct ProcessSnapshot {
+    int pid;
+    ProcessMemoryStats memory;
+    // OOM badness and tolerance (oom_adj is deprecated).
+    int oom_score;
+    int oom_score_adj;
+    // Page faults.
+    unsigned long minor_faults;
+    unsigned long major_faults;
+    // Time spent in userspace and in the kernel.
+    unsigned long utime;
+    unsigned long stime;
+  };
+  using ProcessSnapshotMap =
+      std::map<int, std::unique_ptr<ProcessSnapshot>>;
 
-  ProcessMemoryStats* memory() { return &memory_; }
-  const ProcessMemoryStats* memory() const { return &memory_; }
-  const ThreadInfoMap* threads() const { return &threads_; }
-  const char* name() const { return name_; }
-  const char* exe() const { return exe_; }
+  uint64_t timestamp() const { return timestamp_; };
+  const ProcessSnapshotMap* processes() const { return &processes_; }
 
-  int oom_adj() const { return oom_adj_; }
-  int oom_score_adj() const { return oom_score_adj_; }
-  int oom_score() const { return oom_score_; }
+ private:
+  uint64_t timestamp_;
+  ProcessSnapshotMap processes_;
+  friend class ProcessDumpManager;
+};
 
-  unsigned long minflt() const { return minflt_; }
-  unsigned long majflt() const { return majflt_; }
-  unsigned long utime() const { return utime_; }
-  unsigned long stime() const { return stime_; }
-  unsigned long long start_time() const { return start_time_; }
+class ProcessDumpManager {
+ public:
+  using DumpPredicate =
+      std::function<bool(const PersistentProcessInfo::ProcessInfo*)>;
 
- private:
-  ProcessInfo(const ProcessInfo&) = delete;
-  void operator=(const ProcessInfo&) = delete;
+  void SetFullDumpPredicate(const DumpPredicate& predicate);

[Primiano Tucci (use gerrit), 2017/06/22 07:55:51]

honestly I don't see the need of passing a predicate just to do the filtering
outside. Why the filtering isn't here?

[kraynov, 2017/06/22 09:19:45]

The logic which process gets which dump is the scope of program itself, not a
module. However predicate for graphics dump is really overkill here because it's
quite fast to obtain and very simple rule for filtering. Have removed this
predicate.

[Primiano Tucci (use gerrit), 2017/06/26 10:07:19]

the dump predicate seems to be still here, but I thought you sad it was removed

+  void SetGraphicsDumpPredicate(const DumpPredicate& predicate);
 
-  ProcessMemoryStats memory_;
+  bool TakeSnapshot();
 
-  ThreadInfoMap threads_;
-  char name_[128] = {};
-  char exe_[128] = {};
+  const InstantProcessInfo* last_snapshot() const {
+    return &global_snapshot_;
+  }
 
-  int oom_adj_ = 0;
-  int oom_score_adj_ = 0;
-  int oom_score_ = 0;
+  const PersistentProcessInfo::ProcessMap* all_processes() const {
+    return &persistent_.processes_;
+  }
 
-  unsigned long minflt_ = 0;
-  unsigned long majflt_ = 0;
-  unsigned long utime_ = 0;            // CPU time in user mode.
-  unsigned long stime_ = 0;            // CPU time in kernel mode.
-  unsigned long long start_time_ = 0;  // CPU time in kernel mode.
+ private:
+  PersistentProcessInfo persistent_;
+  InstantProcessInfo global_snapshot_;
 
-  const int pid_;
+  std::unique_ptr<const DumpPredicate> full_dump_predicate_;
+  std::unique_ptr<const DumpPredicate> graphics_dump_predicate_;
 };
 
 #endif  // PROCESS_INFO_H_