From 2a45d487d7231de838a74e1b1ee089a216813e8a Mon Sep 17 00:00:00 2001 From: Sadrul Habib Chowdhury Date: Mon, 20 Apr 2020 18:53:17 +0000 Subject: [PATCH] [cc/metrics] Use std::bitset instead of base::flat_set. The CompositorFrameReportingController keeps track of the existing interactions in a base::flat_set. However, there are only a small number of such interactions possible at the same time, and a std::bitset is sufficient to track all of them. So use the more efficient data type. This makes it possible to simply copy the bitset for each CompositorFrameReporter instance, rather than keeping a raw pointer back to the set owned by the controller. This also avoids a data-race when the reporting of the metrics is moved to a separate thread (in crrev.com/2135418). BUG=1072740 Change-Id: If325dbcb9846c2de0c7bbc884585596926c64970 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2156106 Commit-Queue: Sadrul Chowdhury Reviewed-by: Behdad Bakhshinategh Cr-Commit-Position: refs/heads/master@{#760608} --- cc/metrics/compositor_frame_reporter.cc | 12 +++++++----- cc/metrics/compositor_frame_reporter.h | 18 ++++++++++-------- .../compositor_frame_reporter_unittest.cc | 3 +-- .../compositor_frame_reporting_controller.cc | 8 ++++---- .../compositor_frame_reporting_controller.h | 2 +- cc/metrics/latency_ukm_reporter.cc | 2 +- cc/metrics/latency_ukm_reporter.h | 2 +- cc/trees/ukm_manager.cc | 8 ++++++-- cc/trees/ukm_manager.h | 2 +- 9 files changed, 32 insertions(+), 25 deletions(-) diff --git a/cc/metrics/compositor_frame_reporter.cc b/cc/metrics/compositor_frame_reporter.cc index 7db4af10c26dd8..f7a11bb874942a 100644 --- a/cc/metrics/compositor_frame_reporter.cc +++ b/cc/metrics/compositor_frame_reporter.cc @@ -201,7 +201,7 @@ std::string GetEventLatencyHistogramBaseName( } // namespace CompositorFrameReporter::CompositorFrameReporter( - const base::flat_set* active_trackers, + const ActiveTrackers& active_trackers, const viz::BeginFrameId& id, const base::TimeTicks frame_deadline, LatencyUkmReporter* latency_ukm_reporter, @@ -401,10 +401,12 @@ void CompositorFrameReporter::ReportCompositorLatencyHistograms() const { UMA_HISTOGRAM_ENUMERATION("CompositorLatency.Type", report_type_); for (const StageData& stage : stage_history_) { ReportStageHistogramWithBreakdown(stage); - - for (const auto& frame_sequence_tracker_type : *active_trackers_) { - // Report stage breakdowns. - ReportStageHistogramWithBreakdown(stage, frame_sequence_tracker_type); + for (size_t type = 0; type < active_trackers_.size(); ++type) { + if (active_trackers_.test(type)) { + // Report stage breakdowns. + ReportStageHistogramWithBreakdown( + stage, static_cast(type)); + } } } if (latency_ukm_reporter_) { diff --git a/cc/metrics/compositor_frame_reporter.h b/cc/metrics/compositor_frame_reporter.h index c4c8e82378647b..e558ade2149b64 100644 --- a/cc/metrics/compositor_frame_reporter.h +++ b/cc/metrics/compositor_frame_reporter.h @@ -5,10 +5,10 @@ #ifndef CC_METRICS_COMPOSITOR_FRAME_REPORTER_H_ #define CC_METRICS_COMPOSITOR_FRAME_REPORTER_H_ +#include #include #include -#include "base/containers/flat_set.h" #include "base/optional.h" #include "base/time/time.h" #include "cc/base/base_export.h" @@ -121,12 +121,14 @@ class CC_EXPORT CompositorFrameReporter { ~StageData(); }; - CompositorFrameReporter( - const base::flat_set* active_trackers, - const viz::BeginFrameId& id, - const base::TimeTicks frame_deadline, - LatencyUkmReporter* latency_ukm_reporter, - bool should_report_metrics); + using ActiveTrackers = + std::bitset(FrameSequenceTrackerType::kMaxType)>; + + CompositorFrameReporter(const ActiveTrackers& active_trackers, + const viz::BeginFrameId& id, + const base::TimeTicks frame_deadline, + LatencyUkmReporter* latency_ukm_reporter, + bool should_report_metrics); ~CompositorFrameReporter(); CompositorFrameReporter(const CompositorFrameReporter& reporter) = delete; @@ -239,7 +241,7 @@ class CC_EXPORT CompositorFrameReporter { FrameTerminationStatus frame_termination_status_ = FrameTerminationStatus::kUnknown; - const base::flat_set* active_trackers_; + const ActiveTrackers active_trackers_; LatencyUkmReporter* latency_ukm_reporter_; diff --git a/cc/metrics/compositor_frame_reporter_unittest.cc b/cc/metrics/compositor_frame_reporter_unittest.cc index 80b340d1e9ee89..29c1f5530172a0 100644 --- a/cc/metrics/compositor_frame_reporter_unittest.cc +++ b/cc/metrics/compositor_frame_reporter_unittest.cc @@ -27,10 +27,9 @@ MATCHER(IsWhitelisted, class CompositorFrameReporterTest : public testing::Test { public: - const base::flat_set active_trackers = {}; CompositorFrameReporterTest() : pipeline_reporter_(std::make_unique( - &active_trackers, + CompositorFrameReporter::ActiveTrackers(), viz::BeginFrameId(), base::TimeTicks() + base::TimeDelta::FromMilliseconds(16), nullptr, diff --git a/cc/metrics/compositor_frame_reporting_controller.cc b/cc/metrics/compositor_frame_reporting_controller.cc index 0f88864908b62f..7c38a6032e3b6d 100644 --- a/cc/metrics/compositor_frame_reporting_controller.cc +++ b/cc/metrics/compositor_frame_reporting_controller.cc @@ -69,7 +69,7 @@ void CompositorFrameReportingController::WillBeginImplFrame( } std::unique_ptr reporter = std::make_unique( - &active_trackers_, args.frame_id, + active_trackers_, args.frame_id, args.frame_time + (args.interval * 1.5), latency_ukm_reporter_.get(), should_report_metrics_); reporter->StartStage(StageType::kBeginImplFrameToSendBeginMainFrame, @@ -109,7 +109,7 @@ void CompositorFrameReportingController::WillBeginMainFrame( // deadline yet). So will start a new reporter at BeginMainFrame. std::unique_ptr reporter = std::make_unique( - &active_trackers_, args.frame_id, + active_trackers_, args.frame_id, args.frame_time + (args.interval * 1.5), latency_ukm_reporter_.get(), should_report_metrics_); reporter->StartStage(StageType::kSendBeginMainFrameToCommit, now); @@ -335,12 +335,12 @@ void CompositorFrameReportingController::SetBlinkBreakdown( void CompositorFrameReportingController::AddActiveTracker( FrameSequenceTrackerType type) { - active_trackers_.insert(type); + active_trackers_.set(static_cast(type)); } void CompositorFrameReportingController::RemoveActiveTracker( FrameSequenceTrackerType type) { - active_trackers_.erase(type); + active_trackers_.reset(static_cast(type)); } void CompositorFrameReportingController::AdvanceReporterStage( diff --git a/cc/metrics/compositor_frame_reporting_controller.h b/cc/metrics/compositor_frame_reporting_controller.h index d3edd9cdacc1aa..09fcda5fafb88c 100644 --- a/cc/metrics/compositor_frame_reporting_controller.h +++ b/cc/metrics/compositor_frame_reporting_controller.h @@ -104,7 +104,7 @@ class CC_EXPORT CompositorFrameReportingController { viz::BeginFrameId last_submitted_frame_id_; bool next_activate_has_invalidation_ = false; - base::flat_set active_trackers_; + CompositorFrameReporter::ActiveTrackers active_trackers_; // The latency reporter passed to each CompositorFrameReporter. Owned here // because it must be common among all reporters. diff --git a/cc/metrics/latency_ukm_reporter.cc b/cc/metrics/latency_ukm_reporter.cc index 9678edee540a9a..072ccb31daf153 100644 --- a/cc/metrics/latency_ukm_reporter.cc +++ b/cc/metrics/latency_ukm_reporter.cc @@ -12,7 +12,7 @@ namespace cc { void LatencyUkmReporter::ReportLatencyUkm( CompositorFrameReporter::FrameReportType report_type, const std::vector& stage_history, - const base::flat_set* active_trackers, + const CompositorFrameReporter::ActiveTrackers& active_trackers, const viz::FrameTimingDetails& viz_breakdown) { if (!ukm_manager_) return; diff --git a/cc/metrics/latency_ukm_reporter.h b/cc/metrics/latency_ukm_reporter.h index 9bbc810cec4bcc..3ee27823a4bd08 100644 --- a/cc/metrics/latency_ukm_reporter.h +++ b/cc/metrics/latency_ukm_reporter.h @@ -23,7 +23,7 @@ class CC_EXPORT LatencyUkmReporter { void ReportLatencyUkm( CompositorFrameReporter::FrameReportType report_type, const std::vector& stage_history, - const base::flat_set* active_trackers, + const CompositorFrameReporter::ActiveTrackers& active_trackers, const viz::FrameTimingDetails& viz_breakdown); void SetUkmManager(UkmManager* manager) { ukm_manager_ = manager; } diff --git a/cc/trees/ukm_manager.cc b/cc/trees/ukm_manager.cc index 5a9e68cf8f2dbf..2141cf1a21c700 100644 --- a/cc/trees/ukm_manager.cc +++ b/cc/trees/ukm_manager.cc @@ -190,7 +190,7 @@ void UkmManager::RecordAggregateThroughput(AggregationType aggregation_type, void UkmManager::RecordLatencyUKM( CompositorFrameReporter::FrameReportType report_type, const std::vector& stage_history, - const base::flat_set* active_trackers, + const CompositorFrameReporter::ActiveTrackers& active_trackers, const viz::FrameTimingDetails& viz_breakdown) const { ukm::builders::Graphics_Smoothness_Latency builder(source_id_); @@ -258,7 +258,11 @@ void UkmManager::RecordLatencyUKM( } // Record the active trackers - for (const auto& frame_sequence_tracker_type : *active_trackers) { + for (size_t type = 0; type < active_trackers.size(); ++type) { + if (!active_trackers.test(type)) + continue; + const auto frame_sequence_tracker_type = + static_cast(type); if (frame_sequence_tracker_type == FrameSequenceTrackerType::kUniversal) continue; switch (frame_sequence_tracker_type) { diff --git a/cc/trees/ukm_manager.h b/cc/trees/ukm_manager.h index ee5e5c38dc0dc3..50653814f85692 100644 --- a/cc/trees/ukm_manager.h +++ b/cc/trees/ukm_manager.h @@ -51,7 +51,7 @@ class CC_EXPORT UkmManager { void RecordLatencyUKM( CompositorFrameReporter::FrameReportType report_type, const std::vector& stage_history, - const base::flat_set* active_trackers, + const CompositorFrameReporter::ActiveTrackers& active_trackers, const viz::FrameTimingDetails& viz_breakdown) const; ukm::UkmRecorder* recorder_for_testing() { return recorder_.get(); }