-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Make SyncedMem and Blob be able to increase capacities without deallocation and reallocation #250
Conversation
Rebased and ready for being merged. |
This PR is rebased and tested. @shelhamer, please have it reviewed. Thanks! |
Sorry, there seems to be some kind of Makefile conflict after Jeff's #277. Please rebase another time. |
The Makefile is now consistent. Please review again. |
|
||
const void* SyncedMemory::cpu_data() { | ||
to_cpu(); | ||
return reinterpret_cast<const void*>(thrust::raw_pointer_cast( |
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 use static_cast
for this and the other 3 casts to void *
in this file. I realize that make lint
tells you to use reinterpret_cast
and reinterpret_cast
is used to cast to/from void *
at many places in the code (probably most of which are due to my original lint pass through the code base where I blindly followed the tool's advice), but I've since learned that static_cast
seems to be more correct/portable and reinterpret_cast
should mostly only be used as a last resort: http://stackoverflow.com/questions/5013596/when-convert-a-void-pointer-to-a-specific-type-pointer-which-casting-symbol-is?rq=1
Rebased and the casts replaced. |
Hey @kloudkl, I was going to re-rebase this and merge it myself, but I found that many unit tests seem to fail for me. I first rebased on dev and ran the tests and found they were failing, so I thought I might have messed up the rebase somehow, but then I checked out your unmodified PR and ran the tests there, and still got the same failures. It seems some blob SyncedMem's are now uninitialized so I get errors when the tests try to access their data/diff:
Do you have any thoughts on this? (I'm happy to finish up this PR if you don't have time to do it yourself -- sorry for the delay in merging.) |
I‘m indeed too busy to maintain it recently. Some similar errors were fixed by aae7d91 and all the tests did pass two weeks ago. But I have almost no idea what happened on the bleeding edge of the @jeffdonahue, if you are kind enough to finish it and make it into version 1.0, I would appreciate it very much. |
Closing as I've taken over this change in #355. |
Hotfix: New status code for cuDNN v6
Currently, whenever the Blob reshapes, the underlying SyncedMem is deallocated and reallocated. In contrast, std::vector, cv::Mat/GpuMat, thrust::host_vector/device_vector and many other memory container classes are aware of data size and container capacity. The size is the actual number of elements and the capacity is the max number of elements. Changing the size will cause the capacity to increase to be large enough if necessary but never decrease. While changing the capacity does not change the size.
To address the problem of repeated deallocation and reallocation of memory, this PR delegates the memory management tasks to thrust::host_vector/device_vector to save the trouble of reinvention. When the Blob reshapes and the capacity is not enough, only the extra amount of memory will be allocated. At the same time, memory allocation and reallocation are kept as lazy as possible to avoid wasting memory.