Skip to content

Commit

Permalink
[cc/metrics] Change how main-damage is tracked.
Browse files Browse the repository at this point in the history 
It is possible that the args for begin-main-frames can be processed out
of order. For example, if the first BeginFrameArgs emits a
begin-main-frame, then it is possible that the next BeginFrameArgs can
be handled and responded as 'no damage from main' before the main-thread
responds to the begin-main-frame. So update the sequence-trackers to
handle this case correctly.

BUG=1021963

Change-Id: I28a838d7a1eadb0ead93d0ac7fbafc47a62af789
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1967952
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725194}
  • Loading branch information
@sadrulhc
sadrulhc authored and Commit Bot committed Dec 16, 2019
1 parent 41f466c commit dc34c8b
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 15 deletions.
36 changes: 23 additions & 13 deletions cc/metrics/frame_sequence_tracker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,15 +428,15 @@ void FrameSequenceTracker::ReportBeginImplFrame(

if (ShouldIgnoreBeginFrameSource(args.source_id))
return;

#if DCHECK_IS_ON()
DCHECK(!is_inside_frame_) << TRACKER_DCHECK_MSG;
is_inside_frame_ = true;
#endif

#if DCHECK_IS_ON()
if (args.type == viz::BeginFrameArgs::NORMAL)
impl_frames_.insert(std::make_pair(args.source_id, args.sequence_number));
#endif

TRACKER_TRACE_STREAM << "b(" << args.sequence_number << ")";
UpdateTrackedFrameData(&begin_impl_frame_data_, args.source_id,
args.sequence_number);
Expand Down Expand Up @@ -470,6 +470,8 @@ void FrameSequenceTracker::ReportBeginMainFrame(
<< args.sequence_number << ")";
UpdateTrackedFrameData(&begin_main_frame_data_, args.source_id,
args.sequence_number);
if (!first_received_main_sequence_)
first_received_main_sequence_ = args.sequence_number;
main_throughput().frames_expected +=
begin_main_frame_data_.previous_sequence_delta;
}
Expand All @@ -494,18 +496,25 @@ void FrameSequenceTracker::ReportSubmitFrame(
last_submitted_frame_ = frame_token;

TRACKER_TRACE_STREAM << 's';
const bool main_changes_after_sequence_started =
first_received_main_sequence_ &&
origin_args.sequence_number >= first_received_main_sequence_;
const bool main_changes_include_new_changes =
last_submitted_main_sequence_ == 0 ||
origin_args.sequence_number > last_submitted_main_sequence_;
const bool main_change_had_no_damage =
last_no_main_damage_sequence_ != 0 &&
origin_args.sequence_number == last_no_main_damage_sequence_;

if (!ShouldIgnoreBeginFrameSource(origin_args.source_id) &&
begin_main_frame_data_.previous_sequence &&
origin_args.sequence_number >= begin_main_frame_data_.previous_sequence) {
if (last_submitted_main_sequence_ == 0 ||
origin_args.sequence_number > last_submitted_main_sequence_) {
TRACKER_TRACE_STREAM << 'S';

last_submitted_main_sequence_ = origin_args.sequence_number;
main_frames_.push_back(frame_token);
DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
<< TRACKER_DCHECK_MSG;
}
main_changes_after_sequence_started && main_changes_include_new_changes &&
!main_change_had_no_damage) {
TRACKER_TRACE_STREAM << 'S';

last_submitted_main_sequence_ = origin_args.sequence_number;
main_frames_.push_back(frame_token);
DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
<< TRACKER_DCHECK_MSG;
}

if (has_missing_content) {
Expand Down Expand Up @@ -661,6 +670,7 @@ void FrameSequenceTracker::ReportMainFrameCausedNoDamage(
DCHECK_GT(main_throughput().frames_expected,
main_throughput().frames_produced)
<< TRACKER_DCHECK_MSG;
last_no_main_damage_sequence_ = args.sequence_number;
--main_throughput().frames_expected;
DCHECK_GE(main_throughput().frames_expected, main_frames_.size())
<< TRACKER_DCHECK_MSG;
Expand Down
9 changes: 9 additions & 0 deletions cc/metrics/frame_sequence_tracker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,11 @@ class CC_EXPORT FrameSequenceTracker {
// corresponding frame-token is removed from this collection.
base::circular_deque<uint32_t> main_frames_;

// Keeps track of the sequence-number of the first received begin-main-frame.
// This is used to ignore submitted frames that include updates from earlier
// begin-main-frames.
uint64_t first_received_main_sequence_ = 0;

// Keeps track of the first submitted compositor-frame. This is used to ignore
// reports from frames that were submitted before this tracker had been
// created.
Expand All @@ -359,6 +364,10 @@ class CC_EXPORT FrameSequenceTracker {
// main-thread.
uint64_t last_submitted_main_sequence_ = 0;

// Keeps track of the last sequence-number that produced a frame that did not
// have any damage from the main-thread.
uint64_t last_no_main_damage_sequence_ = 0;

// The time when this tracker is created, or the time when it was previously
// scheduled to report histogram.
base::TimeTicks first_frame_timestamp_;
Expand Down
88 changes: 86 additions & 2 deletions cc/metrics/frame_sequence_tracker_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/test/metrics/histogram_tester.h"
#include "cc/metrics/compositor_frame_reporting_controller.h"
#include "components/viz/common/frame_sinks/begin_frame_args.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/presentation_feedback.h"

Expand Down Expand Up @@ -115,7 +116,7 @@ class FrameSequenceTrackerTest : public testing::Test {
EXPECT_TRUE(collection_.removal_trackers_.empty());
}

void ReportMetrics() {
void ReportMetricsTest() {
base::HistogramTester histogram_tester;

// Test that there is no main thread frames expected.
Expand Down Expand Up @@ -169,6 +170,8 @@ class FrameSequenceTrackerTest : public testing::Test {
"Graphics.Smoothness.Throughput.SlowerThread.TouchScroll", 3u);
}

void ReportMetrics() { tracker_->ReportMetrics(); }

base::TimeDelta TimeDeltaToReort() const {
return tracker_->time_delta_to_report_;
}
Expand Down Expand Up @@ -362,7 +365,7 @@ TEST_F(FrameSequenceTrackerTest, MultipleCheckerboardingFrames) {
}

TEST_F(FrameSequenceTrackerTest, ReportMetrics) {
ReportMetrics();
ReportMetricsTest();
}

TEST_F(FrameSequenceTrackerTest, ReportMetricsAtFixedInterval) {
Expand Down Expand Up @@ -427,4 +430,85 @@ TEST_F(FrameSequenceTrackerTest, MainFrameTracking) {
collection_.NotifyFramePresented(frame_2, feedback);
}

TEST_F(FrameSequenceTrackerTest, MainFrameNoDamageTracking) {
const uint64_t source = 1;
uint64_t sequence = 0;

const auto first_args = CreateBeginFrameArgs(source, ++sequence);
DispatchCompleteFrame(first_args, kImplDamage | kMainDamage);

// Now, start the next frame, but for main, respond with the previous args.
const auto second_args = CreateBeginFrameArgs(source, ++sequence);
StartImplAndMainFrames(second_args);

uint32_t frame_token = NextFrameToken();
collection_.NotifySubmitFrame(frame_token, /*has_missing_content=*/false,
viz::BeginFrameAck(second_args, true),
first_args);
collection_.NotifyFrameEnd(second_args);

// Start and submit the next frame, with no damage from main.
auto args = CreateBeginFrameArgs(source, ++sequence);
StartImplAndMainFrames(args);
frame_token = NextFrameToken();
collection_.NotifyMainFrameCausedNoDamage(args);
collection_.NotifySubmitFrame(frame_token, /*has_missing_content=*/false,
viz::BeginFrameAck(args, true), first_args);
collection_.NotifyFrameEnd(args);

// Now, submit a frame with damage from main from |second_args|.
args = CreateBeginFrameArgs(source, ++sequence);
StartImplAndMainFrames(args);
frame_token = NextFrameToken();
collection_.NotifySubmitFrame(frame_token, /*has_missing_content=*/false,
viz::BeginFrameAck(args, true), second_args);
collection_.NotifyFrameEnd(args);
}

TEST_F(FrameSequenceTrackerTest, BeginMainFrameSubmit) {
const uint64_t source = 1;
uint64_t sequence = 0;

// Start with a bunch of frames so that the metric does get reported at the
// end of the test.
ImplThroughput().frames_expected = 98u;
ImplThroughput().frames_produced = 98u;
MainThroughput().frames_expected = 98u;
MainThroughput().frames_produced = 98u;

// Start a frame, send to main, but end the frame with no-damage before main
// responds.
auto first_args = CreateBeginFrameArgs(source, ++sequence);
collection_.NotifyBeginImplFrame(first_args);
collection_.NotifyBeginMainFrame(first_args);
collection_.NotifyImplFrameCausedNoDamage(
viz::BeginFrameAck(first_args, false));
collection_.NotifyFrameEnd(first_args);

// Start another frame, send to begin, but submit with main-update from the
// first frame (main thread has finally responded by this time to the first
// frame).
auto second_args = CreateBeginFrameArgs(source, ++sequence);
collection_.NotifyBeginImplFrame(second_args);
collection_.NotifyBeginMainFrame(second_args);
uint32_t frame_token = NextFrameToken();
collection_.NotifySubmitFrame(frame_token, /*has_missing_content=*/false,
viz::BeginFrameAck(second_args, true),
first_args);
collection_.NotifyFrameEnd(second_args);

// When the frame is presented, the main-frame should count towards its
// throughput.
base::HistogramTester histogram_tester;
const auto interval = viz::BeginFrameArgs::DefaultInterval();
gfx::PresentationFeedback feedback(base::TimeTicks::Now(), interval, 0);
collection_.NotifyFramePresented(frame_token, feedback);
ReportMetrics();

const char metric[] = "Graphics.Smoothness.Throughput.MainThread.TouchScroll";
histogram_tester.ExpectTotalCount(metric, 1u);
EXPECT_THAT(histogram_tester.GetAllSamples(metric),
testing::ElementsAre(base::Bucket(99, 1)));
}

} // namespace cc

0 comments on commit dc34c8b

Please sign in to comment.