[memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider.

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 d2c00c347f0db3f7f9b13286e2af3edc4ac089b3..d06919aeb38cb920024be0478c01961ed375dae3 100644
--- a/components/tracing/common/process_metrics_memory_dump_provider.cc
+++ b/components/tracing/common/process_metrics_memory_dump_provider.cc
@@ -17,23 +17,22 @@
 #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;
@@ -155,6 +154,34 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file,
   }
   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(
@@ -173,6 +200,11 @@ std::unique_ptr<base::ProcessMetrics> CreateProcessMetrics(
 #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
@@ -211,6 +243,7 @@ void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
       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 =
@@ -229,9 +262,8 @@ void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
 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);
@@ -241,9 +273,16 @@ 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.
@@ -309,4 +348,35 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals(
   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