Skip to content
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

Enhance the CachingMemoryManager to possibly handle high memory pressure #188

Closed
wants to merge 1 commit into from

Conversation

WilliamTambellini
Copy link
Contributor

Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

Original Issue:
#180
closes #180

Summary

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

Test Plan (required)

Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 29, 2020
Copy link
Member

@jacobkahn jacobkahn left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for the PR.

flashlight/test/memory/CachingMemoryManagerTest.cpp Outdated Show resolved Hide resolved
flashlight/test/memory/CachingMemoryManagerTest.cpp Outdated Show resolved Hide resolved
flashlight/test/memory/CachingMemoryManagerTest.cpp Outdated Show resolved Hide resolved
ext/CMakeLists.txt Outdated Show resolved Hide resolved
flashlight/memory/MemoryManagerAdapter.cpp Outdated Show resolved Hide resolved
flashlight/memory/managers/CachingMemoryManager.cpp Outdated Show resolved Hide resolved
flashlight/memory/managers/CachingMemoryManager.cpp Outdated Show resolved Hide resolved
flashlight/memory/managers/CachingMemoryManager.h Outdated Show resolved Hide resolved
flashlight/test/memory/CachingMemoryManagerTest.cpp Outdated Show resolved Hide resolved
flashlight/test/memory/CachingMemoryManagerTest.cpp Outdated Show resolved Hide resolved
Copy link
Member

@jacobkahn jacobkahn left a comment

Choose a reason for hiding this comment

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

See suggestion here - #188 (comment). Almost there.

Copy link
Member

@jacobkahn jacobkahn left a comment

Choose a reason for hiding this comment

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

Nice — almost there! :)

apply-format Outdated Show resolved Hide resolved
ext/CMakeLists.txt Outdated Show resolved Hide resolved
flashlight/memory/managers/CachingMemoryManager.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@WilliamTambellini WilliamTambellini left a comment

Choose a reason for hiding this comment

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

done

apply-format Outdated Show resolved Hide resolved
ext/CMakeLists.txt Outdated Show resolved Hide resolved
flashlight/memory/managers/CachingMemoryManager.h Outdated Show resolved Hide resolved
Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jacobkahn has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jacobkahn
Copy link
Member

@WilliamTambellini - thank again for the contribution - looks good to me - do me a favor and rebase on matter then we'll be ready to merge.

@facebook-github-bot
Copy link

@WilliamTambellini has updated the pull request. You must reimport the pull request before landing.

@WilliamTambellini
Copy link
Contributor Author

@WilliamTambellini - thank again for the contribution - looks good to me - do me a favor and rebase on matter then we'll be ready to merge.

done

@facebook-github-bot
Copy link

@WilliamTambellini has updated the pull request. You must reimport the pull request before landing.

target_link_libraries(Train flashlight-app-asr)
target_link_libraries(Test flashlight-app-asr)
target_link_libraries(Decoder flashlight-app-asr)
target_link_libraries(Train flashlight-app-asr ${CMAKE_DL_LIBS})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacobkahn This fixes circleCI link errors:
[ 8%] Linking CXX executable Train
/usr/bin/ld: CMakeFiles/Train.dir/flashlight/common/Plugin.cpp.o: undefined reference to symbol 'dlclose@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libdl.so.2: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

Enhance the CachingMemoryManager to possibly handle high memory
pressure by adding 2 options.
Note: the default behavior is unchanged.
@facebook-github-bot
Copy link

@WilliamTambellini has updated the pull request. You must reimport the pull request before landing.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jacobkahn has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jacobkahn has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link

@jacobkahn merged this pull request in 1b5796e.

avidov pushed a commit to avidov/flashlight that referenced this pull request Jan 14, 2021
Add flags to set configurable limits of the CachingMemoryManager. These flags
allow customization for different workloads. This diff changes the default recycling
size limit for training to 256MB. Following investigation of OOM on dynamic batching
nd based on past allocation analyses we found that this limit works well.
Big thanks for WilliamTambellini@ for his work on improving the memory manager
capability in handling flexible workload:
flashlight#188
avidov pushed a commit to avidov/flashlight that referenced this pull request Jan 14, 2021
Add flags to set configurable limits of the CachingMemoryManager. These flags
allow customization for different workloads. This diff changes the default recycling
size limit for training to 256MB. Following investigation of OOM on dynamic batching
nd based on past allocation analyses we found that this limit works well.
Big thanks for WilliamTambellini@ for his work on improving the memory manager
capability in handling flexible workload:
flashlight#188
avidov pushed a commit to avidov/flashlight that referenced this pull request Jan 14, 2021
Add flags to set configurable limits of the CachingMemoryManager. These flags
allow customization for different workloads. This diff changes the default recycling
size limit for training to 256MB. Following investigation of OOM on dynamic batching
nd based on past allocation analyses we found that this limit works well.
Big thanks for WilliamTambellini@ for his work on improving the memory manager
capability in handling flexible workload:
flashlight#188
avidov pushed a commit to avidov/flashlight that referenced this pull request Jan 20, 2021
Add flags to set configurable limits of the CachingMemoryManager. These flags
allow customization for different workloads. This diff changes the default recycling
size limit for training to 256MB. Following investigation of OOM on dynamic batching
nd based on past allocation analyses we found that this limit works well.
Big thanks for WilliamTambellini@ for his work on improving the memory manager
capability in handling flexible workload:
flashlight#188
jacobkahn pushed a commit to flashlight/text that referenced this pull request Mar 25, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/text that referenced this pull request Mar 25, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/text that referenced this pull request Mar 25, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/text that referenced this pull request Mar 25, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/text that referenced this pull request Mar 25, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/sequence that referenced this pull request Oct 13, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/sequence that referenced this pull request Oct 13, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
jacobkahn pushed a commit to flashlight/sequence that referenced this pull request Oct 13, 2022
…ure (#188)

Summary:
Enhance the CachingMemoryManager to handle high memory pressure by adding 2 options.
Note: the default behavior is unchanged.
Add a new API to MemoryManagerAdapter to setOption().
Side: remove some glog includes failing compilation if glog-dev not installed locally

**Original Issue**:
flashlight/flashlight#180
closes flashlight/flashlight#180

The CachingMemoryManager being greedy in memory. This PR adds 2 options to mitigate this.

### Test Plan (required)
Implemented a new unitest in order to show OOM with the default CachingMM and no OOM if the right option is set.

Pull Request resolved: flashlight/flashlight#188

Reviewed By: vineelpratap

Differential Revision: D24435873

Pulled By: jacobkahn

fbshipit-source-id: 99186439c1e306ad987079cc326ab68fe1f028fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance the CachingMemoryManager for low mem usecase
3 participants