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

Refactoring of the D3D12 back-end #600

Merged
merged 8 commits into from
Sep 24, 2023
Merged

Conversation

slomp
Copy link
Contributor

@slomp slomp commented Aug 22, 2023

Here's an overview of the changes:

  • removed header bloat (<wrl/client.h>)
  • reworked context id assigning
  • reworked clock calibration code
  • reworked error reporting
  • reworked TracyD3D12 macros
  • ensure timestamps are collected when context is destroyed

@wolfpld
Copy link
Owner

wolfpld commented Aug 22, 2023

@adepke can you take a look at this?

@slomp slomp marked this pull request as draft August 22, 2023 17:41
@slomp
Copy link
Contributor Author

slomp commented Aug 22, 2023

(converting to a draft for now, since I realize I need to do some cleanup)

@slomp
Copy link
Contributor Author

slomp commented Aug 22, 2023

@nosferalatu Please have a look at as well!

@slomp slomp marked this pull request as ready for review August 24, 2023 18:19
@slomp
Copy link
Contributor Author

slomp commented Aug 24, 2023

@adepke and @nosferalatu : Ok, I think it's ready for review.

@slomp slomp changed the title refactoring to eliminate redundancy in the D3D12 back-end Refactoring of the D3D12 back-end Aug 24, 2023
@adepke
Copy link
Contributor

adepke commented Sep 3, 2023

Seems pretty solid, the inclusion of tabs to space conversion makes the diff unreadable so I'm just going off a skim. @slomp what's the rationale behind dropping D3D12NewFrame() and using the sporadic Collect() instead? A goal here is feature parity with the vulkan backend, which used the new frame concept when I originally designed this (may have changed)

@slomp
Copy link
Contributor Author

slomp commented Sep 5, 2023

the inclusion of tabs to space conversion makes the diff unreadable

I was thinking the same... I'm going to rework the branch and separate the tab-spaces from the rest.

what's the rationale behind dropping D3D12NewFrame() and using the sporadic Collect() instead?

Well, Collect() needs to be called periodically anyway (some time after each NewFrame() call). The rationale is that in many applications, there may be no such thing as a "frame" (compute-only workloads, or headless rendering). By letting the timestamp pool be circular, there's no need for NewFrame() to "reset" the query slots.

A goal here is feature parity with the vulkan backend, which used the new frame concept

Yup, I'll be refactoring the Vulkan back-end too, but that one requires more substantial refactoring overall (it's not thread-safe which prevents it from being used to record command buffers in parallel). I'm almost finished with a Metal back-end, and will be revisiting the Vulkan back-end after that.

@slomp
Copy link
Contributor Author

slomp commented Sep 5, 2023

@adepke Adding to my reply above: it looks like the Vulkan back-end no longer has a NewFrame() interface.

@adepke
Copy link
Contributor

adepke commented Sep 6, 2023

@slomp Seems reasonable, I like it. Let me know when there's a branch without spaces and I'll take a look when I have some time

@slomp
Copy link
Contributor Author

slomp commented Sep 11, 2023

@adepke I decided to split the PR into two parts: general refactoring (this one) and NewFrame() elimination (coming up).

It should be much easier to follow this PR now that the tabs-to-spaces changes have been merged.
If you have some time, it might be a good idea to check the changes commit-by-commit; I tried to segregate changes based on context.

@adepke
Copy link
Contributor

adepke commented Sep 23, 2023

@slomp just confirming you've tested this on your end successfully? Also the only thing I'm a little concerned about is the busy wait in the dtor but I'm guessing that's fine. If so lgtm

@slomp
Copy link
Contributor Author

slomp commented Sep 24, 2023

@adepke Yup, that's just a short-lived "catch-up" when the context is destroyed (prior to that, it was possible to lose profiling events upon context shutdown). The only thing that could cause it to go on for a longer time is if the user is still posting zone events while the context is being destroyed, but that would be a bug on the user.

@wolfpld wolfpld merged commit 855fd29 into wolfpld:master Sep 24, 2023
5 checks passed
@slomp slomp deleted the slomp/d3d12-refactor branch October 17, 2023 18:19
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