Address reviewer comment and add test

benchmarks/graphics_pipeline/GraphicsBenchmarkApp.cpp

@@ -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()
@@ -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");
     }
 }
 
@@ -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(),
@@ -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();
         }
     }

include/ppx/application.h

@@ -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);

include/ppx/metrics.h

@@ -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);
@@ -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;

src/ppx/application.cpp

@@ -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;
@@ -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)

src/ppx/metrics.cpp

@@ -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)
@@ -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);
 }