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

[Core] Optimize String::insert #92555

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips added this to the 4.x milestone May 30, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner May 30, 2024 15:44
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master c73ac74), it works as expected.

Testing project: test_pr_92555.zip

Benchmark

PC specifications
  • CPU: Intel Core i9-13900K
  • GPU: NVIDIA GeForce RTX 4090
  • RAM: 64 GB (2×32 GB DDR5-5800 C30)
  • SSD: Solidigm P44 Pro 2 TB
  • OS: Linux (Fedora 40)

Using an optimized editor build (optimize=speed lto=full).

20 million iterations of String.insert() (10 million at the beginning of a string, 10 million at the end1):

Before After
2169 ms 952 ms

With much longer strings to insert into:

Before

insert(0) took 1222376 usecs
insert(5) took 3907256 usecs

After

insert(0) took 797245 usecs
insert(5) took 802382 usecs

Footnotes

  1. These take more or less the same time to complete, I wanted to check performance of both cases just to be sure.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Aug 9, 2024
@akien-mga akien-mga merged commit 71ca5aa into godotengine:master Aug 16, 2024
18 checks passed
@AThousandShips AThousandShips deleted the insert_improve branch August 16, 2024 08:48
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

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