Rework host memory allocation mechanics - #1964
Conversation
rhornung67
left a comment
There was a problem hiding this comment.
@lc-hubcast approved
|
@kennyweiss @publixsubfan @BradWhitlock @bmhan12 this is a much smaller, more focused PR that addresses much of what I have been talking about regarding the default host memory allocation scheme in Axom. If you think we should include it in the Axom release, please review. Thank you. |
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks for working though this @rhornung67 (and reworking it, and reworking it).
Overall, this seems like a nice compromise given the constraints and improves our memory management, but might need to be integrated with more of our infrastructure, e.g. axom::Array, sidre::Group, ...
There are some things that we should investigate before merging
(caveat: I haven't built the code, so this is based on staring at the code w/ claude):
- we might need to also update memory_management's
reallocate(MALLOC_ALLOCATOR_ID)to move the malloc handling outside the#else.
The handling ofallocate/deallocateandreallocatewas already inconsistent in axom@develop and not introduced by your changes, but the consequence are perhaps more significant now. - With
AXOM_DEFAULT_HOST_ALLOCATOR=MALLOC,Array<double> a(n)still uses the Host allocator rather than the malloc allocator since Array's default isDynamic, (defined as Umpire's current default allocator). I'm pretty sure we'd want the defaultArray<double>to followAXOM_DEFAULT_HOST_ALLOCATOR - With
AXOM_DEFAULT_HOST_ALLOCATOR=MALLOC, sidre still defaults to umpire Host (getDefaultAllocatorID()rather thandetail::getDefaultHostAllocatorID())
| { | ||
| #ifdef AXOM_USE_UMPIRE | ||
| umpire::ResourceManager& rm = umpire::ResourceManager::getInstance(); | ||
| if(allocId == MALLOC_ALLOCATOR_ID) |
There was a problem hiding this comment.
Shouldn't setDefaultAllocator also be influenced by AXOM_DEFAULT_HOST_ALLOCATOR_USES_UMPIRE_HOST ?
It seems that if we're in an Umpire build, we can never get back to the malloc allocator as the default.
E.g., consider the following in a build with Umpire:
axom::setDefaultAllocator(Host); // starts at Malloc, ends at host
axom::setDefaultAllocator(Malloc); // still ends at HostThere was a problem hiding this comment.
This method is only setting the default allocator in the Umpire ResourceManager. As long as all Axom usage honors the Axom default (Malloc or Host) when appropriate this should be OK.
Do we need to handle the case where a user calls setDefaultAllocator() with something that is neither Malloc or Host, but is still valid on the host, such as Pinned when Umpire is enabled? As I understand it, that is the main purpose of this method.
I think only the method documentation needs to be clarified, which I did.
Does that make sense now?
There was a problem hiding this comment.
This method is only setting the default allocator in the Umpire ResourceManager. As long as all Axom usage honors the Axom default (Malloc or Host) when appropriate this should be OK.
If that's the case, perhaps it should be renamed setUmpireDefaultAllocator ?
(and be a no-op in non-umpire configs)
This is the current doxygen brief for the setDefaultAllocator
\brief Sets the default memory allocator to use.
Until now, we've been using it as Axom's default allocator
(which points to Umpire when Axom is configured against Umpire).
There was a problem hiding this comment.
I modified the doxygen brief comment to indicate that it sets the default allocator in the Umpire ResourceManager, which it did before my changes IIRC. It is a no-op in non-umpire configs.
and resolve merge conflicts
| // Device memory: fill on host, then copy to device | ||
| const auto num_bytes = n * sizeof(T); | ||
| T* src = allocate<T>(num_bytes, rm.getDefaultAllocator().getId()); | ||
| T* src = allocate<T>(n, axom::detail::getDefaultHostAllocatorID()); |
There was a problem hiding this comment.
Good catch.
It's not here but in allocate() if Umpire is enabled then we check whether the passed allocator Id is a valid Umpire allocator before the malloc check. Since we're defaulting the host allocator id to MALLOC, even for Umpire-enabled builds (unless they select UMPIRE_HOST), should we swap the order of the umpire/malloc allocation checks so we can skip checking whether the allocator Id is valid for Umpire? Or, will allocate() more likely be called with allocators that are associated with Umpire?
and resolve merge conflict
Summary
axom::MALLOC_ALLOCATOR_IDthe default host allocator when Axom is configured with Umpire enabled and when it is not.AXOM_DEFAULT_HOST_ALLOCATORwas added as a compile-time option for users who need to make a different default host execution-space policy. Its valid values areMALLOC(default) andUMPIRE_HOST(available when Axom is configured with Umpire), which was previous default.UMPIRE_HOSTcase.setDefaultAllocator()as the interface for Umpire default allocator state has been preserved.No substantial changes to Axom public APIs were made and no static state was introduced.
This PR addresses #1816