Normalizing source file names before calculating hash

chrome/browser/metrics/metrics_log.cc

 // Copyright (c) 2012 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 "chrome/browser/metrics/metrics_log.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 255 matching lines @@
   while (i > 0 && isdigit(thread_name[i - 1])) {
     --i;
   }
 
   if (i == thread_name.length())
     return 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) {
   for (std::vector<tracked_objects::TaskSnapshot>::const_iterator it =
            profiler_data.tasks.begin();
        it != profiler_data.tasks.end(); ++it) {
     const tracked_objects::DeathDataSnapshot& death_data = it->death_data;
     ProfilerEventProto::TrackedObject* tracked_object =
         performance_profile->add_tracked_object();
     tracked_object->set_birth_thread_name_hash(
         MetricsLogBase::Hash(MapThreadName(it->birth.thread_name)));
     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);
     tracked_object->set_exec_count(death_data.count);
     tracked_object->set_exec_time_total(death_data.run_duration_sum);
     tracked_object->set_exec_time_sampled(death_data.run_duration_sample);
     tracked_object->set_queue_time_total(death_data.queue_duration_sum);
     tracked_object->set_queue_time_sampled(death_data.queue_duration_sample);
     tracked_object->set_process_type(AsProtobufProcessType(process_type));
     tracked_object->set_process_id(profiler_data.process_id);
@@ skipping 591 matching lines @@
     ProductDataToProto(google_update_metrics.google_update_data,
                        google_update->mutable_google_update_status());
   }
 
   if (!google_update_metrics.product_data.version.empty()) {
     ProductDataToProto(google_update_metrics.product_data,
                        google_update->mutable_client_status());
   }
 #endif  // defined(GOOGLE_CHROME_BUILD) && defined(OS_WIN)
 }