change Global to use Once #752

bend-n · 2024-06-11T04:56:28Z

improves code quality

lilizoey · 2024-06-11T05:00:35Z

if you're gonna do this you should at least use the most recent master considering #750

but also you'll need to be certain that this doesn't hurt performance

i dont have time to look at this further rn tho

bend-n · 2024-06-11T05:08:44Z

I can run some benchmarks, i guess?

bend-n · 2024-06-11T05:24:42Z

i ran a benchmark and the performance seems to be about the same.

extern crate test;
#[bench]
fn x(b: &mut test::Bencher) {
    b.iter(|| {
        let x = Global::<String>::default();
        for _ in 0..10 {
            *x.lock() += "304.78";
        }
    })
}

it could probably be a bit faster once get_mut_or_init drops.

Bromeon · 2024-06-11T06:37:52Z

Thanks for the PR!

This uses Mutex<OnceLock<T>>, which requires two synchronizations instead of one. I honestly don't see how this is an improvement over the status quo, at best we get similar/same performance.

If you find concrete bugs with the current implementation, I'd prefer to address those directly and write regression tests for it, but I'm not really willing to introduce extra locking just so we can reuse standard facilities. Yes, I find it mindblowing too that something as basic as ergonomic global variables seems to be pioneering work in Rust 😉

bend-n · 2024-06-11T07:10:11Z

wait i can swap it to OnceCell

Bromeon · 2024-06-11T07:13:07Z

Please provide a proper rationale for the change. Your initial message doesn't even have a description.

bend-n · 2024-06-11T07:17:22Z

the rationale is to reduce the necessary maintenance surface, this makes the code much simpler.
its a refactor.
the old code was rather sketchy, this is much less so.

Bromeon · 2024-06-11T07:37:15Z

Some smaller disadvantages I see with your approach:

The function is still accessible after its use. It might be a bit harder if we e.g. wanted to use FnOnce in the future.
The type now occupies space for both the function and the value, while before this was in an enum. It's not a big deal, but it's a problem the current solution doesn't have.

Btw, regarding several of your comments, also on Discord:

can I fix Global

its kinda broken though
like it has a whole useless mutex thats just making api harder

Global: use the wheel, dont reinvent it

the old code was rather sketchy

I could appreciate your changes a lot more without the snarky comments. You may not notice it, but the way you word things comes across as pretty arrogant. I would recommend to familiarize yourself with the existing code and design behind it, before just assuming it's garbage.

Let's keep it technical from now on. Thanks.

bend-n · 2024-06-14T02:11:40Z

i believe this change doesn't make FnOnce much harder to swap to? you can just put an Option around it (which keeps the type size the same).
The size of Global before and after remain the same (64).

My bad, will do.

godot-ffi/src/global.rs

Bromeon · 2024-06-26T17:20:01Z

godot-ffi/src/global.rs

+pub struct Global<T, F = fn() -> T> {
    // When needed, this could be changed to use RwLock and separate read/write guards.
-    value: Mutex<InitState<T>>,
+    value: Mutex<OnceCell<T>>,
+    init_fn: F,
 }


The F parameter is not necessary; new() in its current form can only be invoked with a function pointer. I'd keep this a pure refactoring without changing the public API, we can discuss new features separately if needed.

Bromeon · 2024-06-26T17:23:57Z

godot-ffi/src/global.rs

    pub fn lock(&self) -> GlobalGuard<'_, T> {
-        let mutex_guard = self
-            .value
-            .lock()
-            .expect("Global<T> poisoned; a thread has panicked while holding a lock to it");
-
-        let guard = Self::ensure_init(mutex_guard, true)
-            .unwrap_or_else(|| panic!("previous Global<T> initialization failed due to panic"));
-
-        guard
+        let guard = self.value.lock().unwrap_or_else(PoisonError::into_inner);
+        guard.get_or_init(self.init_fn);
+        // SAFETY: above line guarantees init
+        unsafe { GlobalGuard::new_unchecked(guard) }


Are we sure the get_or_init() isn't more expensive than the previous Self::ensure_init()?

While it may not matter in average cases, it would be a pity to pessimize performance just to make our own life a bit easier.

Nitpick, please also leave an empty line before // comment blocks (here // SAFETY).

the get_or_init here boils down to

if self.get().is_some() { return }; let value = f(); let slot = unsafe { &mut *self.inner.get() }; slot.insert(value);

which seems like an improvement from the previous ensure_init fn.

and ive benchmarked it to be about the same speed.

godot-ffi/src/global.rs

Bromeon · 2024-06-26T17:27:18Z

godot-ffi/src/global.rs

+        std::panic::catch_unwind(|| guard.get_or_init(self.init_fn))
+            .map_err(|_| GlobalLockError::InitFailed)?;
+        Ok(unsafe { GlobalGuard::new_unchecked(guard) })


It's not obvious why this is called again here, could you elaborate?

Also, unsafe would need // SAFETY, can be kept short like above.

i hope ive clarified it now

Bromeon · 2024-06-26T17:27:26Z

godot-ffi/src/global.rs

+    use std::{
+        cell::OnceCell,
+        ops::{Deref, DerefMut},
+        sync::MutexGuard,
+    };


Bromeon · 2024-06-26T17:27:48Z

godot-ffi/src/global.rs

+            match mutex_guard.get() {
+                Some(_) => Some(Self { mutex_guard }),
                _ => None,
            }


You can use map here

could you try to avoid just saying "no" like this? it isnt very helpful most of the time and we need to ask follow up questions anyway to understand more clearly. why doesn't map work?

it results in an error because it moves the mutex guard into a FnOnce

error[E0505]: cannot move out of `mutex_guard` because it is borrowed --> godot-ffi/src/global.rs:114:35 | 113 | pub(super) fn new(mutex_guard: MutexGuard<'a, OnceCell<T>>) -> Option<Self> { | ----------- binding `mutex_guard` declared here 114 | mutex_guard.get().map(|_| Self { mutex_guard }) | ----------- --- ^^^ ----------- move occurs due to use in closure | | | | | | | move out of `mutex_guard` occurs here | | borrow later used by call | borrow of `mutex_guard` occurs here

I can do this though if you prefer

mutex_guard.get().is_some().then_some(Self { mutex_guard })

you see what i have right now is basically an eager map

Ok, then let's leave the match. Can you add a comment mentioning that we can't use .get().map(...) due to the guard remaining borrowed?

godot-ffi/src/global.rs

GodotRust · 2024-06-27T07:02:04Z

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

Bromeon

Thanks! Few things left.

Bromeon · 2024-07-08T15:10:18Z

godot-ffi/src/global.rs

+            // SAFETY: `get_or_init()` cant panic, therefore the object is guaranteed to be initialized.
+            Ok(unsafe { GlobalGuard::new_unchecked(g) })


The safety message is imprecise -- get_or_init can panic, but a potential panic is caught above. Reaching this line implies that it didn't panic.

godot-ffi/src/global.rs

Bromeon · 2024-07-08T15:11:54Z

godot-ffi/src/global.rs

+        // SAFETY: `get_or_init()` cant panic, therefore the object is guaranteed to be initialized.
+        unsafe { GlobalGuard::new_unchecked(guard) }


Also here, get_or_init can panic but the panic is propagated to the caller.

godot-ffi/src/global.rs

Bromeon · 2024-07-08T15:16:25Z

godot-ffi/src/global.rs

+            match mutex_guard.get() {
+                Some(_) => Some(Self { mutex_guard }),
                _ => None,
            }


Ok, then let's leave the match. Can you add a comment mentioning that we can't use .get().map(...) due to the guard remaining borrowed?

lilizoey

i think this mostly looks good to me! was only one thing that i noticed.

godot-ffi/src/global.rs

Bromeon · 2024-07-12T07:09:43Z

godot-ffi/src/global.rs

-
+        /// The value must be initialized.
+        pub(super) unsafe fn new_unchecked(mutex_guard: MutexGuard<'a, OnceCell<T>>) -> Self {
+            debug_assert!(mutex_guard.get().is_some(), "safety precondition violated");


"safety precondition violated" isn't very expressive, better would be "cell not initialized" or so.

do you mind "safety precondition violated: cell not initialized"?

That's fine, too!

although actually i think unwrap_unchecked already has an assertion so maybe i dont need it at all but it cant hurt anyways

godot-ffi/src/global.rs

Bromeon · 2024-07-12T07:11:31Z

godot-ffi/src/global.rs

        fn deref(&self) -> &Self::Target {
-            // SAFETY: `self` is `Initialized`.
-            unsafe { self.mutex_guard.as_initialized().unwrap_unchecked() }
+            // SAFETY: [`GlobalGuard::new`] being the sole constructor guarantees that the cell is initialized.


This isn't quite accurate; there is another constructor new_unchecked() with the same visibility.

But I think it's enough to mention that the invariant of GlobalGuard is that the cell is initialized. Then ensuring this (and proving its truth via comments) can be done in the constructors.

Bromeon · 2024-07-12T07:12:49Z

godot-ffi/src/global.rs

        }
    }

    impl<T> DerefMut for GlobalGuard<'_, T> {
        fn deref_mut(&mut self) -> &mut Self::Target {
-            // SAFETY: `self` is `Initialized`.
-            unsafe { self.mutex_guard.as_initialized_mut().unwrap_unchecked() }
+            // SAFETY: [`GlobalGuard::new`] being the sole constructor guarantees that the cell is initialized.


Bromeon · 2024-07-13T13:41:21Z

The commit message still mentions

Global: changed to wrap Mutex

but you're now using OnceCell. Could you update that? After, the PR should be ready 🙂

Bromeon · 2024-07-14T11:57:35Z

Thanks!

bend-n force-pushed the wheels branch 2 times, most recently from 7871677 to 37df2af Compare June 11, 2024 05:08

bend-n force-pushed the wheels branch from 37df2af to 60ff4a8 Compare June 11, 2024 05:11

Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Jun 11, 2024

bend-n force-pushed the wheels branch from 60ff4a8 to 1f54246 Compare June 11, 2024 07:13

bend-n changed the title ~~change Global to use OnceLock~~ change Global to use Once Jun 11, 2024

Bromeon reviewed Jun 26, 2024

View reviewed changes

bend-n force-pushed the wheels branch 7 times, most recently from be74d96 to d5c22f9 Compare June 27, 2024 04:24

Bromeon reviewed Jul 8, 2024

View reviewed changes

bend-n force-pushed the wheels branch 3 times, most recently from 45df6a7 to f1c9301 Compare July 8, 2024 16:59

lilizoey reviewed Jul 11, 2024

View reviewed changes

godot-ffi/src/global.rs Show resolved Hide resolved

bend-n force-pushed the wheels branch from f1c9301 to 761d9f2 Compare July 12, 2024 02:08

Bromeon reviewed Jul 12, 2024

View reviewed changes

bend-n force-pushed the wheels branch 3 times, most recently from 5b76701 to d949495 Compare July 13, 2024 11:38

Global: changed to wrap Mutex<OnceCell>

7d01523

bend-n force-pushed the wheels branch from d949495 to 7d01523 Compare July 13, 2024 13:57

Bromeon added this pull request to the merge queue Jul 14, 2024

Merged via the queue into godot-rust:master with commit f0fc6f6 Jul 14, 2024
15 checks passed

bend-n deleted the wheels branch July 14, 2024 11:50

		// SAFETY: `get_or_init()` cant panic, therefore the object is guaranteed to be initialized.
		Ok(unsafe { GlobalGuard::new_unchecked(g) })

		// SAFETY: `get_or_init()` cant panic, therefore the object is guaranteed to be initialized.
		unsafe { GlobalGuard::new_unchecked(guard) }

change Global to use Once #752

change Global to use Once #752

Conversation

bend-n commented Jun 11, 2024 • edited Loading

lilizoey commented Jun 11, 2024 • edited Loading

bend-n commented Jun 11, 2024

bend-n commented Jun 11, 2024 • edited Loading

Bromeon commented Jun 11, 2024 • edited Loading

bend-n commented Jun 11, 2024

Bromeon commented Jun 11, 2024

bend-n commented Jun 11, 2024

Bromeon commented Jun 11, 2024 • edited Loading

bend-n commented Jun 14, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GodotRust commented Jun 27, 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

lilizoey 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

Choose a reason for hiding this comment

Bromeon commented Jul 13, 2024

Bromeon commented Jul 14, 2024

bend-n commented Jun 11, 2024 •

edited

Loading

lilizoey commented Jun 11, 2024 •

edited

Loading

bend-n commented Jun 11, 2024 •

edited

Loading

Bromeon commented Jun 11, 2024 •

edited

Loading

Bromeon commented Jun 11, 2024 •

edited

Loading

bend-n commented Jun 14, 2024 •

edited

Loading