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

[FEA] Add NVTX ranges to pool allocation/deallocation #495

Closed
rongou opened this issue Aug 13, 2020 · 9 comments · Fixed by #1558
Closed

[FEA] Add NVTX ranges to pool allocation/deallocation #495

rongou opened this issue Aug 13, 2020 · 9 comments · Fixed by #1558
Labels
? - Needs Triage Need team to review and classify feature request New feature or request inactive-30d inactive-90d
Projects
Feature Planning

Comments

@rongou
Copy link
Contributor

rongou commented Aug 13, 2020

Is your feature request related to a problem? Please describe.
It would be helpful to be able to see on an Nsight profile how much time is spend on allocating/deallocating memory in the pool memory resource, especially in a multi-threaded environment with per-thread default stream.

Describe the solution you'd like
Add NVTX ranges to memory allocation/deallocation in the pool.

Additional context
cuDF has a macro defined: https://github.com/rapidsai/cudf/blob/branch-0.15/cpp/include/cudf/detail/nvtx/ranges.hpp
@rongou rongou added ? - Needs Triage Need team to review and classify feature request New feature or request labels Aug 13, 2020
@github-actions github-actions bot added this to Needs prioritizing in Feature Planning Aug 13, 2020
@jrhemstad
Copy link
Contributor

@harrism went about this in the past. We ended up not going through with it because most allocation events are faster than the recommended 1us minimum time for events to annotate with NVTX. That said I don't see anything wrong with adding it as an option that is disabled by default.

I think we can take a simpler/coarser grained approach than #336 and just annotate the device_memory_resource base class allocate/deallocate calls, that way we can see the annotations no matter what resource is being used.

@jrhemstad
Copy link
Contributor

We're likely going to run into some difficulty/conflicts with having the nvtx3.hpp header in both RMM and libcudf until NVIDIA/NVTX#2 is merged and we can pull the header from there to ensure both have the same version of the header.

@rongou
Copy link
Contributor Author

rongou commented Aug 14, 2020

There is some concern with contentions around CUDA events used in the pool between different threads/streams, it'd be nice to have better insight into that scenario.

@harrism
Copy link
Member

harrism commented Aug 14, 2020

I have an open PR #336 but I am waiting (as Jake points out) on NVIDIA/NVTX#2 and I will just add NVTX regions at the top-level rather than at low levels within pool_memory_resource.

@github-actions
Copy link

This issue has been marked rotten due to no recent activity in the past 90d. 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.

@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. 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 marked rotten if there is no activity in the next 60d.

@harrism
Copy link
Member

harrism commented Feb 16, 2021 •
edited

Still waiting on NVIDIA/NVTX#2 to be put in a release.

@github-actions
Copy link

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.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 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.

@harrism harrism mentioned this issue May 9, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this issue May 23, 2024
@harrism
Add NVTX support and RMM_FUNC_RANGE() macro (#1558)
4e9482f 
Let's get RMM allocate/deallocates showing up in profiler timelines.

Closes #495

Authors:
  - Mark Harris (https://github.com/harrism)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
? - Needs Triage Need team to review and classify feature request New feature or request inactive-30d inactive-90d
Projects
RMM Project Board
Status: Done
Feature Planning
Needs prioritizing
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add NVTX support and RMM_FUNC_RANGE() macro harrism/rmm
3 participants
@rongou @harrism @jrhemstad