Fill in PlatformPrivateFootprint on Linux

components/tracing/common/process_metrics_memory_dump_provider.cc

Index: components/tracing/common/process_metrics_memory_dump_provider.cc
diff --git a/components/tracing/common/process_metrics_memory_dump_provider.cc b/components/tracing/common/process_metrics_memory_dump_provider.cc
index 3f2abde07f9c7143049f87ab7065c24e39efa490..718e1631e3c92bd32ee9df4b278e5e084898ed8d 100644
--- a/components/tracing/common/process_metrics_memory_dump_provider.cc
+++ b/components/tracing/common/process_metrics_memory_dump_provider.cc
@@ -180,15 +180,18 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file,
   return num_valid_regions;
 }
 
-bool GetResidentSizeFromStatmFile(int fd, uint64_t* resident_pages) {
+bool GetResidentAndSharedPagesFromStatmFile(int fd,
+                                            uint64_t* resident_pages,
+                                            uint64_t* shared_pages) {
   lseek(fd, 0, SEEK_SET);
   char line[kMaxLineSize];
   int res = read(fd, line, kMaxLineSize - 1);
   if (res <= 0)
     return false;
   line[res] = '\0';
-  int num_scanned = sscanf(line, "%*s %" SCNu64, resident_pages);
-  return num_scanned == 1;
+  int num_scanned =
+      sscanf(line, "%*s %" SCNu64 " %" SCNu64, resident_pages, shared_pages);
+  return num_scanned == 2;
 }
 
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
@@ -633,6 +636,28 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals(
 
   uint64_t peak_rss_bytes = 0;
 
+#if defined(OS_LINUX) || defined(OS_ANDROID)
+  base::trace_event::ProcessMemoryTotals::PlatformPrivateFootprint& footprint =

[Primiano Tucci (use gerrit), 2017/05/03 19:21:21]

const auto& footprint = ...?

[hjd, 2017/05/04 10:46:35]

After this the line is *exactly* 80 chars => Best review comment ever!

Although actually I think that it can't be const? We write to field below. :(

+      pmd->process_totals()->GetPlatformPrivateFootprint();
+
+  base::ScopedFD autoclose;
+  int statm_fd = fast_polling_statm_fd_.get();
+  if (statm_fd == -1) {
+    autoclose = OpenStatm();
+    statm_fd = autoclose.get();
+  }
+  if (statm_fd != -1) {

[Primiano Tucci (use gerrit), 2017/05/03 19:21:21]

I'd just return false if statm_Fd == -1  here. Given the importance I am not
sure we wanto silently ignore this failure.

[hjd, 2017/05/04 10:46:35]

Done.

+    static size_t page_size = base::GetPageSize();

[Primiano Tucci (use gerrit), 2017/05/03 19:21:22]

static const size_t kPageSize

[hjd, 2017/05/04 10:46:35]

Done.

+    uint64_t resident_pages;
+    uint64_t shared_pages;
+    bool success = GetResidentAndSharedPagesFromStatmFile(
+        statm_fd, &resident_pages, &shared_pages);
+    if (success) {

[Primiano Tucci (use gerrit), 2017/05/03 19:21:21]

ditto, if(!success) return false;

[hjd, 2017/05/04 10:46:35]

Done.

+      footprint.rss_anon_bytes = (resident_pages - shared_pages) * page_size;
+    }
+  }
+#endif  // defined(OS_LINUX)

[Primiano Tucci (use gerrit), 2017/05/03 19:21:21]

add a // TODO(hjd): implement swap in the next CL.

[hjd, 2017/05/04 10:46:35]

Done.

+
 #if !defined(OS_IOS)
   peak_rss_bytes = process_metrics_->GetPeakWorkingSetSize();
 #if defined(OS_LINUX) || defined(OS_ANDROID)
@@ -676,27 +701,34 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals(
   return true;
 }
 
+base::ScopedFD ProcessMetricsMemoryDumpProvider::OpenStatm() {
+  std::string name =
+      "/proc/" +
+      (process_ == base::kNullProcessId ? "self"
+                                        : base::IntToString(process_)) +
+      "/statm";
+  base::ScopedFD fd = base::ScopedFD(open(name.c_str(), O_RDONLY));
+  DCHECK(fd.is_valid());
+  return fd;
+}
+
 void ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal(
     uint64_t* memory_total) {
   *memory_total = 0;
 #if defined(OS_LINUX) || defined(OS_ANDROID)
+
   int statm_fd = fast_polling_statm_fd_for_testing;
-  if (statm_fd == -1) {
-    if (!fast_polling_statm_fd_.is_valid()) {
-      std::string name = "/proc/" + (process_ == base::kNullProcessId
-                                         ? "self"
-                                         : base::IntToString(process_)) +
-                         "/statm";
-      fast_polling_statm_fd_.reset(open(name.c_str(), O_RDONLY));
-      DCHECK(fast_polling_statm_fd_.is_valid());
-    }
+  if (!fast_polling_statm_fd_.is_valid())
+    fast_polling_statm_fd_ = OpenStatm();
+  if (statm_fd == -1)

[Primiano Tucci (use gerrit), 2017/05/03 19:21:21]

I think this is changing the semantic for the for_testing .
the previous code was first checking for invalidity of the statm_fd (which is
the for_testing) and then if -1 resorting to open

[hjd, 2017/05/04 10:46:35]

Done, thanks!

     statm_fd = fast_polling_statm_fd_.get();
-  }
   if (statm_fd == -1)
     return;
 
   uint64_t rss_pages = 0;
-  if (!GetResidentSizeFromStatmFile(statm_fd, &rss_pages))
+  uint64_t shared_pages = 0;
+  if (!GetResidentAndSharedPagesFromStatmFile(statm_fd, &rss_pages,
+                                              &shared_pages))
     return;
 
   static size_t page_size = base::GetPageSize();