Merge pull request #547 from godot-rust/feature/global-cell

`Global<T>` to store global variables ergonomically

Author: Bromeon

godot-core/src/builtin/meta/class_name.rs

@@ -8,23 +8,15 @@
 use godot_ffi as sys;
 use std::collections::HashMap;
 use std::ffi::CStr;
-use std::{fmt, sync};
-
+use std::fmt;
 use std::hash::{Hash, Hasher};
 
 use crate::builtin::*;
+use sys::Global;
 
-// Why is this so ugly?
-// - Mutex: needed for global access (Sync).
-// - Option: needed to initialize lazily, because HashMap::new() is not const.
-// - Box: needed for pointer stability (HashMap insertion may invalidate pointers -- with_capacity() would be an alternative,
-//   but we don't know how many classes).
-// In theory a static mut would do the job, however if we allow for manual class registration (at any time), we need to count with
-// later adjustments.
-// We may also consider OnceLock with a static per class, but that needs to be code-generated (for #[derive] and engine classes), and
-// any manually registered classes would need to replicate it later.
-static CACHED_STRING_NAMES: sync::Mutex<Option<HashMap<ClassName, Box<StringName>>>> =
-    sync::Mutex::new(None);
+// Box is needed for pointer stability (HashMap insertion may invalidate pointers -- with_capacity() would be an alternative,
+// but we don't know how many classes).
+static CACHED_STRING_NAMES: Global<HashMap<ClassName, Box<StringName>>> = Global::default();
 
 /// Name of a class registered with Godot.
 ///
@@ -80,8 +72,7 @@ impl ClassName {
 
     // Takes a closure because the mutex guard protects the reference; so the &StringName cannot leave the scope.
     fn with_string_name<R>(&self, func: impl FnOnce(&StringName) -> R) -> R {
-        let mut guard = CACHED_STRING_NAMES.lock().unwrap();
-        let map = guard.get_or_insert_with(HashMap::new);
+        let mut map = CACHED_STRING_NAMES.lock();
 
         let value = map
             .entry(*self)

godot-core/src/builtin/meta/registration/constant.rs

@@ -27,7 +27,7 @@ impl IntegerConstant {
         }
     }
 
-    fn register(&self, class_name: &ClassName, enum_name: &StringName, is_bitfield: bool) {
+    fn register(&self, class_name: ClassName, enum_name: &StringName, is_bitfield: bool) {
         unsafe {
             interface_fn!(classdb_register_extension_class_integer_constant)(
                 sys::get_library(),
@@ -56,7 +56,7 @@ pub enum ConstantKind {
 }
 
 impl ConstantKind {
-    fn register(&self, class_name: &ClassName) {
+    fn register(&self, class_name: ClassName) {
         match self {
             ConstantKind::Integer(integer) => {
                 integer.register(class_name, &StringName::default(), false)
@@ -87,6 +87,6 @@ impl ExportConstant {
     }
 
     pub fn register(&self) {
-        self.kind.register(&self.class_name)
+        self.kind.register(self.class_name)
     }
 }

godot-core/src/engine/mod.rs

@@ -7,11 +7,12 @@
 
 //! Godot engine classes and methods.
 
-// Re-exports of generated symbols
 use crate::builtin::{GString, NodePath};
 use crate::obj::dom::EngineDomain;
 use crate::obj::{Gd, GodotClass, Inherits, InstanceId};
+use std::collections::HashSet;
 
+// Re-exports of generated symbols
 pub use crate::gen::central::global;
 pub use crate::gen::classes::*;
 pub use crate::gen::utilities;
@@ -246,15 +247,13 @@ pub(crate) fn ensure_object_alive(
 
 #[cfg(debug_assertions)]
 pub(crate) fn ensure_object_inherits(
-    derived: &ClassName,
-    base: &ClassName,
+    derived: ClassName,
+    base: ClassName,
     instance_id: InstanceId,
 ) -> bool {
-    // TODO static cache.
-
     if derived == base
-        || base == &Object::class_name() // always true
-        || ClassDb::singleton().is_parent_class(derived.to_string_name(), base.to_string_name())
+        || base == Object::class_name() // for Object base, anything inherits by definition
+        || is_derived_base_cached(derived, base)
     {
         return true;
     }
@@ -265,6 +264,30 @@ pub(crate) fn ensure_object_inherits(
     )
 }
 
+/// Checks if `derived` inherits from `base`, using a cache for _successful_ queries.
+#[cfg(debug_assertions)]
+fn is_derived_base_cached(derived: ClassName, base: ClassName) -> bool {
+    use sys::Global;
+    static CACHE: Global<HashSet<(ClassName, ClassName)>> = Global::default();
+
+    let mut cache = CACHE.lock();
+    let key = (derived, base);
+    if cache.contains(&key) {
+        return true;
+    }
+
+    // Query Godot API (takes linear time in depth of inheritance tree).
+    let is_parent_class =
+        ClassDb::singleton().is_parent_class(derived.to_string_name(), base.to_string_name());
+
+    // Insert only successful queries. Those that fail are on the error path already and don't need to be fast.
+    if is_parent_class {
+        cache.insert(key);
+    }
+
+    is_parent_class
+}
+
 // Separate function, to avoid constructing string twice
 // Note that more optimizations than that likely make no sense, as loading is quite expensive
 fn load_impl<T>(path: &GString) -> Option<Gd<T>>

godot-core/src/lib.rs

@@ -51,7 +51,7 @@ pub mod private {
     pub use crate::gen::classes::class_macros;
     pub use crate::registry::{callbacks, ClassPlugin, ErasedRegisterFn, PluginComponent};
     pub use crate::storage::as_storage;
-    pub use godot_ffi::out;
+    pub use sys::out;
 
     use crate::{log, sys};
 

godot-core/src/obj/rtti.rs

@@ -44,7 +44,7 @@ impl ObjectRtti {
     #[inline]
     pub fn check_type<T: GodotClass>(&self) -> InstanceId {
         #[cfg(debug_assertions)]
-        crate::engine::ensure_object_inherits(&self.class_name, &T::class_name(), self.instance_id);
+        crate::engine::ensure_object_inherits(self.class_name, T::class_name(), self.instance_id);
 
         self.instance_id
     }

godot-core/src/registry.rs

@@ -5,31 +5,27 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-#![allow(dead_code)] // FIXME
-
+use crate::builtin::meta::ClassName;
+use crate::builtin::StringName;
 use crate::init::InitLevel;
 use crate::log;
 use crate::obj::*;
+use crate::out;
 use crate::private::as_storage;
 use crate::storage::InstanceStorage;
 use godot_ffi as sys;
 
-use sys::interface_fn;
+use sys::{interface_fn, Global, GlobalGuard, GlobalLockError};
 
-use crate::builtin::meta::ClassName;
-use crate::builtin::StringName;
-use crate::out;
 use std::any::Any;
 use std::collections::HashMap;
-use std::sync::{Mutex, MutexGuard, TryLockError};
 use std::{fmt, ptr};
 
-// For now, that variable is needed for class unregistering. It's populated during class
-// registering. There is no actual concurrency here, because Godot call register/unregister in main
-// thread - Mutex is just casual way to ensure safety in this performance non-critical path.
-// Note that we panic on concurrent access instead of blocking - that's fail fast approach. If that
-// happen, most likely something changed on Godot side and analysis required to adopt these changes.
-static LOADED_CLASSES: Mutex<Option<HashMap<InitLevel, Vec<ClassName>>>> = Mutex::new(None);
+// Needed for class unregistering. The variable is populated during class registering. There is no actual concurrency here, because Godot
+// calls register/unregister in the main thread. Mutex is just casual way to ensure safety in this non-performance-critical path.
+// Note that we panic on concurrent access instead of blocking (fail-fast approach). If that happens, most likely something changed on Godot
+// side and analysis required to adopt these changes.
+static LOADED_CLASSES: Global<HashMap<InitLevel, Vec<ClassName>>> = Global::default();
 
 // TODO(bromeon): some information coming from the proc-macro API is deferred through PluginComponent, while others is directly
 // translated to code. Consider moving more code to the PluginComponent, which allows for more dynamic registration and will
@@ -182,6 +178,7 @@ struct ClassRegistrationInfo {
     godot_params: sys::GDExtensionClassCreationInfo,
     #[cfg(since_api = "4.2")]
     godot_params: sys::GDExtensionClassCreationInfo2,
+    #[allow(dead_code)] // Currently unused; may be useful for diagnostics in the future.
     init_level: InitLevel,
     is_editor_plugin: bool,
 }
@@ -276,9 +273,7 @@ pub fn auto_register_classes(init_level: InitLevel) {
         fill_class_info(elem.component.clone(), class_info);
     });
 
-    let mut loaded_classes_guard = get_loaded_classes_with_mutex();
-    let loaded_classes_by_level = loaded_classes_guard.get_or_insert_with(HashMap::default);
-
+    let mut loaded_classes_by_level = global_loaded_classes();
     for info in map.into_values() {
         out!(
             "Register class:   {} at level `{init_level:?}`",
@@ -298,26 +293,25 @@ pub fn auto_register_classes(init_level: InitLevel) {
 }
 
 pub fn unregister_classes(init_level: InitLevel) {
-    let mut loaded_classes_guard = get_loaded_classes_with_mutex();
-    let loaded_classes_by_level = loaded_classes_guard.get_or_insert_with(HashMap::default);
+    let mut loaded_classes_by_level = global_loaded_classes();
     let loaded_classes_current_level = loaded_classes_by_level
         .remove(&init_level)
         .unwrap_or_default();
     out!("Unregistering classes of level {init_level:?}...");
     for class_name in loaded_classes_current_level.iter().rev() {
-        unregister_class_raw(class_name);
+        unregister_class_raw(*class_name);
     }
 }
 
-fn get_loaded_classes_with_mutex() -> MutexGuard<'static, Option<HashMap<InitLevel, Vec<ClassName>>>>
-{
+fn global_loaded_classes() -> GlobalGuard<'static, HashMap<InitLevel, Vec<ClassName>>> {
     match LOADED_CLASSES.try_lock() {
         Ok(it) => it,
         Err(err) => match err {
-            TryLockError::Poisoned(_err) => panic!(
-                "LOADED_CLASSES poisoned. seems like class registration or deregistration panicked."
+            GlobalLockError::Poisoned {..} => panic!(
+                "global lock for loaded classes poisoned; class registration or deregistration may have panicked"
             ),
-            TryLockError::WouldBlock => panic!("unexpected concurrent access detected to CLASSES"),
+            GlobalLockError::WouldBlock => panic!("unexpected concurrent access to global lock for loaded classes"),
+            GlobalLockError::InitFailed => unreachable!("global lock for loaded classes not initialized"),
         },
     }
 }
@@ -494,7 +488,7 @@ fn register_class_raw(mut info: ClassRegistrationInfo) {
     assert!(!info.is_editor_plugin);
 }
 
-fn unregister_class_raw(class_name: &ClassName) {
+fn unregister_class_raw(class_name: ClassName) {
     out!("Unregister class: {class_name}");
     unsafe {
         #[allow(clippy::let_unit_value)]