-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Optimize scanning routines in the project manager #86271
Optimize scanning routines in the project manager #86271
Conversation
@@ -2183,15 +2230,6 @@ void ProjectManager::shortcut_input(const Ref<InputEvent> &p_ev) { | |||
} | |||
} | |||
|
|||
void ProjectManager::_load_recent_projects() { | |||
_project_list->set_search_term(search_box->get_text().strip_edges()); |
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.
I'm not really sure why we used to do this before updating the project list. Changing the filter already sets the search term, and during the first scan we don't have any search term anyway, it's all empty.
So this bit I simply removed even before removing the method itself. I tested and didn't see issues. The search term was still applied after scanning and importing dropped folders.
The only problem with scanning big folders is that Project Manager does not show any progres whatsoever (#5953). User might pick something very big for scan and ends up with unresponsive window. |
const char *ProjectList::SIGNAL_LIST_CHANGED = "list_changed"; | ||
const char *ProjectList::SIGNAL_SELECTION_CHANGED = "selection_changed"; | ||
const char *ProjectList::SIGNAL_PROJECT_ASK_OPEN = "project_ask_open"; |
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.
Unrelated, but these should be StringNames (probably not static then).
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.
Yeah, I tried making them static StringNames, but it didn't quite work. I didn't want to spend time adding the usual framework to make them accessible outside of the class with this PR, so I just followed the existing pattern. But I will rework this in a future PR. We probably can have a "project manager string names" file, like we do with other contexts.
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.
Left some style comments, but the overall logic is ok.
6bcd43a
to
3d4b33d
Compare
Thanks for reviews! |
Cherry-picked for 4.2.2. |
I previously worked on improving built-in benchmarking capabilities in #85885 in hopes to get a better picture of the startup times in the project manager. Unfortunately, most of the time is eaten by DisplayServer and other main setup, and not by the project manager itself.
I couldn't find much that could be immediately improved, but I did notice that we weren't using some of the method related to scanning/project list updates efficiently. In fact the method that is specifically commented with a performance notice could be called significantly more often than it was needed. So this PR looks into such cases and tries to improve them a bit.
Two things I've identified:
Both of these cases are addressed now, and in both of them we now only do one expensive refresh. The refresh itself should likely be optimized as well, but that's a bit out of scope of what I'm trying to achieve here right now.
I also moved the relevant methods from
ProjectManager
toProjectList
because, like many other methods currently placed inProjectManager
, they were nothing but shells for calling the project list APIs. So moving them there makes for better organization (something I hope to tackle more generally soon). Some things were adjusted to allow for this change, but the overall logic should be exactly the same as before.The only change in logic that I decided to do relates to the confirmation dialog when you drop multiple folders for scanning. I removed the checks and the dialog completely, and my reasoning here is that the check itself wasn't really saving the user any time. It was added in #5993, long long time ago, and from the discussion the intent seems vague to me. The way it worked was as follows:
project.godot
), don't show a confirmation dialog.So not showing the dialog when we drop one folder and it's a folder with a Godot project makes sense, we want this to work quickly. However, we don't do it quickly in practice, because the entire folder structure is scanned anyway. Which can take time for a bigger existing project. If the idea would be to immediately import the project instead of scanning, then this is not how it works in practice.
On the other hand, showing a confirmation dialog ahead of scanning makes sense to warn the user about it being slow. But that is not really dependent on how many folders we have selected and dropped. One folder with an expansive internal structure can take way longer than 10 folders which are shallow. We also don't show the dialog when you select the folder for scanning by other means.
So given all that I just removed the confirmation. We just scan all the dropped folders immediately. And scanning is a bit faster now thanks to the rest of this PR, so it's less of an issue. Here's a comparison, btw, of me dragging and dropping 2 folders full of Godot projects before and after this PR (the confirmation dialog times not included, of course):
This is on a fast drive and on a fast laptop that is plugged in. And that's about 80 imported projects with all their subfolders checked. I think it's representative enough to say that scans such as this can be 25% faster now. Although not sure how often people do that in practice.
I am marking this as cherry-pickable. I specifically tried to tackle this before doing more extensive changes, and I think this may be desirable for current stable versions.