[tracing] Smaps provider uses fscanf instead of istream

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 2dcdd37bd93077b111459e1fd486a2e79007717e..acabb46202df466a4cf285e0f1241d282a9ae259 100644
--- a/base/trace_event/process_memory_maps_dump_provider.cc
+++ b/base/trace_event/process_memory_maps_dump_provider.cc
@@ -4,35 +4,42 @@
 
 #include "base/trace_event/process_memory_maps_dump_provider.h"
 
-#include <cctype>
-#include <fstream>
-
 #include "base/logging.h"
-#include "base/process/process_metrics.h"
 #include "base/trace_event/process_memory_dump.h"
+
+#if defined(OS_LINUX) || defined(OS_ANDROID)
+#include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/strings/string_util.h"
 #include "base/trace_event/process_memory_maps.h"
+#endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
 namespace base {
 namespace trace_event {
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 // static
-std::istream* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
+ScopedFILE* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
 
 namespace {
 
 const uint32 kMaxLineSize = 4096;
+const uint32 kMaxCounterNameSize = 20;
 
-bool ParseSmapsHeader(std::istream* smaps,
-                      ProcessMemoryMaps::VMRegion* region) {
+bool ParseSmapsHeader(FILE* smaps_file, 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 end_addr;
-  std::string protection_flags;
-  std::string ignored;
-  *smaps >> std::hex >> region->start_address;
-  smaps->ignore(1);
-  *smaps >> std::hex >> end_addr;
+  uint64 end_addr = 0;
+  char protection_flags[5] = {0};
+  char mapped_file[kMaxLineSize];
+
+  if (fscanf(smaps_file,
+             "%" SCNx64 "-%" SCNx64 " %4c %*s %*s %*s%4096[^\n]\n" SCNx64,
+             &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 {
@@ -42,8 +49,6 @@ bool ParseSmapsHeader(std::istream* smaps,
   }
 
   region->protection_flags = 0;
-  *smaps >> protection_flags;
-  CHECK_EQ(4UL, protection_flags.size());
   if (protection_flags[0] == 'r') {
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsRead;
@@ -56,84 +61,78 @@ bool ParseSmapsHeader(std::istream* smaps,
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
   }
-  *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).
 
-  while (smaps->peek() == ' ')
-    smaps->ignore(1);
-  char mapped_file[kMaxLineSize];
-  smaps->getline(mapped_file, sizeof(mapped_file));
   region->mapped_file = mapped_file;
+  base::TrimString(region->mapped_file, " ", &region->mapped_file);
 

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

use TrimWhitespaceASCII ?

[ssid, 2015/09/30 11:15:13]

Done.

   return res;
 }
 
-uint64 ReadCounterBytes(std::istream* smaps) {
+uint64 ReadCounterBytes(FILE* smaps_file) {
   uint64 counter_value = 0;

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

Is there any need of this function at this point? Can you handle everything in
PArseSMaps counter below with one single scanf?

[ssid, 2015/09/30 11:15:12]

Yes, This function will only be called when the counter_name is one of "pss:" or
"private_dirty:"...
In case of vmflags it is not in format to read a counter.

-  *smaps >> std::dec >> counter_value;
+  CHECK_EQ(1, fscanf(smaps_file, "%" SCNu64, &counter_value));
   return counter_value * 1024;
 }
 
-uint32 ParseSmapsCounter(std::istream* smaps,
+uint32 ParseSmapsCounter(FILE* smaps_file,
                          ProcessMemoryMaps::VMRegion* region) {
   // A smaps counter lines looks as follows: "RSS:  0 Kb\n"
   uint32 res = 1;
-  std::string counter_name;
-  *smaps >> counter_name;
+  char counter_name[kMaxCounterNameSize];
+  int did_read = fscanf(smaps_file, "%s kB", counter_name);
+  DCHECK_EQ(1, did_read);

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

I think at this point you can do a single scanf here doing "%s: %SCNu64 kB"

[ssid, 2015/09/30 11:15:12]

No, see my other comment.

 
   // TODO(primiano): "Swap" should also be accounted as resident. Check
   // whether Rss isn't already counting swapped and fix below if that is

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

Since you are here, plz remove this TODO, we're dealing with this in the UI

[ssid, 2015/09/30 11:15:12]

Done.

   // the case.
-  if (counter_name == "Pss:") {
-    region->byte_stats_proportional_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Dirty:") {
-    region->byte_stats_private_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Clean:") {
-    region->byte_stats_private_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Dirty:") {
-    region->byte_stats_shared_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Clean:") {
-    region->byte_stats_shared_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Swap:") {
-    region->byte_stats_swapped = ReadCounterBytes(smaps);
+  if (strcmp(counter_name, "Pss:") == 0) {
+    region->byte_stats_proportional_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Private_Dirty:") == 0) {
+    region->byte_stats_private_dirty_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Private_Clean:") == 0) {
+    region->byte_stats_private_clean_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Shared_Dirty:") == 0) {
+    region->byte_stats_shared_dirty_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Shared_Clean:") == 0) {
+    region->byte_stats_shared_clean_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Swap:") == 0) {
+    region->byte_stats_swapped = ReadCounterBytes(smaps_file);
   } else {
     res = 0;
   }
 
-#ifndef NDEBUG
-  // Paranoid check against changes of the Kernel /proc interface.
-  if (res) {
-    std::string unit;
-    *smaps >> unit;
-    DCHECK_EQ("kB", unit);
-  }
-#endif
-
-  smaps->ignore(kMaxLineSize, '\n');
+  // Ignore the rest of the line.
+  char ignore[kMaxLineSize];

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

At this point this should be uneeded (provided that the previous scanf swallows
the \n)

[ssid, 2015/09/30 11:15:12]

That scanf cannot be used to swallow everything since the format can be
different in different lines. like in case of vmflags.

+  fgets(ignore, sizeof(ignore), smaps_file);
 
   return res;
 }
 
-uint32 ReadLinuxProcSmapsFile(std::istream* smaps, ProcessMemoryMaps* pmm) {
-  if (!smaps->good())
+uint32 ReadLinuxProcSmapsFile(FILE* smaps_file, ProcessMemoryMaps* pmm) {
+  if (!smaps_file)
     return 0;
 
+  fseek(smaps_file, 0, SEEK_SET);
+
   const uint32 kNumExpectedCountersPerRegion = 6;
   uint32 counters_parsed_for_current_region = 0;
   uint32 num_valid_regions = 0;
   ProcessMemoryMaps::VMRegion region;
   bool should_add_current_region = false;
   for (;;) {
-    int next = smaps->peek();
-    if (next == std::ifstream::traits_type::eof() || next == '\n')
+    char next = 0;
+    // Peek into the next integer and move pointer back.
+    next = fgetc(smaps_file);

[Primiano Tucci (use gerrit), 2015/09/30 09:37:02]

how is this fgetc + ungetc (brrr) different from feof()?

[ssid, 2015/09/30 11:15:13]

feof only returns a boolean. Result of fgetc + ungetc (brrr) is a character,
which is used later.

Can't find a better way to implement peek.

[Primiano Tucci (use gerrit), 2015/09/30 15:36:35]

At this point change this code to be line buffered:
read one line per time, then pass the line to the functions, instead of the
file*

+    if (next == EOF)
       break;
+    ungetc(next, smaps_file);
     if (isxdigit(next) && !isupper(next)) {
       region = ProcessMemoryMaps::VMRegion();
       counters_parsed_for_current_region = 0;
-      should_add_current_region = ParseSmapsHeader(smaps, &region);
+      should_add_current_region = ParseSmapsHeader(smaps_file, &region);
     } else {
-      counters_parsed_for_current_region += ParseSmapsCounter(smaps, &region);
+      counters_parsed_for_current_region +=
+          ParseSmapsCounter(smaps_file, &region);
       DCHECK_LE(counters_parsed_for_current_region,
                 kNumExpectedCountersPerRegion);
       if (counters_parsed_for_current_region == kNumExpectedCountersPerRegion) {
@@ -175,10 +174,11 @@ bool ProcessMemoryMapsDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   if (UNLIKELY(proc_smaps_for_testing)) {
-    res = ReadLinuxProcSmapsFile(proc_smaps_for_testing, pmd->process_mmaps());
+    res = ReadLinuxProcSmapsFile(proc_smaps_for_testing->get(),
+                                 pmd->process_mmaps());
   } else {
-    std::ifstream proc_self_smaps("/proc/self/smaps");
-    res = ReadLinuxProcSmapsFile(&proc_self_smaps, pmd->process_mmaps());
+    ScopedFILE smaps_file(OpenFile(FilePath("/proc/self/smaps"), "r"));
+    res = ReadLinuxProcSmapsFile(smaps_file.get(), pmd->process_mmaps());
   }
 #else
   LOG(ERROR) << "ProcessMemoryMaps dump provider is supported only on Linux";