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.

Sign up for GitHub

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

Jump to bottom

[BUG] Improve C++ test coverage #906

Closed
harrism opened this issue Nov 4, 2021 · 7 comments · Fixed by #916
Closed

[BUG] Improve C++ test coverage #906

harrism opened this issue Nov 4, 2021 · 7 comments · Fixed by #916
Assignees
@rongou @harrism
Labels
bug Something isn't working tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general

Comments

@harrism
Copy link
Member

harrism commented Nov 4, 2021

With #905 we now have C++ code coverage analysis for RMM. Coverage is not bad in a lot of places, but can be improved (we're at about 77%).

It shouldn't be hard to close the gaps, they are mostly tests of simple infrequently used methods on memory resources and containers.
@harrism harrism added bug Something isn't working tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general labels Nov 4, 2021
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@harrism
Copy link
Member Author

harrism commented Dec 6, 2021

Currently only arena_mr and arena.hpp need coverage improved. As of #920 coverage of most files is 100%.

@harrism
Copy link
Member Author

harrism commented Dec 6, 2021

We also need to look at getting coverage testing into CI.

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@harrism
Copy link
Member Author

harrism commented Jan 11, 2022

@rongou have you checked test coverage with your latest arena testing improvements in #916 ?

@rongou
Copy link
Contributor

rongou commented Jan 12, 2022

With the latest code it shows mr/device/detail/arena.hpp at 94.1%. I'll see if I can improve it further.

@rongou
Copy link
Contributor

rongou commented Jan 12, 2022

At 100% now.

@rongou rongou mentioned this issue Jan 12, 2022
Merged
@rapids-bot rapids-bot bot closed this as completed in #916 Jan 12, 2022
rapids-bot bot pushed a commit that referenced this issue Jan 12, 2022
@rongou
Improve the Arena allocator to reduce memory fragmentation (#916)
ea807e8 
Currently the arena allocator divides GPU memory into a global arena and per-thread arenas. For smaller allocations, a per-thread arena allocates large chunks of memory (superblocks) from the global arena and divides them up for individual allocations. However, when deallocating from another arena (producer/consumer pattern), or when we run out of memory and return everything to the global arena, the superblock boundaries are broken. Overtime, this could cause the memory to get more and more fragmented.

This PR makes superblocks concrete objects, not just virtual boundaries, and the only units of exchange between the global arena and per-thread arenas. This should make the allocator more resistant to memory fragmentation, especially for long running processes under constant memory pressure.

Other notable changes:
* The allocator now allocates a fixed but configurable amount of memory from CUDA. This introduces less fragmentation comparing to growing the pool size gradually.
* Switched to C++17 `std::shared_mutex`.
* Added a bunch of unit tests.

fixes #919 
fixes #906

Authors:
  - Rong Ou (https://github.com/rongou)

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Mark Harris (https://github.com/harrism)

URL: #916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rongou rongou

@harrism harrism

Labels
bug Something isn't working tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve the Arena allocator to reduce memory fragmentation rongou/rmm
2 participants
@rongou @harrism