-
Notifications
You must be signed in to change notification settings - Fork 609
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
Shrink host buffers #1712
Shrink host buffers #1712
Conversation
Reduce the size of the underlying allocation for non-pinned host buffers when the new size is less than 90% of current allocation size. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
!build |
CI MESSAGE: [1107586]: BUILD STARTED |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
!build |
CI MESSAGE: [1107944]: BUILD STARTED |
@@ -270,6 +271,21 @@ class Buffer { | |||
|
|||
DISABLE_COPY_MOVE_ASSIGN(Buffer); | |||
|
|||
static void SetGrowthFactor(double factor) { | |||
assert(factor >= 1.0); | |||
growth_factor_ = factor; |
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.
mutex lock?
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.
I think it's not necessary and definitely quite costly. We could make it atomic, but that's still a bit of overdoing things. It's not a critical resource and the function is extremely unlikely to be used at the time the value is used. The timing is not critical and there can be no race condition, since the value is only used once in ResizeHelper
.
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.
fair enough
CI MESSAGE: [1107944]: BUILD FAILED |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
b181bb7
to
7ce5aec
Compare
!build |
CI MESSAGE: [1107971]: BUILD STARTED |
@@ -40,13 +43,30 @@ void subscribe_signals() { | |||
|
|||
#endif | |||
|
|||
void InitializeBufferPolicies() { | |||
if (const char *threshold_str = std::getenv("DALI_HOST_BUFFER_SHRINK_THRESHOLD")) { |
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.
Maybe we can extract this setter to some common place as now we have ranges spread in two places.
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.
I moved the max growth factor to buffer.h.
!build |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
CI MESSAGE: [1107586]: BUILD PASSED |
CI MESSAGE: [1108086]: BUILD STARTED |
CI MESSAGE: [1107971]: BUILD PASSED |
CI MESSAGE: [1108086]: BUILD PASSED |
- updates `Custom operator` documentation to reflects the most recent DALI operator API - updates `Memory consumption` docs section to reflect shape inference and changes from `Shrink host buffers (#1712)` Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Signed-off-by: Michal Zientkiewicz michalz@nvidia.com
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
JIRA TASK: N/A