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

Add a fence pool for frame tracking fences #5921

Merged
merged 4 commits into from
Jul 12, 2023
Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 11, 2023

Ignoring the fact that these tracking fences are doing nothing in D3D (they are set immediately https://github.com/ppy/veldrid/blob/6a43198af1b069e56ac53770249a5424c1859500/src/Veldrid/D3D11/D3D11GraphicsDevice.cs#L229-L232):

This is a regression so let's fix it locally for the time being.

Time spent in ManualResetEvent construction:

Parallels Desktop 2023-07-11 at 05 01 20

Before this PR:

Parallels Desktop 2023-07-11 at 05 07 32

After this PR:

Parallels Desktop 2023-07-11 at 05 05 52

@@ -215,6 +220,9 @@ protected override void Initialise(IGraphicsSurface graphicsSurface)
BufferUpdateCommands = Factory.CreateCommandList();

pipeline.Outputs = Device.SwapchainFramebuffer.OutputDescription;

for (int i = 0; i < 16; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this 16 a magic number? What happens in the (unlikely) even that the pool runs dry, crash on Dequeue()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. On metal I've never seen usages increase above 1. If we exceed 3 then we're probably going to see texture corruption and stuff elsewhere though? I can use 3 instead of 16 if it feels more robust.

Copy link
Member Author

@peppy peppy Jul 11, 2023

Choose a reason for hiding this comment

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

Or we can change it to add to the pool locally, should also be fine.

diff --git a/osu.Framework/Graphics/Veldrid/VeldridRenderer.cs b/osu.Framework/Graphics/Veldrid/VeldridRenderer.cs
index 341a48ea6..7e0b11a9a 100644
--- a/osu.Framework/Graphics/Veldrid/VeldridRenderer.cs
+++ b/osu.Framework/Graphics/Veldrid/VeldridRenderer.cs
@@ -220,9 +220,6 @@ protected override void Initialise(IGraphicsSurface graphicsSurface)
             BufferUpdateCommands = Factory.CreateCommandList();
 
             pipeline.Outputs = Device.SwapchainFramebuffer.OutputDescription;
-
-            for (int i = 0; i < 16; i++)
-                perFrameFencePool.Enqueue(Factory.CreateFence(false));
         }
 
         private Vector2 currentSize;
@@ -296,7 +293,8 @@ protected internal override void FinishFrame()
 
             // This is returned via the end-of-lifetime tracking in `pendingFrameFences`.
             // See `updateLastCompletedFrameIndex`.
-            Fence fence = perFrameFencePool.Dequeue();
+            if (!perFrameFencePool.TryDequeue(out Fence? fence))
+                fence = Factory.CreateFence(false);
 
             Commands.End();
             Device.SubmitCommands(Commands, fence);

@peppy peppy added the next-release Pull requests which are almost there label Jul 11, 2023
@peppy
Copy link
Member Author

peppy commented Jul 12, 2023

I think we still want this in, at least for now. Can potentially be reverted if we decide we don't want end-of-frame fencing on all platforms.

@peppy peppy requested a review from smoogipoo July 12, 2023 11:06
@smoogipoo smoogipoo merged commit 4e2be5c into ppy:master Jul 12, 2023
@peppy peppy deleted the pool-fences branch September 14, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release Pull requests which are almost there size/S type:performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants