Rework available query processing #3278
Conversation
And do so when adding additional query handles, in order to avoid allocating queries indefinitely if End is never called, which Halo:MCC supposedly does. Co-authored-by: Sam Edwards <[email protected]>
There was a problem hiding this comment.
This approach is a lot cleaner than mine, and thank you for the co-author tag!
I have had a very busy day today so I'll have to test this change sometime this weekend, but it does look like it does the job quite nicely.
My only code comment on this PR is related to whether m_ended really needs to be atomic after all: It may even be good to do away with it, or use it only in debug builds for an assert. But either way, I don't feel very strongly about it. Do as you like.
| @@ -34,15 +31,14 @@ namespace dxvk { | |||
| DxvkGpuQueryStatus DxvkGpuQuery::getData(DxvkQueryData& queryData) const { | |||
| queryData = DxvkQueryData(); | |||
|
|
|||
| if (!m_ended) | |||
| // Callers must ensure that no begin call is pending when | |||
There was a problem hiding this comment.
I decided to check just to make sure that this assurance holds; here's my sketch of a proof for it:
Given:
- The D3D11Query implementation uses a simple, lock-protected FSM to guarantee that it alternates between starting Begin and End operations: an out-of-order Begin is discarded, and an out-of-order End has a Begin injected before it.
- Operations are sent to the CS thread and dispatched in strict FIFO order (I am not 100% on this)
- D3D11Query also has an atomic counter that accurately tracks the number of in-flight End operations.
Therefore:
- The D3D
GetData()implementation will only attempt to call the underlying query'sgetData()if it is in the "ended" state (meaning the most recent operation started was an End, per given 1) and there are no in-flight End operations (per given 3). And, if the most recently started operation is no longer in-flight, then no other operations are in-flight (per given 2) at all. Also, the atomic-sub has release semantics, and theGetData()'s implied atomic-load defaults to seq_cst, so data is also properly synchronized.
Unless I am missing something, this means that there was no race between DxvkGpuQuery::begin()/DxvkGpuQuery::end() and DxvkGpuQuery::getData() in the first place! So making m_ended atomic may be redundant: perhaps you decide that's a good thing for robustness, but if not it's an unnecessary performance cost.
In general I prefer an all-or-nothing approach to making objects guarantee thread safety: DxvkGpuQuery should be responsible for all of its synchronization guarantees or none of them. That is, if we're going to require that callers guarantee that no begin() call is pending when calling getData(), we may as well also require that callers guarantee that no end() call is pending either. (And we are changing the getData() contract anyway by making it non-const, so it might be good to codify these requirements at the same time.)
|
Just tested and confirmed that this PR does indeed fix the problem in Halo: MCC (both tests done on the pause menu):
|
Supersedes #3273.
@CFSworks please let me know if this looks alright to you. I made the proposed changes yesterday and put you as a co-author since it's the same idea.