Skip to content

Add opt-in Instance downcasting support via tracking #211

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

Closed
wants to merge 1 commit into from
Closed

Add opt-in Instance downcasting support via tracking #211

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 23, 2019

...also this thing which I've been dogfooding (albeit not very extensively yet) for a little while:

With customizable userdata merged, downcasting to NativeClass instances can now be supported with an opt-in userdata wrapper adaptor that performs pointer tracking, as described in the future possibilities section of the original Instance PR.

Tracking is performed only on construction, destruction, and downcasts, and only for types that opt-in. There is no additional runtime cost for calls from Godot.

Downcast attempts on types that are not tracked (or can otherwise be checked) are statically prevented.

Explanation

The Tracked<UD<T>> type is a user-data wrapper adapter, which wraps around wrapper implementations, tracking the pointers they produce in a global HashMap:

  • On into_user_data, the pointer is inserted into a set corresponding to the TypeId of the target type.
  • On consume_user_data_unchecked, the pointer is removed.
  • On try_clone_from_user_data, the map is queried with the given pointer. If the pointer is present in the set for the type, it's assumed to be valid. Otherwise, None is returned.

Like other locking types, it makes use of parking_lot::RwLock which is very efficient in uncontended situations. The map itself is a lazy_static, so it's not created at all if Tracked is unused.

Once const_fn is stablized, the lazy_static can be replaced with a RwLock<Option<_>> and initialized with a const new, removing the extra overhead.

Drawbacks

I don't believe there is much except introducing more APIs in the crate, since it's purely opt-in. I think it's useful enough to warrant an addition, especially in cases where the code is expected to interact fairly often with GDScript.

Also "user-data wrapper adapter" sounds like a tongue twister...

Rationale and alternatives

External interfaces

The use case for this overlaps a bit with #200, but with some key differences:

  • For types defined in Rust, this allows much faster calls for a relatively cheap initial check, since it doesn't need to go through Variant.
  • Meanwhile, external interfaces works with types defined in other languages.

I believe both features can co-exist serving different needs.

TypeId tags

Instead of tracking pointers in a global map, it's also possible to store the TypeId beside the data, removing the need to perform locks and hash map lookups. This, however, poses its own problem when other GDNative libraries are in play.

As considered in the original Instance PR, the current implementation of godot_nativescript_get_userdata in Godot only checks that the script is any NativeScript: that is, not necessarily from this library or even language. Thus, it's well possible to read invalid data from a foreign pointer, or even cause segfaults in some cases (e.g. Rust ZSTs).

For projects where there will be only one GDNative library present, a tagged representation can be useful. Regardless, I consider it less fit for inclusion in the core library, because of the level of attention required in its use.

With customizable userdata merged, downcasting to NativeClass instances can
now be supported with an opt-in userdata wrapper adaptor that performs pointer
tracking, as described in the future possibilities section of the original
Instance PR.

Tracking is performed only on construction, destruction, and downcasts, and
only for types that opt-in. There is no additional runtime cost for calls
from Godot.

Downcast attempts on types that are not tracked (or can otherwise be checked)
are statically prevented.
@karroffel
Copy link
Member

I want to mention that with #207 merged we now have access to the nativescript-1.1 API. That API includes functions for associating a type tag with an instance without having to retrieve and dereference the user data pointer.

The C++ bindings already make use of this feature for providing cheap safe casts (right now all casts are going through string comparisons in the Rust bindings).

I did not yet review the code, but it seems like the type-tag API in nativescript-1.1 does have some overlap with this PR.

@ghost
Copy link
Author

ghost commented Sep 24, 2019

It does seem like with nativescript-1.1 one can set and get library-scoped type tags, so downcasts can be cheaply provided for all types. It's indeed a better approach than this one, which was written before 1.1 was available.

I remember reading the type tag API's implementation earlier, but somehow missed it when preparing the PR.

Closing. Will replace with a downcast implementation using nativescript-1.1 type tags.

@ghost ghost closed this Sep 24, 2019
@ghost ghost deleted the feature/instance-downcast branch September 26, 2019 06:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant