Skip to content

Make using fences on DX12 optional via cargo feature #225

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

OneXDeveloper
Copy link

I noticed pretty significant FPS drops in my games when injecting even the most basic hello world examples. I was able to track it down to this fence usage. As an example, in the game I am working on I get ~460FPS sitting in the town, but as soon as I inject the hello world example from the README it drops down to around ~240FPS. With this change I stick within ~10FPS of the game without injection. I didn't notice any weird synchronization issues on my game but I realize that this could potentially cause some if we remove it outright for all which is why I made it an optional feature for DX12 only and enabled it by default to retain existing functionality.

P.S, your discord links in the readme/troubleshooting guide on all your repos has expired. :)

Copy link
Owner

@veeenu veeenu left a comment

Choose a reason for hiding this comment

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

I agree that the current synchronization model in hudhook is not great, as it basically blocks the CPU while the GPU is working, but I also think this PR is not the right approach to support a large number of games and creates extra maintenance burden due to having to test and maintain the version with the feature flag and the version without.

I'm not opposed to introducing changes though as ~4ms drop (if my math is correct) is a long time... Let's discuss alternative approaches.

We are injecting rendering commands just before a call to Present, which means that we have to have completed rendering before that moment. I'm not sure what would happen if e.g. we get to the next Present and start rendering to the render target while the previous frame is still in flight in a case where there is no double buffering. My guess is that there would be a hard crash due to a locked resource being touched by something else.

On the other hand, we do know that hudhook's code runs only in between calls to Present so arguably the previous frame has surely been rendered to its render target, but... what happens if hudhook is not done rendering by the time the buffer is swapped?

Another point against introducing this change is that the official imgui dx12 backend does the same thing which makes sense as the entire context is thread local and you can't rely on the host doing their synchronization in a compatible way. I suspect that when you are trying to provide a generic abstracted solution, like we are, there is some cost to be paid.

All in all, if we can find a more elegant solution that saves time without requiring a feature flag I'd be up for it, but I'd like to have soundness for the concurrency part -- could be e.g. waiting on the previous fence at the start of the frame and increasing it at the end, or maybe using multiple fences so that you only wait on the one for the current frame in the case of multiple buffering.

(I want to try running this against a few dx12 games anyway just to see what happens... though I no longer have Windows so that'll really be running against Vulkan)

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