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

NVTX range helpers #416

Merged
merged 16 commits into from
Dec 17, 2021
Merged

NVTX range helpers #416

merged 16 commits into from
Dec 17, 2021

Conversation

achirkin
Copy link
Contributor

Moves the helpers for NVTX ranges from cuml with minor additions.

Also adds the helpers to couple places in the codebase - as a way of testing the new feature.

@achirkin achirkin requested review from a team as code owners December 10, 2021 09:54
@achirkin achirkin added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change labels Dec 10, 2021
@achirkin achirkin requested a review from cjnolet December 10, 2021 10:12
@achirkin achirkin mentioned this pull request Dec 10, 2021
Closed
wphicks
wphicks requested changes Dec 10, 2021
View reviewed changes
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
@achirkin achirkin requested a review from wphicks December 13, 2021 08:48
@achirkin achirkin mentioned this pull request Dec 13, 2021
Merged
@achirkin achirkin changed the title NVTX ranges helpers NVTX range helpers Dec 13, 2021
cjnolet
cjnolet requested changes Dec 13, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I'm really happy to host this in RAFT and hopeful other consumers might also be able to make use of it. Mostly minor things and my thoughts on the macro usage.

cpp/include/raft/common/detail/nvtx.hpp Outdated Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Outdated Show resolved Hide resolved
cpp/test/nvtx.cpp Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Dec 14, 2021

Also tagging some folks from cugraph for their thoughts about the macro

Cc @seunghwak @ChuckHastings @BradReesWork @afender

@achirkin achirkin requested a review from cjnolet December 14, 2021 10:27
cjnolet
cjnolet requested changes Dec 16, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Changes look great, I just found one more very small thing as I was doing my final review.

cpp/include/raft/common/detail/nvtx.hpp Outdated Show resolved Hide resolved
@achirkin
Copy link
Contributor Author

As @teju85 suggested, I probably should also remove the versions of the helpers that synchronize cuda streams. The reason is that modern nsys profiler can map the ranges on gpu streams well, but addition of the lots of synchronize calls may bias the profiler/performance overview. @cjnolet, @wphicks do you have any objections to that?

@cjnolet
Copy link
Member

cjnolet commented Dec 16, 2021

If this is the case with all the ctk versions supported by rapids (Eg ctk 11+) then it's fine with me. Probably better not to modify the performance while we are trying to measure the performance.

@wphicks
Copy link
Contributor

wphicks commented Dec 16, 2021

No objections here. Makes sense!

wphicks
wphicks approved these changes Dec 16, 2021
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Since no one else has a strong opinion on the macro issue, I say it's better to be biased toward approving and we should move ahead. Let's take care of the stream synchronization issue and @cjnolet's last note on namespacing, then this looks good as far as I'm concerned.

@achirkin
Copy link
Contributor Author

All done, thanks for the input!

Note, I also added a template parameter Domain, which should allow an easy way to customize the NVTX domain message in downstream projects/applications.

One creates a domain by declaring a struct (anywhere):

raft/cpp/include/raft/common/nvtx.hpp

Lines 32 to 34 in d8357b2

struct raft {
static constexpr char const* name{"raft"};
};

And this is how to use it:

raft/cpp/include/raft/linalg/svd.cuh

Lines 67 to 68 in d8357b2

common::nvtx::range<common::nvtx::domain::raft> fun_scope(
"raft::linalg::svdQR(%d, %d)", n_rows, n_cols);

cjnolet
cjnolet approved these changes Dec 16, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

The changes look great, Artem. Thanks for moving this to raft and improving it!

@cjnolet
Copy link
Member

cjnolet commented Dec 16, 2021

Just a little side note: the usage links are great but it would also be nice to have usage examples right in the doxygen.

@achirkin
Copy link
Contributor Author

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Dec 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 754cc88 into rapidsai:branch-22.02 Dec 17, 2021
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Dec 17, 2021
@achirkin
Move NVTX range helpers to raft (#4445)
d630156 
Move NVTX range helpers to raft  and extend them a little bit.
Corresponding raft PR: rapidsai/raft#416 .

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #4445
@achirkin achirkin deleted the fea-nvtx-ranges branch March 31, 2022 06:10
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@achirkin
Move NVTX range helpers to raft (rapidsai#4445)
0be4062 
Move NVTX range helpers to raft  and extend them a little bit.
Corresponding raft PR: rapidsai/raft#416 .

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

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

@wphicks wphicks wphicks approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review CMake cpp feature request New feature or request non-breaking Non-breaking change python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@achirkin @cjnolet @wphicks