Metrics provider for statistical stack profiler

chrome/browser/metrics/call_stack_profile_metrics_provider.cc

Index: chrome/browser/metrics/call_stack_profile_metrics_provider.cc
diff --git a/chrome/browser/metrics/call_stack_profile_metrics_provider.cc b/chrome/browser/metrics/call_stack_profile_metrics_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6e4c9a29048e2ecf4f40a6d97a7a807730fe0f7f
--- /dev/null
+++ b/chrome/browser/metrics/call_stack_profile_metrics_provider.cc
@@ -0,0 +1,124 @@
+// Copyright 2015 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 "call_stack_profile_metrics_provider.h"
+
+#include <map>
+#include <utility>
+
+#include "base/logging.h"
+#include "base/md5.h"
+#include "base/profiler/stack_sampling_profiler.h"
+#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
+
+using base::StackSamplingProfiler;
+using metrics::CallStackEntry;

[Alexei Svitkine (slow), 2015/03/17 23:10:10]

If this file is moved to metrics component, all these using declaration can be
removed.

[Mike Wittman, 2015/03/18 01:48:35]

Done.

+using metrics::CallStackProfile;
+using metrics::ChromeUserMetricsExtension;
+using metrics::ModuleIdentifier;
+using metrics::SampledProfile;
+
+namespace {
+
+// The protobuf expects the MD5 checksum prefix of the module name.
+uint64 HashModuleFilename(const base::FilePath& filename) {

[Alexei Svitkine (slow), 2015/03/17 23:10:10]

Can you just use HashMetricName()?

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

[Mike Wittman, 2015/03/18 01:48:35]

Done.

+  const base::FilePath::StringType basename = filename.BaseName().value();
+  base::MD5Digest md5;
+  base::MD5Sum(&basename[0], basename.size() * sizeof(base::FilePath::CharType),
+               &md5);
+  return *reinterpret_cast<uint64*>(&md5.a[0]);

[Ilya Sherman, 2015/03/17 04:54:35]

Are you sure this is endianness-safe?  Also, IIRC, reinterpret_cast has
undefined behavior when used in this way.  Seems like it might be wise to re-use
the existing hashing code, by moving the implementation into a location where it
can be shared.

[Mike Wittman, 2015/03/18 00:54:27]

Looks like I can reuse the existing code as-is. Also updated the code to avoid
undefined behavior, although it seems a net loss in readability.
(reinterpret_cast to a pointer of a different type is used all across the Chrome
codebase AFAICT.)

+}
+
+// Transcode |sample| into |proto_sample|, using base addresses in |modules| to
+// compute module IP offsets.

[Ilya Sherman, 2015/03/17 04:54:34]

Optional nit: I assume that "IP" is "instruction pointer"; but in Chrome code,
it might as easily be "Internet protocol".  Up to you whether you want to
disambiguate.

[Mike Wittman, 2015/03/18 00:54:27]

Done.

+void CopySampleToProto(
+    const StackSamplingProfiler::Sample& sample,
+    const std::vector<StackSamplingProfiler::Module>& modules,
+    CallStackProfile::Sample* proto_sample) {
+  for (const StackSamplingProfiler::Frame& frame : sample) {
+    CallStackEntry* call_stack_entry = proto_sample->add_entry();
+    int64 module_offset =
+        reinterpret_cast<const char*>(frame.instruction_pointer) -
+        reinterpret_cast<const char*>(modules[frame.module_index].base_address);
+    DCHECK_GE(module_offset, 0);
+    call_stack_entry->set_address(static_cast<uint64>(module_offset));
+    call_stack_entry->set_module_id_index(frame.module_index);
+  }
+}
+
+// Transcode |profile| into |proto_profile|.
+void CopyProfileToProto(
+    const StackSamplingProfiler::Profile& profile,
+    metrics::CallStackProfile* proto_profile) {
+  if (profile.samples.empty())
+    return;
+
+  if (profile.preserve_sample_ordering) {

[Ilya Sherman, 2015/03/17 04:54:35]

It's still not clear to me, given that all of the decisions seem to be made at
the base layer, why the base layer doesn't just use a data structure that
pre-computes this representation.  Besides making the metrics code simpler
(which is probably a wash, since it makes the base code more complex), it would
save on memory use, which seems like a definite win.

[Mike Wittman, 2015/03/18 00:54:27]

It could be computed in base, but it's not clear yet what representation will
work best for all possible users of collected stacks (in-proc as well as UMA).
It's possible we may want an even more aggressive compression scheme than this.

We're initially targeting just getting some representative data from a small
percentage of Win x64 users on startup. Can we table the decision on the
compression approach until we have that data and can make an informed decision?

+    // Collapse only consecutive repeated samples together.
+    CallStackProfile::Sample* current_sample_proto = nullptr;
+    for (auto it = profile.samples.begin(); it != profile.samples.end(); ++it) {
+      if (!current_sample_proto || *it != *(it - 1)) {
+        current_sample_proto = proto_profile->add_sample();
+        CopySampleToProto(*it, profile.modules, current_sample_proto);
+        current_sample_proto->set_count(1);
+      } else {
+        current_sample_proto->set_count(current_sample_proto->count() + 1);
+      }
+    }
+  } else {
+    // Collapse all repeated samples together.
+    std::map<StackSamplingProfiler::Sample, int> sample_index;
+    for (auto it = profile.samples.begin(); it != profile.samples.end(); ++it) {
+      auto loc = sample_index.find(*it);

[Ilya Sherman, 2015/03/17 04:54:34]

nit: Please spell out "location"

[Mike Wittman, 2015/03/18 00:54:27]

Done.

+      if (loc == sample_index.end()) {
+        CallStackProfile::Sample* sample_proto = proto_profile->add_sample();
+        CopySampleToProto(*it, profile.modules, sample_proto);
+        sample_proto->set_count(1);
+        sample_index.insert(
+            std::make_pair(
+                *it, static_cast<int>(proto_profile->sample().size()) - 1));
+      } else {
+        CallStackProfile::Sample* sample_proto =
+            proto_profile->mutable_sample()->Mutable(loc->second);
+        sample_proto->set_count(sample_proto->count() + 1);
+      }
+    }
+  }
+
+  for (const StackSamplingProfiler::Module& module : profile.modules) {
+    ModuleIdentifier* module_id = proto_profile->add_module_id();
+    module_id->set_build_id(module.id);
+    module_id->set_name_md5_prefix(HashModuleFilename(module.filename));
+  }
+
+  proto_profile->set_profile_duration_ms(
+      profile.profile_duration.InMilliseconds());
+  proto_profile->set_sampling_period_ms(
+      profile.sampling_period.InMilliseconds());
+}
+}  // namespace
+
+CallStackProfileMetricsProvider::CallStackProfileMetricsProvider() {}
+
+CallStackProfileMetricsProvider::~CallStackProfileMetricsProvider() {}
+
+void CallStackProfileMetricsProvider::ProvideGeneralMetrics(
+    metrics::ChromeUserMetricsExtension* uma_proto) {
+  std::vector<StackSamplingProfiler::Profile> profiles;
+  if (!source_profiles_for_test_.empty())
+    profiles.swap(source_profiles_for_test_);
+  else
+    StackSamplingProfiler::GetPendingProfiles(&profiles);
+
+  for (StackSamplingProfiler::Profile profile : profiles) {

[Alexei Svitkine (slow), 2015/03/17 23:10:09]

Nit: const ref

[Mike Wittman, 2015/03/18 01:48:35]

Done.

+    CallStackProfile* call_stack_profile =
+        uma_proto->add_sampled_profile()->mutable_call_stack_profile();
+    CopyProfileToProto(profile, call_stack_profile);
+  }
+}
+
+void CallStackProfileMetricsProvider::SetSourceProfilesForTest(
+    const std::vector<StackSamplingProfiler::Profile>& profiles) {
+  source_profiles_for_test_ = profiles;
+}