Add paddle/memory/README.md #2552
Conversation
paddle/README.md
Outdated
| In `paddle/memory/memory.h` we have: | ||
|
|
||
| ```cpp | ||
| template <typeanme Place> void* Alloc(Place, size_t); |
There was a problem hiding this comment.
typeame -> typename
Since Place is a variant, why not directly use the typeid to static analyze place type during compilation?
void* Alloc(Place, size_t);
void* Alloc(Place place, size_t size) {
if (place.type() == typeid(GPUPlace)) {
....
} else {...}
}There was a problem hiding this comment.
Because we want the mismatched-type error get reported as early as possible. Err at compile time is earlier than runtime.
There was a problem hiding this comment.
I got it. Thanks.
But variant also supports throw the mismatched-type error during compile time.
There was a problem hiding this comment.
Actually, I think if we regulate the way we use GPUPlace and CPUPlace, like here -- two and only two specializations of Alloc<Place>, we might not need typedef boost::variant<GPUPlace, CPUPlace> Place at all. And this might help us remove the dependency to boost.
There was a problem hiding this comment.
It's great to think about this. The hard part to remove boost::variant is Dim.
There was a problem hiding this comment.
Yeah. Agree. And thanks for reminding of boost::variant. Let's start by minimizing dependencies of each piece of our work.
paddle/README.md
Outdated
| BuddyAllocator* GetCPUBuddyAllocator() { | ||
| static BuddyAllocator* a = NULL; | ||
| if (a == NULL) { | ||
| a = new BuddyAllocator(new CPUAllocator /*backup allocator*/, ...); |
There was a problem hiding this comment.
All member functions in CPUAllocator are static functions, I thought it's better if CPUAllocator /CPUAllocator could be a template type of BuddyAllocator
There was a problem hiding this comment.
I don't think GPUAllocator::Alloc could be a static method, as it reads GPUAllocator::place_ and writes GPUAllocator::used_.
There was a problem hiding this comment.
OK, I guess we do not need the member variables in here.
GPUAllocator::place_ is unnecessary because you got place from memory::Alloc(place ... each time and GPUAllocator::Alloc insides memory::Alloc.
GPUAllocator::used_ is unnecessary. Because each time when you retrieve/query the used memory, it's non-deterministic and likely to change and fluctuate.
There was a problem hiding this comment.
I mean, in this design, each GPUAllocator instance corresponds to a GPU device.
There was a problem hiding this comment.
Why GPUAllocator::used_ is unnecessary? I think memory profiling is very important for a DL system?
There was a problem hiding this comment.
Yeah, It's useful. I mean it's unnecessary to save it in GPUAllocator::used_. because its behavior always to change and fluctuate.
There was a problem hiding this comment.
I see. I cannot tell how valuable GPUAllocator::used_ could be. But I'd like to have it, as it enables us to profile the memory usage. We may figure out good usages of this piece of information later.
paddle/README.md
Outdated
|
|
||
| ### The Implementation | ||
|
|
||
| `GetCPUBuddyAllocator` and `GetGPUBuddyAllocator` are singletions. |
There was a problem hiding this comment.
Singleton is great in here.
paddle/README.md
Outdated
|
|
||
| ```cpp | ||
| template<> | ||
| void Alloc(GPUPlace)(GPUPlace p, size_t size) { |
There was a problem hiding this comment.
Alloc(GPUPlace) -> Alloc<GPUPlace>
paddle/README.md
Outdated
| private: | ||
| struct Block { | ||
| size_t size; | ||
| Blobk* left, right; |
paddle/README.md
Outdated
|
|
||
| The `GPUAllocator` and `CPUAllocator` are calls *system allocators*. They hold information about the device, including the amount of memory has been allocated. So that we can call | ||
|
|
||
| - `GPUAllocator::Used` and |
There was a problem hiding this comment.
It seems that System Allocators are used when constructing a BuddyAllocator and they are private data member inside a BuddyAllocator. How can we get GPUAllocator::Used for each device?
Is our code like this?
auto* buddyAllocator = GetGPUBuddyAllocator(0);
buddyAllocator->SystemAllocator()->Used();There was a problem hiding this comment.
or
GetGPUAllocator(0)->Used();There was a problem hiding this comment.
Thanks for the reminder. Let me explain more here.
paddle/memory/README.md
Outdated
| @@ -0,0 +1,145 @@ | |||
| In my mind, the memory package works like the following: | |||
There was a problem hiding this comment.
Should this line be removed?
No description provided.