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

Memory Leaks in SubViewport When Window is Minimized #90030

Closed
MizunagiKB opened this issue Mar 30, 2024 · 3 comments · Fixed by #100257
Closed

Memory Leaks in SubViewport When Window is Minimized #90030

MizunagiKB opened this issue Mar 30, 2024 · 3 comments · Fixed by #100257

Comments

@MizunagiKB
Copy link

Tested versions

4.2.stable

System information

Godot v4.2.stable - macOS 14.3.1 - Vulkan (Forward+) - integrated Apple M2 Max - Apple M2 Max (12 Threads)

Issue description

I've observed that memory leaks occur when explicit initialization (setting to transparent_bg) is performed every time when using SubViewport.
This issue could lead to significant performance issues and needs to be resolved.

Additional Information:
This issue is not specifically related to whether the SubViewport is connected to a node.
It seems to occur regardless of the node connection status of the SubViewport.

This can be reproduced with the following code:

extends Node2D

var v: SubViewport

func _ready():
    v = SubViewport.new()
    
func _process(_delta):
    v.transparent_bg = true

mleak_vp_transparent_bg

This issue was discovered during the investigation of the following issue:
#89651
#90017

Steps to reproduce

To reproduce this issue, execute the mleak_vp_transparent_bg.tscn file included in the attached project.

While the application is running, open the Debugger/Monitors in the Editor and pay attention to the value of Memory/Static.

This value will gradually increase while the application being debugged is minimized.
When the application is restored from minimization, the increased memory is released.

Minimal reproduction project (MRP)

mleak_vp_transparent_bg.zip

@Sauermann
Copy link
Contributor

I have tried the MRP on Windows 10 with v4.3.dev.custom_build [29b3d9e] and there seems to be no memory leak.

image

@clayjohn
Copy link
Member

clayjohn commented Apr 2, 2024

#90055 will fix this MRP. But the issue should still be reproducible with the following code:

extends Node2D

var v: SubViewport

func _ready():
    v = SubViewport.new()
    
func _process(_delta):
    v.transparent_bg = !v.transparent_bg 

@MizunagiKB Can you test with the code I provided here and let me know if the leak looks the same?

@MizunagiKB
Copy link
Author

@clayjohn I have executed a build applying the specified pull request #90055.

Upon applying the pull request, the memory leak that I reported in my script no longer occurred.

fig1_ss


However, the same memory leak occurred in the provided script (the code that changes the transparent_bg value every frame).

fig2_ss

In the screenshot, the renderer is set to Mobile, but the same result was observed with Forward+.

@AThousandShips AThousandShips modified the milestones: 4.x, 4.4 Dec 12, 2024
tGautot pushed a commit to tGautot/godot that referenced this issue Feb 5, 2025
Fixes godotengine#90017
Fixes godotengine#90030
Fixes godotengine#98044

This PR makes the following changes:

# Force processing of GPU commands for frame_count frames

The variable `frames_pending_resources_for_processing` is added to track
this.

The ticket godotengine#98044 suggested to use `_flush_and_stall_for_all_frames()`
while minimized.

Technically this works and is a viable solution.

However I noticed that this issue was happening because Logic/Physics
continue to work "business as usual" while minimized(\*). Only Graphics
was being deactivated (which caused commands to accumulate until window
is restored).

To continue this behavior of "business as usual", I decided that GPU
work should also "continue as usual" by buffering commands in a double
or triple buffer scheme until all commands are done processing (if they
ever stop coming). This is specially important if the app specifically
intends to keep processing while minimized.

Calling `_flush_and_stall_for_all_frames()` would fix the leak, but it
would make  Godot's behavior different while minimized vs while the
window is presenting.

\* `OS::add_frame_delay` _does_ consider being minimized, but it just
throttles CPU usage. Some platforms such as Android completely disable
processing because the higher level code stops being called when the app
goes into background. But this seems like an implementation-detail that
diverges from the rest of the platforms (e.g. Windows, Linux & macOS
continue to process while minimized).

# Rename p_swap_buffers for p_present

**This is potentially a breaking change** (if it actually breaks
anything, I ignore. But I strongly suspect it doesn't break anything).

"Swap Buffers" is a concept carried from OpenGL, where a frame is "done"
when `glSwapBuffers()` is called, which basically means "present to the
screen".

However it _also_ means that OpenGL internally swaps its internal
buffers in a double/triple buffer scheme (in Vulkan, we do that
ourselves and is tracked by `RenderingDevice::frame`).

Modern APIs like Vulkan differentiate between "submitting GPU work" and
"presenting".

Before this PR, calling `RendererCompositorRD::end_frame(false)` would
literally do nothing. This is often undesired and the cause of the leak.
After this PR, calling `RendererCompositorRD::end_frame(false)` will now
process commands, swap our internal buffers in a double/triple buffer
scheme **but avoid presenting to the screen**.

Hence the rename of the variable from `p_swap_buffers` to `p_present`
(which slightly alters its behavior).
If we want `RendererCompositorRD::end_frame(false)` to do nothing, then
we should not call it at all.

This PR reflects such change: When we're minimized **_and_**
`has_pending_resources_for_processing()` returns false, we don't call
`RendererCompositorRD::end_frame()` at all.

But if `has_pending_resources_for_processing()` returns true, we will
call it, but with `p_present = false` because we're minimized.

There's still the issue that Godot keeps processing work (logic,
scripts, physics) while minimized, which we shouldn't do by default. But
that's work for follow up PR.
tGautot pushed a commit to tGautot/godot that referenced this issue Feb 5, 2025
Fixes godotengine#90017
Fixes godotengine#90030
Fixes godotengine#98044

This PR makes the following changes:

# Force processing of GPU commands for frame_count frames

The variable `frames_pending_resources_for_processing` is added to track
this.

The ticket godotengine#98044 suggested to use `_flush_and_stall_for_all_frames()`
while minimized.

Technically this works and is a viable solution.

However I noticed that this issue was happening because Logic/Physics
continue to work "business as usual" while minimized(\*). Only Graphics
was being deactivated (which caused commands to accumulate until window
is restored).

To continue this behavior of "business as usual", I decided that GPU
work should also "continue as usual" by buffering commands in a double
or triple buffer scheme until all commands are done processing (if they
ever stop coming). This is specially important if the app specifically
intends to keep processing while minimized.

Calling `_flush_and_stall_for_all_frames()` would fix the leak, but it
would make  Godot's behavior different while minimized vs while the
window is presenting.

\* `OS::add_frame_delay` _does_ consider being minimized, but it just
throttles CPU usage. Some platforms such as Android completely disable
processing because the higher level code stops being called when the app
goes into background. But this seems like an implementation-detail that
diverges from the rest of the platforms (e.g. Windows, Linux & macOS
continue to process while minimized).

# Rename p_swap_buffers for p_present

**This is potentially a breaking change** (if it actually breaks
anything, I ignore. But I strongly suspect it doesn't break anything).

"Swap Buffers" is a concept carried from OpenGL, where a frame is "done"
when `glSwapBuffers()` is called, which basically means "present to the
screen".

However it _also_ means that OpenGL internally swaps its internal
buffers in a double/triple buffer scheme (in Vulkan, we do that
ourselves and is tracked by `RenderingDevice::frame`).

Modern APIs like Vulkan differentiate between "submitting GPU work" and
"presenting".

Before this PR, calling `RendererCompositorRD::end_frame(false)` would
literally do nothing. This is often undesired and the cause of the leak.
After this PR, calling `RendererCompositorRD::end_frame(false)` will now
process commands, swap our internal buffers in a double/triple buffer
scheme **but avoid presenting to the screen**.

Hence the rename of the variable from `p_swap_buffers` to `p_present`
(which slightly alters its behavior).
If we want `RendererCompositorRD::end_frame(false)` to do nothing, then
we should not call it at all.

This PR reflects such change: When we're minimized **_and_**
`has_pending_resources_for_processing()` returns false, we don't call
`RendererCompositorRD::end_frame()` at all.

But if `has_pending_resources_for_processing()` returns true, we will
call it, but with `p_present = false` because we're minimized.

There's still the issue that Godot keeps processing work (logic,
scripts, physics) while minimized, which we shouldn't do by default. But
that's work for follow up PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants