Fix memory leak in assume() - #1657
Open
siramok wants to merge 4 commits into
Open
Conversation
cyrush
reviewed
Jul 27, 2026
Comment on lines
+1430
to
+1442
| static size_t | ||
| umpire_bytes_allocated(const char *pool_name) | ||
| { | ||
| return umpire::ResourceManager::getInstance() | ||
| .getAllocator(pool_name) | ||
| .getCurrentSize(); | ||
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| static void | ||
| expect_no_leak(void (*run_fn)(Node &, ExecutionPolicy), | ||
| index_t node_alloc_id, | ||
| ExecutionPolicy policy) |
Member
There was a problem hiding this comment.
should these live in the execution test utils instead?
Member
There was a problem hiding this comment.
these tests are very cool
Comment on lines
+625
to
+634
|
|
||
| // Allow m_node_ptr to take ownership of m_data so that future | ||
| // release()/reset() calls will free it, lest we leak memory. | ||
| const index_t owning_allocator_id = | ||
| execution::DeviceMemory::is_device_ptr(m_data) | ||
| ? execution::get_device_allocator_id() | ||
| : execution::get_host_allocator_id(); | ||
| m_node_ptr->assume_data_ptr(m_data, | ||
| dtype().element_bytes() * number_of_elements(), | ||
| owning_allocator_id); |
Member
There was a problem hiding this comment.
I just wonder if this is bulletproof to other kinds of allocators? Like if someone had their own device/host allocator I wonder if this would matter that we use the default ones? I imagine this will be fine in all or almost all cases.
JustinPrivitera
approved these changes
Jul 28, 2026
JustinPrivitera
left a comment
JustinPrivitera
left a comment
Member
There was a problem hiding this comment.
Thanks for catching this and being so thorough in fixing/testing! Great work 🦾
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a memory leak within the
assume()implementation for bothDataAccessorandDataArray.After creating #1656, I was performing device-based test runs on my local PC to see how quick the new implementation was running relative to the old implementation. I noticed that I wasn't able to go above 256 dim test meshes on my GPU due to CUDA trying to allocate too much memory.
Maybe a 256 dim braid mesh is just too much for my 12GB of VRAM to handle, I thought. But by random chance, I simultaneously scaled the dim sizes down but increased the iteration count, and to my surprise, this also caused CUDA to ask for too much memory and crash. I did some digging to try to understand why device memory wasn't getting freed and believe I found the issue:
Previously,
assume()was handing off a pointer to its execution location buffer without handing off ownership (i.e. letting the receiving node know that it was now holding allocated memory). This resulted in.reset()never actually freeing that memory. The fix is to set the necessary fields in the receiving node (in addition to thedata_ptritself) so that ownership of the buffer is effectively transferred.I included a test that demonstrates the memory leak, which fails if the changes in this PR are reverted. I initially believed that
sync()was leaking memory as well (which is why those tests are here), but splitting them into 4 separate tests proved thatsync()was correctly cleaning up after itself.I can now do hundreds of large mesh conversion tests without running out of device memory.