Fix memory pressure in DefaultMemoryManager#2801
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 |
umar456
left a comment
There was a problem hiding this comment.
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. |
* 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_buffersinstead oftotal_bufferswhich 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.