Remove usage of `static mut` #581

lilizoey · 2024-01-25T16:55:30Z

Replaces the usage of static mut with single and multi-threaded version of a binding storage. The main difference being whether they use OnceLock or OnceCell.

From what i can tell, the only real performance difference between the two lie in initialization, with the multi-threaded one being slower. Once the values have been initialized they seem basically identical in most cases. The multi-threaded one often has around like 1-3% more lines of assembly when a ffi call is made. But i'm not sure if that actually creates a performance impact, i haven't been able to confidently measure it at least.

I made the various tables and such not directly implement Sync/Send, instead the components that make up the table do. This will hopefully avoid a situation where the tables are changed for some reason, in a way that makes them non-sync or non-send, but we dont notice because we just manually implement Sync and Send for the table.

GodotRust · 2024-01-25T17:01:14Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-581

…version of bindings and interface manager

Bromeon

Thanks a lot! 🙂

There is quite a bit of duplication across single/multi threaded bindings which I think could be extracted: namely the common "data" (all the method tables, library, config etc). Wouldn't it be enough to just have the thread access and thread-ID validation different, but store the data in a single struct?

We should avoid nested locks/cells, see also comments.

godot-ffi/src/binding/single_threaded.rs

Bromeon · 2024-01-26T10:24:12Z

godot-ffi/src/binding/single_threaded.rs

+    pub(super) interface: GDExtensionInterface,
+    pub(super) library: GDExtensionClassLibraryPtr,
+    pub(super) global_method_table: BuiltinLifecycleTable,
+    pub(super) class_server_method_table: OnceCell<ClassServersMethodTable>,
+    pub(super) class_scene_method_table: OnceCell<ClassSceneMethodTable>,
+    pub(super) class_editor_method_table: OnceCell<ClassEditorMethodTable>,
+    pub(super) builtin_method_table: OnceCell<BuiltinMethodTable>,
+    pub(super) utility_function_table: UtilityFunctionTable,
+    pub(super) runtime_metadata: GdextRuntimeMetadata,
+    pub(super) config: GdextConfig,


Those may as well be pub(crate), usually less hassle when refactoring.

godot-ffi/src/binding/single_threaded.rs

godot-ffi/src/binding/multi_threaded.rs

godot-ffi/src/toolbox.rs

godot-ffi/src/lib.rs

Bromeon · 2024-01-26T10:41:51Z

godot-ffi/src/lib.rs

+    // Ensure both crates are checked regardless of cfg, for the sake of development convenience.
+    #[cfg_attr(not(feature = "experimental-threads"), allow(dead_code))]
+    mod multi_threaded;
+    #[cfg_attr(feature = "experimental-threads", allow(dead_code))]
+    mod single_threaded;


Not a huge deal, but the problem is that this allocates additional statics even in the unused configuration.

Or do you know if those are optimized out?

They should be optimized out if they're not accessed (which they aren't), i can try to check if that is the case though.

From what i can tell, it seems like there are no references at all to the module that is not active in the assembly, barring debug info if that is enabled. (which it isn't for release builds by default). So yes it seems that they are actually optimized out.

Bromeon · 2024-01-31T18:10:23Z

Thanks for the answers! I think using OnceCell is fine if we access it through .get().unwrap_unchecked().
What is still open is this part:

There is quite a bit of duplication across single/multi threaded bindings which I think could be extracted: namely the common "data" (all the method tables, library, config etc). Wouldn't it be enough to just have the thread access and thread-ID validation different, but store the data in a single struct?

Using OnceCell in both single- and multi-threaded implementations would facilitate this. See also comment.

Bromeon · 2024-02-01T23:58:14Z

Just for my understanding: what does UnsafeOnceCell as proposed here concretely improve over static mut? Preventing handing-out of &mut references?

Because that alone can also be achieved by encapsulating in a private module. Do you think we can reuse the cell in many places?

lilizoey · 2024-02-03T15:11:29Z

Just for my understanding: what does UnsafeOnceCell as proposed here concretely improve over static mut? Preventing handing-out of &mut references?

Because that alone can also be achieved by encapsulating in a private module. Do you think we can reuse the cell in many places?

Arguably more idiomatic, which means that many of rust's features work better together with such a solution.
We can statically ensure the relevant structs implement Sync and Send. With static mut we just bypass that entire thing, making it easy to accidentally add something thread-unsafe.
I can easily add in some debug-only safety checks, like ensuring things are/aren't initialized when appropriate.
Easier to reason about safety invariants which must be upheld when initializing things like this.
Harder to mess up in the future, as static mut doesn't really provide any guarantees beyond requiring unsafe.

I'm not sure if we can reuse them much, but maybe. Currently i added an explicit caveat that we should discuss more and possibly upgrade it to more of a first-class feature if we want to do that. I think this is the main place where we need this kind of unsafe initialization.

Bromeon

Thanks a lot! Also, really nice work on the safety documentations all over the place 👍

Bromeon · 2024-02-03T15:59:04Z

godot-ffi/src/binding/multi_threaded.rs

+    /// Initialize the binding storage, this must be called before any other public functions.
+    ///
+    /// # Safety
+    ///
+    /// - Must only be called once.
+    /// - Must not be called concurrently with [`get_binding_unchecked`](BindingStorage::get_binding_unchecked).
+    pub unsafe fn initialize(binding: GodotBinding) -> Result<(), ()> {
+        let storage = Self::storage();
+
+        // SAFETY: `initialize` is only called once, and is not called concurrently with `get_binding_unchecked`, which is the
+        // only place where other methods are called on the binding.
+        unsafe { storage.binding.set(binding) }
+
+        Ok(())
+    }


This only ever returns Ok -- I wonder also if any violates should be detected internally (via debug_assert!) and thus panic directly?

There is not much point in returning Result to the caller, as there is no way to react to this properly.

i guess since the performance of the initialization function isn't too important, then i could just check that the storage isn't initialized before initializing it. it shouldn't ever fail but we also only call the function once during the entire lifetime of the library.

Bromeon · 2024-02-03T16:02:22Z

godot-ffi/src/binding/mod.rs

+// This struct is generic over `Config`, this is only to make it possible to use two different implementations of `GdextConfig`.
+// As we cannot safely use the same `GdextConfig` in both single and multi-threaded code without requiring extra unsafe in `godot-core`.
+// This is because we'd need to implement `Sync` for `GdextConfig`, since it'd be used in the multi-threaded version. But then you could
+// share references to it between threads, which is not safe to do.
+pub(crate) struct GodotBindingRaw<Config> {


It looks to me like the generic parameter could be replaced with a a #[cfg] on the config field. You have the #[cfg] dispatching on thread support just a few lines above, so they might as well be here.

This would also remove the need for yet another indirection, namely type GodotBinding = GodotBindingRaw<GdextConfig> (which btw looks the same in all places, although it uses types from different modules). Thus simplifying code a bit, with 2 less names one needs to be aware of.

If i do that then i have to #[cfg] out the multi_threaded module entirely when experimental-threads is disabled, as otherwise we get an error in there since GodotBinding doesn't implement Sync. Which i can do but then we do lose the development convenience of having both modules available at the same time during development.

Ah, I see.

Do you think having both #[cfg]'s available at the same time will still be that useful in the future? We don't generally do this in other places (or for other features), and we still have CI as a backup 🤔

Just asking because it seems now that this convenience doesn't come entirely for free...

it could be, in that it'd make a change that makes GodotBinding not sync (even when GdextConfig is sync) become an error before hitting CI. but it's probably not a very common scenario to do that. so i'll just cfg them for now.

Bromeon · 2024-02-03T16:07:49Z

godot-ffi/src/binding/mod.rs

+// SAFETY: It is safe to have access to the library pointer from any thread, as we ensure any access is thread-safe through other means.
+// Even without "experimental-threads", there is no way without `unsafe` to cause UB by accessing the library from different threads.
+unsafe impl Sync for ClassLibraryPtr {}
+// SAFETY: See `Sync` impl safety doc.
+unsafe impl Send for ClassLibraryPtr {}


Maybe this could mention that "accessing" here refers to read/write the data on a pure Rust level.

Calling functions in Godot is not automatically thread-safe, but that's beyond the responsibility of Send/Sync here and needs to be enforced by higher-level APIs.

(I guess that's what you meant with "other means"; I guess it doesn't hurt to be explicit though).

godot-ffi/src/binding/mod.rs

godot-ffi/src/binding/single_threaded.rs

godot-ffi/src/toolbox.rs

Make binding use `UnsafeOnceCell` Remove most code duplication

Bromeon · 2024-02-04T13:47:45Z

Thanks a lot! 🚀

…t-safety" This reverts commit b4a91a6, reversing changes made to 6243fc6.

lilizoey added the c: ffi Low-level components and interaction with GDExtension API label Jan 25, 2024

lilizoey force-pushed the fix/static-mut-safety branch from fd5f593 to a76dd19 Compare January 25, 2024 16:58

Remove usage of static mut, replace with single and multi-threaded …

157f8e0

…version of bindings and interface manager

lilizoey force-pushed the fix/static-mut-safety branch from a76dd19 to 157f8e0 Compare January 25, 2024 17:08

Bromeon added the quality-of-life No new functionality, but improves ergonomics/internals label Jan 25, 2024

Bromeon reviewed Jan 26, 2024

View reviewed changes

vortexofdoom mentioned this pull request Feb 2, 2024

Fix question mark lint. #592

Merged

lilizoey force-pushed the fix/static-mut-safety branch 4 times, most recently from 75962ed to 4877b0b Compare February 3, 2024 15:47

Bromeon reviewed Feb 3, 2024

View reviewed changes

lilizoey force-pushed the fix/static-mut-safety branch 3 times, most recently from 94736ca to 9cddf56 Compare February 4, 2024 13:27

Add UnsafeOnceCell

fdf5ae0

Make binding use `UnsafeOnceCell` Remove most code duplication

lilizoey force-pushed the fix/static-mut-safety branch from 9cddf56 to fdf5ae0 Compare February 4, 2024 13:31

Bromeon added this pull request to the merge queue Feb 4, 2024

Merged via the queue into godot-rust:master with commit b4a91a6 Feb 4, 2024
16 checks passed

This was referenced Feb 5, 2024

Breakage after gdext multi-thread sanitizing. StatisMike/gd-props#15

Closed

gdext-based custom resource format tools needs experimental-threads feature #597

Open

Bromeon mentioned this pull request Feb 9, 2024

Review safety in low-level code #5

Closed

8 tasks

StatisMike mentioned this pull request Feb 18, 2024

Nested Exported Resource requires experimental-threads feature #610

Open

TCROC added a commit to Lange-Studios/gdext that referenced this pull request Feb 25, 2024

Revert "Merge pull request godot-rust#581 from lilizoey/fix/static-mu…

e53ad82

…t-safety" This reverts commit b4a91a6, reversing changes made to 6243fc6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of `static mut` #581

Remove usage of `static mut` #581

lilizoey commented Jan 25, 2024

GodotRust commented Jan 25, 2024

Bromeon left a comment

Bromeon Jan 26, 2024

Bromeon Jan 26, 2024

lilizoey Jan 31, 2024

lilizoey Feb 3, 2024

Bromeon commented Jan 31, 2024

Bromeon commented Feb 1, 2024 •

edited

Loading

lilizoey commented Feb 3, 2024

Bromeon left a comment

Bromeon Feb 3, 2024

lilizoey Feb 3, 2024

Bromeon Feb 3, 2024

lilizoey Feb 3, 2024

Bromeon Feb 3, 2024

lilizoey Feb 4, 2024 •

edited

Loading

Bromeon Feb 3, 2024

Bromeon commented Feb 4, 2024

Remove usage of static mut #581

Remove usage of static mut #581

Conversation

lilizoey commented Jan 25, 2024

GodotRust commented Jan 25, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Jan 31, 2024

Bromeon commented Feb 1, 2024 • edited Loading

lilizoey commented Feb 3, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Feb 4, 2024

Remove usage of `static mut` #581

Remove usage of `static mut` #581

Bromeon commented Feb 1, 2024 •

edited

Loading

lilizoey Feb 4, 2024 •

edited

Loading