-
Notifications
You must be signed in to change notification settings - Fork 4.2k
ARROW-530: [C++/Python] Provide subpools for better memory allocation … #2057
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
Changes from 1 commit
62dcdb0
7c4b769
f90b897
17a5c9b
0645e50
195a136
5bebf97
96e58ee
eabd8a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,4 +201,45 @@ int64_t LoggingMemoryPool::max_memory() const { | |
| std::cout << "max_memory: " << mem << std::endl; | ||
| return mem; | ||
| } | ||
|
|
||
| ProxyMemoryPool::ProxyMemoryPool(MemoryPool* pool) : pool_(pool), proxy_bytes_allocated_(0) {} | ||
|
|
||
| Status ProxyMemoryPool::Allocate(int64_t size, uint8_t** out) { | ||
| proxy_bytes_allocated_ += size; | ||
| Status s = pool_->Allocate(size, out); | ||
| std::cout << "Allocate: size = " << size << std::endl; | ||
| return s; | ||
| } | ||
|
|
||
| Status ProxyMemoryPool::Reallocate(int64_t old_size, int64_t new_size, uint8_t** ptr) { | ||
| Status s = pool_->Reallocate(old_size, new_size, ptr); | ||
| proxy_bytes_allocated_ += new_size - old_size; | ||
| std::cout << "Reallocate: old_size = " << old_size << " - new_size = " << new_size | ||
| << " - proxy_allocated - " << proxy_bytes_allocated_ << std::endl; | ||
| return s; | ||
| } | ||
|
|
||
| void ProxyMemoryPool::Free(uint8_t* buffer, int64_t size) { | ||
| pool_->Free(buffer, size); | ||
| proxy_bytes_allocated_ -= size; | ||
| std::cout << "Free: size = " << size << std::endl; | ||
| } | ||
|
|
||
| int64_t ProxyMemoryPool::bytes_allocated() const { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realised that |
||
| int64_t nb_bytes = pool_->bytes_allocated(); | ||
| std::cout << "bytes_allocated: " << nb_bytes << std::endl; | ||
| return nb_bytes; | ||
| } | ||
|
|
||
| int64_t ProxyMemoryPool::proxy_bytes_allocated() const { | ||
| int64_t nb_bytes = proxy_bytes_allocated_; | ||
| std::cout << "bytes_allocated: " << nb_bytes << std::endl; | ||
| return nb_bytes; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| } | ||
|
|
||
| int64_t ProxyMemoryPool::max_memory() const { | ||
| int64_t mem = pool_->max_memory(); | ||
| std::cout << "max_memory: " << mem << std::endl; | ||
| return mem; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } // namespace arrow | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,27 @@ class ARROW_EXPORT LoggingMemoryPool : public MemoryPool { | |
| MemoryPool* pool_; | ||
| }; | ||
|
|
||
| class ARROW_EXPORT ProxyMemoryPool : public MemoryPool { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we consider some alternative names than "Proxy"? Can you add a doxygen comment describing what this class is?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about SubMemoryPool? MemorySubPool? PartialMemoryPool? |
||
| public: | ||
| explicit ProxyMemoryPool (MemoryPool* pool); | ||
| ~ProxyMemoryPool () override = default; | ||
|
|
||
| Status Allocate(int64_t size, uint8_t** out) override; | ||
| Status Reallocate(int64_t old_size, int64_t new_size, uint8_t** ptr) override; | ||
|
|
||
| void Free(uint8_t* buffer, int64_t size) override; | ||
|
|
||
| int64_t bytes_allocated() const override; | ||
|
|
||
| int64_t max_memory() const override; | ||
|
|
||
| int64_t proxy_bytes_allocated_; | ||
| int64_t proxy_bytes_allocated() const; | ||
|
|
||
| private: | ||
| MemoryPool* pool_; | ||
| }; | ||
|
|
||
| ARROW_EXPORT MemoryPool* default_memory_pool(); | ||
|
|
||
| #ifdef ARROW_NO_DEFAULT_MEMORY_POOL | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,18 @@ cdef class LoggingMemoryPool(MemoryPool): | |
| self.init(self.logging_pool.get()) | ||
|
|
||
|
|
||
| cdef class ProxyMemoryPool(MemoryPool): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docstring
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other classes are not documented in memory.pxi. Is there another location for documentation I'm missing?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is the main location where we should document things. We were not so good with adding docstrings in the past. I'll probably make a pass over the old classes soon and add some documentation but we should start adding documentation to new classes once we add them.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Would be happy to help with the old classes as well.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That would be very much appreciated! |
||
| cdef: | ||
| unique_ptr[CProxyMemoryPool] proxy_pool | ||
|
|
||
| def __cinit__(self, MemoryPool pool): | ||
| self.proxy_pool.reset(new CProxyMemoryPool(pool.pool)) | ||
| self.init(self.proxy_pool.get()) | ||
|
|
||
| def proxy_bytes_allocated(self): | ||
| return self.proxy_pool.get().proxy_bytes_allocated() | ||
|
|
||
|
|
||
| def default_memory_pool(): | ||
| cdef: | ||
| MemoryPool pool = MemoryPool() | ||
|
|
@@ -58,6 +70,8 @@ def set_memory_pool(MemoryPool pool): | |
| cdef MemoryPool _default_memory_pool = default_memory_pool() | ||
| cdef LoggingMemoryPool _logging_memory_pool = ( | ||
| LoggingMemoryPool(_default_memory_pool)) | ||
| cdef ProxyMemoryPool _proxy_memory_pool = ( | ||
| ProxyMemoryPool(_default_memory_pool)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of this? |
||
|
|
||
|
|
||
| def log_memory_allocations(enable=True): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the calls to
std::cout. If the user wants this, he should explicitly chose theLoggingMemoryPool.