Skip to content

Conversation

@Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Mar 25, 2025

This PR simplifies some functions in variant_call.cpp. In particular:

  • #define METHOD_CLASS and others are replaced with templated register_method and others.
  • vc_get_argument_type and vc_get_argument_count are replaced with vc_get_argument_types.

The goal of this PR is to reduce complexity in the file by:

  • Turning more #define wrapped code into 'real' code, making it easier to edit and understand.
  • Removing a layer of abstraction between calling a register macro and registering the function.

My ulterior motive is to eventually propose exposing this API to GDExtension. But this PR should be defendable without this motive :)

Benchmarks

I do not expect either an improvement or regression, but it should be tested to be sure about it.
I benchmarked editor launch as a quick and dirty entry test for this:

 ❯ hyperfine -m25 -iw1 "bin/godot.macos.editor.arm64.master --path /Users/lukas/dev/godot/empty-game --editor --quit" "bin/godot.macos.editor.arm64 --path /Users/lukas/dev/godot/empty-game --editor --quit"
Benchmark 1: bin/godot.macos.editor.arm64.master --path /Users/lukas/dev/godot/empty-game --editor --quit
  Time (mean ± σ):      4.223 s ±  0.110 s    [User: 2.770 s, System: 0.363 s]
  Range (min … max):    3.990 s …  4.359 s    25 runs

Benchmark 2: bin/godot.macos.editor.arm64 --path /Users/lukas/dev/godot/empty-game --editor --quit
  Time (mean ± σ):      4.192 s ±  0.106 s    [User: 2.778 s, System: 0.363 s]
  Range (min … max):    3.978 s …  4.342 s    25 runs

Summary
  bin/godot.macos.editor.arm64 --path /Users/lukas/dev/godot/empty-game --editor --quit ran
    1.01 ± 0.04 times faster than bin/godot.macos.editor.arm64.master --path /Users/lukas/dev/godot/empty-game --editor --quit

I'm taking this as no difference (change within expected fluctuations).

I also benchmarked the unit tests:

 ❯ hyperfine -m5 -iw1 "bin/godot.macos.editor.arm64.master --test" "bin/godot.macos.editor.arm64 --test"
Benchmark 1: bin/godot.macos.editor.arm64.master --test
  Time (mean ± σ):     13.234 s ±  0.113 s    [User: 6.353 s, System: 0.843 s]
  Range (min … max):   13.051 s … 13.326 s    5 runs

Benchmark 2: bin/godot.macos.editor.arm64 --test
  Time (mean ± σ):     13.086 s ±  0.056 s    [User: 6.333 s, System: 0.817 s]
  Range (min … max):   13.029 s … 13.164 s    5 runs

Summary
  bin/godot.macos.editor.arm64 --test ran
    1.01 ± 0.01 times faster than bin/godot.macos.editor.arm64.master --test

The difference is similar as before, so probably within expected fluctuations.

A better test would be benchmarking actual calls (call, validated_call and ptrcall). I might do this, but I'd also appreciate if someone else stepped up!

Size Decrease

As a side effect, the size of the final binary shrinks by about 0.8mb (0.5%).
I checked with bloaty about this, and the decrease is indeed caused by the templates taking up less space than the structs we had before:

 ❯ diff <(bloaty bin/godot.macos.editor.arm64.master -d symbols -- -n 500) <(bloaty bin/godot.macos.editor.arm64 -d symbols -- -n 500)
3,4c3,4
<   41.3%  68.4Mi  41.0%  68.8Mi    [116355 Others]
<   10.2%  16.9Mi  10.1%  16.9Mi    [__LINKEDIT]
---
>   41.0%  67.7Mi  40.7%  68.0Mi    [112454 Others]
>   10.2%  16.8Mi  10.1%  16.8Mi    [__LINKEDIT]
[...]
<    0.2%   395Ki   0.2%   395Ki    _register_variant_builtin_methods_array()
58d57
<    0.2%   311Ki   0.2%   311Ki    _register_variant_builtin_methods_math()
74d72
<    0.1%   251Ki   0.1%   251Ki    _register_variant_builtin_methods_string()
89d86
<    0.1%   201Ki   0.1%   201Ki    _register_variant_builtin_methods_misc()
104a102
>    0.1%   178Ki   0.1%   178Ki    _register_variant_builtin_methods_array()
127a126
>    0.1%   159Ki   0.1%   159Ki    register_convert_method<>()::{lambda()#1}::__invoke()
145a145
>    0.1%   142Ki   0.1%   142Ki    _register_variant_builtin_methods_string()
148a149
>    0.1%   134Ki   0.1%   134Ki    _register_variant_builtin_methods_math()
150a152
[...]
<  100.0%   165Mi 100.0%   167Mi    TOTAL
---
>  100.0%   164Mi 100.0%   166Mi    TOTAL

@Ivorforce Ivorforce added this to the 4.x milestone Mar 25, 2025
@Ivorforce Ivorforce requested a review from a team as a code owner March 25, 2025 23:20
@Ivorforce Ivorforce force-pushed the variant-call-templates branch 5 times, most recently from d654b65 to 3f2e055 Compare March 26, 2025 00:21
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. Code looks good to me.

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Jun 19, 2025
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