Skip to content
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

Fixed a memory leak issue in InternalThread (and removed caffe::Thread) #1335

Merged
merged 2 commits into from
Jan 16, 2015

Conversation

ryotat
Copy link

@ryotat ryotat commented Oct 20, 2014

This PR fixes the memory leak issue I posted here:
#1316 (comment)

@shelhamer
Copy link
Member

caffe::Thread is required for cross-platform compatibility -- see #1009 and #1010. Restore the wrapper but keep your fix.

@seanbell
Copy link

I think that there should be a comment above the wrapper explaining its purpose. Otherwise it seems useless. I deleted it in my fork, not realizing that it had a use on other platforms.

@shelhamer
Copy link
Member

Good idea. @ryotat please add a comment to explain the caffe::Thread facade like the one for boost RNG here: https://github.com/BVLC/caffe/blob/master/include/caffe/common.hpp#L83-L84

@ryotat
Copy link
Author

ryotat commented Oct 21, 2014

How about forward declaring boost::thread instead of using a wrapper class? Could someone try my commit 85477b7 if it compiles correctly on OSX?

@shelhamer shelhamer self-assigned this Jan 15, 2015
@shelhamer shelhamer merged commit 85477b7 into BVLC:master Jan 16, 2015
shelhamer added a commit that referenced this pull request Jan 16, 2015
Fix leaking thread and groom internal thread implementation.
@shelhamer
Copy link
Member

Thanks @ryotat -- forward declaration worked fine on OS X. I linted the code after merge, but please remember to do make lint for PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants