Metrics provider for statistical stack profiler

chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc

Index: chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc
diff --git a/chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc b/chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..69eb17bf6cfbda260a36b14643ebcc1f9f3eb828
--- /dev/null
+++ b/chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc
@@ -0,0 +1,376 @@
+// 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 "chrome/browser/metrics/call_stack_profile_metrics_provider.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/profiler/stack_sampling_profiler.h"
+#include "base/strings/string_number_conversions.h"
+#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::StackSamplingProfiler;
+using Frame = StackSamplingProfiler::Frame;
+using Module = StackSamplingProfiler::Module;
+using Profile = StackSamplingProfiler::Profile;
+using Sample = StackSamplingProfiler::Sample;

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

nit: Why the different syntax on lines 14-17 vs. line 13?

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

"using X::Y" is only valid if X is a namespace.

+
+// Checks that all properties from multiple profiles are filled as expected.
+TEST(CallStackProfileMetricsProviderTest, MultipleProfiles) {
+  const uintptr_t module1_base_address = 0x1000;
+  const uintptr_t module2_base_address = 0x2000;
+  const uintptr_t module3_base_address = 0x3000;
+
+  const Module profile_modules[][2] = {
+    {
+      {
+        reinterpret_cast<const void*>(module1_base_address),
+        "ABCD",
+#if defined(OS_WIN)

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

Do the actual paths matter?

If not, I suggest simplifying by using FILE_PATH_LITERAL to remove the ifdef.

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

Not that much. However, even if we use FILE_PATH_LITERAL here, we still need the
ifdefs in profile_expected_name_md5_prefixes due to the md5 sum over the
different character representations. I'd prefer to leave the ifdefs in both
places to avoid confusion over this subtlety.

+        base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
+#else
+        base::FilePath("/some/path/to/chrome")
+#endif
+      },
+      {
+        reinterpret_cast<const void*>(module2_base_address),
+        "EFGH",
+#if defined(OS_WIN)
+        base::FilePath(L"c:\\some\\path\\to\\third_party.dll")
+#else
+        base::FilePath("/some/path/to/third_party.so")
+#endif
+      }
+    },
+    {
+      {
+        reinterpret_cast<const void*>(module3_base_address),
+        "MNOP",
+#if defined(OS_WIN)
+        base::FilePath(L"c:\\some\\path\\to\\third_party2.dll")
+#else
+        base::FilePath("/some/path/to/third_party2.so")
+#endif
+      },
+      { // Repeated from the first profile.
+        reinterpret_cast<const void*>(module1_base_address),
+        "ABCD",
+#if defined(OS_WIN)
+        base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
+#else
+        base::FilePath("/some/path/to/chrome")
+#endif
+      },
+    }
+  };
+
+  // Values for Windows generated with:
+  // perl -MDigest::MD5=md5 -MEncode=encode \
+  // -e 'for(@ARGV){printf "%x\n", unpack "Q", md5 encode "UTF-16LE", $_}' \
+  // chrome.exe third_party.dll third_party2.dll
+  //
+  // Values for Linux generated with:
+  // perl -MDigest::MD5=md5 \
+  // -e 'for(@ARGV){printf "%x\n", unpack "Q", md5 $_}' \
+  // chrome third_party.so third_party2.so
+  const uint64 profile_expected_name_md5_prefixes[][2] = {
+    {
+#if defined(OS_WIN)
+      0x2ac596616e4c346ULL,
+      0xba1aaedefd8b2b7eULL
+#else
+      0x6cc31a45a8384855UL,
+      0xf8c9598614613684UL
+#endif
+    },
+    {
+#if defined(OS_WIN)
+      0xcad5a473456fb687ULL,
+      0x2ac596616e4c346ULL
+#else
+      0x9eeca69f537e64b4UL,
+      0x6cc31a45a8384855UL
+#endif
+    }
+  };
+
+  const Frame profile_sample_frames[][2][3] = {

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

It's not immediately clear to me what this variable represents.  Could you
please add a comment?

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

Done.

+    {
+      {
+        { reinterpret_cast<const void*>(module1_base_address + 0x10), 0 },
+        { reinterpret_cast<const void*>(module2_base_address + 0x20), 1 },
+        { reinterpret_cast<const void*>(module1_base_address + 0x30), 0 }
+      },
+      {
+        { reinterpret_cast<const void*>(module2_base_address + 0x10), 1 },
+        { reinterpret_cast<const void*>(module1_base_address + 0x20), 0 },
+        { reinterpret_cast<const void*>(module2_base_address + 0x30), 1 }
+      }
+    },
+    {
+      {
+        { reinterpret_cast<const void*>(module3_base_address + 0x10), 0 },
+        { reinterpret_cast<const void*>(module1_base_address + 0x20), 1 },
+        { reinterpret_cast<const void*>(module3_base_address + 0x30), 0 }
+      },
+      {
+        { reinterpret_cast<const void*>(module1_base_address + 0x10), 1 },
+        { reinterpret_cast<const void*>(module3_base_address + 0x20), 0 },
+        { reinterpret_cast<const void*>(module1_base_address + 0x30), 1 }
+      }
+    }
+  };
+
+  base::TimeDelta profile_durations[2] = {
+    base::TimeDelta::FromMilliseconds(100),
+    base::TimeDelta::FromMilliseconds(200)
+  };
+
+  base::TimeDelta profile_sampling_periods[2] = {
+    base::TimeDelta::FromMilliseconds(10),
+    base::TimeDelta::FromMilliseconds(20)
+  };
+
+  std::vector<Profile> profiles;
+  for (size_t i = 0; i < arraysize(profile_sample_frames); i++) {

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

nit: ++i, ++j below

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

Done.

+    Profile profile;
+    profile.modules.insert(
+        profile.modules.end(), &profile_modules[i][0],
+        &profile_modules[i][0] + arraysize(profile_modules[i]));
+
+    for (size_t j = 0; j < arraysize(profile_sample_frames[i]); j++) {
+      profile.samples.push_back(Sample());
+      Sample& sample = profile.samples.back();
+      sample.insert(sample.end(), &profile_sample_frames[i][j][0],
+                    &profile_sample_frames[i][j][0] +
+                    arraysize(profile_sample_frames[i][j]));
+    }
+
+    profile.profile_duration = profile_durations[i];
+    profile.sampling_period = profile_sampling_periods[i];
+    profile.preserve_sample_ordering = false;
+
+    profiles.push_back(profile);
+  }
+
+  scoped_ptr<CallStackProfileMetricsProvider> provider(
+      new CallStackProfileMetricsProvider);

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

nit: Why not stack-allocate?

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

Stack allocation works.

+  provider->SetSourceProfilesForTest(profiles);
+  metrics::ChromeUserMetricsExtension uma_proto;
+  provider->ProvideGeneralMetrics(&uma_proto);
+
+  ASSERT_EQ(static_cast<int>(arraysize(profile_sample_frames)),
+            uma_proto.sampled_profile().size());
+  for (size_t i = 0; i < arraysize(profile_sample_frames); i++) {
+    SCOPED_TRACE("profile " + base::IntToString(i));
+    const metrics::SampledProfile& sampled_profile =
+        uma_proto.sampled_profile().Get(i);
+    ASSERT_TRUE(sampled_profile.has_call_stack_profile());
+    const metrics::CallStackProfile& call_stack_profile =
+        sampled_profile.call_stack_profile();
+
+    ASSERT_EQ(static_cast<int>(arraysize(profile_sample_frames[i])),
+              call_stack_profile.sample().size());
+    for (size_t j = 0; j < arraysize(profile_sample_frames[i]); j++) {
+      SCOPED_TRACE("sample " + base::IntToString(j));
+      const metrics::CallStackProfile::Sample& proto_sample =
+          call_stack_profile.sample().Get(j);
+      ASSERT_EQ(static_cast<int>(arraysize(profile_sample_frames[i][j])),
+                proto_sample.entry().size());
+      ASSERT_TRUE(proto_sample.has_count());
+      EXPECT_EQ(1u, proto_sample.count());
+      for (size_t k = 0; k < arraysize(profile_sample_frames[i][j]); k++) {
+        SCOPED_TRACE("frame " + base::IntToString(k));
+        const metrics::CallStackEntry& entry = proto_sample.entry().Get(k);
+        ASSERT_TRUE(entry.has_address());
+        const char *instruction_pointer = reinterpret_cast<const char *>(

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

nit: "char *" -> "char* "

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

Done.

+            profile_sample_frames[i][j][k].instruction_pointer);
+        const char *module_base_address = reinterpret_cast<const char *>(
+            profile_modules[i][profile_sample_frames[i][j][k].module_index]
+            .base_address);
+        EXPECT_EQ(static_cast<uint64>(instruction_pointer -
+                                      module_base_address), entry.address());
+        ASSERT_TRUE(entry.has_module_id_index());
+        EXPECT_EQ(profile_sample_frames[i][j][k].module_index,
+                  entry.module_id_index());
+      }
+    }
+
+    ASSERT_EQ(static_cast<int>(arraysize(profile_modules[i])),
+              call_stack_profile.module_id().size());
+    for (size_t j = 0; j < arraysize(profile_modules[i]); j++) {
+      SCOPED_TRACE("module " + base::IntToString(j));
+      const metrics::ModuleIdentifier& module_identifier =
+          call_stack_profile.module_id().Get(j);
+      ASSERT_TRUE(module_identifier.has_build_id());
+      EXPECT_EQ(profile_modules[i][j].id, module_identifier.build_id());
+      ASSERT_TRUE(module_identifier.has_name_md5_prefix());
+      EXPECT_EQ(profile_expected_name_md5_prefixes[i][j],
+                module_identifier.name_md5_prefix());
+    }
+
+    ASSERT_TRUE(call_stack_profile.has_profile_duration_ms());
+    EXPECT_EQ(profile_durations[i].InMilliseconds(),
+              call_stack_profile.profile_duration_ms());
+    ASSERT_TRUE(call_stack_profile.has_sampling_period_ms());
+    EXPECT_EQ(profile_sampling_periods[i].InMilliseconds(),
+              call_stack_profile.sampling_period_ms());
+  }
+}

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

This test is super long and has a lot of nested loops, which makes it rather
hard to follow.  It probably tests the right thing, but if a bug were to slip
in(to the test code), it would be easy to miss.  I wonder whether it might be at
least a little easier to follow if the loops were unrolled, and if some of the
constants were moved to the top of the file -- WDYT?

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

It is very long. But it also tests that there's no cross-contamination between
different profiles across all the represented state, which I think is valuable
and I'm not sure how to adequately test otherwise.

Although the looping/indexing isn't the easiest to follow, it does have the nice
property that a test bug is unlikely to work across all tested inputs
simultaneously.

+
+// Checks that all duplicate samples are collapsed with
+// preserve_sample_ordering = false.
+TEST(CallStackProfileMetricsProviderTest, RepeatedStacksUnordered) {
+  const uintptr_t module_base_address = 0x1000;
+
+  const Module modules[] = {
+    {
+      reinterpret_cast<const void*>(module_base_address),
+      "ABCD",
+#if defined(OS_WIN)
+      base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
+#else
+      base::FilePath("/some/path/to/chrome")
+#endif
+    }
+  };
+
+  // Duplicate samples in slots 0, 2, and 3.
+  const Frame sample_frames[][1] = {
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x20), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, },
+  };
+
+  Profile profile;
+  profile.modules.insert(profile.modules.end(), &modules[0],
+      &modules[0] + arraysize(modules));
+
+  for (size_t i = 0; i < arraysize(sample_frames); i++) {
+    profile.samples.push_back(Sample());
+    Sample& sample = profile.samples.back();
+    sample.insert(sample.end(), &sample_frames[i][0],
+                  &sample_frames[i][0] + arraysize(sample_frames[i]));
+  }
+
+  profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
+  profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
+  profile.preserve_sample_ordering = false;
+
+  scoped_ptr<CallStackProfileMetricsProvider> provider(
+      new CallStackProfileMetricsProvider);
+  provider->SetSourceProfilesForTest(std::vector<Profile>(1, profile));
+  metrics::ChromeUserMetricsExtension uma_proto;
+  provider->ProvideGeneralMetrics(&uma_proto);
+
+  ASSERT_EQ(1, uma_proto.sampled_profile().size());
+  const metrics::SampledProfile& sampled_profile =
+      uma_proto.sampled_profile().Get(0);
+  ASSERT_TRUE(sampled_profile.has_call_stack_profile());
+  const metrics::CallStackProfile& call_stack_profile =
+      sampled_profile.call_stack_profile();
+
+  ASSERT_EQ(2, call_stack_profile.sample().size());
+  for (int i = 0; i < 2; i++) {
+    SCOPED_TRACE("sample " + base::IntToString(i));
+    const metrics::CallStackProfile::Sample& proto_sample =
+        call_stack_profile.sample().Get(i);
+    ASSERT_EQ(static_cast<int>(arraysize(sample_frames[i])),
+              proto_sample.entry().size());
+    ASSERT_TRUE(proto_sample.has_count());
+    EXPECT_EQ(i == 0 ? 3u : 1u, proto_sample.count());
+    for (size_t j = 0; j < arraysize(sample_frames[i]); j++) {
+      SCOPED_TRACE("frame " + base::IntToString(j));
+      const metrics::CallStackEntry& entry = proto_sample.entry().Get(j);
+      ASSERT_TRUE(entry.has_address());
+      const char *instruction_pointer = reinterpret_cast<const char *>(
+          sample_frames[i][j].instruction_pointer);
+      const char *module_base_address = reinterpret_cast<const char *>(
+          modules[sample_frames[i][j].module_index].base_address);
+      EXPECT_EQ(static_cast<uint64>(instruction_pointer - module_base_address),
+                entry.address());
+      ASSERT_TRUE(entry.has_module_id_index());
+      EXPECT_EQ(sample_frames[i][j].module_index, entry.module_id_index());
+    }
+  }
+}
+
+// Checks that only contiguous duplicate samples are collapsed with
+// preserve_sample_ordering = true.
+TEST(CallStackProfileMetricsProviderTest, RepeatedStacksOrdered) {
+  const uintptr_t module_base_address = 0x1000;
+
+  const Module modules[] = {
+    {
+      reinterpret_cast<const void*>(module_base_address),
+      "ABCD",
+#if defined(OS_WIN)
+      base::FilePath(L"c:\\some\\path\\to\\chrome.exe")
+#else
+      base::FilePath("/some/path/to/chrome")
+#endif
+    }
+  };
+
+  // Duplicate samples in slots 0, 2, and 3.
+  const Frame sample_frames[][1] = {
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x20), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, },
+    { { reinterpret_cast<const void*>(module_base_address + 0x10), 0 }, }
+  };
+
+  Profile profile;
+  profile.modules.insert(profile.modules.end(), &modules[0],
+      &modules[0] + arraysize(modules));
+
+  for (size_t i = 0; i < arraysize(sample_frames); i++) {
+    profile.samples.push_back(Sample());
+    Sample& sample = profile.samples.back();
+    sample.insert(sample.end(), &sample_frames[i][0],
+                  &sample_frames[i][0] + arraysize(sample_frames[i]));
+  }
+
+  profile.profile_duration = base::TimeDelta::FromMilliseconds(100);
+  profile.sampling_period = base::TimeDelta::FromMilliseconds(10);
+  profile.preserve_sample_ordering = true;
+
+  scoped_ptr<CallStackProfileMetricsProvider> provider(
+      new CallStackProfileMetricsProvider);
+  provider->SetSourceProfilesForTest(std::vector<Profile>(1, profile));
+  metrics::ChromeUserMetricsExtension uma_proto;
+  provider->ProvideGeneralMetrics(&uma_proto);
+
+  ASSERT_EQ(1, uma_proto.sampled_profile().size());
+  const metrics::SampledProfile& sampled_profile =
+      uma_proto.sampled_profile().Get(0);
+  ASSERT_TRUE(sampled_profile.has_call_stack_profile());
+  const metrics::CallStackProfile& call_stack_profile =
+      sampled_profile.call_stack_profile();
+
+  ASSERT_EQ(3, call_stack_profile.sample().size());
+  for (int i = 0; i < 3; i++) {
+    SCOPED_TRACE("sample " + base::IntToString(i));
+    const metrics::CallStackProfile::Sample& proto_sample =
+        call_stack_profile.sample().Get(i);
+    ASSERT_EQ(static_cast<int>(arraysize(sample_frames[i])),
+              proto_sample.entry().size());
+    ASSERT_TRUE(proto_sample.has_count());
+    EXPECT_EQ(i == 2 ? 2u : 1u, proto_sample.count());
+    for (size_t j = 0; j < arraysize(sample_frames[i]); j++) {
+      SCOPED_TRACE("frame " + base::IntToString(j));
+      const metrics::CallStackEntry& entry = proto_sample.entry().Get(j);
+      ASSERT_TRUE(entry.has_address());
+      const char *instruction_pointer = reinterpret_cast<const char *>(
+          sample_frames[i][j].instruction_pointer);
+      const char *module_base_address = reinterpret_cast<const char *>(
+          modules[sample_frames[i][j].module_index].base_address);
+      EXPECT_EQ(static_cast<uint64>(instruction_pointer - module_base_address),
+                entry.address());
+      ASSERT_TRUE(entry.has_module_id_index());
+      EXPECT_EQ(sample_frames[i][j].module_index, entry.module_id_index());
+    }
+  }
+}