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

Adding out of memory handler, making arena bins more detailed #73

Merged
merged 1 commit into from
Nov 10, 2015
Merged

Adding out of memory handler, making arena bins more detailed #73

merged 1 commit into from
Nov 10, 2015

Conversation

borisfom
Copy link

@borisfom borisfom commented Nov 9, 2015

No description provided.

@lukeyeager
Copy link
Member

This would be easier to review with whitespace fixes in a separate commit.

To anyone else trying to review this, add the ?w= flag to view non-whitespace changes only:
https://github.com/NVIDIA/caffe/pull/73/files?w=

@thatguymike
Copy link

My primary concern is the modifications into CUB. We should get those discussed in CUB or create a separate branch to avoid confusion and later cross merge issues.

@borisfom
Copy link
Author

Well, I am a bit confused what is PR guideline is exactly: last few times I was squashing my commits into one to avoid extra chatter.
Also, many of those lines that differ only in whitespace, is a result of code reindent after changing if()/while)() blocks - again, diff would be better if I do not reinvent, but the result would be ugly...
We may agree of submitting diffs for review first without formatting and anti-lint changes, and then squaring, I suppose - let me know if that is when you want.

Best,
-Boris.

From: Luke Yeager <notifications@github.commailto:notifications@github.com>
Reply-To: NVIDIA/caffe <reply@reply.github.commailto:reply@reply.github.com>
Date: Monday, November 9, 2015 at 3:45 PM
To: NVIDIA/caffe <caffe@noreply.github.commailto:caffe@noreply.github.com>
Cc: Boris Fomitchev <bfomitchev@nvidia.commailto:bfomitchev@nvidia.com>
Subject: Re: [caffe] Adding out of memory handler, making arena bins more detailed (#73)

This would be easier to review with whitespace fixes in a separate commit.

To anyone else trying to review this, add the ?w= flag to view non-whitespace changes only:
https://github.com/NVIDIA/caffe/pull/73/files?w=

Reply to this email directly or view it on GitHubhttps://github.com//pull/73#issuecomment-155235387.


This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by

reply email and destroy all copies of the original message.

@borisfom
Copy link
Author

Hi Mike,

You're right. I was planning todays changes to be entirely in gpu_memory.
Then I have noticed my previous changes to CUB allocator had some corner cases uncovered so I decided to put the whole handler there.
We'll revise the changes with CUB people later anyway - there are more things to consider there anyway, like scoped locks, c++ std mutex, etc.
The worst case scenario is our version of allocator diverging from Cub for a while.

Best,
-Boris.

From: Michael Houston <notifications@github.commailto:notifications@github.com>
Reply-To: NVIDIA/caffe <reply@reply.github.commailto:reply@reply.github.com>
Date: Monday, November 9, 2015 at 3:48 PM
To: NVIDIA/caffe <caffe@noreply.github.commailto:caffe@noreply.github.com>
Cc: Boris Fomitchev <bfomitchev@nvidia.commailto:bfomitchev@nvidia.com>
Subject: Re: [caffe] Adding out of memory handler, making arena bins more detailed (#73)

My primary concern is the modifications into CUB. We should get those discussed in CUB or create a separate branch to avoid confusion and later cross merge issues.

Reply to this email directly or view it on GitHubhttps://github.com//pull/73#issuecomment-155235829.


This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by

reply email and destroy all copies of the original message.

@lukeyeager
Copy link
Member

Well, I am a bit confused what is PR guideline is exactly: last few times I was squashing my commits into one to avoid extra chatter.

@borisfom fair point - sorry for the nitpicking. I'm trying to follow the coding style that BVLC uses, since we hope to upstream our changes there eventually. They don't document their PR requirements, exactly, but you can infer it from various comments.

Rearranging lines is fine, but let's have a separate commit for that.
BVLC#3088 (comment)

Also, there seems to be a long irrelevant commit history here... please do squash your changes into a single commit (which should be fine here, although a few logically separated commits is also fine).
BVLC#2946 (comment)

Rebased now that 2836 is merged, and unrelated changes to TestGradientBasedSolver are now in separate commits.
BVLC#2866 (comment)

This looks good to me, so please squash for merge. However 526372 can stay its own commit to explain Travis CI is CPU-only although the message should change to reflect that.
BVLC#2523 (comment)

That commit should really probably have been 3 separate commits with good clarifying messages, as they were pretty substantial now that I look back on it...
BVLC#1977 (comment)

http://caffe.berkeleyvision.org/development.html

@borisfom
Copy link
Author

Thanks Luke! Duly noted. For this small commit it seems logical just to merge as it is.

borisfom added a commit that referenced this pull request Nov 10, 2015
Adding out of memory handler, making arena bins more detailed
@borisfom borisfom merged commit 934b378 into NVIDIA:caffe-0.14 Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants