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

Cleaner, faster StringBuilder implementation #77152

Closed

Conversation

SlugFiller
Copy link
Contributor

Reimplements StringBuilder using a single exponentially growing character buffer, instead of 3 separate arrays of strings. According to benchmarks, this achieves around a 5-7 times boost in performance. And it does it with fewer lines of code.

@SlugFiller SlugFiller requested a review from a team as a code owner May 17, 2023 07:27
@YuriSizov YuriSizov added this to the 4.x milestone May 17, 2023
// -1 means it's a Godot String
// a natural number means C string.
Vector<int32_t> appended_strings;
char32_t *buffer;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can just use a LocalVector instead of handling memory internally? This whole class seems much like a wrapper on LocalVector. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would actually just increase the code size, and force me to double-check that every assumption holds for the specific method calls I use. And would be susceptible to future changes in LocalVector. I refer you to the story of how left-pad took down the internet. Not everything has to be a web of inter-dependencies.

At any rate the discussion is moot. It turns out there's another class called StringBuffer that does everything this naive implementation does, plus a couple of extras, achieving better performance on benchmarks. So an even smaller solution would be to just typedef StringBuilder to StringBuffer. Or remove use of the prior from the code completely.

@SlugFiller
Copy link
Contributor Author

Superseded by #77158

@SlugFiller SlugFiller closed this May 19, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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.

4 participants