- Notifications
You must be signed in to change notification settings - Fork2.5k
Remove busy loops in SDL_GPUFence#14547
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
SDL_GPUFence is implemented as just a busy loop on an atomic int, a SDL_GPUCondition gives the cpu some rest
removed bad casts
TheSpydog commentedDec 2, 2025
Will defer to@thatcosmonaut here. All of this logic is shared between Vulkan, D3D12, and Metal, so whatever strategy we use here should apply to all drivers. |
thatcosmonaut commentedDec 2, 2025
I think Metal is kind of unique here, because if I recall correctly it doesn't provide an explicit wait mechanism. Vulkan and D3D12 provide waits in the spec. |
alexgu754 commentedDec 16, 2025
is nobody going to merge? seems pretty serious. I think when the user calls wait for the gpu work to finish, they don't expect it to busy loop on apple devices, destroying the battery life |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/gpu/metal/SDL_gpu_metal.m Outdated
| // Notify the fence when the command buffer has completed | ||
| [metalCommandBuffer->handleaddCompletedHandler:^(id<MTLCommandBuffer> buffer) { | ||
| SDL_AtomicIncRef(&metalCommandBuffer->fence->complete); | ||
| SDL_LockMutex(metalCommandBuffer->fence->mutex); |
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.
Again, create a local variable for the fence instead of indirecting it several times in a row.
slouken commentedDec 16, 2025
METAL_INTERNAL_FenceOpen() seems like an odd choice for a function name. What is it opening? Would METAL_INTERNAL_FenceComplete() be more appropriate? |
slouken commentedDec 16, 2025
This code feels very much like something thrown together very quickly. If you clean it up and@thatcosmonaut approves, we can potentially merge this for 3.4, but it's more likely to need more testing and go in for 3.6. |
semantic fixesCo-authored-by: Sam Lantinga <slouken@libsdl.org>
more semantic changes
alexgu754 commentedDec 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
ok, good I just want people to be aware that these issues exist in the metal implementation and it won't pass app store review for example. The max frames in flight, waiting on a command buffer, waiting for a swapchain texture, etc. is done through a busy loop and I hope the pull requests or other fixes will be merged in the foreseeable future so I can ship a game |
src/gpu/metal/SDL_gpu_metal.m Outdated
| while(!((MetalFence *)fences[i])->complete) { | ||
| SDL_WaitCondition(((MetalFence *)fences[i])->condition, ((MetalFence *)fences[i])->mutex); |
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.
| while(!((MetalFence *)fences[i])->complete) { | |
| SDL_WaitCondition(((MetalFence *)fences[i])->condition,((MetalFence *)fences[i])->mutex); | |
| while(!fence->complete) { | |
| SDL_WaitCondition(fence->condition,fence->mutex); |
src/gpu/metal/SDL_gpu_metal.m Outdated
| while(!((MetalFence *)fences[i])->complete) { | ||
| SDL_WaitCondition(((MetalFence *)fences[i])->condition, ((MetalFence *)fences[i])->mutex); | ||
| } | ||
| SDL_UnlockMutex(((MetalFence *)fences[i])->mutex); |
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.
| SDL_UnlockMutex(((MetalFence *)fences[i])->mutex); | |
| SDL_UnlockMutex(fence->mutex); |
alexgu754 commentedDec 16, 2025
yeah, I'm going to be honest. syntax highlighting doesn't work in .mm files on clion so I don't see anything |
syntactic changes
fix missing bracket
Uh oh!
There was an error while loading.Please reload this page.
src/gpu/metal/SDL_gpu_metal.m Outdated
| if (windowData->inFlightFences[windowData->frameCounter] !=NULL) { | ||
| if (!METAL_WaitForFences( | ||
| driverData, | ||
| (SDL_GPURenderer *)driverData, |
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.
Is this cast necessary?
src/gpu/metal/SDL_gpu_metal.m Outdated
| while (!SDL_GetAtomicInt(&renderer->submittedCommandBuffers[i]->fence->complete)) { | ||
| // Spin! | ||
| } | ||
| METAL_WaitForFences((SDL_GPURenderer *)renderer,true, (SDL_GPUFence **)&renderer->submittedCommandBuffers[i]->fence,1); |
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.
Are these casts necessary? You can just pass driverData for the first one.
TheSpydog commentedDec 16, 2025
Apologies for the lack of review, I've been very busy and haven't had an opportunity to test this.
At least one app has already passed App Store review on iOS and tvOS with the current implementation. Busy-waiting may not be optimal but it's not prohibitive for shipping games, so I agree with Sam that we should hold off on this until 3.6. |
alexgu754 commentedDec 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
ok, well that's because max frames in flight also currently does nothing so that issue silences this issue for now, it waits when you call next drawable on CAMetalLayer* after maxing out drawables, so it never ends up waiting on the gpu fence even when frames in flight=1, and the developer didn't use the wait for swapchain api or wait for command buffers in his app |
Co-authored-by: Sam Lantinga <slouken@libsdl.org>
removed redundant casts
SDL_GPUFence is implemented as just a busy loop on an atomic int, a SDL_GPUCondition gives the cpu some rest