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

Add 1ns buffer to Roctracer Events #992

Closed
wants to merge 1 commit into from

Conversation

sraikund16
Copy link
Contributor

Summary: As reported in ROCm/roctracer#105, there is an issue where event starts and ends can "tie". This can cause a visual issue in the traces. Lets add a tiny buffer so the events are separate. At the single nanosecond level, the timings are inaccurate anyways so it doesn't really hurt to add this buffer in the meanwhile. Remove/wrap in ifdef once it is issue is resolved

Differential Revision: D63296093

Summary: As reported in ROCm/roctracer#105, there is an issue where event starts and ends can "tie". This can cause a visual issue in the traces. Lets add a tiny buffer so the events are separate. At the single nanosecond level, the timings are inaccurate anyways so it doesn't really hurt to add this buffer in the meanwhile. Remove/wrap in ifdef once it is issue is resolved

Differential Revision: D63296093
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63296093

return activity_.begin + 1;
}
int64_t duration() const override {
return activity_.end - (activity_.begin + 1);
Copy link
Member

Choose a reason for hiding this comment

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

should it be -2 , so that it finishes right before? Or is it okay if end time == parent end time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we are only shaving off a nanosecond off the beginning since this is on the gpu track only so there won't be any parent/child minus the annotations which isn't a big deal since we set those ourselves.

Copy link
Member

@aaronenyeshi aaronenyeshi left a comment

Choose a reason for hiding this comment

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

LGTM!

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 0aacc09.

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