Extend no_std support

[Jinxit]

Feedback very much welcome as I haven't studied this library in close enough detail to see the full consequences of this PR.

I've changed a few things:

1. Switched from std to core and alloc in macros, fixing no-std support for macros #106.
2. Added atomic-polyfill as an optional feature to support targets which don't have full atomic CAS support.
3. As lazy_static does not support atomic-polyfill, I changed over to using once_cell::race::OnceBox instead. The downside to using a OnceBox is that multiple threads can start generating the same cell, but in the end only the first completed cell is used. The upside is that it requires simpler synchronization primitives.
4. Added a default-enabled feature atomic which enables creating worlds with the atomically increasing ID that is currently used in World::new() and World::default(). When disabled the function World::new_with_id(id: u64) must be used instead.

1 and 2 feel pretty natural, but 3 and 4 definitely requires some scrutiny to ensure I didn't mess something up. The tests run green and the Bundle derive macro seems to work as well.

[adamreichold]

I don't think we need spin and atomic-polyfill. IMHO, it would be most reasonable to require either support for atomics via core or via atomic-polyfill. Usage of spin::Mutex should then be replaced by an atomic counter.

This seems preferable as World::new_with_id would have to unsafe as providing duplicate ID would break memory safety.

[Jinxit]

Makes perfect sense, I've adjusted accordingly. I also got rid of the atomic feature and just used spin instead as it's more clear.

[adamreichold]

Makes perfect sense, I've adjusted accordingly. I also got rid of the atomic feature and just used spin instead as it's more clear.

But do we need to spin-based at all if we have either core::sync::atomic or atomic-polyfill? Does atomic-polyfill not support MIPS and PPC?

Or conversely, could we use only spin because it provides lazy-initialization as well? Does that require atomic-polyfill or something similar?

I think having both present is a significant increase in complexity that should be avoid if at all possible.

[Ralith]

Thanks for working on this (and thanks @adamreichold for helping with the review)! If you like, you could split out the more straightforward parts of this into a separate PR that we can merge right away.

The downside to using a OnceBox is that multiple threads can start generating the same cell

This is completely fine. It will rarely occur in practice and do no harm when it does.

[Jinxit]

But do we need to spin-based at all if we have either core::sync::atomic or atomic-polyfill? Does atomic-polyfill not support MIPS and PPC?

I don't think so. Any architectures not listed in the README will just forward to the core::atomic implementation.

Or conversely, could we use only spin because it provides lazy-initialization as well? Does that require atomic-polyfill or something similar?

spin relies on compare_exchange_weak or another polyfill called portable-atomics, which does not currently support the platform I'm on (thumbv4t). Both of them do essentially the same thing, though I find atomic-polyfill easier to extend to your own platform without first having to get it merged upstream.

I think having both present is a significant increase in complexity that should be avoid if at all possible.

Another option would be to optionally outsource the ID generation using a macro (to set a global ID generator), but that doesn't sound much better to me.

[adamreichold]

Another option would be to optionally outsource the ID generation using a macro (to set a global ID generator), but that doesn't sound much better to me.

Maybe we should approach this differently: The world ID are used only to match up prepared queries. So maybe we should make the ID field and all types related to prepared queries dependent on the presence of atomics or (either atomic-polyfill or spin but not both)?

That would mean that platforms not supporting the atomics fallback of our choice do not have access to prepared queries, but that appears to be a rather graceful degradation of functionality to me since everything else should continue to work.

[Jinxit]

That would mean that platforms not supporting the atomics fallback of our choice do not have access to prepared queries, but that appears to be a rather graceful degradation of functionality to me since everything else should continue to work.

Seeing as MIPS/PPC support using spin is already merged, I suppose that'd be the fallback of choice. I guess it's an ok compromise, but it doesn't sit completely well with me seeing as it could work.

But ultimately it's up to you, if that's the way you want to go I could try implementing it.

[Ralith]

Restricting prepared query availability makes sense to me. It prevents limitations on one platform from affecting everyone else, I'm not sure anyone's even using those APIs, and we can always restore them in the future without breaking anything.

[Jinxit]

As atomics are used elsewhere in hecs, keeping only spin was not an option for supporting both MIPS/PPC and non-CAS targets. I've instead kept only atomic-polyfill and put prepared queries behind a feature which is enabled by default.

If you're happy with the outline I can tidy up the query::prepared module, move it into its own folder and so on, but is this roughly what you intended?

[adamreichold]

As atomics are used elsewhere in hecs, keeping only spin was not an option for supporting both MIPS/PPC and non-CAS targets. I've instead kept only atomic-polyfill and put prepared queries behind a feature which is enabled by default.

If atomics are used elsewhere and hence atomic-polyfill is a mandatory dependency, we should be able to keep prepared queries without a feature gate as well as the world ID does not require a mutex but just as well be generated using an atomic counter. (We probably just need to reduce it to AtomicUsize and make sure to check for overflow to maximise portability?)

Sorry for leading you down the wrong path here. I completely forgot that borrow checking is thread safe and uses atomics as well.

[Jinxit]

If atomics are used elsewhere and hence atomic-polyfill is a mandatory dependency, we should be able to keep prepared queries without a feature gate as well as the world ID does not require a mutex but just as well be generated using an atomic counter. (We probably just need to reduce it to AtomicUsize and make sure to check for overflow to maximise portability?)

Just to double check, do you then want me to rip out the spin feature? Because that would remove MIPS/PPC support, which seems unfortunate. Or should I just roll back the last commit?

[adamreichold]

Just to double check, do you then want me to rip out the spin feature? Because that would remove MIPS/PPC support, which seems unfortunate. Or should I just roll back the last commit?

I was suggesting to remove spin but to keep MIPS/PPC support by using AtomicUsize instead of AtomicU64 to generate the world ID. This should work as MIPS/PPC would be without AtomicBorrow otherwise.

[taiki-e]

Hi @Jinxit. I'm a maintainer of portable-atomics crate.

spin relies on compare_exchange_weak or another polyfill called portable-atomics, which does not currently support the platform I'm on (thumbv4t).

I would love to add thumbv4t support to portable-atomics.
I created a list of approaches I think will work and implemented one of them... Which approach do you prefer? (off-topic for this PR, so I'd appreciate if you could comment in taiki-e/portable-atomic#26)

remove MIPS/PPC support

FWIW, portable-atomics supports 64-bit atomics on these targets using lock-based fallback; crates such as metrics are using portable-atomics to support these targets.

[Jinxit]

Gonna revisit this with the new changes to portable-atomics whenever I have time.

[wenyuzhao]

Any progress on this PR? I'd love to see the macro crate can actually support no_std.

[Ralith]

I don't think this work is active right now. If you're interested in working on this yourself, the portion that fixes #106 could probably be split out and merged pretty quickly, independent of the more difficult stuff. Maybe now that OnceCell is in core the rest of this is simpler too.

[wenyuzhao]

Yeah I'd love to take over the job and at least fix #106

[Ralith]

Please feel free! I'd recommend making a new PR.

Oh, you already did; great!