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

Drop the global lock around most of the Vulkan API commands #2192

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

Qining
Copy link
Contributor

@Qining Qining commented Sep 11, 2018

Unlock before calling the underlying API command calls, and lock again after
return from the underlying API command calls.

This improves the tracing performance, especially when the application
builds Vulkan pipelines in multiple threads.

Copy link
Contributor

@ben-clayton ben-clayton left a comment

Choose a reason for hiding this comment

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

I strongly suspect this will break many things.
There are assumptions throughout the .api files and likely gapii that assumes there are no external changes to the state or encoders for the duration of a call.

We have considered adding an annotation to very specific commands in the API files which may unlock at the fence, but we need to spend a lot of time thoroughly checking these commands are unlock-safe, as well as the all the existing gapii code.

@AWoloszyn
Copy link
Contributor

We have looked at this on the Vulkan side at least, and I think? everything is probably in good shape, but I agree just making the change across every call may be dangerous.

Qining, what about changing the annotation from @Blocking to @threadsafe (or something).
Since we know pipeline creation is one of the big ones, that can certainly help.

Unlock before calling some of the underlying API command calls, and lock
again after return from the underlying API command calls.

This improves the tracing performance, especially when the application
builds Vulkan pipelines in multiple threads.
@Qining Qining force-pushed the unblock-all-api-cmds branch from c3309dd to c974f65 Compare September 13, 2018 02:21
@Qining Qining changed the title Drop the global lock around calling the underlying API commands Drop the global lock around calling some of the underlying API commands Sep 13, 2018
@Qining Qining changed the title Drop the global lock around calling some of the underlying API commands Drop the global lock around some of the underlying API commands Sep 13, 2018
@Qining
Copy link
Contributor Author

Qining commented Sep 13, 2018

Annotation: "threadsafe" has been added to some of the Vulkan commands. Now we use "threadsafe" instead of "blocking" to control the unlock and relock behavior.

@ben-clayton
Copy link
Contributor

shrug if you folks are sure that the rest of gapii is safe with this, then let's give it a try. I'm sure random crashes will alert us to any issues :)

@Qining Qining changed the title Drop the global lock around some of the underlying API commands Drop the global lock around most of the Vulkan API commands Sep 13, 2018
@Qining Qining force-pushed the unblock-all-api-cmds branch from d352fc4 to cb075bf Compare September 13, 2018 14:57
@Qining Qining merged commit 7532c5c into google:master Sep 13, 2018
@Qining Qining deleted the unblock-all-api-cmds branch October 23, 2018 17:32
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.

3 participants