Skip to content

Commit

Permalink
Address reviewer comment and add test
Browse files Browse the repository at this point in the history
  • Loading branch information
@hysw
hysw committed Jan 16, 2024
1 parent 557479c commit cbb395f
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 81 deletions.
24 changes: 12 additions & 12 deletions benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,9 @@ void GraphicsBenchmarkApp::SetupMetrics()
mMetricsData.metrics[MetricsData::kTypeBandwidth] = AllocateMetricID();
mMetricsData.metrics[MetricsData::kTypeGPUWorkDuration] = AllocateMetricID();

AddLiveMetric(mMetricsData.metrics[MetricsData::kTypeCPUSubmissionTime]);
AddLiveMetric(mMetricsData.metrics[MetricsData::kTypeBandwidth]);
AddLiveMetric(mMetricsData.metrics[MetricsData::kTypeGPUWorkDuration]);
BindLiveMetric(mMetricsData.metrics[MetricsData::kTypeCPUSubmissionTime]);
BindLiveMetric(mMetricsData.metrics[MetricsData::kTypeBandwidth]);
BindLiveMetric(mMetricsData.metrics[MetricsData::kTypeGPUWorkDuration]);
}

void GraphicsBenchmarkApp::SetupMetricsRun()
Expand All @@ -321,18 +321,18 @@ void GraphicsBenchmarkApp::SetupMetricsRun()
"ms",
ppx::metrics::MetricInterpretation::LOWER_IS_BETTER,
{0.f, 10000.f}};
ppx::metrics::MetricID res = AddMetric(metadata, mMetricsData.metrics[MetricsData::kTypeCPUSubmissionTime]);
PPX_ASSERT_MSG(res != ppx::metrics::kInvalidMetricID, "Failed to add CPU Submission Time metric");
bool bindres = BindMetric(mMetricsData.metrics[MetricsData::kTypeCPUSubmissionTime], metadata);
PPX_ASSERT_MSG(bindres, "Failed to bind CPU Submission Time metric");
}
{
ppx::metrics::MetricMetadata metadata = {
ppx::metrics::MetricType::GAUGE,
"Bandwidth",
"GB/s",
"GiB/s",
ppx::metrics::MetricInterpretation::HIGHER_IS_BETTER,
{0.f, 10000.f}};
ppx::metrics::MetricID res = AddMetric(metadata, mMetricsData.metrics[MetricsData::kTypeBandwidth]);
PPX_ASSERT_MSG(res != ppx::metrics::kInvalidMetricID, "Failed to add Bandwidth metric");
bool bindres = BindMetric(mMetricsData.metrics[MetricsData::kTypeBandwidth], metadata);
PPX_ASSERT_MSG(bindres, "Failed to bind Bandwidth metric");
}
}

Expand Down Expand Up @@ -1333,7 +1333,7 @@ void GraphicsBenchmarkApp::DrawExtraInfo()
ImGui::Text("Write Bandwidth");
ImGui::NextColumn();
ImGui::Text(
"%.2f GB/s min=%.2f max=%.2f\n%.2f%s%.2f GB/s",
"%.2f GiB/s min=%.2f max=%.2f\n%.2f%s%.2f GiB/s",
bandwidthLive.Latest(),
bandwidthLive.Min(),
bandwidthLive.Max(),
Expand All @@ -1346,17 +1346,17 @@ void GraphicsBenchmarkApp::DrawExtraInfo()
const auto bandwidth = GetGaugeBasicStatistics(mMetricsData.metrics[MetricsData::kTypeBandwidth]);
ImGui::Text("Average Write Bandwidth");
ImGui::NextColumn();
ImGui::Text("%.2f GB/s", bandwidth.average);
ImGui::Text("%.2f GiB/s", bandwidth.average);
ImGui::NextColumn();

ImGui::Text("Min Write Bandwidth");
ImGui::NextColumn();
ImGui::Text("%.2f GB/s", bandwidth.min);
ImGui::Text("%.2f GiB/s", bandwidth.min);
ImGui::NextColumn();

ImGui::Text("Max Write Bandwidth");
ImGui::NextColumn();
ImGui::Text("%.2f GB/s", bandwidth.max);
ImGui::Text("%.2f GiB/s", bandwidth.max);
ImGui::NextColumn();
}
}
Expand Down
17 changes: 9 additions & 8 deletions include/ppx/application.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ class Application
virtual void DispatchRender();
virtual void DispatchInitKnobs();
virtual void DispatchUpdateMetrics();
virtual void DrawGui(){}; // Draw additional project-related information to ImGui.
virtual void DrawGui() {}; // Draw additional project-related information to ImGui.

// Override these methods in a derived class to change the default behavior of metrics.
virtual void SetupMetrics(); // Called once on application startup
Expand Down Expand Up @@ -527,18 +527,19 @@ class Application
// Allocate a metric id to be used for a combind live/recorded metric.
metrics::MetricID AllocateMetricID();

// Adds a metric to the current run. If no run is active, returns metrics::kInvalidMetricID.
// See StartMetricsRun for why this wrapper is necessary.
metrics::MetricID AddMetric(const metrics::MetricMetadata& metadata);

// Bind a metric to the current run, Return false if no run is active.
bool BindMetric(metrics::MetricID metricID, const metrics::MetricMetadata& metadata);

// Add a live metric, the returned MetricID can also be used for recorded metric.
metrics::MetricID AddLiveMetric(metrics::MetricID metricID = metrics::kInvalidMetricID);
bool BindLiveMetric(metrics::MetricID metricID = metrics::kInvalidMetricID);

// Clear history of live metric, usually after knob changed.
void ClearLiveMetricsHistory();

// Adds a metric to the current run. If no run is active, returns metrics::kInvalidMetricID.
// See StartMetricsRun for why this wrapper is necessary.
metrics::MetricID AddMetric(
const metrics::MetricMetadata& metadata,
metrics::MetricID metricID = metrics::kInvalidMetricID);

// Record data for the given metric ID. Metrics for completed runs will be discarded.
// See StartMetricsRun for why this wrapper is necessary.
bool RecordMetricData(metrics::MetricID id, const metrics::MetricData& data);
Expand Down
22 changes: 8 additions & 14 deletions include/ppx/metrics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,23 +387,18 @@ class Manager final
bool HasActiveRun() const;

// Allocate a MetricID.
MetricID AllocateID() { return EnsureAllocateID(kInvalidMetricID); }
MetricID AllocateID();

// Adds a metric to the current run. A run must be started to add a metric.
// Failure to add a metric returns kInvalidMetricID.
// Optionally bind the metric to an existing metricID.
MetricID AddMetric(
const MetricMetadata& metadata,
MetricID metricID = kInvalidMetricID);

// Adds a live metric.
// Optionally bind the metric to an existing metricID.
MetricID AddLiveMetric(
double halfLife = LiveMetric::kDefaultHalfLife,
MetricID metricID = kInvalidMetricID);
MetricID AddMetric(const MetricMetadata& metadata)
{
MetricID metricID = AllocateID();
return BindMetric(metricID, metadata) ? metricID : kInvalidMetricID;
}

// Adds a live metric to current run.
void BindMetric(MetricID liveMetricID, const MetricMetadata& metadata);
bool BindMetric(MetricID metricID, const MetricMetadata& metadata);
bool BindLiveMetric(MetricID metricID, double halfLife = LiveMetric::kDefaultHalfLife);

// Records data for the given metric ID. Metrics for completed runs will be discarded.
bool RecordMetricData(MetricID id, const MetricData& data);
Expand Down Expand Up @@ -432,7 +427,6 @@ class Manager final

// Must be stored with the manager; Runs should not share MetricIDs.
MetricID mNextMetricID = kInvalidMetricID + 1;
MetricID EnsureAllocateID(MetricID reuseID);

// Convenient to store with the manager, so the hop of going through the Run isn't necessary.
std::unordered_map<MetricID, Metric*> mActiveMetrics;
Expand Down
50 changes: 31 additions & 19 deletions src/ppx/application.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1763,26 +1763,29 @@ void Application::SetupMetricsRun()
metadata.name = "cpu_frame_time";
metadata.unit = "ms";
metadata.interpretation = metrics::MetricInterpretation::LOWER_IS_BETTER;
metrics::MetricID res = mMetrics.manager.AddMetric(metadata, mMetrics.cpuFrameTimeId);
PPX_ASSERT_MSG(res != metrics::kInvalidMetricID, "Failed to create frame time metric");

bool bindres = mMetrics.manager.BindMetric(mMetrics.cpuFrameTimeId, metadata);
PPX_ASSERT_MSG(bindres, "Failed to create frame time metric");
}
{
metrics::MetricMetadata metadata = {};
metadata.type = metrics::MetricType::GAUGE;
metadata.name = "framerate";
metadata.unit = "";
metadata.interpretation = metrics::MetricInterpretation::HIGHER_IS_BETTER;
metrics::MetricID res = mMetrics.manager.AddMetric(metadata, mMetrics.framerateId);
PPX_ASSERT_MSG(res != metrics::kInvalidMetricID, "Failed to create framerate metric");

bool bindres = mMetrics.manager.BindMetric(mMetrics.framerateId, metadata);
PPX_ASSERT_MSG(bindres, "Failed to create framerate metric");
}
{
metrics::MetricMetadata metadata = {};
metadata.type = metrics::MetricType::COUNTER;
metadata.name = "frame_count";
metadata.unit = "";
metadata.interpretation = metrics::MetricInterpretation::NONE;
metrics::MetricID res = mMetrics.manager.AddMetric(metadata, mMetrics.frameCountId);
PPX_ASSERT_MSG(res != metrics::kInvalidMetricID, "Failed to create frame count metric");

bool bindres = mMetrics.manager.BindMetric(mMetrics.frameCountId, metadata);
PPX_ASSERT_MSG(bindres, "Failed to create frame count metric");
}

mMetrics.resetFramerateTracking = true;
Expand Down Expand Up @@ -1811,29 +1814,38 @@ metrics::MetricID Application::AllocateMetricID()
return mMetrics.manager.AllocateID();
}

metrics::MetricID Application::AddLiveMetric(metrics::MetricID metricID)
metrics::MetricID Application::AddMetric(const metrics::MetricMetadata& metadata)
{
// Use default half life since there's not much reason to have different value.
constexpr double kHalfLife = metrics::LiveMetric::kDefaultHalfLife;
return mMetrics.manager.AddLiveMetric(kHalfLife, metricID);
}
if (!mStandardOpts.pEnableMetrics->GetValue()) {
PPX_LOG_ERROR("Attempting to add a metric with metrics disabled; ignoring.");
return metrics::kInvalidMetricID;
}

void Application::ClearLiveMetricsHistory()
{
mMetrics.manager.ClearLiveMetricsHistory();
// This function already covers all other cases.
return mMetrics.manager.AddMetric(metadata);
}

metrics::MetricID Application::AddMetric(
const metrics::MetricMetadata& metadata,
metrics::MetricID metricID)
bool Application::BindMetric(metrics::MetricID metricID, const metrics::MetricMetadata& metadata)
{
if (!mStandardOpts.pEnableMetrics->GetValue()) {
PPX_LOG_ERROR("Attempting to add a metric with metrics disabled; ignoring.");
return metrics::kInvalidMetricID;
return false;
}

// This function already covers all other cases.
return mMetrics.manager.AddMetric(metadata, metricID);
return mMetrics.manager.BindMetric(metricID, metadata);
}

bool Application::BindLiveMetric(metrics::MetricID metricID)
{
// Use default half life since there's not much reason to have different value.
constexpr double kHalfLife = metrics::LiveMetric::kDefaultHalfLife;
return mMetrics.manager.BindLiveMetric(metricID, kHalfLife);
}

void Application::ClearLiveMetricsHistory()
{
mMetrics.manager.ClearLiveMetricsHistory();
}

bool Application::RecordMetricData(metrics::MetricID id, const metrics::MetricData& data)
Expand Down
40 changes: 24 additions & 16 deletions src/ppx/metrics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,37 +324,43 @@ bool Manager::HasActiveRun() const
return (mActiveRun != nullptr);
}

MetricID Manager::EnsureAllocateID(MetricID reuseID)
MetricID Manager::AllocateID()
{
if (reuseID != kInvalidMetricID) {
return reuseID;
}
return mNextMetricID++;
}

MetricID Manager::AddMetric(const MetricMetadata& metadata, MetricID metricID)
bool Manager::BindMetric(MetricID metricID, const MetricMetadata& metadata)
{
if (metricID == kInvalidMetricID) {
PPX_LOG_WARN("Attempted to bind to an invalid metric id.");
return false;
}

if (mActiveRun == nullptr) {
return kInvalidMetricID;
PPX_LOG_WARN("Attempted to bind to an inactive run.");
return false;
}

auto* metric = mActiveRun->AddMetric(metadata);
if (metric == nullptr) {
return kInvalidMetricID;
return false;
}
metricID = EnsureAllocateID(metricID);
mActiveMetrics.emplace(metricID, metric);
return metricID;
return true;
}

MetricID Manager::AddLiveMetric(double halfLife, MetricID metricID)
bool Manager::BindLiveMetric(MetricID metricID, double halfLife)
{
if (metricID != kInvalidMetricID && mLiveMetrics.count(metricID) > 0) {
return kInvalidMetricID;
if (metricID == kInvalidMetricID) {
PPX_LOG_WARN("Attempted to bind to an invalid metric id.");
return false;
}
if (mLiveMetrics.count(metricID) > 0) {
PPX_LOG_WARN("Attempted to bind to an existing live metric.");
return false;
}
metricID = EnsureAllocateID(metricID);
mLiveMetrics[metricID] = LiveMetric(halfLife);
return metricID;
return true;
}

bool Manager::RecordMetricData(MetricID id, const MetricData& data)
Expand All @@ -367,15 +373,17 @@ bool Manager::RecordMetricData(MetricID id, const MetricData& data)
if (mActiveRun == nullptr) {
if (!hasLiveMetric) {
PPX_LOG_WARN("Attempted to record a metric entry with no active run.");
return false;
}
return false;
return true;
}
auto findResult = mActiveMetrics.find(id);
if (findResult == mActiveMetrics.end()) {
if (!hasLiveMetric) {
PPX_LOG_ERROR("Attempted to record a metric entry against an invalid ID.");
return false;
}
return false;
return true;
}
return findResult->second->RecordEntry(data);
}
Expand Down
Loading

0 comments on commit cbb395f

Please sign in to comment.