Skip to content

Improvement thread storage from std::array to stable_vector with dynamic growth #2542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anujshuk-amd wants to merge 5 commits into from
+29 −7
Closed

Improvement thread storage from std::array to stable_vector with dynamic growth #2542

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3971d01
Convert thread storage from std::array to std::vector with dynamic gr…
anujshuk-amd Jan 8, 2026
b68a11b
Convert thread storage from std::array to std::vector with dynamic gr…
anujshuk-amd Jan 8, 2026
ea379a7
refactor: address code review comments for dynamic thread storage
anujshuk-amd Jan 20, 2026
e9a38d4
Apply clang-format-18 to config.cpp and counters.hpp
anujshuk-amd Jan 28, 2026
ff0473d
Merge branch 'develop' into anujshuk/anujshuk-amd/dynamic-thread-storage
anujshuk-amd Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion projects/rocprofiler-systems/external/timemory
Open in desktop
Submodule timemory updated 2 files
+53 −5 source/timemory/operations/types.hpp
+1 −1 source/timemory/utility/filepath.cpp
19 changes: 18 additions & 1 deletion projects/rocprofiler-systems/source/lib/core/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2758,7 +2758,24 @@ get_tmp_file(std::string _basename, std::string _ext)
_cfg.use_suffix = true;
_cfg.suffix = "%pid%";
_cfg.explicit_path = get_tmpdir();
_cfg.subdirectory = JOIN('/', settings::output_path(), "%ppid%", "");

auto _output_path = settings::output_path();
if(!_output_path.empty() && _output_path.front() == '/')
{
// Extract the last directory component to use as subdirectory
auto _last_slash = _output_path.find_last_of('/');
if(_last_slash != std::string::npos && _last_slash < _output_path.size() - 1)
_output_path = _output_path.substr(_last_slash + 1);
else if(_last_slash == _output_path.size() - 1 && _last_slash > 0)
{
// Path ends with slash, find the second-to-last slash
auto _prev_slash = _output_path.find_last_of('/', _last_slash - 1);
if(_prev_slash != std::string::npos)
_output_path =
_output_path.substr(_prev_slash + 1, _last_slash - _prev_slash - 1);
}
}
_cfg.subdirectory = JOIN('/', _output_path, "%ppid%", "");
auto _fname =
settings::compose_output_filename(std::move(_basename), std::move(_ext), _cfg);

Expand Down
30 changes: 23 additions & 7 deletions projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk/counters.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@

#pragma once

#include "common/defines.h"

#include "common/synchronized.hpp"
#include "core/debug.hpp"
#include "core/perfetto.hpp"
#include "core/timemory.hpp"
#include "library/rocprofiler-sdk/fwd.hpp"

#include <timemory/operations/types.hpp>
#include <timemory/utility/types.hpp>

#include <rocprofiler-sdk/agent.h>
Expand Down Expand Up @@ -119,24 +122,34 @@ namespace tim
{
namespace operation
{

template <>
struct set_storage<::rocprofsys::rocprofiler_sdk::counter_data_tracker>
: public dynamic_storage_base<
storage<::rocprofsys::rocprofiler_sdk::counter_data_tracker>*,
ROCPROFSYS_MAX_THREADS>
{
static constexpr size_t max_threads = 4096;
static constexpr size_t max_threads = ROCPROFSYS_MAX_THREADS;
using type = ::rocprofsys::rocprofiler_sdk::counter_data_tracker;
using storage_array_t = std::array<storage<type>*, max_threads>;
friend struct get_storage<rocprofsys::rocprofiler_sdk::counter_data_tracker>;
using base_type = dynamic_storage_base<storage<type>*, max_threads>;
using storage_array_t = typename base_type::storage_array_t;

ROCPROFSYS_DEFAULT_OBJECT(set_storage)

auto operator()(storage<type>* _v, size_t _idx) const { get().at(_idx) = _v; }
auto operator()(storage<type>* _v, size_t _idx) const
{
base_type::ensure_capacity(get(), _idx);
get().at(_idx) = _v;
}
auto operator()(type&, size_t) const {}
auto operator()(storage<type>* _v) const { get().fill(_v); }
auto operator()(storage<type>* _v) const
{
std::fill(get().begin(), get().end(), _v);
}

private:
static storage_array_t& get()
{
static storage_array_t _v = { nullptr };
static storage_array_t _v(max_threads, nullptr);
return _v;
}
};
Expand All @@ -161,6 +174,9 @@ struct get_storage<::rocprofsys::rocprofiler_sdk::counter_data_tracker>

auto operator()(size_t _idx) const
{
// Thread-safe read using atomic capacity
const size_t current_capacity = operation::set_storage<type>::get_capacity();
if(_idx >= current_capacity) return static_cast<storage<type>*>(nullptr);

@habajpai-amd habajpai-amd Jan 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observed a potential data race edge case

The atomic capacity check on lines 178-179 doesn't fully protect against a concurrent resize. If ensure_capacity() reallocates the vector while this read is in progress, the reader could access freed memory.

Scenario:

  1. Reader checks get_capacity() -> returns valid index
  2. Writer calls ensure_capacity() -> vector reallocates
  3. Reader calls get().at(_idx) -> accesses freed memory (Undefined Behaviour)

Practical risk: Only occurs when exceeding ROCPROFSYS_MAX_THREADS(4096) during concurrent access. When we run AI workloads like tensorflow workloads then it easily exceeds MAX_THREADS but not in other cases.

Not blocking, but worth noting for future consideration:

  • Could use std::shared_mutex (read/write lock) for full protection
  • Or document that exceeding max_threads during concurrent access has undefined behavior

return operation::set_storage<type>::get().at(_idx);
}

Expand Down
Loading