Normalizing source file names before calculating hash

chrome/browser/metrics/metrics_log.cc

Index: chrome/browser/metrics/metrics_log.cc
diff --git a/chrome/browser/metrics/metrics_log.cc b/chrome/browser/metrics/metrics_log.cc
index f9721f4a5c02158cb7821e1970d187d1c4e01631..cf42ba3cdc51773fbfe8d29402413acd55653680 100644
--- a/chrome/browser/metrics/metrics_log.cc
+++ b/chrome/browser/metrics/metrics_log.cc
@@ -273,6 +273,23 @@ std::string MapThreadName(const std::string& thread_name) {
   return thread_name.substr(0, i) + '*';
 }
 
+// Normalizes a source filename (which is platform- and build-method-dependent)
+// by extracting the last component of the full file name.
+// Example: "c:\b\build\slave\win\build\src\chrome\app\chrome_main.cc" =>
+// "chrome_main.cc".
+std::string NormalizeFileName(const std::string& file_name) {

[jar (doing other things), 2014/03/29 00:32:25]

nit: The argument is actually a const char* (before you convert it to a string).
 You could have just used that... but perhaps this is wasteful optimization.  It
would look like:

const char* Normalize(const char* file_name) {
  const char* result = file_name;
  while (*file_name) {
    if (*file_name == '/' || *file_name == '\\')
      result = file_name + 1;
  }
  return result;
}

...or you could use strrch()

  const char* better = strrchr(file_name, '/');
  if (better)
    file_name = better + 1;
  better = strrchr(file_name, '\\');
  if (better)
    file_name = better + 1;
  return file_name;


Thinking more, I guess this is not very perf critical. It may be safer(?) to
stick with conversion to strings, and only hang onto using "const char*" in the
perf critical sections of the profiler.  

YMMV.  Your choice.

[vadimt, 2014/03/29 00:45:50]

In fact, the parameter std::string.
https://code.google.com/p/chromium/codesearch#chromium/src/base/location.h&rc...

So, to avoid conversion I chose to pass a string.
BTW, given that these strings are passed between processes, it makes sense to
keep them as std::string in Location class.

[jar (doing other things), 2014/03/29 00:52:23]

Ah... quite correct.  SGTM.

+  size_t i = file_name.length();
+
+  while (i > 0) {
+    const char current_character = file_name[i - 1];
+    if (current_character == '/' || current_character == '\\')
+      break;
+    --i;
+  }
+
+  return file_name.substr(i);

[Ilya Sherman, 2014/03/29 01:06:49]

IMO this method would be clearer if written using the Tokenize() function from
base/strings/string_util.h: tokenize, then return the last component.

[vadimt, 2014/03/31 18:10:14]

Yeah, but that is much less efficient, since we build multiple components (while
needing only one), and implementation of Tokenize uses find_first_of, which
build a lookup table per each token (and this inefficiency gets multiplied by
the number of tokens).
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

[Ilya Sherman, 2014/03/31 23:44:48]

I'm not convinced that this is a place where such a minor efficiency difference
matters.  IMO the increased code clarity is worth the very minor inefficiency,
unless you have some data to indicate that this would become a hot spot in the
code.

[vadimt, 2014/04/01 18:24:10]

Done, but using find_last_of. Looks OK?

+}
+
 void WriteProfilerData(const ProcessDataSnapshot& profiler_data,
                        int process_type,
                        ProfilerEventProto* performance_profile) {
@@ -287,7 +304,7 @@ void WriteProfilerData(const ProcessDataSnapshot& profiler_data,
     tracked_object->set_exec_thread_name_hash(
         MetricsLogBase::Hash(MapThreadName(it->death_thread_name)));
     tracked_object->set_source_file_name_hash(
-        MetricsLogBase::Hash(it->birth.location.file_name));
+        MetricsLogBase::Hash(NormalizeFileName(it->birth.location.file_name)));

[jar (doing other things), 2014/03/29 00:32:25]

Should we consider expanding the protobuf, and putting this in a different
(additional?) slot?

[vadimt, 2014/03/29 00:45:50]

Given uselessness of the existing full filename hash, probably, no. We are
replacing something that could not be used in any reasonable way with something
less meaningless; no reason to keep the old one.

[jar (doing other things), 2014/03/29 00:52:23]

OK.  The old one disambiguates... but I guess if we *could* decode the old one,
we could map to the "new" one.

SGTM too.

     tracked_object->set_source_function_name_hash(
         MetricsLogBase::Hash(it->birth.location.function_name));
     tracked_object->set_source_line_number(it->birth.location.line_number);