Add PerfResultReporter for gtest perf

Adds the PerfResultReporter class for use in gtest-based perf tests that
currently use perf_test::PrintResult directly. This is to help prevent
incorrect usage of PrintResult, as doing so can end up creating usable
perf data that gets accepted by the perf dashboard, but exhibits
unexpected behavior under certain conditions.

Also ports usage of PrintResult in the VR perf tests over to use
PerfResultReporter as a sample.

Bug: 923564
Change-Id: I7e39db974cde314417f3a81c4ef1cf5bd4afcfa4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1750205
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687772}

chrome/browser/vr/text_perftest.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/vr/test/constants.h"
 #include "chrome/browser/vr/test/gl_test_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "testing/perf/perf_result_reporter.h"
 #include "testing/perf/perf_test.h"
 #include "third_party/skia/include/core/SkSurface.h"
 
@@ -35,17 +36,26 @@ class TextPerfTest : public testing::Test {
   }
 
   void TearDown() override {
+    PrintResults();
     text_element_.reset();
     provider_.reset();
     gl_test_environment_.reset();
   }
 
  protected:
-  void PrintResults(const std::string& name) {
-    perf_test::PrintResult("TextPerfTest", ".render_time_avg", name,
-                           timer_.TimePerLap().InMillisecondsF(), "ms", true);
-    perf_test::PrintResult("TextPerfTest", ".number_of_runs", name,
-                           static_cast<size_t>(timer_.NumLaps()), "runs", true);
+  void SetupReporter(const std::string& test_name,
+                     const std::string& story_name) {
+    reporter_ =
+        std::make_unique<perf_test::PerfResultReporter>(test_name, story_name);
+    reporter_->RegisterImportantMetric(".render_time_avg", "ms");
+    reporter_->RegisterImportantMetric(".number_of_runs", "runs");
+  }
+
+  void PrintResults() {
+    reporter_->AddResult(".render_time_avg",
+                         timer_.TimePerLap().InMillisecondsF());
+    reporter_->AddResult(".number_of_runs",
+                         static_cast<size_t>(timer_.NumLaps()));
   }
 
   void RenderAndLapTimer() {
@@ -57,6 +67,9 @@ class TextPerfTest : public testing::Test {
   }
 
   std::unique_ptr<Text> text_element_;
+  // It would be better to initialize this during SetUp(), but there doesn't
+  // appear to be a good way to get the test name from within testing::Test.
+  std::unique_ptr<perf_test::PerfResultReporter> reporter_;
   base::LapTimer timer_;
 
  private:
@@ -65,25 +78,25 @@ class TextPerfTest : public testing::Test {
 };
 
 TEST_F(TextPerfTest, RenderLoremIpsum100Chars) {
+  SetupReporter("TextPerfTest", "render_lorem_ipsum_100_chars");
   base::string16 text = base::UTF8ToUTF16(kLoremIpsum100Chars);
   timer_.Reset();
   for (size_t i = 0; i < kNumberOfRuns; i++) {
     text[0] = 'a' + (i % 26);
     text_element_->SetText(text);
     RenderAndLapTimer();
   }
-  PrintResults("render_lorem_ipsum_100_chars");
 }
 
 TEST_F(TextPerfTest, RenderLoremIpsum700Chars) {
+  SetupReporter("TextPerfTest", "render_lorem_ipsum_700_chars");
   base::string16 text = base::UTF8ToUTF16(kLoremIpsum700Chars);
   timer_.Reset();
   for (size_t i = 0; i < kNumberOfRuns; i++) {
     text[0] = 'a' + (i % 26);
     text_element_->SetText(text);
     RenderAndLapTimer();
   }
-  PrintResults("render_lorem_ipsum_700_chars");
 }
 
 }  // namespace vr

testing/perf/BUILD.gn

@@ -5,6 +5,8 @@
 source_set("perf") {
   testonly = true
   sources = [
+    "perf_result_reporter.cc",
+    "perf_result_reporter.h",
     "perf_test.cc",
     "perf_test.h",
   ]

testing/perf/perf_result_reporter.cc

@@ -0,0 +1,62 @@
+// Copyright 2019 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 "testing/perf/perf_result_reporter.h"
+#include "base/logging.h"
+#include "testing/perf/perf_test.h"
+
+namespace perf_test {
+
+PerfResultReporter::PerfResultReporter(const std::string& metric_basename,
+                                       const std::string& story_name)
+    : metric_basename_(metric_basename), story_name_(story_name) {}
+
+PerfResultReporter::~PerfResultReporter() = default;
+
+void PerfResultReporter::RegisterFyiMetric(const std::string& metric_suffix,
+                                           const std::string& units) {
+  RegisterMetric(metric_suffix, units, false);
+}
+
+void PerfResultReporter::RegisterImportantMetric(
+    const std::string& metric_suffix,
+    const std::string& units) {
+  RegisterMetric(metric_suffix, units, true);
+}
+
+void PerfResultReporter::AddResult(const std::string& metric_suffix,
+                                   size_t value) {
+  auto iter = metric_map_.find(metric_suffix);
+  CHECK(iter != metric_map_.end());
+
+  PrintResult(metric_basename_, metric_suffix, story_name_, value,
+              iter->second.units, iter->second.important);
+}
+
+void PerfResultReporter::AddResult(const std::string& metric_suffix,
+                                   double value) {
+  auto iter = metric_map_.find(metric_suffix);
+  CHECK(iter != metric_map_.end());
+
+  PrintResult(metric_basename_, metric_suffix, story_name_, value,
+              iter->second.units, iter->second.important);
+}
+
+void PerfResultReporter::AddResult(const std::string& metric_suffix,
+                                   const std::string& value) {
+  auto iter = metric_map_.find(metric_suffix);
+  CHECK(iter != metric_map_.end());
+
+  PrintResult(metric_basename_, metric_suffix, story_name_, value,
+              iter->second.units, iter->second.important);
+}
+
+void PerfResultReporter::RegisterMetric(const std::string& metric_suffix,
+                                        const std::string& units,
+                                        bool important) {
+  CHECK(metric_map_.count(metric_suffix) == 0);
+  metric_map_.insert({metric_suffix, {units, important}});
+}
+
+}  // namespace perf_test

testing/perf/perf_result_reporter.h

@@ -0,0 +1,61 @@
+// Copyright 2019 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.
+
+#ifndef TESTING_PERF_PERF_RESULT_REPORTER_H_
+#define TESTING_PERF_PERF_RESULT_REPORTER_H_
+
+#include <string>
+#include <unordered_map>
+
+namespace perf_test {
+
+struct MetricInfo {
+  std::string units;
+  bool important;
+};
+
+// A helper class for using the perf test printing functions safely, as
+// otherwise it's easy to accidentally mix up arguments to produce usable but
+// malformed perf data. See https://crbug.com/923564.
+
+// Sample usage:
+// auto reporter = PerfResultReporter("TextRendering", "100_chars");
+// reporter.RegisterImportantMetric(".wall_time", "ms");
+// reporter.RegisterImportantMetric(".cpu_time", "ms");
+// ...
+// reporter.AddResult(".wall_time", GetWallTime());
+// reporter.AddResult(".cpu_time", GetCpuTime());
+
+// This would end up reporting "TextRendering.wall_time" and
+// "TextRendering.cpu_time" metrics on the dashboard, made up of results from
+// a single "100_chars" story. If an additional story run is added, e.g.
+// "200_chars", then the metrics will be averaged over both runs with the
+// ability to drill down into results for specific stories.
+class PerfResultReporter {
+ public:
+  PerfResultReporter(const std::string& metric_basename,
+                     const std::string& story_name);
+  ~PerfResultReporter();
+
+  void RegisterFyiMetric(const std::string& metric_suffix,
+                         const std::string& units);
+  void RegisterImportantMetric(const std::string& metric_suffix,
+                               const std::string& units);
+  void AddResult(const std::string& metric_suffix, size_t value);
+  void AddResult(const std::string& metric_suffix, double value);
+  void AddResult(const std::string& metric_suffix, const std::string& value);
+
+ private:
+  void RegisterMetric(const std::string& metric_suffix,
+                      const std::string& units,
+                      bool important);
+
+  std::string metric_basename_;
+  std::string story_name_;
+  std::unordered_map<std::string, MetricInfo> metric_map_;
+};
+
+}  // namespace perf_test
+
+#endif  // TESTING_PERF_PERF_RESULT_REPORTER_H_