Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Tidy up storage allocation and deallocation #14480

Merged
merged 6 commits into from
Mar 28, 2019
Merged

Tidy up storage allocation and deallocation #14480

merged 6 commits into from
Mar 28, 2019

Conversation

yuxihu
Copy link
Member

@yuxihu yuxihu commented Mar 20, 2019 •
edited
Loading

Currently we have inconsistent allocation and deallocation behaviors among storage managers. There are two major observations:

  1. Different storage managers handle size zero allocation differently. For example, we round the size (0) to the page size (4096 bytes) in GPUPooledStorageManager; we perform aligned allocation (16/64 bytes) in CPUDeviceStorage; we do not round up the size in PinnedMemoryStorage.
  2. We allocated memory for size 0 storage handle in some storage managers but its memory was not freed because we skipped calling Free when size == 0. This caused memory leaks Gluon RNN memory leaks with extra variables #13951 Memory builds up when creating size-zero NDArray in a loop #14358

These inconsistencies make it error-prone to write code. In this PR, we tidy up the storage allocation and deallocation behaviors by doing:

  1. For size 0 storage handle (e.g. mx.nd.array([]), mx.nd.ones(0)), we skip allocating memory in all storage managers and set dtpr to nullptr in storage handle.
  2. We perform sanity check (if dptr is nullptr) in Free/DirectFree APIs to ensure sane behavior of memory deallocation.

This PR fixes #13951, fixes #14358

@yuxihu yuxihu requested a review from anirudh2290 as a code owner March 20, 2019 17:47
@yuxihu
Copy link
Member Author

yuxihu commented Mar 20, 2019

@mxnet-label-bot add [pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Mar 20, 2019
@yuxihu yuxihu changed the title Fix memory leak for size-zero ndarray Tidy up storage allocation and deallocation Mar 20, 2019
@yuxihu yuxihu force-pushed the fix_mem_leak branch 3 times, most recently from cfbb447 to d9396da Compare March 21, 2019 19:15
@yuxihu
Copy link
Member Author

yuxihu commented Mar 21, 2019 •
edited
Loading

@eric-haibin-lin @apeforest @anirudh2290 Please help review.

@yuxihu
Copy link
Member Author

yuxihu commented Mar 25, 2019

@mxnet-label-bot update [pr-awaiting-review]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Mar 25, 2019
larroy
larroy reviewed Mar 26, 2019
View reviewed changes
src/storage/cpu_device_storage.h Outdated Show resolved Hide resolved
src/storage/cpu_shared_storage_manager.h Show resolved Hide resolved
src/storage/gpu_device_storage.h Outdated Show resolved Hide resolved
src/storage/pinned_memory_storage.h Outdated Show resolved Hide resolved
src/storage/pinned_memory_storage.h Outdated Show resolved Hide resolved
apeforest
apeforest approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. just two small nit comments for styling.

@yuxihu
Copy link
Member Author

yuxihu commented Mar 28, 2019

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Mar 28, 2019
@anirudh2290 anirudh2290 merged commit 645c778 into apache:master Mar 28, 2019
@yuxihu yuxihu deleted the fix_mem_leak branch March 28, 2019 04:33
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@yuxihu @vdantu
Tidy up storage allocation and deallocation (apache#14480)
c1e2535 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
ZhennanQin pushed a commit to ZhennanQin/incubator-mxnet that referenced this pull request Apr 3, 2019
@yuxihu @ZhennanQin
Tidy up storage allocation and deallocation (apache#14480)
ed0c2d8 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@yuxihu @nswamy
Tidy up storage allocation and deallocation (#14480)
a310b44 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
yuxihu added a commit to yuxihu/incubator-mxnet that referenced this pull request Apr 22, 2019
@yuxihu
Tidy up storage allocation and deallocation (apache#14480)
464d74d 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
szha pushed a commit that referenced this pull request Apr 23, 2019
@yuxihu @szha
Tidy up storage allocation and deallocation (#14480) (#14768)
b986d06 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@yuxihu
Tidy up storage allocation and deallocation (apache#14480)
313446e 
* free memory when dptr is not nullptr

* skip memory allocation when handle size is 0

* update comments

* update Alloc in naive storage manager

* address comments

* add unit test for size 0 allocation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest approved these changes

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@larroy larroy larroy left review comments

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory builds up when creating size-zero NDArray in a loop Gluon RNN memory leaks with extra variables
5 participants
@yuxihu @larroy @anirudh2290 @apeforest @marcoabreu