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 String.count and String.countn by avoiding repeated reallocations. #100294

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 11, 2024

This is a fairly straight-forward change.
I'm guessing the author was not aware that find and find have a p_from parameter (or they did not when the function was written).

Benchmark

I tested the following benchmark:

	String s = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";

	auto t0 = std::chrono::high_resolution_clock::now();
	for (int i = 0; i < 100000; i ++) {
		// Test
		int x = s.count("it");
	}
	auto t1 = std::chrono::high_resolution_clock::now();
	std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";

This yielded:

So, we can expect an up to 3x speedup (specifically if many instances of the string are found).

@Ivorforce Ivorforce requested a review from a team as a code owner December 11, 2024 23:32
@RandomShaper
Copy link
Member

Looks good. I'm wondering if the one-time copies in the lines above could also be replaced by some additional awareness of the start and end indices, while you're at it.

@Ivorforce
Copy link
Contributor Author

Looks good. I'm wondering if the one-time copies in the lines above could also be replaced by some additional awareness of the start and end indices, while you're at it.

I did consider that too, but it was impossible because find does not take a p_to parameter. It will be possible once #100293 is merged though, by simply clamping the view.

Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

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

I'm guessing the author was not aware that find and find have a p_from parameter (or they did not when the function was written).

That's something I definitely would do at a similar scenario now. Thanks for this optimization!

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems good to me.

@akien-mga akien-mga merged commit 321bd35 into godotengine:master Dec 12, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Ivorforce Ivorforce deleted the count-no-realloc branch December 12, 2024 13:25
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