bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add `no_std` support to `bevy_reflect`

Open bushrat011899 opened this issue 1 year ago • 7 comments

Objective

  • Contributes to #15460

Solution

  • Added std feature (enabled by default)

Testing

  • CI
  • cargo check -p bevy_reflect --no-default-features --target "x86_64-unknown-none"
  • UEFI demo application runs with this branch of bevy_reflect, allowing derive(Reflect)

Notes

  • The spin crate has been included to provide RwLock and Once (as an alternative to OnceLock) when the std feature is not enabled. Another alternative may be more desirable, please provide feedback if you have a strong opinion here!
  • Certain items (Box, String, ToString) provided by alloc have been added to __macro_exports as a way to avoid alloc vs std namespacing. I'm personally quite annoyed that we can't rely on alloc as a crate name in std environments within macros. I'd love an alternative to my approach here, but I suspect it's the least-bad option.
  • I would've liked to have an alloc feature (for allocation-free bevy_reflect), unfortunately, erased_serde unconditionally requires access to Box. Maybe one day we could design around this, but for now it just means bevy_reflect requires alloc.

bushrat011899 avatar Nov 06 '24 01:11 bushrat011899

Tested with my demo UEFI application and confirmed it works as expected, letting me derive Reflect and query for type information at runtime. CI task has been updated to ensure bevy_reflect will continue to compile. I have noticed that because bevy_reflect has a lot of macros, and they aren't checked at compile time in the crate that defines them (only at the compile time of the crate that uses them) we'll probably want to add a no_std example to Bevy to really ensure everything works as expected for end users.

bushrat011899 avatar Nov 06 '24 04:11 bushrat011899

IIUC it's possible to make alloc optional, since it's only used in reflect de/serialization. If you feature gate Reflect(De)Serialize, and the logic in TypedReflect(De)Serializer, it should be possible to get no alloc. These are optional features anyway (without alloc you just can't override de/serialization for your types), and I don't think this will be super complex, but I don't know if it's worth it for the extra complexity.

aecsocket avatar Nov 06 '24 10:11 aecsocket

I'll give this another look to see if serde can be made optional. Right now it definitely isn't optional, but that would be all that's really needed to allow a no_alloc experience.

bushrat011899 avatar Nov 06 '24 10:11 bushrat011899

Actually, reflection uses a lot of Box<dyn PartialReflect> and Box<dyn Reflect>, so alloc may be more necessary than I initially thought. In which case, it might be pointless to feature gate it. Although I suppose there are use cases which don't involve string boxed reflect types.

aecsocket avatar Nov 06 '24 14:11 aecsocket

Hey, I've never touched Bevy before but I do have a strong opinion against spin locks. Would it be better to use critical-section instead of a spin lock, so no_std platforms can provide their own locking mechanism?

GnomedDev avatar Nov 07 '24 14:11 GnomedDev

Hey, I've never touched Bevy before but I do have a strong opinion against spin locks. Would it be better to use critical-section instead of a spin lock, so no_std platforms can provide their own locking mechanism?

That looks perfect honestly, thanks for the advice! I'll update this PR to use critical-section instead.

bushrat011899 avatar Nov 07 '24 18:11 bushrat011899

Ok @GnomedDev I've spent a good couple of hours working with critical-section, reading the attached blog-post, and doing some further research into the area. As far as I can tell, I believe a spin-lock is the best we can do without access to the standard library.

critical-section provides a global mutex, where only one thread can access it at any one time. How it achieves this is configurable via features/dependencies by the user. At first that sounds great, since spinning wastes CPU cycles and can be deadlocked. However, there are two major issues with this approach within the context of bevy_reflect:

  1. bevy_reflect needs a RwLock, not a mutex
  2. The way bevy_reflect uses these locks is via a sometimes long-living guard (long being relative to the typical lifespan of a lock).

Point 1 could be addressed by just accepting that we lose the concurrency boost an RwLock provides. But that further exacerbates point 2, since instead of a small number of writes blocking the critical section, we now have a large number of reads doing it too. Effectively, to replicate an RwLock using critical sections, I believe you would:

  1. Use a CS to lock the RwLock into read mode.
  2. Pass out a guard object which will decrement a counter on drop.
  3. Once counter hits zero, use a CS to unlock.

The problem being if a write lock is requested between steps 1 and 3 (remembering that this time-frame can be large, since multiple readers are valid), the CS isn't actually held, so the writing thread would acquire the CS, see that the lock is in use, and then have no way to park execution, other than just spinning.

To confirm this belief, I attempted to find a pre-made RwLock implementation using critical sections, and came up blank, short of this 15 year old Stack Overflow answer basically saying it can't be done.

So, where to from here? In my opinion, a spin::RwLock is as good as I can provide for a no_std port of bevy_reflect without changing its behaviour to not use read-write locks in general. As such, I believe this PR should go ahead as-is. However, if you (or anyone else!) can provide a suitable replacement for RwLock I will gladly incorporate it here and likely use it in other areas of Bevy too! Otherwise, this might be something to be tackled in a followup PR.

bushrat011899 avatar Nov 08 '24 05:11 bushrat011899