Switch from 'Option' to 'ManuallyDrop' for blocking guard's inner

[Aandreba]

This change will remove a panic point and improve performance when accessing a bound godot class

[TitanNano]

This change will remove a panic point

But we are trading it for more unsafe code. It's not a huge deal, but maintenance wise, I don't think having to verify unsafe code is better than verifying possible panics.

and improve performance

can you elaborate how this is improving performance?

[Aandreba]

can you elaborate how this is improving performance?

Not having to check if the Option is None improves performance slightly, but the biggest improvement comes from the optimizations that the compiler can perform by not having to worry about panicking every time it dereferences the guard. The guard should also be smaller now, since we're not storing the None case.

[GodotRust]

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

[Bromeon]

Thanks for your contribution! 🙂

---

Not having to check if the Option is None improves performance slightly, but the biggest improvement comes from the optimizations that the compiler can perform by not having to worry about panicking every time it dereferences the guard.

This is a fair point, also for self-documentation reasons: seeing unwrap() immediately rings alarm bells about potential panics. Not that unsafe code is much better only to drop a field earlier in the destructor, but it's reasonably localized, and with a safety statement it should be clear enough.

I haven't found a well-established idiom for this problem, which is a pity. There seem to be specialized solutions like parking_lot's unlocked(), but nothing that we could use here.

let guard = mutex.lock();
guard.unlocked(|guard| {
    // Mutex is temporarily unlocked here.
    condition.notify_all();
    // When this closure returns, `guard` is re-locked.
});

---

The guard should also be smaller now, since we're not storing the None case.

From a brief test, it seems to be 32 bytes with both Option and ManuallyDrop. It looks like niche optimizations are at play here.

---

Could you add the safety statements (see comments) and rebase onto master, while keeping the PR at 1 commit?

[Bromeon]

Please add a safety statement:

Suggested change

	unsafe {
	ManuallyDrop::drop(&mut self.inner);
	}
	// SAFETY: guard is dropped exactly once, here.
	unsafe {
	ManuallyDrop::drop(&mut self.inner);
	}

[Bromeon]

Also here.