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

[anujshuk-amd]

Using: ROCm/timemory#21

Motivation

This pull request updates the way thread-local storage is managed for counter_data_tracker in the rocprofiler SDK, improving its scalability and flexibility. The most important changes include switching from a fixed-size array to a dynamically sized stable_vector, ensuring storage can grow as needed, and updating related initialization and access patterns.

Technical Details

Thread-local storage improvements:

• Changed the thread storage container in set_storage<counter_data_tracker> from a fixed-size std::array to a dynamically sized stable_vector, allowing the storage to grow beyond a fixed thread limit.
• Replaced the hardcoded maximum thread count (4096) with a configurable ROCPROFSYS_MAX_THREADS for initial capacity, improving flexibility.
• Added an ensure_capacity helper function to dynamically expand the storage vector when accessing or assigning storage for a thread index, preventing out-of-bounds errors. [1] [2]
• Updated the storage filling logic to iterate over the current vector size, ensuring all existing elements are set correctly, rather than relying on array fill.
• Updated the submodule timemory to a new commit, which may bring in upstream improvements or fixes.

Submodule update

• Updated the timemory submodule to a newer commit, likely to incorporate upstream improvements or fixes.

JIRA ID

Using: ROCm/timemory#21
TBA

Test Plan

TBA

Test Result

TBA

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

[mradosav-amd]

Can we just have static member instead of static variable inside function?
It would be great to avoid this style:
get_capacity().load(std::memory_order_acquire)
get_capacity().store(_v.size(), std::memory_order_release);

Because it's introducing confusion with get_* and then we are calling store, which is setting. It's contradictory.

With static member, we can directly call:
m_capacity.load(std::memory_order_acquire)
m_capacity.store(_v.size(), std::memory_order_release);

Which will be much readable.

[anujshuk-amd]

@mradosav-amd I yet to implement/considering your review comment. Wating to all check once to get passed. Why I took it as inside function is Lazy (on first call). And timemory Uses Function-Local Static.

[Copilot]

Pull request overview

This PR refactors thread-local storage for counter_data_tracker from a fixed-size std::array to a dynamically resizing std::vector, eliminating the hard 4096 thread limit and reducing memory overhead while maintaining thread safety through atomic capacity tracking and mutex-protected resize operations.

Changes:

• Replaced std::array with std::vector in thread storage, introducing dynamic capacity management with geometric growth
• Added thread-safe capacity checks and resize operations using std::atomic<size_t> and std::mutex
• Updated the timemory submodule to incorporate upstream changes supporting this refactoring

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk/counters.hpp	Implements dynamic thread storage with capacity management, replaces fixed array operations with vector operations, and adds thread-safe bounds checking
projects/rocprofiler-systems/external/timemory	Updates submodule to newer commit with compatible dynamic storage infrastructure

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The type alias at line 125 is defined outside the template specialization, making it accessible in the broader operation namespace. This could lead to naming conflicts or confusion. Consider moving the type alias inside the set_storage struct to keep it scoped to where it's used, or use a more specific name like counter_data_tracker_type.

[Copilot]

Exposing get_capacity from the protected base class breaks encapsulation. This allows external code to directly access the capacity, which should remain an internal implementation detail. Consider providing a controlled interface method in set_storage if capacity needs to be queried externally, or keep get_capacity access limited to friend classes.

[Copilot]

The multi-line condition with get_capacity().load(std::memory_order_acquire) is hard to read. Consider extracting this into a local variable like const size_t current_capacity = operation::set_storage<type>::get_capacity().load(std::memory_order_acquire); and then using that in the condition for better readability.

Suggested change

	if(_idx >=
	operation::set_storage<type>::get_capacity().load(std::memory_order_acquire))
	const size_t current_capacity =
	operation::set_storage<type>::get_capacity().load(std::memory_order_acquire);
	if(_idx >= current_capacity)

[habajpai-amd]

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

[jrmadsen]

This is huge mistake. You need something like stable_vector which exists somewhere in rocprofiler-systems or timemory -- it is effectively a vector of arrays. std::vector guarantees contiguous memory -- which means unless your vector is storing pointers (and thus, every access to the thread-local data requires pointer chasing), a reallocation of the underlying memory in order to expand the vector will cause extremely difficult to debug/reproduce memory errors. And trying to implement a scheme with R/W mutexes makes it unwieldy to use properly since you have to hold the lock the entire time you access the data, not just hold the lock to get a reference/pointer... and holding the lock is going to introduce overhead that doesn't exist currently. I knew exactly what I was doing when I used std::array instead of std::vector and the ability to dynamically expand via stable_vector is the solution here.

[jrmadsen]

See previous comment. This PR suggests there is a fundamental misunderstanding about how the memory allocations of std::vector work under the hood and the dangers of resizing in multithreading scenarios.

[anujshuk-amd]

This is huge mistake. You need something like stable_vector which exists somewhere in rocprofiler-systems or timemory -- it is effectively a vector of arrays. std::vector guarantees contiguous memory -- which means unless your vector is storing pointers (and thus, every access to the thread-local data requires pointer chasing), a reallocation of the underlying memory in order to expand the vector will cause extremely difficult to debug/reproduce memory errors. And trying to implement a scheme with R/W mutexes makes it unwieldy to use properly since you have to hold the lock the entire time you access the data, not just hold the lock to get a reference/pointer... and holding the lock is going to introduce overhead that doesn't exist currently. I knew exactly what I was doing when I used std::array instead of std::vector and the ability to dynamically expand via stable_vector is the solution here.

Thank you very much for taking the time to review the changes. Your insights are invaluable, and I truly appreciate your keen eye for detail. The suggestions regarding the data structures have been particularly helpful. I kindly request your further assistance in this. Your expertise would greatly enhance the final outcome.