Fix memory pressure in DefaultMemoryManager by padentomasello · Pull Request #2801 · arrayfire/arrayfire · GitHub
Skip to content

Fix memory pressure in DefaultMemoryManager#2801

Merged
umar456 merged 2 commits intoarrayfire:masterfrom
padentomasello:dev/paden/mem-debug
Mar 24, 2020
Merged

Fix memory pressure in DefaultMemoryManager#2801
umar456 merged 2 commits intoarrayfire:masterfrom
padentomasello:dev/paden/mem-debug

Conversation

@padentomasello
Copy link
Copy Markdown
Contributor

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 of total_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 from getMemoryPressure(), 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.

@jacobkahn
Copy link
Copy Markdown
Contributor

jacobkahn commented Mar 21, 2020

Copy link
Copy Markdown
Member

@umar456 umar456 left a 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

@jacobkahn
Copy link
Copy Markdown
Contributor

This gist should cover everything that needs changing for now.

@padentomasello
Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Member

@umar456 umar456 left a 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!

@umar456 umar456 merged commit 10a82db into arrayfire:master Mar 24, 2020
umar456 pushed a commit to umar456/arrayfire that referenced this pull request Mar 26, 2020
* Fix getMemoryPressure comparison, and revert DefaultMemoryManager GC comparison.

Co-authored-by: Paden Tomasello <padentomasello@devfair049.maas>
umar456 pushed a commit that referenced this pull request Mar 27, 2020
* Fix getMemoryPressure comparison, and revert DefaultMemoryManager GC comparison.

Co-authored-by: Paden Tomasello <padentomasello@devfair049.maas>
@9prady9 9prady9 added this to the 3.7.1 milestone Apr 8, 2020
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.

4 participants