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

Use Hashset for dependency list when moving #83941

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Use Hashset for dependency list when moving #83941

merged 1 commit into from
Oct 26, 2023

Conversation

Jordyfel
Copy link
Contributor

@Jordyfel Jordyfel commented Oct 25, 2023

Change the save_scene_list() function in EditorNode to take a HashSet for a faster has() call on potentially large lists. Sometimes only one scene needs to be saved, so added a new function to do that without having to create a data structure.

The _find_file_owners() function is executed every time on move or rename, scans the entire filesystem, and pushes paths of files which depend on moved or renamed files into a list.

It seems to me that the list of depending files is a good candidate for a HashSet, since it's a (potentially large) String list which is only iterated over and is likely to get unneeded duplicates.

Also change the other parameter to HashSet, since I changed it to a Vector in #81657 which is a mistake.

@Jordyfel Jordyfel requested review from a team as code owners October 25, 2023 13:41
@AThousandShips
Copy link
Member

Now HashSet (which was introduced into the codebase relatively recently, probably explaining why it was a HashMap before) seems like the perfect data structure for this, since this String list is only iterated over and wants to ignore duplicates.

Doubtful that this was missed, as the person who introduced the HashSet type did the remapping from Map to HashMap

@Jordyfel
Copy link
Contributor Author

Updated description to the current state of the PR

editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Oct 25, 2023

Not sure why is this labelled as bug. Does it fix anything? Looks like performance enhancement.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 25, 2023

Seemed like a bug from the original description, and was due to a mistake in previous update by OP, so it fixes reduced performance caused by a previous PR

@KoBeWi
Copy link
Member

KoBeWi commented Oct 25, 2023

The reduced performance only takes effect when renaming or moving files and pretty sure it's only noticeable in very extreme cases.

@AThousandShips
Copy link
Member

I'm not sure what to call an introduced performance drop though, feel free to assign a label you feel more appropriate :)

@Calinou
Copy link
Member

Calinou commented Oct 25, 2023

Could you benchmark this and provide before-after results for comparison? (Ideally, upload a reproduction project as well if size permits it.)

@Jordyfel
Copy link
Contributor Author

Some limited testing in a copy of my largest (not very large still) project showed no statistically meaningful difference.
If necessary to justify the change, I will try to make a more comprehensive test case or try on a large public project when time allows.

@akien-mga akien-mga merged commit 80e5484 into godotengine:master Oct 26, 2023
@akien-mga
Copy link
Member

Thanks!

@Jordyfel Jordyfel deleted the hashset-not-vector branch October 26, 2023 18:44
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.

5 participants