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: Use RangePush and Domain #293

Merged

Conversation

evanramos-nvidia
Copy link

This branch makes two changes:

  • Replaces the use of the nvtxRangeStart/RangeEnd API with RangePush/RangePop
  • Tags NVTX events with a domain specific to legate.core for easy filtering

@magnatelee magnatelee self-requested a review July 11, 2022 21:36
@magnatelee
Copy link
Contributor

@evanramos-nvidia thanks for the PR. can you briefly explain me how Domains manifest in Nsight timelines? I was trying to find the domain name of my ranges, but couldn't, probably because I'm not using the latest Nsight?

@@ -76,6 +80,10 @@ static const char* const core_library_name = "legate.core";

const char* sync_stream_view = getenv("LEGATE_SYNC_STREAM_VIEW");
if (sync_stream_view != nullptr && atoi(sync_stream_view) > 0) synchronize_stream_view = true;

#ifdef LEGATE_USE_CUDA
nvtx::Range::initialize(core_library_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would want a separate Domain per Legate library, instead of putting all ranges under the same domain. A prerequisite for that is a centralized registry of all legate libraries, which doesn't exist in this branch. we can certainly merge this PR and tackle the issue in a follow-on PR, as the registry can take some time to implement properly.

Copy link
Author

Choose a reason for hiding this comment

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

After consulting with @jcohen-nvidia, we think your idea would be better suited to the Categories feature in NVTX rather than creating separate domains, which would come with a performance cost and also complicate the implementation of a toggle switch for tracing Legate in Nsight Systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I repeat here what I said in the meeting to remind ourselves: I still think each of Legate libraries should have a separate Domain , as they are physically different libraries with separate APIs that the user may or may not choose to trace, and each library might want to use its own set of categories for the domain operations. We can merge this PR as-is for now and I can make a follow-on PR for a per-library Domain (which would naturally fit to this LibraryContext: https://github.com/nv-legate/legate.core/blob/branch-22.07/src/core/runtime/context.h#L65-L105).

@evanramos-nvidia
Copy link
Author

@evanramos-nvidia thanks for the PR. can you briefly explain me how Domains manifest in Nsight timelines? I was trying to find the domain name of my ranges, but couldn't, probably because I'm not using the latest Nsight?

No problem! Domains are listed on the left side of the timeline view. I've highlighted one of the instances of the Legate domain in this screenshot:

Screenshot from 2022-07-18 17-57-25

@evanramos-nvidia
Copy link
Author

I updated my branch to use Legate as the domain name string rather than legate.core, in line with other tracing functionality in Nsys.

@evanramos-nvidia evanramos-nvidia changed the base branch from branch-22.07 to branch-22.10 August 23, 2022 18:22
@magnatelee magnatelee added the category:improvement PR introduces an improvement and will be classified as such in release notes label Sep 3, 2022
@evanramos-nvidia evanramos-nvidia changed the base branch from branch-22.10 to branch-22.12 January 10, 2023 18:20
@marcinz marcinz changed the base branch from branch-22.12 to branch-23.03 January 26, 2023 01:07
@magnatelee magnatelee added category:task PR is a simple task and will not be included in release notes and removed category:task PR is a simple task and will not be included in release notes labels Feb 2, 2023
@magnatelee magnatelee merged commit 07e86ed into nv-legate:branch-23.03 Feb 2, 2023
magnatelee added a commit to magnatelee/legate.core that referenced this pull request Feb 17, 2023
magnatelee added a commit that referenced this pull request Feb 21, 2023
manopapad pushed a commit that referenced this pull request Mar 5, 2025
…#293)

* Enforce the C++ standards versions, which somehow got lost previously

* add warning message about user setting CMAKE_CXX_STANDARD themselves
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants