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

Performance optimizations #11

Closed
4 of 6 tasks
Bromeon opened this issue Oct 29, 2022 · 6 comments
Closed
4 of 6 tasks

Performance optimizations #11

Bromeon opened this issue Oct 29, 2022 · 6 comments
Labels
feature Adds functionality to the library

Comments

@Bromeon
Copy link
Member

Bromeon commented Oct 29, 2022

There are some low-hanging fruits for making the library faster, this issue tracks such approaches.

Codegen

  • Check rustfmt performance -- parallelize or allow skipping
  • Cache intermediate results in Context, e.g. to_rust_type

Runtime

  • Cache function pointers used by engine + builtin classes in a global method table
  • Provide maximum type information to GDScript, so it can use ptrcalls whenever possible
  • Benchmark if #[inline] has a true impact (otherwise don't clutter the whole codebase)
  • Varargs (e.g. Object::call) could use impl AsVararg + passing as tuple, to allocate argument array on the stack
@Bromeon Bromeon added the feature Adds functionality to the library label Oct 29, 2022
Hapenia-Lans pushed a commit to Hapenia-Lans/gdextension that referenced this issue May 26, 2023
# This is the 1st commit message:

Parse gdextension_interface.h declarations using regex

# This is the commit message #2:

AsUninit trait to convert FFI pointers to their uninitialized versions

# This is the commit message godot-rust#3:

GodotFfi::from_sys_init() now uses uninitialized pointer types

# This is the commit message godot-rust#4:

Introduce GDExtensionUninitialized*Ptr, without changing semantics

# This is the commit message godot-rust#5:

Adjust init code to new get_proc_address mechanism

# This is the commit message godot-rust#6:

Make `trace` feature available in godot-ffi, fix interface access before initialization

# This is the commit message godot-rust#7:

Compatibility layer between Godot 4.0 and 4.1 (different GDExtension APIs)

# This is the commit message godot-rust#8:

Add GdextBuild to access build/runtime metadata

# This is the commit message godot-rust#9:

Detect 4.0 <-> 4.1 mismatches in both directions + missing `compatibility_minimum = 4.1`

# This is the commit message godot-rust#10:

Detect legacy/modern version of C header (also without `custom-godot` feature)

# This is the commit message godot-rust#11:

CI: add jobs that use patched 4.0.x versions

# This is the commit message godot-rust#12:

Remove several memory leaks by constructing into uninitialized pointers

# This is the commit message godot-rust#13:

CI: memcheck jobs for both 4.0.3 and nightly

# This is the commit message godot-rust#14:

Remove ToVariant, FromVariant, and VariantMetadata impls for pointers

This commit splits SignatureTuple into two separate traits:
PtrcallSignatureTuple and VarcallSignatureTuple. The latter is a child
of the former. PtrcallSignatureTuple is used for ptrcall and only
demands GodotFuncMarshall of its arguments. VarcallSignatureTuple is
used for varcall and additionally demands ToVariant, FromVariant, and
VariantMetadata of its arguments, so pointers cannot benefit from the
optimizations provided by varcall over ptrcall.

# This is the commit message godot-rust#15:

Adds FromVariant and ToVariant proc macros

# This is the commit message godot-rust#16:

godot-core: builtin: reimplement Plane functions/methods

# This is the commit message godot-rust#17:

impl GodotFfi for Option<T> when T is pointer sized and nullable godot-rust#240

Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>>
to satisfy all the requirements for ffi and godot_api.

# This is the commit message godot-rust#18:

Fix UB in virtual method calls that take objects
Fix incorrect incrementing of refcount when calling in to godot
Fix refcount not being incremented when we receive a refcounted object in virtual methods

# This is the commit message godot-rust#19:

fix UB caused by preload weirdness

# This is the commit message godot-rust#20:

Implements swizzle and converts from/to tuples
@lilizoey
Copy link
Member

lilizoey commented Jun 17, 2023

caching StringNames in function calls can have a significant improvement. running these functions 1 million times (in a std::hint::black_box(..) call):

fn generate_string_name() {
    let foo = StringName::from("foo");
}

fn cached_string_name() {
    static CACHED: OnceLock<StringName> = OnceLock::new();
    let foo = CACHED.get_or_init(|| StringName::from("foo"));
}

takes on average 0.15 seconds for the first one and 0.0003 seconds for the second one in release mode.

we do make calls like StringName::from("name") a lot in our bindings like for method calls and such.

this does require implementing Send and Sync for StringName, but i think that should be safe. StringName is immutable and i believe thread-safe.

@lilizoey
Copy link
Member

lilizoey commented Jun 17, 2023

Manually writing an implementation of StringName::length where i cache: __method_name only, __call_fn only, and both, then running each function 1 million times with black_box in release mode, i get:

Uncached __method_name __call_fn both
Total Average Time 0.1979221145 0.04312633755 0.170186928 0.01706689335
Percent 100.00% 21.79% 85.99% 8.62%
Percent Diff 0.00% -78.21% -14.01% -91.38%

@WinstonHartnett
Copy link
Contributor

Caching the method bind, builtin method, and utility function pointers with a static OnceLock produces the following speedups:

Function Kind Uncached Cached Percent
Node::get_child_count Method Bind 308ns 8ns 2.60%
Node::is_inside_tree Method Bind 212ns 5ns 2.36%
Node::is_node_ready Method Bind 209ns 5ns 2.39%
minf Utility 94ns 8ns 8.51%
floorf Utility 86ns 5ns 5.81%
StringName::length Builtin 86ns 7ns 8.14%

@Bromeon
Copy link
Member Author

Bromeon commented Dec 20, 2023

The cached function pointers have been implemented, and with them, StringNames also no longer need to be instantiated on each call.

For anyone interested in deeper insights, I wrote about it here:
https://godot-rust.github.io/dev/ffi-optimizations-benchmarking

@Bromeon
Copy link
Member Author

Bromeon commented Aug 2, 2024

I think we can close this, several things have been optimized and new ones probably deserve their own issue.

@Bromeon Bromeon closed this as completed Aug 2, 2024
@Ughuuu
Copy link
Contributor

Ughuuu commented Aug 3, 2024

Damn, impressive. I am ready to give these changes a try. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

4 participants