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 String.repeat() to optimize several String methods #72288

Merged
merged 1 commit into from
May 5, 2023

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Jan 29, 2023

kleonc and I optimized String.repeat() in #64489, so I wondered if there there is any engine code that is using iterative addition instead of this method. Nice surprise though, I found that even some user-exposed String methods could be optimized this way, namely pad_zeros(), pad_decimals(), rpad(), and lpad(). The latter two are even used in String::sprintf() - I have no idea what sprintf does, but it looks important!

The optimization should always result in performance gain, though I haven't benchmarked it extensively.

Edit: It wasn't always a performance gain. It's just that the old repeat() was really slow, but the new one is still slower than a manual loop if it has 1 or 2 iterations. For a single iteration, the performance reduction was 15%, which I wasn't happy with, so I altered String::repeat() a bit to return the string directly if p_count is 1. This doesn't affect performance for other cases in any noticeable way.

Testing new vs. old rpad() after that:

Test Performance boost/reduction
0 repeats +20%
1 repeat +8%
2 repeats -3%
3 repeats +12%
4 repeats +30%
10 repeats +150%
16 repeats +420%

RegEx used: for \(.*\) {\n((\t)*).* \+= .*;\n((\t)*)} and lots of manual sifting. A few I found with slightly different regexes too.

@MewPurPur MewPurPur requested review from a team as code owners January 29, 2023 00:49
@MewPurPur MewPurPur changed the title Use String.repeat() to optimize lpad(), rpad(), pad_zeros(), and pad_decimals() Use String.repeat() to optimize several String methods Jan 29, 2023
@akien-mga akien-mga added this to the 4.0 milestone Jan 29, 2023
@MewPurPur MewPurPur requested a review from a team as a code owner January 29, 2023 02:45
@MewPurPur MewPurPur requested review from a team as code owners January 29, 2023 03:46
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jan 29, 2023

Instances I found but I'm not sure if I should edit: doc_tools.cpp, bvh_debug.inc, gltf_document.cpp

I'll push these too, but I don't understand what some of the files are for, so I'll appreciate a review to make sure I'm not going over the top

@MewPurPur MewPurPur requested a review from a team as a code owner January 29, 2023 04:07
@MewPurPur MewPurPur force-pushed the use-string-repeat branch 2 times, most recently from 5865644 to a8c523e Compare January 29, 2023 05:43
@AThousandShips
Copy link
Member

AThousandShips commented Jan 29, 2023

Makes me wonder if we should add a String(count, char) constructor for convenience

@MewPurPur MewPurPur force-pushed the use-string-repeat branch 2 times, most recently from af36023 to 3f94fd3 Compare January 29, 2023 12:53
@lawnjelly
Copy link
Member

I'll push these too, but I don't understand what some of the files are for, so I'll appreciate a review to make sure I'm not going over the top

Generally it looks pretty easy to add and self-explanatory, so I wouldn't worry too much about over adding it (whether it will make any difference is another matter 😁 ).

Seeing what appears when profiling is usually the gold standard for optimization, but on the other hand text output tends to occur in just a few frames, rather than continually, which can make it harder to identify via profiling.

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Feb 9, 2023

Not 100% safe and not a bugfix. Makes sense to bump the milestone.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 9, 2023
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

LGTM besides the one potential change of the behavior.

core/string/translation.cpp Outdated Show resolved Hide resolved
editor/script_create_dialog.cpp Outdated Show resolved Hide resolved
core/string/ustring.cpp Outdated Show resolved Hide resolved
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Looks good to me, once @kleonc comments are addressed.

@MewPurPur MewPurPur force-pushed the use-string-repeat branch from 0015a2c to 6b84e25 Compare May 1, 2023 00:28
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

LGTM. Also makes the code nicer to read IMO :)

@clayjohn clayjohn merged commit 610877e into godotengine:master May 5, 2023
@clayjohn
Copy link
Member

clayjohn commented May 5, 2023

Great work! Thanks!

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.

10 participants