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

ENGINES: Optimize string handling #5323

Merged
merged 1 commit into from Sep 18, 2023
Merged

Conversation

mikrosk
Copy link
Contributor

@mikrosk mikrosk commented Sep 9, 2023

[v]snprintf() / [v]fprintf() used in String::format() gets called roughly a million times (!) when adding a game.

Reducing the number of calls into libc leads to about 10% speed improvement on the atari backend (which is counting in seconds!)

I wish I could get rid of that Common::String::format("%d", _md5Bytes); construct, too. But even in its present form (i.e. with just reduced content passed to String::format) it leads to a measurable speed-up.

[v]snprintf() / [v]fprintf() used in String::format() gets called
roughly a million times (!) when adding a game.

Reducing the number of calls into libc leads to about 10% speed
improvement on the atari backend (which is counting in seconds!)
hashname += ':';
hashname += fname;
hashname += ':';
hashname += Common::String::format("%d", _md5Bytes);
Copy link
Member

Choose a reason for hiding this comment

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

Could you try using itoa() here and see if it is any faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

itoa is a non-standard function. However out of curiosity I have tried std::to_string and my custom itoa:

  • Common::String::format: 74.290s
  • std::to_string: 73.440s
  • my own itoa: 73.070s
static Common::String itoa(uint val)
{
	Common::String s;

	do
	{
		uint a = val % 10;
		val /= 10;

		s += a + '0';

	} while (val > 0);

	if (s.size() > 1) {
		int start = 0 , end = s.size()-1;
		while (start < end) {
			auto temp = s[start];
			s.setChar(s[end], start);
			s.setChar(temp, end);
			start += 1;
			end -= 1;
		}
	}

	return s;
}

So there is some speedup but not that significant.

Copy link
Member

Choose a reason for hiding this comment

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

Currently it's not an itoa but more an utoa.
I think this could go into BaseString class.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be nice to have in the base string class. Just to my surprise, the code formatting is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I implemented BaseString::itoa() (which would handle also negative numbers), would you accept it?

@sev-
Copy link
Member

sev- commented Sep 18, 2023

Merging, thanks!

@sev- sev- merged commit d52d7b2 into scummvm:master Sep 18, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants