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 comparisons for Object's get_argument_options #86743

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 3, 2024

You could say it's a continuation of #86729 and #86733

I noticed that some get_argument_options() assign their p_function StringName into a normal pf String before comparing the method's name. It turns out this is slightly better because it avoids the conversion every time, and sometimes it just... looks more readable.

This PR makes that consistent across the board and gives a purpose to a lonely, unused pf in AnimationPlayer's method.
One could question why get_argument_options() wants a StringName if it's better to just convert it back to String every time, but who am I to judge?

I have a feeling this and the other previous PRs can be backported to 3.x, even(?).

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

As discussed in RocketChat, I'm not sure what the better approach is anymore. Doing this, having get_argument_options require a String, or a mix of both.

@dalexeev
Copy link
Member

dalexeev commented Jan 3, 2024

I think a faster way would be to compare StringNames (like p_function == CoreStringNames::get_singleton()->call). But it is more verbose, and SNAME macro is not recommended to be used all the time. Could we solve this using code generation or constexpr evaluations in a way that doesn't negatively impact build time when adding/changing a new StringName to the codebase?

@AThousandShips
Copy link
Member

constexpr wouldn't be possible here as StringName isn't compile time due to hashing etc., so other than binding them centrally it would be equivalent to SNAME I'd say

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

Despite this being an optimization, I am not too concerned on performance. It's an editor-exclusive method called once in a while. This is more-so done for consistency and future-proofing if or when more autocompletion options are added.

@dalexeev
Copy link
Member

dalexeev commented Jan 3, 2024

Maybe an option with code generation is possible?

  1. The build system reads the sources and collects all the names from SNAME("name") or similar macro and generates one or more string_names.gen.h files that are included in .cpps, so that this alone will not cause a cascaded recompilation the entire engine.
  2. When compiling .cpp, the macro is expanded to StringNames::get_singleton()->name or something similar.
  3. Now we can always use the SNAME macro instead of CoreStringNames and similar singletons (if the name is not a variable, for this case we can keep the macro option that expands into a lambda).

The tricky part is how to split the names into multiple .gen.hs so that adding a new StringName doesn't recompile all the .cpp files that use SNAME. We can split into groups alphabetically or by area (core, scene, modules).

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

Definitely out of the scope of this PR, but do discuss it with other people more savvy about it, because it does sound neat in theory.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 3, 2024

I have attempted to do some "StringName optimizations" multiple times. E.g. the editor theme uses Strings everywhere for methods that take StringNames, so I though that it's inefficient and made reusable StringName variables. The result was that the code speed was the same, or even slower. It did not improve performance, at most it made binary size smaller. You can check my changes here: KoBeWi@124684a
So far from all my attempts I got the same results. None of them was worth it performance-wise, though they did save a few kBs of exe size. IMO claims about StringNames being more performant are much exaggerated and optimizing their usage is a dead end not worth pursuing. I think they might matter only somewhere deep in core code.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

Hearing that makes my push towards String & StringName having the same methods on the outside feel all the more justified. Still, it's a bit disheartening to hear. Or, perhaps we shouldn't feel so bad, as the compiler does such a good job optimizing... strings? Who knows.

@Mickeon Mickeon force-pushed the autocompletion-optimise-object branch from 0cc5567 to af8c9e6 Compare February 26, 2024 15:04
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 26, 2024

I will probably update this PR to surround all of these with TOOLS_ENABLED, although the PR is good as is, too.

@Mickeon Mickeon force-pushed the autocompletion-optimise-object branch from af8c9e6 to 290ce3a Compare February 26, 2024 19:30
@Mickeon Mickeon requested review from a team as code owners February 26, 2024 19:30
@Mickeon Mickeon force-pushed the autocompletion-optimise-object branch from 290ce3a to 9bafd2d Compare February 26, 2024 19:32
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 26, 2024

Done what I said I'd do...

Is this really worth the review of all of those teams above?

@Mickeon Mickeon force-pushed the autocompletion-optimise-object branch 3 times, most recently from ea18076 to 9692910 Compare February 26, 2024 19:53
core/object/object.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the autocompletion-optimise-object branch from 9692910 to cd2032a Compare February 29, 2024 17:01
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 29, 2024

For your interest see also #87197

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 29, 2024
@akien-mga akien-mga merged commit c137792 into godotengine:master Mar 1, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the autocompletion-optimise-object branch March 1, 2024 14:16
@akien-mga akien-mga changed the title Optimise comparisons for Object's get_argument_options Optimize comparisons for Object's get_argument_options Aug 14, 2024
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.

5 participants