Fix init order for getMetricIdMap (#341)

* Fix init order for getMetricIdMap Ensure that getMetricIdMap() stays valid until the destruction of the counting service. Adds a test to ensure that this behavior is maintained (this test fails without this change). * source formatting (clang-format v11) (#343) Co-authored-by: bwelton <bwelton@users.noreply.github.com> * cmake formatting (cmake-format) (#342) Co-authored-by: bwelton <bwelton@users.noreply.github.com> * Remove threading * Update usage of rocprofiler::counters::get{Metric,MetricId}Map() - uses common::static_object * Update counter init_order testing --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: bwelton <bwelton@users.noreply.github.com> Co-authored-by: Jonathan R. Madsen <jonathanrmadsen@gmail.com> [ROCm/rocprofiler-sdk commit: ef71cc38c1]
2024-01-11 23:35:12 -08:00
@@ -39,3 +39,6 @@

 # Github Workflows
 /.github
+
+# VSCode Workspaces
+*.code-workspace
@@ -20,6 +20,7 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.

+#include <rocprofiler-sdk/fwd.h>
 #include <rocprofiler-sdk/rocprofiler.h>

 #include <fmt/core.h>
@@ -45,7 +46,7 @@ extern "C" {
 rocprofiler_status_t ROCPROFILER_API
 rocprofiler_query_counter_name(rocprofiler_counter_id_t counter_id, const char** name, size_t* size)
 {
-    const auto& id_map = rocprofiler::counters::getMetricIdMap();
+    const auto& id_map = *CHECK_NOTNULL(rocprofiler::counters::getMetricIdMap());

    if(const auto* metric_ptr = rocprofiler::common::get_val(id_map, counter_id.handle))
    {
@@ -75,7 +76,7 @@ rocprofiler_query_counter_instance_count(rocprofiler_agent_t      agent,
                                         rocprofiler_counter_id_t counter_id,
                                         size_t*                  instance_count)
 {
-    const auto& id_map     = rocprofiler::counters::getMetricIdMap();
+    const auto& id_map     = *CHECK_NOTNULL(rocprofiler::counters::getMetricIdMap());
    const auto* metric_ptr = rocprofiler::common::get_val(id_map, counter_id.handle);
    if(!metric_ptr) return ROCPROFILER_STATUS_ERROR_COUNTER_NOT_FOUND;

@@ -40,6 +40,12 @@ namespace counters
 class CounterController
 {
 public:
+    CounterController()
+    {
+        // Pre-read metrics map file to catch faliures during initial setup.
+        rocprofiler::counters::getMetricIdMap();
+    }
+
    // Adds a counter collection profile to our global cache.
    // Note: these profiles can be used across multiple contexts
    //       and are independent of the context.
@@ -115,7 +115,7 @@ get_ast_map()
 {
    static std::unordered_map<std::string, EvaluateASTMap> ast_map = []() {
        std::unordered_map<std::string, EvaluateASTMap> data;
-        const auto&                                     metric_map = counters::getMetricMap();
+        const auto& metric_map = *CHECK_NOTNULL(counters::getMetricMap());
        for(const auto& [gfx, metrics] : metric_map)
        {
            // TODO: Remove global XML from derrived counters...
@@ -26,6 +26,7 @@

 #include "lib/common/defines.hpp"
 #include "lib/common/filesystem.hpp"
+#include "lib/common/static_object.hpp"
 #include "lib/common/synchronized.hpp"
 #include "lib/common/utility.hpp"
 #include "lib/common/xml.hpp"
@@ -182,12 +183,12 @@ getBaseHardwareMetrics()
    return loadXml(counters_path, true);
 }

-const MetricIdMap&
+const MetricIdMap*
 getMetricIdMap()
 {
-    static MetricIdMap id_map = []() {
+    static MetricIdMap*& id_map = common::static_object<MetricIdMap>::construct([]() {
        MetricIdMap map;
-        for(const auto& [_, val] : getMetricMap())
+        for(const auto& [_, val] : *CHECK_NOTNULL(getMetricMap()))
        {
            for(const auto& metric : val)
            {
@@ -195,14 +196,14 @@ getMetricIdMap()
            }
        }
        return map;
-    }();
+    }());
    return id_map;
 }

-const MetricMap&
+const MetricMap*
 getMetricMap()
 {
-    static MetricMap map = []() {
+    static MetricMap*& map = common::static_object<MetricMap>::construct([]() {
        MetricMap ret = getBaseHardwareMetrics();
        for(auto& [key, val] : getDerivedHardwareMetrics())
        {
@@ -213,7 +214,7 @@ getMetricMap()
            }
        }
        return ret;
-    }();
+    }());
    return map;
 }

@@ -221,7 +222,7 @@ const std::vector<Metric>&
 getMetricsForAgent(const std::string& agent)
 {
    static const std::vector<Metric> empty;
-    const auto&                      map = getMetricMap();
+    const auto&                      map = *CHECK_NOTNULL(getMetricMap());
    if(const auto* metric_ptr = rocprofiler::common::get_val(map, agent))
    {
        return *metric_ptr;
@@ -235,5 +236,21 @@ operator<(Metric const& lhs, Metric const& rhs)
 {
    return lhs.id() < rhs.id();
 }
+
+bool
+operator==(Metric const& lhs, Metric const& rhs)
+{
+    auto get_tie = [](auto& x) {
+        return std::tie(x.name_,
+                        x.block_,
+                        x.event_,
+                        x.description_,
+                        x.expression_,
+                        x.special_,
+                        x.id_,
+                        x.empty_);
+    };
+    return get_tie(lhs) == get_tie(rhs);
+}
 }  // namespace counters
 }  // namespace rocprofiler
@@ -69,6 +69,7 @@ public:
    bool               empty() const { return empty_; }

    friend bool operator<(Metric const& lhs, Metric const& rhs);
+    friend bool operator==(Metric const& lhs, Metric const& rhs);

 private:
    std::string name_        = {};
@@ -99,7 +100,7 @@ getDerivedHardwareMetrics();
 /**
 * Combined map containing both base and derived counters
 */
-const MetricMap&
+const MetricMap*
 getMetricMap();

 /**
@@ -112,7 +113,7 @@ getMetricsForAgent(const std::string&);
 /**
 * Get a map of metric::id() -> metric
 */
-const MetricIdMap&
+const MetricIdMap*
 getMetricIdMap();
 }  // namespace counters
 }  // namespace rocprofiler
@@ -3,7 +3,7 @@ rocprofiler_deactivate_clang_tidy()
 include(GoogleTest)

 set(ROCPROFILER_LIB_COUNTER_TEST_SOURCES metrics_test.cpp evaluate_ast_test.cpp
-                                         dimension.cpp)
+                                         dimension.cpp init_order.cpp)

 add_executable(counter-test)

@@ -0,0 +1,181 @@
+// MIT License
+//
+// Copyright (c) 2023 Advanced Micro Devices, Inc. All rights reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+#include <algorithm>
+#include <cstdint>
+#include <sstream>
+#include <tuple>
+
+#include <fmt/core.h>
+#include <gtest/gtest.h>
+
+#include <rocprofiler-sdk/rocprofiler.h>
+
+#include "lib/common/static_object.hpp"
+#include "lib/common/utility.hpp"
+#include "lib/rocprofiler-sdk/buffer.hpp"
+#include "lib/rocprofiler-sdk/context/context.hpp"
+#include "lib/rocprofiler-sdk/counters/id_decode.hpp"
+#include "lib/rocprofiler-sdk/counters/metrics.hpp"
+#include "lib/rocprofiler-sdk/registration.hpp"
+#include "rocprofiler-sdk/registration.h"
+
+using namespace rocprofiler::counters;
+
+#define ROCPROFILER_CALL(result, msg)                                                              \
+    {                                                                                              \
+        rocprofiler_status_t CHECKSTATUS = result;                                                 \
+        if(CHECKSTATUS != ROCPROFILER_STATUS_SUCCESS)                                              \
+        {                                                                                          \
+            std::string status_msg = rocprofiler_get_status_string(CHECKSTATUS);                   \
+            std::cerr << "[" #result "][" << __FILE__ << ":" << __LINE__ << "] " << msg            \
+                      << " failed with error code " << CHECKSTATUS << ": " << status_msg           \
+                      << std::endl;                                                                \
+            std::stringstream errmsg{};                                                            \
+            errmsg << "[" #result "][" << __FILE__ << ":" << __LINE__ << "] " << msg " failure ("  \
+                   << status_msg << ")";                                                           \
+            throw std::runtime_error(errmsg.str());                                                \
+        }                                                                                          \
+    }
+
+struct metric_map_order
+{
+    metric_map_order() = default;
+    ~metric_map_order() { check_copy(); }
+
+    metric_map_order(const metric_map_order&) = delete;
+    metric_map_order& operator=(const metric_map_order&) = delete;
+
+    metric_map_order(metric_map_order&&) noexcept = delete;
+    metric_map_order& operator=(metric_map_order&&) noexcept = delete;
+
+    void check_copy()
+    {
+        ASSERT_TRUE(!copy_.empty());
+
+        const auto* metricIdMap = rocprofiler::counters::getMetricIdMap();
+        int         fini_status = 0;
+        ROCPROFILER_CALL(rocprofiler_is_finalized(&fini_status), "get finalization state");
+
+        if(fini_status > 0)
+        {
+            // this should only be true in the destructor of the static metric_map_order instance
+            ASSERT_TRUE(metricIdMap != nullptr) << "rocprofiler finalization state: " << fini_status
+                                                << ", metricIdMap: " << metricIdMap;
+
+            // this should ensure the metric id map is destroyed
+            rocprofiler::common::destroy_static_objects();
+            metricIdMap = rocprofiler::counters::getMetricIdMap();
+
+            ASSERT_TRUE(metricIdMap == nullptr) << "rocprofiler finalization state: " << fini_status
+                                                << ", metricIdMap: " << metricIdMap;
+        }
+        else
+        {
+            for(const auto& [id, actual] : copy_)
+            {
+                // Assert because this is getting triggered on shutdown and
+                // we want to fail the test if the values in both maps are not equal.
+                const auto* val = rocprofiler::common::get_val(*metricIdMap, id);
+                ASSERT_TRUE(val != nullptr) << "metricIdMap: " << metricIdMap;
+                ASSERT_TRUE(*val == actual) << "metricIdMap: " << metricIdMap;
+            }
+        }
+    }
+
+private:
+    MetricIdMap copy_ = *CHECK_NOTNULL(rocprofiler::counters::getMetricIdMap());
+};
+
+metric_map_order&
+get_metric_map()
+{
+    static metric_map_order order = {};
+    return order;
+}
+
+void
+buffered_callback(rocprofiler_context_id_t,
+                  rocprofiler_buffer_id_t,
+                  rocprofiler_record_header_t**,
+                  size_t,
+                  void*,
+                  uint64_t)
+{}
+
+void
+dispatch_callback(rocprofiler_queue_id_t,
+                  const rocprofiler_agent_t*,
+                  rocprofiler_correlation_id_t,
+                  const hsa_kernel_dispatch_packet_t*,
+                  uint64_t,
+                  void*,
+                  rocprofiler_profile_config_id_t*)
+{}
+
+rocprofiler_context_id_t&
+get_client_ctx()
+{
+    static rocprofiler_context_id_t ctx;
+    return ctx;
+}
+
+rocprofiler_buffer_id_t&
+get_buffer()
+{
+    static rocprofiler_buffer_id_t buf = {};
+    return buf;
+}
+
+// Test that metrics map remains in scope at exit
+TEST(counters_init_order, metric_map_order)
+{
+    rocprofiler::registration::init_logging();
+    // do not call rocprofiler::registration::initialize()!
+    // doing so will add an atexit call which might invoke
+    // rocprofiler::common::destroy_static_objects() before
+    // the get_metric_map() instance is destroyed
+
+    rocprofiler::registration::set_init_status(-1);
+    rocprofiler::context::push_client(1);
+    ROCPROFILER_CALL(rocprofiler_create_context(&get_client_ctx()), "context creation failed");
+    ROCPROFILER_CALL(rocprofiler_create_buffer(get_client_ctx(),
+                                               4096,
+                                               2048,
+                                               ROCPROFILER_BUFFER_POLICY_LOSSLESS,
+                                               buffered_callback,
+                                               nullptr,
+                                               &get_buffer()),
+                     "buffer creation failed");
+    ROCPROFILER_CALL(rocprofiler_configure_buffered_dispatch_profile_counting_service(
+                         get_client_ctx(), get_buffer(), dispatch_callback, nullptr),
+                     "Could not setup buffered service");
+    rocprofiler::registration::set_init_status(1);
+
+    auto& global_metric_map = get_metric_map();
+    global_metric_map.check_copy();
+
+    auto local_metric_map = metric_map_order{};
+    local_metric_map.check_copy();
+
+    rocprofiler::registration::finalize();
+}
@@ -20,6 +20,7 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.

+#include <glog/logging.h>
 #include <rocprofiler-sdk/rocprofiler.h>

 #include "lib/common/synchronized.hpp"
@@ -49,7 +50,7 @@ rocprofiler_create_profile_config(rocprofiler_agent_t              agent,
    std::shared_ptr<rocprofiler::counters::profile_config> config =
        std::make_shared<rocprofiler::counters::profile_config>();

-    const auto& id_map = rocprofiler::counters::getMetricIdMap();
+    const auto& id_map = *CHECK_NOTNULL(rocprofiler::counters::getMetricIdMap());

    for(size_t i = 0; i < counters_count; i++)
    {