-
Notifications
You must be signed in to change notification settings - Fork 548
Fix memory pressure in DefaultMemoryManager #2801
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
Conversation
So after talking to @padentomasello offline, we've discovered the issue is a bit simpler. There are two distinct issues that affect the default memory manager:
tl;dr — we don't need the ratio idea presented in this PR and can use the existing |
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.
Excellent catch!
As @jacobkahn mentioned in his response I think the correct fix would be to revert to total_buffers and fix the condition where ever the getMemoryPressure function is used. I want to avoid changing behavior in bug fixes and patch releases especially if we cannot measure the performance implications of such a change.
I noticed that these comparisons will need to be modified:
https://github.com/arrayfire/arrayfire/blob/master/src/backend/cpu/queue.hpp#L72
https://github.com/arrayfire/arrayfire/blob/master/src/backend/cuda/Array.cpp#L251
https://github.com/arrayfire/arrayfire/blob/master/src/backend/opencl/Array.cpp#L299
This gist should cover everything that needs changing for now. |
be1d0c0
to
2b3b605
Compare
@jacobkahn @umar456 Thank you for the feedback. I did not initially realize that getMemoryPressure was used outside of the memory manager. I have updated the PR to match @jacobkahn's gist. |
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.
This looks perfect. Thanks!
* Fix getMemoryPressure comparison, and revert DefaultMemoryManager GC comparison. Co-authored-by: Paden Tomasello <padentomasello@devfair049.maas>
* Fix getMemoryPressure comparison, and revert DefaultMemoryManager GC comparison. Co-authored-by: Paden Tomasello <padentomasello@devfair049.maas>
Summary: This fixes a bug that caused memory to not be garbage collected until we ran out of memory in v3.7.0.
When we switched from MemoryManagerImpl to DefaultMemoryManager, we switched the memory pressure check to check for
lock_buffers
instead oftotal_buffers
which I'm assuming was unintentional:https://github.com/arrayfire/arrayfire/blob/v3.6.4/src/backend/common/MemoryManagerImpl.hpp#L379
https://github.com/arrayfire/arrayfire/blob/v3.7.0/src/backend/common/DefaultMemoryManager.cpp#L132
Also, we now check against
getMemoryPressureThreshold()
which default value is 1.0. Since we were returning 1.0 fromgetMemoryPressure()
, it would never trigger.(https://github.com/padentomasello/arrayfire/blob/v3.7.0/src/backend/common/DefaultMemoryManager.cpp#L165)
I changed it to return to ratio now instead. This should have the intended behavior, plus allow users to adjust the memory_threshold for meaningful changes.