Skip to content

Commit

Permalink
[cc/metrics] Use std::bitset instead of base::flat_set.
Browse files Browse the repository at this point in the history
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 <sadrul@chromium.org>
Reviewed-by: Behdad Bakhshinategh <behdadb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760608}
  • Loading branch information
sadrulhc authored and Commit Bot committed Apr 20, 2020
1 parent 9dc8f31 commit 2a45d48
Show file tree
Hide file tree
Showing 9 changed files with 32 additions and 25 deletions.
12 changes: 7 additions & 5 deletions cc/metrics/compositor_frame_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ std::string GetEventLatencyHistogramBaseName(
} // namespace

CompositorFrameReporter::CompositorFrameReporter(
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const ActiveTrackers& active_trackers,
const viz::BeginFrameId& id,
const base::TimeTicks frame_deadline,
LatencyUkmReporter* latency_ukm_reporter,
Expand Down Expand Up @@ -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<FrameSequenceTrackerType>(type));
}
}
}
if (latency_ukm_reporter_) {
Expand Down
18 changes: 10 additions & 8 deletions cc/metrics/compositor_frame_reporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
#ifndef CC_METRICS_COMPOSITOR_FRAME_REPORTER_H_
#define CC_METRICS_COMPOSITOR_FRAME_REPORTER_H_

#include <bitset>
#include <memory>
#include <vector>

#include "base/containers/flat_set.h"
#include "base/optional.h"
#include "base/time/time.h"
#include "cc/base/base_export.h"
Expand Down Expand Up @@ -121,12 +121,14 @@ class CC_EXPORT CompositorFrameReporter {
~StageData();
};

CompositorFrameReporter(
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const viz::BeginFrameId& id,
const base::TimeTicks frame_deadline,
LatencyUkmReporter* latency_ukm_reporter,
bool should_report_metrics);
using ActiveTrackers =
std::bitset<static_cast<size_t>(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;
Expand Down Expand Up @@ -239,7 +241,7 @@ class CC_EXPORT CompositorFrameReporter {
FrameTerminationStatus frame_termination_status_ =
FrameTerminationStatus::kUnknown;

const base::flat_set<FrameSequenceTrackerType>* active_trackers_;
const ActiveTrackers active_trackers_;

LatencyUkmReporter* latency_ukm_reporter_;

Expand Down
3 changes: 1 addition & 2 deletions cc/metrics/compositor_frame_reporter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ MATCHER(IsWhitelisted,

class CompositorFrameReporterTest : public testing::Test {
public:
const base::flat_set<FrameSequenceTrackerType> active_trackers = {};
CompositorFrameReporterTest()
: pipeline_reporter_(std::make_unique<CompositorFrameReporter>(
&active_trackers,
CompositorFrameReporter::ActiveTrackers(),
viz::BeginFrameId(),
base::TimeTicks() + base::TimeDelta::FromMilliseconds(16),
nullptr,
Expand Down
8 changes: 4 additions & 4 deletions cc/metrics/compositor_frame_reporting_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void CompositorFrameReportingController::WillBeginImplFrame(
}
std::unique_ptr<CompositorFrameReporter> reporter =
std::make_unique<CompositorFrameReporter>(
&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,
Expand Down Expand Up @@ -109,7 +109,7 @@ void CompositorFrameReportingController::WillBeginMainFrame(
// deadline yet). So will start a new reporter at BeginMainFrame.
std::unique_ptr<CompositorFrameReporter> reporter =
std::make_unique<CompositorFrameReporter>(
&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);
Expand Down Expand Up @@ -335,12 +335,12 @@ void CompositorFrameReportingController::SetBlinkBreakdown(

void CompositorFrameReportingController::AddActiveTracker(
FrameSequenceTrackerType type) {
active_trackers_.insert(type);
active_trackers_.set(static_cast<size_t>(type));
}

void CompositorFrameReportingController::RemoveActiveTracker(
FrameSequenceTrackerType type) {
active_trackers_.erase(type);
active_trackers_.reset(static_cast<size_t>(type));
}

void CompositorFrameReportingController::AdvanceReporterStage(
Expand Down
2 changes: 1 addition & 1 deletion cc/metrics/compositor_frame_reporting_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class CC_EXPORT CompositorFrameReportingController {
viz::BeginFrameId last_submitted_frame_id_;

bool next_activate_has_invalidation_ = false;
base::flat_set<FrameSequenceTrackerType> active_trackers_;
CompositorFrameReporter::ActiveTrackers active_trackers_;

// The latency reporter passed to each CompositorFrameReporter. Owned here
// because it must be common among all reporters.
Expand Down
2 changes: 1 addition & 1 deletion cc/metrics/latency_ukm_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace cc {
void LatencyUkmReporter::ReportLatencyUkm(
CompositorFrameReporter::FrameReportType report_type,
const std::vector<CompositorFrameReporter::StageData>& stage_history,
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const CompositorFrameReporter::ActiveTrackers& active_trackers,
const viz::FrameTimingDetails& viz_breakdown) {
if (!ukm_manager_)
return;
Expand Down
2 changes: 1 addition & 1 deletion cc/metrics/latency_ukm_reporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class CC_EXPORT LatencyUkmReporter {
void ReportLatencyUkm(
CompositorFrameReporter::FrameReportType report_type,
const std::vector<CompositorFrameReporter::StageData>& stage_history,
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const CompositorFrameReporter::ActiveTrackers& active_trackers,
const viz::FrameTimingDetails& viz_breakdown);

void SetUkmManager(UkmManager* manager) { ukm_manager_ = manager; }
Expand Down
8 changes: 6 additions & 2 deletions cc/trees/ukm_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ void UkmManager::RecordAggregateThroughput(AggregationType aggregation_type,
void UkmManager::RecordLatencyUKM(
CompositorFrameReporter::FrameReportType report_type,
const std::vector<CompositorFrameReporter::StageData>& stage_history,
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const CompositorFrameReporter::ActiveTrackers& active_trackers,
const viz::FrameTimingDetails& viz_breakdown) const {
ukm::builders::Graphics_Smoothness_Latency builder(source_id_);

Expand Down Expand Up @@ -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<FrameSequenceTrackerType>(type);
if (frame_sequence_tracker_type == FrameSequenceTrackerType::kUniversal)
continue;
switch (frame_sequence_tracker_type) {
Expand Down
2 changes: 1 addition & 1 deletion cc/trees/ukm_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class CC_EXPORT UkmManager {
void RecordLatencyUKM(
CompositorFrameReporter::FrameReportType report_type,
const std::vector<CompositorFrameReporter::StageData>& stage_history,
const base::flat_set<FrameSequenceTrackerType>* active_trackers,
const CompositorFrameReporter::ActiveTrackers& active_trackers,
const viz::FrameTimingDetails& viz_breakdown) const;

ukm::UkmRecorder* recorder_for_testing() { return recorder_.get(); }
Expand Down

0 comments on commit 2a45d48

Please sign in to comment.