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

Optimize get_path() in EditorFileSystemDirectory #93611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 25, 2024

EditorFileSystemDirectory's get_path() works by traversing its parent directories and concatenating strings. It's horribly inefficient, but seems to be called a lot during scans.

I optimized it using PackedStringArray (the path is built in reverse order, so couldn't really use StringBuilder) and the optimized version seems to be ~150% faster. This of course depends on nesting level, I tested with 6 levels of nesting.

Alternative is adding a cached path String to each EditorFileSystemDirectory, which would be even faster, but use more memory.

@Mickeon
Copy link
Contributor

Mickeon commented Jun 25, 2024

This optimization is great. I just worry the new code may have become a bit difficult to follow.

@akien-mga
Copy link
Member

The code in get_file_path is almost the same and should likely be optimized similarly.

Then I'd suggest adding a comment that explains why it's done this way, so it doesn't get refactored in the more intuitive but slower form.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Makes much more sense now. I like the code between the two methods being united like this.

The performance benefit may be even more with #92550

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Jun 27, 2024
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.

None yet

3 participants