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 TTR, TTRN, RTR, RTRN to accept StringName instead of String for the translation key. #101547

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Jan 14, 2025

This avoids unnecessary allocation, and should make calls to translate slightly faster.

This PR has the side effect of crashing the program if TTR (or any of the other functions) are called before the StringName table is initialized. Unit tests should fail if this were the case anywhere in the codebase, especially because TTR calls will fail before the translation server is started anyway.
(I needed to fix one instance of this problem recently in #101592).

Reasoning

Let's look at the current implementation of TTR as an example.

String TTR(const String &p_text, const String &p_context) {
	if (TranslationServer::get_singleton()) {
		return TranslationServer::get_singleton()->tool_translate(p_text, p_context);
	}

	return p_text;
}
// Usually called like 
TTR("Translation Key")

On call, the char * is converted to String by allocating memory and copying into it.
Then, it could go 2 ways. If TranslationServer exists, tool_translate is called. The function accepts a StringName, and thus has to convert String to StringName. If TranslationServer does not exist, the string is returned unchanged.
To summarize, a String object is always allocated, and is usually converted to StringName.

Let's change the function to take StringName:

String TTR(const StringName &p_text, const String &p_context) {
	if (TranslationServer::get_singleton()) {
		return TranslationServer::get_singleton()->tool_translate(p_text, p_context);
	}

	return p_text;
}

On call, the char * is converted to StringName by searching the StringName table without allocation. This is (nearly) guaranteed because whatever is passed into TTR corresponds to a StringName that was already created for the translation server.
Then, it could go 2 ways. If TranslationServer exists, tool_translate is called. p_text is passed to it without change. If TranslationServer does not exist, the StringName has to be converted to String.
To summarize, a StringName object is always used, and is rarely converted to String.

Verdict

The new implementation is almost always cheaper than the old implementation, except when TranslationServer does not exist, and the input is already a String (instead of char *). In this case, the StringName database is unnecessarily searched (though no unnecessary data is allocated, and thus not much time is lost). I don't think this case is important, because searches for TTR\([^"] and similar do not yield many results.

Caveats

Running the engine seems to be failing with this change:

ERROR: Condition "!configured" is true.
   at: StringName (core/string/string_name.cpp:266)
libc++abi: terminating due to uncaught exception of type std::__1::system_error: recursive_mutex lock failed: Invalid argument
zsh: abort      bin/godot.macos.editor.arm64 --test

This is probably because RTR is called before the StringName table could be set up. This could be solved if the StringName table is set up earlier.
It could also be solved by changing the parameter type of the RTR-like functions to StrRange, but then StrName arguments would have to be hashed unnecessarily (which is the case on current master anyway, but alas).

@Chaosus Chaosus added this to the 4.x milestone Jan 15, 2025
@Ivorforce Ivorforce force-pushed the runtime-translation-stringname-key branch from d780959 to 9c2f889 Compare January 15, 2025 17:09
@Ivorforce Ivorforce marked this pull request as ready for review January 15, 2025 17:11
@Ivorforce Ivorforce requested review from a team as code owners January 15, 2025 17:11
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Jan 15, 2025

I fixed the macOS unit tests locally by rebasing on top of #101592.

This brings the PR to a reviewable state!

Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

You can resolve my threads, seems all fine to me.

@Ivorforce Ivorforce force-pushed the runtime-translation-stringname-key branch from 9c2f889 to eb1a9a0 Compare January 17, 2025 23:22
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.

3 participants