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

roctracer: use multiple tracks for HIP streams #201

Closed
wants to merge 4 commits into from

Conversation

Maetveis
Copy link
Contributor

@Maetveis Maetveis commented Nov 10, 2022

Use different perfetto tracks for each stream, and set the name of these tracks to the stream pointer values. Setting the name like this matches the args in the API traces.
This fixes overlapping work on multiple streams appearing as a call stack.

Here's a screenshot of the results:
out

I would like to add an option to disable the new behaviour, because some applications might create a large number of streams which would obscure the traces. The environment variable and option parsing parts I don't yet fully understand so therefore Draft, and I would appreciate some hints for that.

Use different perfetto tracks for each stream, and set the name of
these tracks to the stream pointer values. Setting the name like this
matches the args in the API traces.
This fixes overlapping work on multiple streams appearing as a call
stack.
@jrmadsen
Copy link
Collaborator

jrmadsen commented Nov 10, 2022

The environment variable and option parsing parts I don't yet fully understand so therefore Draft, and I would appreciate some hints for that.

Create a declaration for getting the option in source/lib/omnitrace/library/config.hpp, add the option right after here (or closer to other similar options in terms of categories):

https://github.com/AMDResearch/omnitrace/blob/654beef6ab730e87d837fec9b2ca33d36b81e781/source/lib/omnitrace/library/config.cpp#L607

The macro is <type>, <name>, <description>, <default-value>, <categories...>.

Then implement getting the value in config.cpp, something like:

bool
get_perfetto_roctracer_per_stream()
{
    static auto _v = get_config()->find("OMNITRACE_<name>");
    return static_cast<tim::tsettings<bool>&>(*_v->second).get();
}

@jrmadsen
Copy link
Collaborator

jrmadsen commented Nov 10, 2022

Build error: https://my.cdash.org/viewBuildError.php?buildid=2242917

[omnitrace/omnitrace/source/lib/omnitrace/library/roctracer.cpp:519](https://github.com/AMDResearch/omnitrace/blob/master/omnitrace/omnitrace/source/lib/omnitrace/library/roctracer.cpp#L519):2: error: extra ‘;’ [-Werror=pedantic]
  519 | };
      |  ^
[omnitrace/omnitrace/source/lib/omnitrace/library/roctracer.cpp:527](https://github.com/AMDResearch/omnitrace/blob/master/omnitrace/omnitrace/source/lib/omnitrace/library/roctracer.cpp#L527):2: error: extra ‘;’ [-Werror=pedantic]
  527 | };
      |  ^

Just noting this to point you to the dashboard since this is nicer (IMO) than the logs from GitHub actions

@Maetveis
Copy link
Contributor Author

For tests I don't know how to tackle this, as I see perfetto traces can be verified by the python script, but it isn't aware of tracks currently.

@Maetveis Maetveis marked this pull request as ready for review November 11, 2022 10:07
@jrmadsen
Copy link
Collaborator

@Maetveis I am going to open a PR onto your branch with these changes. In #206, I am making perfetto annotations optional and I want to get this merged before that to avoid you having to deal with the (likely) conflicts.

@Maetveis
Copy link
Contributor Author

Maetveis commented Nov 14, 2022

I am making perfetto annotations optional and I want to get this merged before that to avoid you having to deal with the (likely) conflicts.

Thanks, altough I might only be able to continue working on this PR next week, so maybe you shouldn't hold out on it.

@jrmadsen
Copy link
Collaborator

Superceded by #209

@jrmadsen jrmadsen closed this Nov 16, 2022
@Maetveis Maetveis deleted the hip_stream_tracks branch November 18, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants