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
Vulkan: Simplify MultiFenceHolder and managing them #4845
Conversation
Removal of unnecessary hashset and dictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks.
Tested about 20 titles : Looked for any performance changes compared to master or any visual oddities that can be reliably spotted in limited playtime for each of the following titles. Key observations :
|
MultiFenceHolder is currently absolutely reliant on all users submitting to the same command buffer pool, which means they must all come from the same thread. This is because waitiers register the command buffer index, which is only valid for entries of the same pool. The locking and dictionary structures greatly slow down the process of binding and ref counting for all types of buffer, while also being unnecessary when these rules are followed.
This PR simplifies MultiFenceHolder in a few ways:
ReservedCommandBuffer.Waitables
is now a List for less overhead. Enforcing uniqueness is done byMultiFenceHolder.AddFence
returning true only when the command buffer index is not yet registered on the waitable. Similarly, the call to Contains has been replaced with asking the waitable if it has the current fence, rather than checking the set on the command buffer.MultiFenceHolder
now maintains an array of fences rather than a dictionary. Each entry in the array represents a command buffer in the pool. If the value in the array at that command buffer index is null, there is no registered fence yet. This removes all cost for trying to add and trying to remove fences, which was pretty considerable.Auto<>
andFenceHolder
to properly handle cases where we get a resource but it might already be disposed. These are a bit slower as they use a compare exchange to increment the value only if it's not 0, but they are at least correct if a bunch of threads are contending with each other.There was an existing case where the rules about keeping
Get(cbs)
uses to the same pool are broken, which is the copy source when flushing data from a device local buffer. The destination is fine, because it is only used by the command buffer pool owned by the background thread. This will probably cause issues right now, but to avoid creating new ones I decided it's worth fixing.I've changed this case to manage keeping the source buffer alive outside the copy method. Since it waits for the copy to complete anyways, it can just increment and decrement the reference count around the use manually.
Improves performance in Vulkan bottlenecked games such as BOTW. (at least on my system)
Testing various games would be helpful to ensure nothing ends up broken.