[throughput metrics] Accumulate metrics across sequences of same type.
If a sequence does not have enough frames, then instead of discarding
them completely, retain the throughput-data, and accumulate them with
the throughput metrics of the subsequent sequences (of the same
interaction-type) until there are enough frames to report the metric.
Bug: 1031251
Change-Id: I4bbe1d0b024e04afd92677426a546699d9cc85d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952529
Reviewed-by: Behdad Bakhshinategh <behdadb@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722588}
diff --git a/cc/BUILD.gn b/cc/BUILD.gn
index 6e92c22..335cbec 100644
--- a/cc/BUILD.gn
+++ b/cc/BUILD.gn
@@ -645,6 +645,7 @@
"metrics/compositor_frame_reporter_unittest.cc",
"metrics/compositor_frame_reporting_controller_unittest.cc",
"metrics/compositor_timing_history_unittest.cc",
+ "metrics/frame_sequence_metrics_unittest.cc",
"metrics/frame_sequence_tracker_unittest.cc",
"mojo_embedder/async_layer_tree_frame_sink_unittest.cc",
"paint/discardable_image_map_unittest.cc",
diff --git a/cc/metrics/frame_sequence_metrics_unittest.cc b/cc/metrics/frame_sequence_metrics_unittest.cc
new file mode 100644
index 0000000..a41eb1c
--- /dev/null
+++ b/cc/metrics/frame_sequence_metrics_unittest.cc
@@ -0,0 +1,75 @@
+// 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 "cc/metrics/frame_sequence_tracker.h"
+
+#include "base/macros.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cc {
+
+TEST(FrameSequenceMetricsTest, MergeMetrics) {
+ // Create a metric with only a small number of frames. It shouldn't report any
+ // metrics.
+ FrameSequenceMetrics first(FrameSequenceTrackerType::kTouchScroll, nullptr,
+ nullptr);
+ first.impl_throughput().frames_expected = 20;
+ first.impl_throughput().frames_produced = 10;
+ EXPECT_FALSE(first.HasEnoughDataForReporting());
+
+ // Create a second metric with too few frames to report any metrics.
+ auto second = std::make_unique<FrameSequenceMetrics>(
+ FrameSequenceTrackerType::kTouchScroll, nullptr, nullptr);
+ second->impl_throughput().frames_expected = 90;
+ second->impl_throughput().frames_produced = 60;
+ EXPECT_FALSE(second->HasEnoughDataForReporting());
+
+ // Merge the two metrics. The result should have enough frames to report
+ // metrics.
+ first.Merge(std::move(second));
+ EXPECT_TRUE(first.HasEnoughDataForReporting());
+}
+
+TEST(FrameSequenceMetricsTest, AllMetricsReported) {
+ base::HistogramTester histograms;
+
+ // Create a metric with enough frames on impl to be reported, but not enough
+ // on main.
+ FrameSequenceMetrics first(FrameSequenceTrackerType::kTouchScroll, nullptr,
+ nullptr);
+ first.impl_throughput().frames_expected = 120;
+ first.impl_throughput().frames_produced = 80;
+ first.main_throughput().frames_expected = 20;
+ first.main_throughput().frames_produced = 10;
+ EXPECT_TRUE(first.HasEnoughDataForReporting());
+ first.ReportMetrics();
+
+ // The compositor-thread metric should be reported, but not the main-thread
+ // metric.
+ histograms.ExpectTotalCount(
+ "Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 1u);
+ histograms.ExpectTotalCount(
+ "Graphics.Smoothness.Throughput.MainThread.TouchScroll", 0u);
+
+ // There should still be data left over for the main-thread.
+ EXPECT_TRUE(first.HasDataLeftForReporting());
+
+ auto second = std::make_unique<FrameSequenceMetrics>(
+ FrameSequenceTrackerType::kTouchScroll, nullptr, nullptr);
+ second->impl_throughput().frames_expected = 110;
+ second->impl_throughput().frames_produced = 100;
+ second->main_throughput().frames_expected = 90;
+ first.Merge(std::move(second));
+ EXPECT_TRUE(first.HasEnoughDataForReporting());
+ first.ReportMetrics();
+ histograms.ExpectTotalCount(
+ "Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 2u);
+ histograms.ExpectTotalCount(
+ "Graphics.Smoothness.Throughput.MainThread.TouchScroll", 1u);
+ // All the metrics have now been reported. No data should be left over.
+ EXPECT_FALSE(first.HasDataLeftForReporting());
+}
+
+} // namespace cc
diff --git a/cc/metrics/frame_sequence_tracker.cc b/cc/metrics/frame_sequence_tracker.cc
index ce500fb..5d8cfab6 100644
--- a/cc/metrics/frame_sequence_tracker.cc
+++ b/cc/metrics/frame_sequence_tracker.cc
@@ -57,9 +57,9 @@
namespace {
-// Avoid reporting any throughput metric for sequences that had a small amount
-// of frames.
-constexpr int kMinFramesForThroughputMetric = 4;
+// Avoid reporting any throughput metric for sequences that do not have a
+// sufficient number of frames.
+constexpr int kMinFramesForThroughputMetric = 100;
constexpr int kBuiltinSequenceNum = FrameSequenceTrackerType::kMaxType + 1;
constexpr int kMaximumHistogramIndex = 3 * kBuiltinSequenceNum;
@@ -110,17 +110,42 @@
}
FrameSequenceMetrics::~FrameSequenceMetrics() {
+ if (HasDataLeftForReporting())
+ ReportMetrics();
+}
+
+void FrameSequenceMetrics::Merge(
+ std::unique_ptr<FrameSequenceMetrics> metrics) {
+ DCHECK_EQ(type_, metrics->type_);
+ impl_throughput_.Merge(metrics->impl_throughput_);
+ main_throughput_.Merge(metrics->main_throughput_);
+ frames_checkerboarded_ += metrics->frames_checkerboarded_;
+
+ // Reset the state of |metrics| before destroying it, so that it doesn't end
+ // up reporting the metrics.
+ metrics->impl_throughput_ = {};
+ metrics->main_throughput_ = {};
+ metrics->frames_checkerboarded_ = 0;
+}
+
+bool FrameSequenceMetrics::HasEnoughDataForReporting() const {
+ return impl_throughput_.frames_expected >= kMinFramesForThroughputMetric ||
+ main_throughput_.frames_expected >= kMinFramesForThroughputMetric;
+}
+
+bool FrameSequenceMetrics::HasDataLeftForReporting() const {
+ return impl_throughput_.frames_expected > 0 ||
+ main_throughput_.frames_expected > 0;
+}
+
+void FrameSequenceMetrics::ReportMetrics() {
DCHECK_LE(impl_throughput_.frames_produced, impl_throughput_.frames_expected);
DCHECK_LE(main_throughput_.frames_produced, main_throughput_.frames_expected);
- DCHECK_LE(main_throughput_.frames_produced, impl_throughput_.frames_produced);
TRACE_EVENT_ASYNC_END2(
"cc,benchmark", "FrameSequenceTracker", this, "args",
ThroughputData::ToTracedValue(impl_throughput_, main_throughput_),
"checkerboard", frames_checkerboarded_);
- ReportMetrics();
-}
-void FrameSequenceMetrics::ReportMetrics() {
// Report the throughput metrics.
base::Optional<int> impl_throughput_percent = ThroughputData::ReportHistogram(
type_, "CompositorThread",
@@ -169,7 +194,14 @@
base::LinearHistogram::FactoryGet(
GetCheckerboardingHistogramName(type_), 1, 100, 101,
base::HistogramBase::kUmaTargetedHistogramFlag));
+ frames_checkerboarded_ = 0;
}
+
+ // Reset the metrics that have already been reported.
+ if (impl_throughput_percent.has_value())
+ impl_throughput_ = {};
+ if (main_throughput_percent.has_value())
+ main_throughput_ = {};
}
////////////////////////////////////////////////////////////////////////////////
@@ -275,6 +307,26 @@
for (auto& tracker : removal_trackers_)
tracker->ReportFramePresented(frame_token, feedback);
+ for (auto& tracker : removal_trackers_) {
+ if (tracker->termination_status() ==
+ FrameSequenceTracker::TerminationStatus::kReadyForTermination) {
+ // The tracker is ready to be terminated. Take the metrics from the
+ // tracker, merge with any outstanding metrics from previous trackers of
+ // the same type. If there are enough frames to report the metrics, then
+ // report the metrics and destroy it. Otherwise, retain it to be merged
+ // with follow-up sequences.
+ auto metrics = tracker->TakeMetrics();
+ if (accumulated_metrics_.contains(tracker->type())) {
+ metrics->Merge(std::move(accumulated_metrics_[tracker->type()]));
+ accumulated_metrics_.erase(tracker->type());
+ }
+ if (metrics->HasEnoughDataForReporting())
+ metrics->ReportMetrics();
+ if (metrics->HasDataLeftForReporting())
+ accumulated_metrics_[tracker->type()] = std::move(metrics);
+ }
+ }
+
// Destroy the trackers that are ready to be terminated.
base::EraseIf(
removal_trackers_,
@@ -321,7 +373,11 @@
FrameSequenceTrackerType type,
UkmManager* manager,
ThroughputUkmReporter* throughput_ukm_reporter)
- : type_(type), metrics_(type, manager, throughput_ukm_reporter) {
+ : type_(type),
+ metrics_(
+ std::make_unique<FrameSequenceMetrics>(type,
+ manager,
+ throughput_ukm_reporter)) {
DCHECK_LT(type_, FrameSequenceTrackerType::kMaxType);
}
@@ -329,7 +385,7 @@
}
void FrameSequenceTracker::ReportMetrics() {
- metrics_.ReportMetrics();
+ metrics_->ReportMetrics();
}
void FrameSequenceTracker::ReportBeginImplFrame(
@@ -446,7 +502,7 @@
TRACKER_TRACE_STREAM << 'P';
TRACE_EVENT_ASYNC_STEP_INTO_WITH_TIMESTAMP0(
- "cc,benchmark", "FrameSequenceTracker", this, "FramePresented",
+ "cc,benchmark", "FrameSequenceTracker", metrics_.get(), "FramePresented",
feedback.timestamp);
const bool was_presented = !feedback.timestamp.is_null();
if (was_presented && last_submitted_frame_) {
@@ -490,7 +546,7 @@
DCHECK(!interval.is_zero()) << TRACKER_DCHECK_MSG;
constexpr base::TimeDelta kEpsilon = base::TimeDelta::FromMilliseconds(1);
int64_t frames = (difference + kEpsilon) / interval;
- metrics_.add_checkerboarded_frames(frames);
+ metrics_->add_checkerboarded_frames(frames);
}
const bool frame_had_checkerboarding =
@@ -614,10 +670,13 @@
bool FrameSequenceTracker::ShouldReportMetricsNow(
const viz::BeginFrameArgs& args) const {
- if (!first_frame_timestamp_.is_null() &&
- args.frame_time - first_frame_timestamp_ >= time_delta_to_report_)
- return true;
- return false;
+ return metrics_->HasEnoughDataForReporting() &&
+ !first_frame_timestamp_.is_null() &&
+ args.frame_time - first_frame_timestamp_ >= time_delta_to_report_;
+}
+
+std::unique_ptr<FrameSequenceMetrics> FrameSequenceTracker::TakeMetrics() {
+ return std::move(metrics_);
}
base::Optional<int> FrameSequenceMetrics::ThroughputData::ReportHistogram(
diff --git a/cc/metrics/frame_sequence_tracker.h b/cc/metrics/frame_sequence_tracker.h
index 5276aad40..ac18280 100644
--- a/cc/metrics/frame_sequence_tracker.h
+++ b/cc/metrics/frame_sequence_tracker.h
@@ -61,6 +61,7 @@
static std::unique_ptr<base::trace_event::TracedValue> ToTracedValue(
const ThroughputData& impl,
const ThroughputData& main);
+
// Returns the throughput in percent, a return value of base::nullopt
// indicates that no throughput metric is reported.
static base::Optional<int> ReportHistogram(
@@ -68,6 +69,12 @@
const char* thread_name,
int metric_index,
const ThroughputData& data);
+
+ void Merge(const ThroughputData& data) {
+ frames_expected += data.frames_expected;
+ frames_produced += data.frames_produced;
+ }
+
// Tracks the number of frames that were expected to be shown during this
// frame-sequence.
uint32_t frames_expected = 0;
@@ -77,6 +84,9 @@
uint32_t frames_produced = 0;
};
+ void Merge(std::unique_ptr<FrameSequenceMetrics> metrics);
+ bool HasEnoughDataForReporting() const;
+ bool HasDataLeftForReporting() const;
void ReportMetrics();
ThroughputData& impl_throughput() { return impl_throughput_; }
@@ -178,6 +188,10 @@
// LayerTreeHostImpl::ukm_manager_ lives as long as the LayerTreeHostImpl, so
// this pointer should never be null as long as LayerTreeHostImpl is alive.
UkmManager* ukm_manager_ = nullptr;
+
+ base::flat_map<FrameSequenceTrackerType,
+ std::unique_ptr<FrameSequenceMetrics>>
+ accumulated_metrics_;
};
// Tracks a sequence of frames to determine the throughput. It tracks this by
@@ -250,6 +264,11 @@
// Returns true if we should ask this tracker to report its throughput data.
bool ShouldReportMetricsNow(const viz::BeginFrameArgs& args) const;
+ FrameSequenceMetrics* metrics() { return metrics_.get(); }
+ FrameSequenceTrackerType type() const { return type_; }
+
+ std::unique_ptr<FrameSequenceMetrics> TakeMetrics();
+
private:
friend class FrameSequenceTrackerCollection;
friend class FrameSequenceTrackerTest;
@@ -259,10 +278,10 @@
ThroughputUkmReporter* throughput_ukm_reporter);
FrameSequenceMetrics::ThroughputData& impl_throughput() {
- return metrics_.impl_throughput();
+ return metrics_->impl_throughput();
}
FrameSequenceMetrics::ThroughputData& main_throughput() {
- return metrics_.main_throughput();
+ return metrics_->main_throughput();
}
void ScheduleTerminate() {
@@ -313,7 +332,7 @@
TrackedFrameData begin_impl_frame_data_;
TrackedFrameData begin_main_frame_data_;
- FrameSequenceMetrics metrics_;
+ std::unique_ptr<FrameSequenceMetrics> metrics_;
CheckerboardingData checkerboarding_;
diff --git a/cc/metrics/frame_sequence_tracker_unittest.cc b/cc/metrics/frame_sequence_tracker_unittest.cc
index 2393aeb..d2c1fa4 100644
--- a/cc/metrics/frame_sequence_tracker_unittest.cc
+++ b/cc/metrics/frame_sequence_tracker_unittest.cc
@@ -128,7 +128,9 @@
"Graphics.Smoothness.Throughput.SlowerThread.TouchScroll", 1u);
// Test that both are reported.
- tracker_->main_throughput().frames_expected = 50u;
+ tracker_->impl_throughput().frames_expected = 100u;
+ tracker_->impl_throughput().frames_produced = 85u;
+ tracker_->main_throughput().frames_expected = 150u;
tracker_->main_throughput().frames_produced = 25u;
tracker_->ReportMetrics();
histogram_tester.ExpectTotalCount(
@@ -152,10 +154,10 @@
"Graphics.Smoothness.Throughput.SlowerThread.TouchScroll", 2u);
// Test the case where compositor and main thread have the same throughput.
- tracker_->impl_throughput().frames_expected = 20u;
- tracker_->impl_throughput().frames_produced = 18u;
- tracker_->main_throughput().frames_expected = 20u;
- tracker_->main_throughput().frames_produced = 18u;
+ tracker_->impl_throughput().frames_expected = 120u;
+ tracker_->impl_throughput().frames_produced = 118u;
+ tracker_->main_throughput().frames_expected = 120u;
+ tracker_->main_throughput().frames_produced = 118u;
tracker_->ReportMetrics();
histogram_tester.ExpectTotalCount(
"Graphics.Smoothness.Throughput.CompositorThread.TouchScroll", 3u);
@@ -187,16 +189,16 @@
return tracker_->ignored_frame_tokens_;
}
- FrameSequenceMetrics::ThroughputData ImplThroughput() const {
+ FrameSequenceMetrics::ThroughputData& ImplThroughput() const {
return tracker_->impl_throughput();
}
- FrameSequenceMetrics::ThroughputData MainThroughput() const {
+ FrameSequenceMetrics::ThroughputData& MainThroughput() const {
return tracker_->main_throughput();
}
protected:
uint32_t number_of_frames_checkerboarded() const {
- return tracker_->metrics_.frames_checkerboarded();
+ return tracker_->metrics_->frames_checkerboarded();
}
std::unique_ptr<CompositorFrameReportingController>
@@ -374,6 +376,7 @@
EXPECT_EQ(NumberOfTrackers(), 1u);
EXPECT_EQ(NumberOfRemovalTrackers(), 0u);
+ ImplThroughput().frames_expected += 100;
// Now args.frame_time is 5s since the tracker creation time, so this tracker
// should be scheduled to report its throughput.
args = CreateBeginFrameArgs(source, ++sequence,
diff --git a/tools/perf/page_sets/rendering/throughput_test_cases.py b/tools/perf/page_sets/rendering/throughput_test_cases.py
index 1c82fb3..a83c3b9 100644
--- a/tools/perf/page_sets/rendering/throughput_test_cases.py
+++ b/tools/perf/page_sets/rendering/throughput_test_cases.py
@@ -12,7 +12,7 @@
def RunPageInteractions(self, action_runner):
with action_runner.CreateGestureInteraction('AnimationOnTap'):
action_runner.PressKey(' ')
- action_runner.Wait(5)
+ action_runner.Wait(10)
action_runner.PressKey(' ')