Implement InstanceStorage optionally as sync for #18

[TitanNano]

This is a second implementation of InstanceStorage that is Sync and Send and should make it suitable for usage with rust threads.

I hope it fits with the general plans for multithreading in #18. Feedback is more than welcome.

[Bromeon]

Thanks a lot for driving this topic forward! 🙂

It looks like this PR enables Sync/Send primarily by making GdRef/GdMut generic over RefCell and RwLock storage types. It supports rust-based sharing of T inside Gd<T>, but doesn't yet look into Godot interactions (or Godot threads), which can be a problem as Ts hold a base which is managed by Godot. But maybe before even discussing implementation, more generally:

Can you describe a bit the use case you had that inspired this approach? Which situations require sharing of Gd<T> rather than just the T inside?

[TitanNano]

Can you describe a bit the use case you had that inspired this approach? Which situations require sharing of Gd<T> rather than just the T inside?

I'm currently accepting a Gd instance of a struct that is being created in GDScript. In a method call, I then create a parallel iterator that accesses &self to calculate my terrain mesh.

To make all this work, I have a Shared<T> new type that has an unsafe impl of Sync and Send. This obviously disregards the fact the Gd<T> is not actually Sync and makes the .bind() method go haywire / panic when being called in multiple threads. By making the InstanceStorage sync, I was able to rectify this issue.

To my understanding the Gd<T> is now behaving like an Arc<RwLock<T>> which is quite convenient. I haven't tried sharing a &T across threads yet though, that might work as well.

How would it be possible to access the base of T without going through T? I wasn't aware this is possible.

[Bromeon]

How would it be possible to access the base of T without going through T? I wasn't aware this is possible.

Many options:

• when storing a Gd<T> equivalent reference on GDScript side
• through the scene tree
• Gd::upcast()
• Gd::from_instance_id()
• Gd::from_variant()
• ...

That's kind of what makes the whole threading so complex: we need to take into account that the Godot engine holds references to our types.

The gdnative library addressed this via Unique typestate: the user asserts that they hold the only reference to an instance, but it's very brittle in practice because unchecked. An alternative would be something like Arc::get_mut(), which could extract a single T from a Gd<T> in case it holds the only reference. However I'm not sure if we can reliably runtime-check this -- for manually managed instances, there is no ref-count. And even for reference-counted ones, there's stuff like WeakRef, and of course Godot can also get references out of the aether via instance ID or node path. So a solution would likely boild down to user assertion + best-effort runtime checks.

[TitanNano]

The gdnative library addressed this via Unique typestate: the user asserts that they hold the only reference to an instance, but it's very brittle in practice because unchecked. An alternative would be something like Arc::get_mut(), which could extract a single T from a Gd in case it holds the only reference. However I'm not sure if we can reliably runtime-check this -- for manually managed instances, there is no ref-count. And even for reference-counted ones, there's stuff like WeakRef, and of course Godot can also get references out of the aether via instance ID or node path. So a solution would likely boild down to user assertion + best-effort runtime checks.

In my opinion, the only sensible approach here would be to uphold borrowing rules for the rust code but make no guarantees for anything happening in C++ / Godot. As you say, there is no reliable way to perform runtime checks.

Do you see any value in this patch as it is right now?

[lilizoey]

I found something we might be able to use. It seems like Godot allows us to attach an "InstanceBinding" to our objects. using object_set_instance_binding and object_get_instance_binding.

These InstanceBinding objects allow us to set callbacks which Godot will call whenever it:

• frees the instance
• references the instance

With this i feel like we should be able to do something to ensure memory safety.

It does unfortunately seem like the reference callback only runs for RefCounted objects, however the free callback runs for every object.

edit: actually the reference callback runs both when godot references and unreferences a RefCounted object, passing along a bool to indicate which happened. I'm not sure if that'll be very useful but good to know.

[Bromeon]

Do you see any value in this patch as it is right now?

Definitely, I think it's a good first step toward exploring multi-threading.

Maybe we should just start somewhere -- even if an implementation is not solving all the issues, it's probably better to work on this iteratively than plan out a huge design (which may easily miss things as well). It can however well happen that we later completely change the way how we approach threads, I hope you'd be fine with breaking changes too 🙂

Question regarding GdRef/GdMut -- instead of using generics, can you use conditional compilation based on the feature to implement two variants of it? The user doesn't benefit from the generic C parameter, as they don't simultaneously use multiple monomorphizations. That is, the two types would have the old definition:

pub struct GdRef<'a, T>

[TitanNano]

@Bromeon I removed the generic parameter C from GdRef again and switched to conditional compilation. I also added the threads feature to the job matrix for the full-CI tests so that the alternative implementation is also being tested.

[Bromeon]

Thanks for the conditional compilation changes!

Regarding CI, we need to be careful to avoid combinatoric explosion for different features (we had similar discussion for the double-precision). This would currently add 6 new CI jobs just for multi-threading, and we don't even actively test much.

So I'd recommend to do the following for now:

• Leave unit-test unchanged from master, there are no unit tests even testing this.
• For godot-itest, instead of a 3x2 matrix, just add a single extra configuration to include:
  • This should be a Linux runner, with the threads feature flag.

Since threading is in a very early stage, I wouldn't spend too much compute resources (time, not money) on them. We can always adapt this over time.

[Bromeon]

Thanks a lot for this first step towards threading!

bors r+

[bors]

Build failed:

• full-ci

[Bromeon]

CI failed, the artifact name was wrongly assumed to be linux-threads.
Corrected line in suggestion.

[Bromeon]

bors r+

[bors]

Build failed:

• full-ci

[Bromeon]

bors r+

[bors]

Build succeeded:

• full-ci