You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "zifengyu (via GitHub)" <gi...@apache.org> on 2023/04/21 14:50:43 UTC

[GitHub] [arrow] zifengyu opened a new issue, #35270: Memory leak in SwissTable::grow_double()

zifengyu opened a new issue, #35270:
URL: https://github.com/apache/arrow/issues/35270

   ### Describe the bug, including details regarding any error messages, version, and platform.
   
   There are two allocation in grow_double. If the first suceeded but the second failed, the function will return without Free the first buffer 
   ```
   RETURN_NOT_OK(pool_->Allocate(block_size_total_after, &blocks_new));
   RETURN_NOT_OK(pool_->Allocate(hashes_size_total_after, &hashes_new_8B));
   ```
   
   https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/key_map.cc#L670
   
   
   ### Component(s)
   
   C++


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] zifengyu commented on issue #35270: Memory leak in SwissTable::grow_double()

Posted by "zifengyu (via GitHub)" <gi...@apache.org>.
zifengyu commented on issue #35270:
URL: https://github.com/apache/arrow/issues/35270#issuecomment-1518931332

   Hi @mapleFU,
   
   SwissTable managers the pool memory directly instead of using PoolBuffer. If the grow_doulbe quit in the middle, the explict release memory/Free is not called.
   https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/key_map.cc#L767


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mapleFU commented on issue #35270: Memory leak in SwissTable::grow_double()

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on issue #35270:
URL: https://github.com/apache/arrow/issues/35270#issuecomment-1518520250

   I think `pool_->Allocate` returns a buffer with `MemoryManager`:
   
   ```c++
     static std::unique_ptr<PoolBuffer> MakeUnique(MemoryPool* pool, int64_t alignment) {
       std::shared_ptr<MemoryManager> mm;
       if (pool == nullptr) {
         pool = default_memory_pool();
         mm = default_cpu_memory_manager();
       } else {
         mm = CPUDevice::memory_manager(pool);
       }
       return std::make_unique<PoolBuffer>(std::move(mm), pool, alignment);
     }
   ```
   
   When `PoolBuffer` destruct, it will release pool memory:
   
   ```c++
     ~PoolBuffer() override {
       // Avoid calling pool_->Free if the global pools are destroyed
       // (XXX this will not work with user-defined pools)
   
       // This can happen if a Future is destructing on one thread while or
       // after memory pools are destructed on the main thread (as there is
       // no guarantee of destructor order between thread/memory pools)
       uint8_t* ptr = mutable_data();
       if (ptr && !global_state.is_finalizing()) {
         pool_->Free(ptr, capacity_, alignment_);
       }
     }
   ```
   
   If I'm wrong, please point out that


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on issue #35270: Memory leak in SwissTable::grow_double()

Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #35270:
URL: https://github.com/apache/arrow/issues/35270#issuecomment-1523958239

   Thanks for the catch.  I think smart pointers should be preferred in general and I've added a PR that switches these two buffers to using Buffer.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou closed issue #35270: Memory leak in SwissTable::grow_double()

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou closed issue #35270: Memory leak in SwissTable::grow_double()
URL: https://github.com/apache/arrow/issues/35270


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mapleFU commented on issue #35270: Memory leak in SwissTable::grow_double()

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on issue #35270:
URL: https://github.com/apache/arrow/issues/35270#issuecomment-1518932353

   Yes, you're right, seems this may causing memory leak. @westonpace Mind take a look?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org