[memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider.

components/tracing/common/process_metrics_memory_dump_provider.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 "components/tracing/common/process_metrics_memory_dump_provider.h"
 
 #include <fcntl.h>
 #include <stdint.h>
 
 #include <map>
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/format_macros.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/process/process_metrics.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
+#include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/process_memory_maps.h"
 #include "base/trace_event/process_memory_totals.h"
+#include "base/trace_event/trace_log.h"
 #include "build/build_config.h"
 
 namespace tracing {
 
 namespace {
 
-base::LazyInstance<
-    std::map<base::ProcessId,
-             std::unique_ptr<ProcessMetricsMemoryDumpProvider>>>::Leaky
-    g_dump_providers_map = LAZY_INSTANCE_INITIALIZER;
-
 #if defined(OS_LINUX) || defined(OS_ANDROID)
+const int kInvalidFD = -1;

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

honestly I think that directly using -1 makes the code more readable (especially
in the >= 0 condition down below in SetEnabled)

[ssid, 2016/12/14 02:59:32]

Done.

 const char kClearPeakRssCommand[] = "5";
 
 const uint32_t kMaxLineSize = 4096;
 
 bool ParseSmapsHeader(const char* header_line,
                       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};
@@ skipping 101 matching lines @@
         if (should_add_current_region) {
           pmm->AddVMRegion(region);
           ++num_valid_regions;
           should_add_current_region = false;
         }
       }
     }
   }
   return num_valid_regions;
 }
+
+int OpenStatmFile(base::ProcessId process) {
+  std::string name =
+      "/proc/" +
+      (process == base::kNullProcessId ? "self" : base::IntToString(process)) +
+      "/statm";
+  return open(name.c_str(), O_RDONLY);
+}
+
+bool GetResidentSizesFromStatmFile(int file_des,

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

s/file_des/fd/

[ssid, 2016/12/14 02:59:32]

Done.

+                                   size_t* resident_bytes,
+                                   size_t* shared_bytes) {
+  lseek(file_des, 0, SEEK_SET);
+  char line[kMaxLineSize];
+  int res = read(file_des, line, kMaxLineSize);
+  if (res <= 0)
+    return false;
+  line[res] = '\0';
+  base::CStringTokenizer t(line, line + strlen(line), " ");

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

hmm CStringTokenizer will implicitly convert line to a std::string (so make a
copy on the heap). Unfortunately there doesn't seem to be any Tokenizer that can
be used on a StringPiece.
How about a good old 
int res = sscanf(line, "%*zu %zu %zu", resident_bytes, shared_bytes) ?
if (res != 2)
  return false;

which will reduce both the code and the runtime of this?

[ssid, 2016/12/14 02:59:32]

Makes sense. fixed.

+  t.GetNext();
+  // Ignore the first first number.
+  t.GetNext();
+  base::StringToSizeT(t.token().c_str(), resident_bytes);
+  t.GetNext();
+  base::StringToSizeT(t.token().c_str(), shared_bytes);
+  return true;
+}
+
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
 std::unique_ptr<base::ProcessMetrics> CreateProcessMetrics(
     base::ProcessId process) {
   if (process == base::kNullProcessId)
     return base::ProcessMetrics::CreateCurrentProcessMetrics();
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   // Just pass ProcessId instead of handle since they are the same in linux and
   // android.
   return base::ProcessMetrics::CreateProcessMetrics(process);
 #else
   // Creating process metrics for child processes in mac or windows requires
   // additional information like ProcessHandle or port provider.
   NOTREACHED();
   return std::unique_ptr<base::ProcessMetrics>();
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 }
 
+base::LazyInstance<

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

is there a functional reason why you moved this? IIRC the code style variable
declarations should be above function blocks.

[ssid, 2016/12/14 02:59:32]

Sorry this was a mistake.

+    std::map<base::ProcessId,
+             std::unique_ptr<ProcessMetricsMemoryDumpProvider>>>::Leaky
+    g_dump_providers_map = LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 // static
 uint64_t ProcessMetricsMemoryDumpProvider::rss_bytes_for_testing = 0;
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 
 // static
 FILE* ProcessMetricsMemoryDumpProvider::proc_smaps_for_testing = nullptr;
 
@@ skipping 18 matching lines @@
 }
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
 // static
 void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
     base::ProcessId process) {
   std::unique_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider(
       new ProcessMetricsMemoryDumpProvider(process));
   base::trace_event::MemoryDumpProvider::Options options;
   options.target_pid = process;
+  options.is_fast_polling_supported = true;
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       metrics_provider.get(), "ProcessMemoryMetrics", nullptr, options);
   bool did_insert =
       g_dump_providers_map.Get()
           .insert(std::make_pair(process, std::move(metrics_provider)))
           .second;
   if (!did_insert) {
     DLOG(ERROR) << "ProcessMetricsMemoryDumpProvider already registered for "
                 << (process == base::kNullProcessId
                         ? "current process"
                         : "process id " + base::IntToString(process));
   }
 }
 
 // static
 void ProcessMetricsMemoryDumpProvider::UnregisterForProcess(
     base::ProcessId process) {
   auto iter = g_dump_providers_map.Get().find(process);
-  if (iter == g_dump_providers_map.Get().end()) {
+  if (iter == g_dump_providers_map.Get().end())
     return;
-  }
   base::trace_event::MemoryDumpManager::GetInstance()
       ->UnregisterAndDeleteDumpProviderSoon(std::move(iter->second));
   g_dump_providers_map.Get().erase(iter);
 }
 
 ProcessMetricsMemoryDumpProvider::ProcessMetricsMemoryDumpProvider(
     base::ProcessId process)
     : process_(process),
       process_metrics_(CreateProcessMetrics(process)),
-      is_rss_peak_resettable_(true) {}
+      is_rss_peak_resettable_(true) {
+#if defined(OS_LINUX) || defined(OS_ANDROID)
+  proc_statm_file_ = kInvalidFD;
+#endif
+}
 
-ProcessMetricsMemoryDumpProvider::~ProcessMetricsMemoryDumpProvider() {}
+ProcessMetricsMemoryDumpProvider::~ProcessMetricsMemoryDumpProvider() {
+  // Clear files in case polling was not disabled yet.
+  SetFastMemoryPollingEnabled(false);
+}
 
 // Called at trace dump point time. Creates a snapshot of the memory maps for
 // the current process.
 bool ProcessMetricsMemoryDumpProvider::OnMemoryDump(
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* pmd) {
   bool res = DumpProcessTotals(args, pmd);
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   if (args.level_of_detail ==
@@ skipping 45 matching lines @@
 #endif  // !defined(OS_IOS)
 
   pmd->process_totals()->set_resident_set_bytes(rss_bytes);
   pmd->set_has_process_totals();
   pmd->process_totals()->set_peak_resident_set_bytes(peak_rss_bytes);
 
   // Returns true even if other metrics failed, since rss is reported.
   return true;
 }
 
+void ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal(
+    uint64_t* memory_total) {
+  *memory_total = 0;
+#if defined(OS_LINUX) || defined(OS_ANDROID)
+  if (proc_statm_file_ == kInvalidFD)
+    proc_statm_file_ = OpenStatmFile(process_);
+  if (proc_statm_file_ < 0)
+    return;
+
+  size_t rss = 0, shared = 0;
+  if (!GetResidentSizesFromStatmFile(proc_statm_file_, &rss, &shared))
+    return;
+
+  // When adding total for child processes, do not include "shared" since it
+  // will be included in the current processes' total.
+  *memory_total = (process_ == base::kNullProcessId ? rss : rss - shared) *

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

hmm not sure I am convinced here.
The math should change depending on which process you dump from. This is just an
implementation detail about the sandbox workaround. 
I think always counting priv+shared is easier to reason about.

[ssid, 2016/12/14 02:59:32]

Hm but we will miss the shared memory increases if we leave shared from all
processes.

[Primiano Tucci (use gerrit), 2016/12/15 15:00:02]

I was suggesting the other way round, *always*count shared, i.e. don't have this
special path that subtracts it.
The reason is the following: the "shared" field from /proc/.../statm is
different from the "shared" that you get from /proc/.../smaps
the one in statm is really "share-able". It doesn't count the fact that pages
are really shared or not (that's one of the reasons why reading statm is so
fast).
For statm the definition of "shared" is simply: "if the memory map is backed by
a file, i.e. not anonymous"
This means that if you have a scenario where a child process is the only one
using a /ashmem segment, you will not count it here with this code (but you will
if you remove the "- shared" part).
IMHO better double count (if you have the same in browser and renderer) than not
count at all.

The real downside (that we knew since the beginning of times) is that this
"shared" counter will also count clean memory from mmaped filed. So if somebody
mmaps() a file read-only you will see an increase.
On the other side, our shared memory is based on files. So IMHO better be
pessimistic, and count it, than ignore shared memory at all (Which is what you
are doing here, if you don't to this discount for child processes)

[ssid, 2016/12/16 03:16:41]

makes sense. Changed to just resident size.

+                  base::GetPageSize();
+#else
+  *memory_total = process_metrics_->GetWorkingSetSize();
+#endif
+}
+
+void ProcessMetricsMemoryDumpProvider::SetFastMemoryPollingEnabled(
+    bool enabled) {
+#if defined(OS_LINUX) || defined(OS_ANDROID)
+  if (proc_statm_file_ >= 0)

[Primiano Tucci (use gerrit), 2016/12/13 20:02:39]

Ok I see how doing the auto-opening makes things easier. But at this point just
call this function SetFastMemoryPollingDisabled.
no reason to pass a boolean that you don't use.
Also ironically now this is called set...enabled but you call it only for
disabling :)

[ssid, 2016/12/14 02:59:32]

It is weird to have SetFastMemoryPollingDisabled method in the MDP interface. I
think it is better to have it as SetFastMemoryPollingEnabled.
I was using an |is_final_poll| argument in PollFastMemoryTotal instead, if we
watn to keep the number of methods in MDP lesser.
Either ways discussion in
https://codereview.chromium.org/2537363003/diff/260001/base/trace_event/memor...

Fixed this to clear only on disable.

+    close(proc_statm_file_);
+  proc_statm_file_ = kInvalidFD;
+#endif
+}
+
 }  // namespace tracing