Skip to content

Commit

Permalink
Remove expired DrawIntervalWithCustomPropertyAnimations2
Browse files Browse the repository at this point in the history
This metric has been expired for almost a year. It also had known issues
with its calculation.

This change removes it and the piping used to calculate it.

OBSOLETE_HISTOGRAM[Scheduling.Renderer.DrawIntervalWithCustomPropertyAnimations2]=remove expired metric

Bug: 1251627
Change-Id: I05e5bde32baec7004b42b2b8659dbde7a3c118ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4512700
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150337}
  • Loading branch information
Jonathan Ross authored and Chromium LUCI CQ committed May 29, 2023
1 parent be395ee commit 292358a
Show file tree
Hide file tree
Showing 11 changed files with 6 additions and 79 deletions.
28 changes: 1 addition & 27 deletions cc/metrics/compositor_timing_history.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ class CompositorTimingHistory::UMAReporter {
virtual void AddBeginImplFrameLatency(base::TimeDelta delta) = 0;
virtual void AddDrawDuration(base::TimeDelta duration) = 0;

// crbug.com/758439: the following functions are used to report timing in
// certain conditions targeting blink / compositor animations.
// Only the renderer would get the meaningful data.
virtual void AddDrawIntervalWithCustomPropertyAnimations(
base::TimeDelta duration) = 0;

virtual void AddImplFrameDeadlineType(
CompositorTimingHistory::DeadlineMode deadline_mode) = 0;
};
Expand Down Expand Up @@ -329,13 +323,6 @@ class RendererUMAReporter : public CompositorTimingHistory::UMAReporter {
interval);
}

void AddDrawIntervalWithCustomPropertyAnimations(
base::TimeDelta interval) override {
UMA_HISTOGRAM_CUSTOM_TIMES_VSYNC_ALIGNED(
"Scheduling.Renderer.DrawIntervalWithCustomPropertyAnimations",
interval);
}

void AddBeginImplFrameLatency(base::TimeDelta delta) override {
UMA_HISTOGRAM_CUSTOM_TIMES_DURATION(
"Scheduling.Renderer.BeginImplFrameLatency", delta);
Expand All @@ -361,9 +348,6 @@ class BrowserUMAReporter : public CompositorTimingHistory::UMAReporter {
// browser rendering fps is not at 60.
void AddDrawInterval(base::TimeDelta interval) override {}

void AddDrawIntervalWithCustomPropertyAnimations(
base::TimeDelta interval) override {}

void AddBeginImplFrameLatency(base::TimeDelta delta) override {
UMA_HISTOGRAM_CUSTOM_TIMES_DURATION(
"Scheduling.Browser.BeginImplFrameLatency", delta);
Expand All @@ -385,8 +369,6 @@ class NullUMAReporter : public CompositorTimingHistory::UMAReporter {
public:
~NullUMAReporter() override = default;
void AddDrawInterval(base::TimeDelta interval) override {}
void AddDrawIntervalWithCustomPropertyAnimations(
base::TimeDelta inverval) override {}
void AddBeginImplFrameLatency(base::TimeDelta delta) override {}
void AddDrawDuration(base::TimeDelta duration) override {}
void AddImplFrameDeadlineType(
Expand Down Expand Up @@ -771,8 +753,7 @@ void CompositorTimingHistory::WillDraw() {
draw_start_time_ = Now();
}

void CompositorTimingHistory::DidDraw(bool used_new_active_tree,
bool has_custom_property_animations) {
void CompositorTimingHistory::DidDraw() {
DCHECK_NE(base::TimeTicks(), draw_start_time_);
base::TimeTicks draw_end_time = Now();
base::TimeDelta draw_duration = draw_end_time - draw_start_time_;
Expand Down Expand Up @@ -807,16 +788,9 @@ void CompositorTimingHistory::DidDraw(bool used_new_active_tree,
draw_end_time);
g_num_long_draw_intervals++;
}
if (has_custom_property_animations &&
previous_frame_had_custom_property_animations_)
uma_reporter_->AddDrawIntervalWithCustomPropertyAnimations(draw_interval);
}
previous_frame_had_custom_property_animations_ =
has_custom_property_animations;
draw_end_time_prev_ = draw_end_time;

if (used_new_active_tree)
new_active_tree_draw_end_time_prev_ = draw_end_time;
draw_start_time_ = base::TimeTicks();
}

Expand Down
7 changes: 1 addition & 6 deletions cc/metrics/compositor_timing_history.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ class CC_EXPORT CompositorTimingHistory {
void WillActivate();
void DidActivate();
void WillDraw();
void DidDraw(bool used_new_active_tree,
bool has_custom_property_animations);
void DidDraw();
void WillInvalidateOnImplSide();

// Record the scheduler's deadline mode and send to UMA.
Expand Down Expand Up @@ -112,7 +111,6 @@ class CC_EXPORT CompositorTimingHistory {

// Used to calculate frame rates of Main and Impl threads.
bool compositor_drawing_continuously_;
base::TimeTicks new_active_tree_draw_end_time_prev_;
base::TimeTicks draw_end_time_prev_;

// If you add any history here, please remember to reset it in
Expand Down Expand Up @@ -169,9 +167,6 @@ class CC_EXPORT CompositorTimingHistory {

// Owned by LayerTreeHost and is destroyed when LayerTreeHost is destroyed.
raw_ptr<RenderingStatsInstrumentation> rendering_stats_instrumentation_;

// Used only for reporting animation targeted UMA.
bool previous_frame_had_custom_property_animations_ = false;
};

} // namespace cc
Expand Down
4 changes: 2 additions & 2 deletions cc/metrics/compositor_timing_history_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ TEST_F(CompositorTimingHistoryTest, AllSequential_Commit) {
AdvanceNowBy(one_second);
timing_history_.WillDraw();
AdvanceNowBy(draw_duration);
timing_history_.DidDraw(true, false);
timing_history_.DidDraw();

EXPECT_EQ(begin_main_frame_queue_duration,
timing_history_.BeginMainFrameQueueDurationCriticalEstimate());
Expand Down Expand Up @@ -173,7 +173,7 @@ TEST_F(CompositorTimingHistoryTest, AllSequential_BeginMainFrameAborted) {
AdvanceNowBy(one_second);
timing_history_.WillDraw();
AdvanceNowBy(draw_duration);
timing_history_.DidDraw(false, false);
timing_history_.DidDraw();

EXPECT_EQ(base::TimeDelta(),
timing_history_.BeginMainFrameQueueDurationCriticalEstimate());
Expand Down
9 changes: 2 additions & 7 deletions cc/scheduler/scheduler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -804,15 +804,11 @@ void Scheduler::FinishImplFrameSynchronous() {
void Scheduler::DrawIfPossible() {
DCHECK(!inside_scheduled_action_);
base::AutoReset<bool> mark_inside(&inside_scheduled_action_, true);
bool drawing_with_new_active_tree =
state_machine_.active_tree_needs_first_draw() &&
!state_machine_.previous_pending_tree_was_impl_side();
compositor_timing_history_->WillDraw();
state_machine_.WillDraw();
DrawResult result = client_->ScheduledActionDrawIfPossible();
state_machine_.DidDraw(result);
compositor_timing_history_->DidDraw(drawing_with_new_active_tree,
client_->HasInvalidationAnimation());
compositor_timing_history_->DidDraw();
}

void Scheduler::DrawForced() {
Expand All @@ -832,8 +828,7 @@ void Scheduler::DrawForced() {
state_machine_.WillDraw();
DrawResult result = client_->ScheduledActionDrawForced();
state_machine_.DidDraw(result);
compositor_timing_history_->DidDraw(drawing_with_new_active_tree,
client_->HasInvalidationAnimation());
compositor_timing_history_->DidDraw();
}

void Scheduler::SetDeferBeginMainFrame(bool defer_begin_main_frame) {
Expand Down
3 changes: 0 additions & 3 deletions cc/scheduler/scheduler.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,6 @@ class SchedulerClient {
base::TimeTicks time) = 0;
virtual void FrameIntervalUpdated(base::TimeDelta interval) = 0;

// Functions used for reporting animation targeting UMA, crbug.com/758439.
virtual bool HasInvalidationAnimation() const = 0;

protected:
virtual ~SchedulerClient() {}
};
Expand Down
2 changes: 0 additions & 2 deletions cc/scheduler/scheduler_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,6 @@ class FakeSchedulerClient : public SchedulerClient,
PushAction("RemoveObserver(this)");
}

bool HasInvalidationAnimation() const override { return false; }

protected:
bool InsideBeginImplFrameCallback(bool state) {
return inside_begin_impl_frame_ == state;
Expand Down
4 changes: 0 additions & 4 deletions cc/trees/proxy_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,6 @@ void ProxyImpl::SetVideoNeedsBeginFrames(bool needs_begin_frames) {
scheduler_->SetVideoNeedsBeginFrames(needs_begin_frames);
}

bool ProxyImpl::HasInvalidationAnimation() const {
return host_impl_->mutator_host()->HasInvalidationAnimation();
}

bool ProxyImpl::IsInsideDraw() {
return inside_draw_;
}
Expand Down
1 change: 0 additions & 1 deletion cc/trees/proxy_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient,
void ScheduledActionBeginMainFrameNotExpectedUntil(
base::TimeTicks time) override;
void FrameIntervalUpdated(base::TimeDelta interval) override {}
bool HasInvalidationAnimation() const override;

DrawResult DrawInternal(bool forced_draw);

Expand Down
7 changes: 0 additions & 7 deletions cc/trees/single_thread_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -525,13 +525,6 @@ void SingleThreadProxy::SetVideoNeedsBeginFrames(bool needs_begin_frames) {
scheduler_on_impl_thread_->SetVideoNeedsBeginFrames(needs_begin_frames);
}

bool SingleThreadProxy::HasInvalidationAnimation() const {
// DebugScopedSetImplThread here is just a formality; all SchedulerClient
// methods should have it.
DebugScopedSetImplThread impl(task_runner_provider_);
return false;
}

bool SingleThreadProxy::IsInsideDraw() {
DCHECK(!task_runner_provider_->HasImplThread() ||
task_runner_provider_->IsImplThread());
Expand Down
1 change: 0 additions & 1 deletion cc/trees/single_thread_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ class CC_EXPORT SingleThreadProxy : public Proxy,
void ScheduledActionBeginMainFrameNotExpectedUntil(
base::TimeTicks time) override;
void FrameIntervalUpdated(base::TimeDelta interval) override;
bool HasInvalidationAnimation() const override;

// LayerTreeHostImplClient implementation
void DidLoseLayerTreeFrameSinkOnImplThread() override;
Expand Down
19 changes: 0 additions & 19 deletions tools/metrics/histograms/metadata/scheduler/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -88,25 +88,6 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="Scheduling.Renderer.DrawIntervalWithCustomPropertyAnimations2"
units="microseconds" expires_after="2022-07-03">
<owner>xidachen@chromium.org</owner>
<owner>animations-dev@chromium.org</owner>
<summary>
The time delta between the draw times of back-to-back BeginImplFrames,
regardless of whether or not they result in a swap, when there is at least
one custom property animation.

The interval is only recorded when every BeginImplFrame wants to draw.

Warning: This metric may include reports from clients with low-resolution
clocks (i.e. on Windows, ref. |TimeTicks::IsHighResolution()|). Such reports
will cause this metric to have an abnormal distribution. When considering
revising this histogram, see UMA_HISTOGRAM_CUSTOM_MICROSECONDS_TIMES for the
solution.
</summary>
</histogram>

<histogram base="true" name="ThreadPool.NumTasksBeforeDetach" units="tasks"
expires_after="M85">
<owner>fdoray@chromium.org</owner>
Expand Down

0 comments on commit 292358a

Please sign in to comment.