[tracing] Improve the memory maps dumper generation format.

base/trace_event/process_memory_maps_dump_provider.cc

Index: base/trace_event/process_memory_maps_dump_provider.cc
diff --git a/base/trace_event/process_memory_maps_dump_provider.cc b/base/trace_event/process_memory_maps_dump_provider.cc
index fbe0eb1e8c32db46bffdd08cfaba96c32eb3087c..e1297fdff524a92587bf7ea546b177e8555ca5bb 100644
--- a/base/trace_event/process_memory_maps_dump_provider.cc
+++ b/base/trace_event/process_memory_maps_dump_provider.cc
@@ -60,7 +60,7 @@ bool ParseSmapsHeader(std::istream* smaps,
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
   }
-  *smaps >> std::hex >> region->mapped_file_offset;
+  *smaps >> ignored;  // Ignore mapped file offset.
   *smaps >> ignored;  // Ignore device maj-min (fc:01 in the example above).
   *smaps >> ignored;  // Ignore inode number (1234 in the example above).
 
@@ -79,17 +79,24 @@ uint32 ParseSmapsCounter(std::istream* smaps,
   uint32 res = 0;
   std::string counter_name;
   *smaps >> counter_name;
+  uint64 counter_value = 0;
 
   // TODO(primiano): "Swap" should also be accounted as resident. Check
   // whether Rss isn't already counting swapped and fix below if that is
   // the case.
-  if (counter_name == "Rss:") {
-    *smaps >> std::dec >> region->byte_stats_resident;
-    region->byte_stats_resident *= 1024;
+  if (counter_name == "Pss:") {
+    *smaps >> std::dec >> counter_value;
+    region->byte_stats_proportional_resident = (counter_value * 1024);

[petrcermak, 2015/03/20 11:23:34]

Why do you *assign* to byte_stats_proportional_resident but *add* to
byte_stats_private_resident and byte_stats_shared_resident? If that's
intentional, you should probably add a comment explaining it.

[petrcermak, 2015/03/20 11:23:34]

There seems to be a little bit of code duplication here now. How about factoring
some code out into a function:

uint64 ExtractSmapsCounterBytes(std::istream* smaps) {
  uint64 counter_value;
  *smaps >> std::dec >> counter_value;
  return counter_value * 1024;
}

This would allow you to get rid of the counter_value variable and simplify the
if statement:

if (counter_name == "Pss:") {
  region->byte_states_proportional_resident += ExtractSmapsCounterBytes(smaps);
  res = 1;
} else if ...

[petrcermak, 2015/03/20 11:23:34]

No reason for parentheses around 'counter_value * 1024' (same below).

[Primiano Tucci (use gerrit), 2015/03/20 13:59:14]

Done.

[Primiano Tucci (use gerrit), 2015/03/20 13:59:14]

Done.

[Primiano Tucci (use gerrit), 2015/03/20 13:59:14]

I thought that was clear by the fact that Private/Shared has two items (shared
and clean) which are added up.
Added comment.

     res = 1;
-  } else if (counter_name == "Anonymous:") {
-    *smaps >> std::dec >> region->byte_stats_anonymous;
-    region->byte_stats_anonymous *= 1024;
+  } else if (counter_name == "Private_Dirty:" ||
+             counter_name == "Private_Clean:") {
+    *smaps >> std::dec >> counter_value;
+    region->byte_stats_private_resident += (counter_value * 1024);
+    res = 1;
+  } else if (counter_name == "Shared_Dirty:" ||
+             counter_name == "Shared_Clean:") {
+    *smaps >> std::dec >> counter_value;
+    region->byte_stats_shared_resident += (counter_value * 1024);
     res = 1;
   }
 
@@ -111,7 +118,7 @@ uint32 ReadLinuxProcSmapsFile(std::istream* smaps, ProcessMemoryMaps* pmm) {
   if (!smaps->good())
     return 0;
 
-  const uint32 kNumExpectedCountersPerRegion = 2;
+  const uint32 kNumExpectedCountersPerRegion = 5;
   uint32 counters_parsed_for_current_region = 0;
   uint32 num_valid_regions = 0;
   ProcessMemoryMaps::VMRegion region;