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

Enforce barrier when sampling timestamps #2236

Merged
merged 1 commit into from
May 14, 2024

Conversation

PENGUINLIONG
Copy link
Contributor

@PENGUINLIONG PENGUINLIONG commented May 9, 2024

Some context: #2235.

@CLAassistant
Copy link

CLAassistant commented May 9, 2024

CLA assistant check
All committers have signed the CLA.

@SRSaunders
Copy link
Contributor

Will there be a performance hit by doing this? If so, then perhaps it needs to be an optional parm.

@PENGUINLIONG
Copy link
Contributor Author

@SRSaunders Is there anything I could refer to?

Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

Interesting catch! Good work. And thanks for submitting this fix!

@billhollings
Copy link
Contributor

billhollings commented May 14, 2024

Will there be a performance hit by doing this? If so, then perhaps it needs to be an optional parm.

This is a good point, and probably the reason we originally set barrier to NO.

Apple's doc does indicate that it may affect GPU performance, as one would expect from any barrier.

However, the Vulkan spec explicitly states:

When vkCmdWriteTimestamp is submitted to a queue, it defines an execution dependency on commands that were submitted before it, and writes a timestamp to a query pool.

which would indicate that a barrier is required and expected here, and that timestamp scope accuracy is more important than absolute performance.

@billhollings billhollings merged commit 6b9e559 into KhronosGroup:main May 14, 2024
6 checks passed
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.

4 participants