Repository navigation
Refactor memory manager. Remove buffer_info. Simplify allocs. Disable Events - #2689
Conversation
* Removed buffer_info from the allocation calls The buffer_info object was created so that the event synchonization was performed in the memAlloc call instead of in the memory manager. It did not have significant benefit over performing the synchronization within the memory manager and increased complexity of the API. Now that the events API is exposed, it shouldn't be necessary to include this. * Moved memory manager classes into separate files in the common directory * Use memAlloc instead of bufferAlloc in OpenCL Array<T> initializtion to provide type context to the memory manager. Avoid inconsistencies between backends. * Use objects instead of handles within the codebase
9prady9
left a comment
There was a problem hiding this comment.
Since you are refactoring bufferAlloc calls under OpenCL backend to memAlloc, can you do it other places also - they are mostly done in vision backend implementations.
|
For one of my NN, the speed seems a little better than the master branch (as today): |
WilliamTambellini
left a comment
There was a problem hiding this comment.
Just checking : is that change disabling events ?
|
Yes. This PR also disables events. |
jacobkahn
left a comment
There was a problem hiding this comment.
Looks good to me.
Is the intention here to completely remove events from memory management components for now rather than gating them? Removing af_buffer_info seems like it'll make adding events back later a breaking API change, unless we want to make Arrays aware of events rather than the memory manager
| ASSERT_EQ(payload->lastElementSize, 1); | ||
| } | ||
| ASSERT_EQ(payload->lastDims, af::dim4(bDim * b.numdims())); | ||
| ASSERT_EQ(payload->lastElementSize, sizeof(float)); |
| // DefaultMemoryManager(const DefaultMemoryManager &&other) = default; | ||
| DefaultMemoryManager &operator=(const DefaultMemoryManager &other) = delete; | ||
| // DefaultMemoryManager &operator=(const DefaultMemoryManager &&other) = | ||
| // default; |
There was a problem hiding this comment.
remove or uncomment the move ctor/assignment op? Although this should be movable.
|
I think the events could be handled in the memory manager without exposing it to ArrayFire. Earlier, I was thinking we could add the returned events and make it part of the Array object. I don't think this is necessary because the memory manager can enqueue those waits before it returns the pointer. The end result would be the same. With the events API exposed the memory manager can handle this sort of thing externally. |
|
@9prady9 converting from bufferAlloc to memAlloc would be a pretty large change. I think we should do that on another PR. |
The buffer_info object was created so that the event synchonization was
performed in the memAlloc call instead of in the memory manager. It did not have
significant benefit over performing the synchronization within the memory
manager and increased complexity of the API. Now that the events API is
exposed, it shouldn't be necessary to include this.
provide type context to the memory manager. Avoid inconsistencies between
backends.