[tracing] Dump child processes' memory metrics in browser

components/tracing/process_metrics_memory_dump_provider_linux.cc

Index: components/tracing/process_metrics_memory_dump_provider_linux.cc
diff --git a/base/trace_event/process_memory_maps_dump_provider.cc b/components/tracing/process_metrics_memory_dump_provider_linux.cc
similarity index 54%
rename from base/trace_event/process_memory_maps_dump_provider.cc
rename to components/tracing/process_metrics_memory_dump_provider_linux.cc
index 4c3959fe9be57f861777fb968a5704142efe66f0..2b14c1f87b2d83b7b578e2d752e318e7b070bee1 100644
--- a/base/trace_event/process_memory_maps_dump_provider.cc
+++ b/components/tracing/process_metrics_memory_dump_provider_linux.cc
@@ -2,29 +2,32 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/trace_event/process_memory_maps_dump_provider.h"
+#include "components/tracing/process_metrics_memory_dump_provider.h"
 
+#include <fcntl.h>
 #include <stdint.h>
 
+#include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/process/process_metrics.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/process_memory_maps.h"
 
-namespace base {
-namespace trace_event {
-
-// static
-FILE* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
+namespace tracing {
 
 namespace {
 
+bool kernel_supports_rss_peak_reset = true;
+const char kClearPeakRssCommand[] = "5";
+
 const uint32_t kMaxLineSize = 4096;
 
 bool ParseSmapsHeader(const char* header_line,
-                      ProcessMemoryMaps::VMRegion* region) {
+                      base::trace_event::ProcessMemoryMaps::VMRegion* region) {
   // e.g., "00400000-00421000 r-xp 00000000 fc:01 1234  /foo.so\n"
   bool res = true;  // Whether this region should be appended or skipped.
   uint64_t end_addr = 0;
@@ -47,19 +50,20 @@ bool ParseSmapsHeader(const char* header_line,
   region->protection_flags = 0;
   if (protection_flags[0] == 'r') {
     region->protection_flags |=
-        ProcessMemoryMaps::VMRegion::kProtectionFlagsRead;
+        base::trace_event::ProcessMemoryMaps::VMRegion::kProtectionFlagsRead;
   }
   if (protection_flags[1] == 'w') {
     region->protection_flags |=
-        ProcessMemoryMaps::VMRegion::kProtectionFlagsWrite;
+        base::trace_event::ProcessMemoryMaps::VMRegion::kProtectionFlagsWrite;
   }
   if (protection_flags[2] == 'x') {
     region->protection_flags |=
-        ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
+        base::trace_event::ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
   }
 
   region->mapped_file = mapped_file;
-  TrimWhitespaceASCII(region->mapped_file, TRIM_ALL, &region->mapped_file);
+  base::TrimWhitespaceASCII(region->mapped_file, base::TRIM_ALL,
+                            &region->mapped_file);
 
   return res;
 }
@@ -67,17 +71,18 @@ bool ParseSmapsHeader(const char* header_line,
 uint64_t ReadCounterBytes(char* counter_line) {
   uint64_t counter_value = 0;
   int res = sscanf(counter_line, "%*s %" SCNu64 " kB", &counter_value);
-  DCHECK_EQ(1, res);
-  return counter_value * 1024;
+  return res == 1 ? counter_value * 1024 : 0;
 }
 
-uint32_t ParseSmapsCounter(char* counter_line,
-                           ProcessMemoryMaps::VMRegion* region) {
+uint32_t ParseSmapsCounter(
+    char* counter_line,
+    base::trace_event::ProcessMemoryMaps::VMRegion* region) {
   // A smaps counter lines looks as follows: "RSS:  0 Kb\n"
   uint32_t res = 1;
   char counter_name[20];
   int did_read = sscanf(counter_line, "%19[^\n ]", counter_name);
-  DCHECK_EQ(1, did_read);
+  if (did_read != 1)

[Primiano Tucci (use gerrit), 2016/01/05 11:55:17]

out of curiosity. did you encounter cases where this fails?

[ssid, 2016/01/06 20:26:33]

Not this case, I removed to be safe. 

+    return 0;
 
   if (strcmp(counter_name, "Pss:") == 0) {
     region->byte_stats_proportional_resident = ReadCounterBytes(counter_line);
@@ -98,7 +103,8 @@ uint32_t ParseSmapsCounter(char* counter_line,
   return res;
 }
 
-uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file, ProcessMemoryMaps* pmm) {
+uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file,
+                                base::trace_event::ProcessMemoryMaps* pmm) {
   if (!smaps_file)
     return 0;
 
@@ -108,15 +114,14 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file, ProcessMemoryMaps* pmm) {
   const uint32_t kNumExpectedCountersPerRegion = 6;
   uint32_t counters_parsed_for_current_region = 0;
   uint32_t num_valid_regions = 0;
-  ProcessMemoryMaps::VMRegion region;
+  base::trace_event::ProcessMemoryMaps::VMRegion region;
   bool should_add_current_region = false;
   for (;;) {
     line[0] = '\0';
-    if (fgets(line, kMaxLineSize, smaps_file) == nullptr)
+    if (fgets(line, kMaxLineSize, smaps_file) == nullptr || !strlen(line))
       break;
-    DCHECK_GT(strlen(line), 0u);
     if (isxdigit(line[0]) && !isupper(line[0])) {
-      region = ProcessMemoryMaps::VMRegion();
+      region = base::trace_event::ProcessMemoryMaps::VMRegion();
       counters_parsed_for_current_region = 0;
       should_add_current_region = ParseSmapsHeader(line, &region);
     } else {
@@ -138,39 +143,65 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file, ProcessMemoryMaps* pmm) {
 }  // namespace
 
 // static
-ProcessMemoryMapsDumpProvider* ProcessMemoryMapsDumpProvider::GetInstance() {
-  return Singleton<ProcessMemoryMapsDumpProvider,
-                   LeakySingletonTraits<ProcessMemoryMapsDumpProvider>>::get();
-}
+FILE* ProcessMetricsMemoryDumpProvider::proc_smaps_for_testing = nullptr;
 
-ProcessMemoryMapsDumpProvider::ProcessMemoryMapsDumpProvider() {
-}
+bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals(

[Primiano Tucci (use gerrit), 2016/01/05 11:55:17]

to be honest, this function doesn't seem that much different  (% the kernel rss
support) than the base one.
Maybe you should just make this a single one #ifdef-ing the rss part?
Or am I missing something else?

[ssid, 2016/01/06 20:26:33]

Yes, I had made 2 files because i didn't want one large file. Also if mac memory
maps is added it should be split again.
Feels like this file is getting dirty with if-defs.

+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  const uint64_t rss_bytes = rss_bytes_for_testing
+                                 ? rss_bytes_for_testing
+                                 : process_metrics_->GetWorkingSetSize();
+
+  // rss_bytes will be 0 if the process ended while dumping.
+  if (!rss_bytes)
+    return false;
 
-ProcessMemoryMapsDumpProvider::~ProcessMemoryMapsDumpProvider() {
+  uint64_t peak_rss_bytes = 0;
+  peak_rss_bytes = process_metrics_->GetPeakWorkingSetSize();
+
+  // This file cannot be wriiten for other processes. So, do not try.

[Primiano Tucci (use gerrit), 2016/01/05 11:55:17]

I'd reword this as:
Due to UID isolation, clear_refs can only be written by the process itself and
cannot be used in the cases of a process dumping on behalf of another one.

[ssid, 2016/01/06 20:26:33]

Done.

+  if (kernel_supports_rss_peak_reset && process_ == base::kNullProcessHandle) {
+    int clear_refs_fd = open("/proc/self/clear_refs", O_WRONLY);
+    if (clear_refs_fd > 0 &&
+        base::WriteFileDescriptor(clear_refs_fd, kClearPeakRssCommand,
+                                  sizeof(kClearPeakRssCommand))) {
+      pmd->process_totals()->set_is_peak_rss_resetable(true);
+    } else {
+      kernel_supports_rss_peak_reset = false;
+    }
+    close(clear_refs_fd);
+  }
+
+  pmd->process_totals()->set_resident_set_bytes(rss_bytes);
+  pmd->process_totals()->set_peak_resident_set_bytes(peak_rss_bytes);
+  pmd->set_has_process_totals();
+  // Returns true even if other metrics failed, since rss is reported.
+  return true;
 }
 
-// Called at trace dump point time. Creates a snapshot of the memory maps for
-// the current process.
-bool ProcessMemoryMapsDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
-                                                 ProcessMemoryDump* pmd) {
-  // Snapshot of memory maps is not taken for light dump requests.
-  if (args.level_of_detail == MemoryDumpLevelOfDetail::LIGHT)
+bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  // Snapshot of memory maps is taken only for detailed dump requests.
+  if (args.level_of_detail !=
+      base::trace_event::MemoryDumpLevelOfDetail::DETAILED)
     return true;
 
   uint32_t res = 0;
   if (UNLIKELY(proc_smaps_for_testing)) {

[Primiano Tucci (use gerrit), 2016/01/05 11:55:17]

not your fault, since you are here can you remove this UNLIKELY?
Considered how slow is the mmaps dumping, this is not going to make any
difference, just makes the code more unreadable

[ssid, 2016/01/06 20:26:33]

Done.

     res = ReadLinuxProcSmapsFile(proc_smaps_for_testing, pmd->process_mmaps());
   } else {
-    ScopedFILE smaps_file(fopen("/proc/self/smaps", "r"));
+    std::string file_name = "/proc/" + (process_ == base::kNullProcessHandle

[Primiano Tucci (use gerrit), 2016/01/05 11:55:17]

and that's why you want ProcessId and not ProcessHandle here.
This is now a bit of a hack.
You are relying on the fact that on linux ProcessId and ProcessHandle are the
same thing :)

[ssid, 2016/01/06 20:26:33]

Done.

+                                            ? "self"
+                                            : base::IntToString(process_)) +
+                            "/smaps";
+    base::ScopedFILE smaps_file(fopen(file_name.c_str(), "r"));
     res = ReadLinuxProcSmapsFile(smaps_file.get(), pmd->process_mmaps());
   }
 
-  if (res > 0) {
+  if (res)
     pmd->set_has_process_mmaps();
-    return true;
-  }
-  return false;
+  return res;
 }
 
-}  // namespace trace_event
-}  // namespace base
+}  // namespace tracing