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

NVTX range helpers #416

Merged
merged 16 commits into from
Dec 17, 2021
Merged

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.

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 changed the title NVTX ranges helpers NVTX range helpers Dec 13, 2021
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
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!

Copy link
Contributor

@wphicks wphicks left a comment

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):

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

And this is how to use it:

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

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
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants