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

Emit filesystem_changed only once per frame #98584

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

Nazarwadim
Copy link
Contributor

@Nazarwadim Nazarwadim commented Oct 27, 2024

This PR makes it so that only one deferred signal is emitted instead of dozens for each updated file. This was done in order to avoid unnecessary FileSystemDock::update_all, which was writing millions of items to the EditorResourcePreview

Array udata;
udata.push_back(tree_update_id);
udata.push_back(file_item);
EditorResourcePreview::get_singleton()->queue_resource_preview(file_metadata, this, "_tree_thumbnail_done", udata);

and creating huge memory usage.

Tested on MRP from #97610.
Before that, it used 9GB of RAM.
Now it use 1GB.
Also, the import time improved 2-3 times.

Closes #97610
Edit: Does not help #95979

@Nazarwadim Nazarwadim requested a review from a team as a code owner October 27, 2024 17:00
@Mickeon
Copy link
Contributor

Mickeon commented Oct 27, 2024

Doing this may introduce some unexpected regressions for plugins, as this signal is exposed and has been usable outside of internal code. Although this sounds desirable, it needs to be documented at least.

Edit: I'm told this signal is already deferred.

@Saul2022
Copy link

Pretty interesting, can’t test rn as i am way too busy, but it could work atleast for ram as my system just has 8gb of ram.

editor/editor_file_system.h Outdated Show resolved Hide resolved
editor/editor_file_system.h Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine. Usually we have a dedicated method for this pattern, e.g. queue_redraw() works like that (i.e. it does a deferred call once per frame). Maybe it's worth applying it for other uses of the filesystem_changed signal? But for now this will suffice.

@Maran23
Copy link
Contributor

Maran23 commented Oct 28, 2024

This reminds me on a proposal I saw one time. It proposed to have a way to 'batch' signal emits so that everything that connects to that signal is executed just once per frame.
Might be worth evaluating as we have this pattern multiple times in the Engine (e.g. LineEdit). And it would be useful for games too.

@Mickeon
Copy link
Contributor

Mickeon commented Oct 28, 2024

We may be talking about the same thing. A long time ago there was a proposal (or PR?) for a signal connection flag to defer a single call, regardless of how many times the signal was emitted (behavior similar to queue_redraw). Back in day it was not thought to be useful, but I am very much willing to disagree on that. Just a vague memory.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 28, 2024

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Wow great find and the improvements on import time is amazing.

@clayjohn clayjohn merged commit a997001 into godotengine:master Oct 30, 2024
20 checks passed
@clayjohn
Copy link
Member

Fantastic work! Thank you!

@Nazarwadim Nazarwadim deleted the emit_only_one_signal branch October 31, 2024 08:10
Ryan-000 pushed a commit to Ryan-000/godot that referenced this pull request Nov 29, 2024
…gnal

Emit `filesystem_changed` only once per frame
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terrible memory leak when importing a large git cloned repository.
8 participants