Skip to content
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.

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

Destroy the entire temp_memory handle before each allocation can be error-prone #76

Open
chang-l opened this issue Oct 6, 2023 · 1 comment

Comments

@chang-l
Copy link
Contributor

chang-l commented Oct 6, 2023

Hi,

Within temp_memory_handle class, can we avoid completely free the object when invoking each type of *_malloc() function? I thought, by design if needed, users/developers should be able to allocate memory multiple times using the same set of env_fns or handle object, like:

{
  temp_memory_handle  scratch_space(env_fns);
  ptr = scratch_space.device_malloc();
  /*  do something */
  ptr = scratch_space.device_malloc();  // calling free_memory() before malloc_fn()
  /* do something */
}

However, free_memory() function serves as the destructor of the entire handle object, for example, here:

void* device_malloc(size_t elt_count, wholememory_dtype_t data_type)
{
free_memory();
wholememory_tensor_description_t tensor_description;
get_tensor_description(&tensor_description, elt_count, data_type);
ptr_ = temp_mem_fns_->malloc_fn(
it not only frees the memory but also deletes memory context. Thus, the above code snippet would crash with segfault from calling *malloc_fn function (memory_context_ becomes nullptr after each free_memory call).

Therefore, I suggest to add a new free_data function to just deallcoate memory but not memory context, before each *_malloc() call.

@dongxuy04
Copy link
Contributor

I think it is OK to add reuse functionality to temp_memory_handle objects. The implementation now is single-use object as maybe in most cases, different memory object should use their own handle. For something like scratch memory, it is OK to reuse the object, and this feature can be supported.

rapids-bot bot pushed a commit that referenced this issue Apr 30, 2024
fix to[ issue 76](#76), which allows temp_memory_handler to allocate memory for multiple times.

Authors:
  - https://github.com/linhu-nv

Approvers:
  - Chuang Zhu (https://github.com/chuangz0)

URL: #161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants