[tracing] Dump child processes' memory metrics in browser

components/tracing/process_metrics_memory_dump_provider_linux.cc

 // 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.
 
-#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;
   char protection_flags[5] = {0};
   char mapped_file[kMaxLineSize];
 
   if (sscanf(header_line, "%" SCNx64 "-%" SCNx64 " %4c %*s %*s %*s%4095[^\n]\n",
              &region->start_address, &end_addr, protection_flags,
              mapped_file) != 4)
     return false;
 
   if (end_addr > region->start_address) {
     region->size_in_bytes = end_addr - region->start_address;
   } else {
     // This is not just paranoia, it can actually happen (See crbug.com/461237).
     region->size_in_bytes = 0;
     res = false;
   }
 
   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;
 }
 
 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);
   } else if (strcmp(counter_name, "Private_Dirty:") == 0) {
     region->byte_stats_private_dirty_resident = ReadCounterBytes(counter_line);
   } else if (strcmp(counter_name, "Private_Clean:") == 0) {
     region->byte_stats_private_clean_resident = ReadCounterBytes(counter_line);
   } else if (strcmp(counter_name, "Shared_Dirty:") == 0) {
     region->byte_stats_shared_dirty_resident = ReadCounterBytes(counter_line);
   } else if (strcmp(counter_name, "Shared_Clean:") == 0) {
     region->byte_stats_shared_clean_resident = ReadCounterBytes(counter_line);
   } else if (strcmp(counter_name, "Swap:") == 0) {
     region->byte_stats_swapped = ReadCounterBytes(counter_line);
   } else {
     res = 0;
   }
 
   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;
 
   fseek(smaps_file, 0, SEEK_SET);
 
   char line[kMaxLineSize];
   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 {
       counters_parsed_for_current_region += ParseSmapsCounter(line, &region);
       DCHECK_LE(counters_parsed_for_current_region,
                 kNumExpectedCountersPerRegion);
       if (counters_parsed_for_current_region == kNumExpectedCountersPerRegion) {
         if (should_add_current_region) {
           pmm->AddVMRegion(region);
           ++num_valid_regions;
           should_add_current_region = false;
         }
       }
     }
   }
   return num_valid_regions;
 }
 
 }  // namespace
 
 // static
-ProcessMemoryMapsDumpProvider* ProcessMemoryMapsDumpProvider::GetInstance() {
-  return Singleton<ProcessMemoryMapsDumpProvider,
-                   LeakySingletonTraits<ProcessMemoryMapsDumpProvider>>::get();
+FILE* ProcessMetricsMemoryDumpProvider::proc_smaps_for_testing = nullptr;
+
+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;
+
+  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;
 }
 
-ProcessMemoryMapsDumpProvider::ProcessMemoryMapsDumpProvider() {
-}
-
-ProcessMemoryMapsDumpProvider::~ProcessMemoryMapsDumpProvider() {
-}
-
-// 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