Skip to content

Prevent resource manager from being created by empty managed arrays #357

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

Open
liu15 wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Prevent resource manager from being created by empty managed arrays #357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a5d284
Prevent resource manager from being created by empty managed arrays
liu15 Feb 25, 2026
937876c
Merge branch 'develop' into delayRM
adayton1 Feb 26, 2026
cb34112
Limit resource manager creation to allocations
liu15 Feb 26, 2026
93b071c
Merge branch 'develop' into delayRM
adayton1 Mar 2, 2026
938bb96
Add comments
liu15 Mar 2, 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
4 changes: 4 additions & 0 deletions src/chai/ManagedArray.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ class ManagedArray : public CHAICopyable
/*!
* \brief Allocate data for the ManagedArray in the specified space.
*
* Once a ManagedArray is allocated, it will have a valid resource manager.
*
* \param elems Number of elements to allocate.
* \param space Execution space in which to allocate data.
* \param cback User defined callback for memory events (alloc, free, move)
Expand Down Expand Up @@ -289,6 +291,7 @@ class ManagedArray : public CHAICopyable
* \brief Return the value of element i in the ManagedArray.
* ExecutionSpace space to the current one
*
* \pre ManagedArray must be allocated
* \param index The index of the element to be fetched
* \param space The index of the element to be fetched
* \return The value of the i-th element in the ManagedArray.
Expand All @@ -299,6 +302,7 @@ class ManagedArray : public CHAICopyable
/*!
* \brief Set the value of element i in the ManagedArray to be val.
*
* \pre ManagedArray must be allocated
* \param index The index of the element to be set
* \param val Source location of the value
* \tparam T The type of data value in ManagedArray.
Expand Down
13 changes: 11 additions & 2 deletions src/chai/ManagedArray.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ CHAI_HOST_DEVICE ManagedArray<T>::ManagedArray():
m_is_slice(false)
{
#if !defined(CHAI_DEVICE_COMPILE)
m_resource_manager = ArrayManager::getInstance();
m_pointer_record = &ArrayManager::s_null_record;
#endif
}
Expand All @@ -37,6 +36,7 @@ ManagedArray<T>::ManagedArray(
std::initializer_list<umpire::Allocator> allocators):
ManagedArray()
{
m_resource_manager = ArrayManager::getInstance();
m_pointer_record = new PointerRecord();
int i = 0;
for (int s = CPU; s < NUM_EXECUTION_SPACES; ++s) {
Expand Down Expand Up @@ -169,6 +169,9 @@ CHAI_HOST void ManagedArray<T>::allocate(
const UserCallback& cback)
{
if(!m_is_slice) {
if (m_resource_manager == nullptr) {
Copy link
Member

@adayton1 adayton1 Feb 26, 2026

Choose a reason for hiding this comment

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

I'm not sure I really like these changes where almost any operation now has to check if the ArrayManager exists. The class no longer establishes an invariant.

Copy link
Member

@adayton1 adayton1 Feb 26, 2026

Choose a reason for hiding this comment

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

If we really have to, a getter would be much better.

Copy link
Collaborator Author

@liu15 liu15 Feb 26, 2026

Choose a reason for hiding this comment

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

I can probably remove the calls from pick and set, since those should only be valid for allocated arrays. For the registerTouch and reset functions, perhaps they should just return if there is no resource manager?

Copy link
Member

@adayton1 adayton1 Mar 2, 2026

Choose a reason for hiding this comment

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

What about reallocate? I just don't like that we have to decide on a per function basis what the state of the object is supposed to be.

Copy link
Member

@adayton1 adayton1 Mar 2, 2026

Choose a reason for hiding this comment

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

Would it be possible to push this up the chain to ArrayManager? Have ArrayManager not make any pre-main calls until it has to?

Copy link
Collaborator Author

@liu15 liu15 Mar 2, 2026

Choose a reason for hiding this comment

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

reallocate only need the resource manager if it is already allocated. Basically, the resource manage is present if the managed array has been allocated at any point.

Copy link
Collaborator Author

@liu15 liu15 Mar 2, 2026

Choose a reason for hiding this comment

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

Would it be possible to push this up the chain to ArrayManager? Have ArrayManager not make any pre-main calls until it has to?

I guess that could be possible. Would you suggest making ArrayManager::m_resource_manager a getter?

Copy link
Collaborator Author

@liu15 liu15 Mar 2, 2026

Choose a reason for hiding this comment

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

It it was a getter, would it be better in ManagedArray or ArrayManager?

m_resource_manager = ArrayManager::getInstance();
}
if (elems > 0) {
CHAI_LOG(Debug, "Allocating array of size " << elems << " in space " << space);

Expand Down Expand Up @@ -300,6 +303,9 @@ template<typename T>
CHAI_INLINE
CHAI_HOST void ManagedArray<T>::reset()
{
if (m_resource_manager == nullptr) {
return;
}
m_resource_manager->resetTouch(m_pointer_record);
}

Expand All @@ -312,6 +318,9 @@ CHAI_HOST_DEVICE size_t ManagedArray<T>::size() const {
template<typename T>
CHAI_INLINE
CHAI_HOST void ManagedArray<T>::registerTouch(ExecutionSpace space) {
if (m_resource_manager == nullptr) {
return;
}
if (m_active_pointer && (m_pointer_record == nullptr || m_pointer_record == &ArrayManager::s_null_record)) {
CHAI_LOG(Warning,"registerTouch called on ManagedArray with nullptr pointer record.");
m_pointer_record = m_resource_manager->makeManaged((void *)m_active_base_pointer,m_size,space,true);
Expand Down Expand Up @@ -556,7 +565,7 @@ ManagedArray<T>::operator= (std::nullptr_t) {
m_offset = 0;
#if !defined(CHAI_DEVICE_COMPILE)
m_pointer_record = &ArrayManager::s_null_record;
m_resource_manager = ArrayManager::getInstance();
m_resource_manager = nullptr;
#else
m_pointer_record = nullptr;
m_resource_manager = nullptr;
Expand Down