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

Move StringName != operator to the header file to make it inlineable. #99815

Merged

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Nov 29, 2024

Small inconsistency I came across in my string endeavours. Shouldn't make a big difference, but it will make a small one, probably even with LTO enabled.

By allowing the function to be inlined, the compiler is allowed to reason about the code instead of having to make a Blackbox call, and surrounding code unlocks a whole slew of possible optimizations.

Notice how operator== is already in the header file right above the new != entry, ready to be inlined.

@Ivorforce Ivorforce requested a review from a team as a code owner November 29, 2024 00:08
@Chaosus Chaosus added this to the 4.4 milestone Nov 29, 2024
@arkology
Copy link
Contributor

arkology commented Nov 29, 2024

Idk about operators, but at least functions with -O2 optimization level will be inlined by compiler if it is needed even without inline attribute. At least as far as I remember. So this one will make difference only for Godot Web builds which use -Os.
But it would probably be better if this is confirmed or refuted by someone more knowledgeable in C++ (as I'm mostly C programmer for embedded systems).

@Ivorforce
Copy link
Member Author

Ivorforce commented Nov 29, 2024

Idk about operators, but at least functions with -O2 optimization level will be inlined by compiler if it is needed even without inline attribute. At least as far as I remember. So this one will make difference only for Godot Web builds which use -Os. But it would probably be better if this is confirmed or refuted by someone more knowledgeable in C++ (as I'm mostly C programmer for embedded embedded systems).

This is not the case if the function is defined inside a module. In this case the function is considered to be blackbox, and a call is unavoidable. It may still be inlined by the LTO later, although not all release builds of Godot have LTO enabled. Plus, when it's LTO's turn, a lot of optimizations are already over.
The only option that makes this change redundant is single compilation unit (scu), which i don't know if it's enabled for official release builds (Edit: looks like it's only enabled in parts of the linux pipeline). But in any case, it can't hurt to move == next to != to have consistency.

@AThousandShips AThousandShips modified the milestones: 4.4, 4.x Nov 29, 2024
@Ivorforce Ivorforce force-pushed the string-name-not-equal-inline branch from fd7375d to 862006f Compare November 29, 2024 11:04
@adamscott adamscott requested review from Calinou and KoBeWi November 30, 2024 20:11
@KoBeWi
Copy link
Member

KoBeWi commented Nov 30, 2024

The change makes sense, I don't see a reason why != would be defined somewhere else than ==. I'd ask @reduz if there is a technical reason for that.

@Ivorforce Ivorforce force-pushed the string-name-not-equal-inline branch from 862006f to a47d29c Compare December 1, 2024 18:50
@akien-mga akien-mga changed the title Move StringName != to the header file to make it inlineable. Move StringName != operator to the header file to make it inlineable. Dec 2, 2024
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 2, 2024
@akien-mga akien-mga merged commit d7515dd into godotengine:master Dec 2, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Ivorforce Ivorforce deleted the string-name-not-equal-inline branch December 2, 2024 15:46
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.

7 participants