Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow async and sync nats to compile on MIPS and PowerPC #1210

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

protochron
Copy link
Contributor

@protochron protochron commented Feb 4, 2024

Now that nats.rs depends on a version of rustls that includes the latest version of ring, it can now be compiled to run on additional types of CPU architectures.

Specifically this converts all uses of AtomicU64 to use the type provided by the portable-atomic crate, which uses the native version of AtomicU64 on all architectures except MIPS and PowerPC where that type isn't supported.

I went ahead and updated sync nats with the latest version of rustls which inflated the size and scope of this PR beyond just swapping out the AtomicU64 implementation.

@protochron
Copy link
Contributor Author

protochron commented Feb 4, 2024

I just ran across https://github.com/taiki-e/portable-atomic which might be better because it would provide support for even more architectures
Went ahead and just used the portable-atomic crate instead.

@protochron protochron force-pushed the sync_nats_ring branch 3 times, most recently from 8d9d229 to 31c509b Compare February 4, 2024 17:07
@Jarema
Copy link
Member

Jarema commented Feb 5, 2024

Thanks for this contribution!

I checked briefly the portable-atomics and it looks good, with minimal depedencies.
Will bench it soon and review it.
This is nice timing too, as we're now investigating options to support more architectures.

@paolobarbolini
Copy link
Contributor

Going by the popularity I'd also consider crossbeam-utils crossbeam_utils::atomic::AtomicCell

@Jarema
Copy link
Member

Jarema commented Feb 26, 2024

@protochron I would like this to be merged, however, it would be good to check the crossbeam_utils against the less know crate :).

@protochron
Copy link
Contributor Author

protochron commented Feb 26, 2024

@Jarema I think using crossbeam_utils directly would be a much more substantial change to the code. I'd have add some directives around the AtomicU64 imports and calls where we need AtomicU64s to using an AtomicCell instead which is effectively what the portable-atomics crate does already. Also the author of the portable-atomics crate is one of the owners of crossbeam-utils, so I'm pretty sure they've done a better job of implementing atomics on these platforms than I will :D

@thomastaylor312
Copy link
Contributor

@Jarema I also have a vested interest in multiple platforms and I think I agree with @protochron here. FWIW, I don't think portable-atomics is less well known as it has 15 million downloads from crates.io and the maintainer is also a maintainer of crossbeam. Also, because it delegates to existing atomic implementations if they exist, it seems like a really strong pick to enable what is needed here

@Jarema
Copy link
Member

Jarema commented Feb 28, 2024

Ok, point taken @thomastaylor312 and @protochron :). Those are really strong arguments.

Let's stick to portable atomics.
I will review it shortly.

@protochron can you please resolve the conflicts?

@Jarema
Copy link
Member

Jarema commented Mar 1, 2024

@protochron I would like to merge this before doing a release, so a kind nudge to fix conflicts :).

@protochron
Copy link
Contributor Author

Sorry about that! Just synced main and resolved the conflicts

Now that nats.rs depends on a version of rustls that includes the latest
version of ring, it can now be compiled to run on additional types of
CPU architectures.

Specifically this converts all uses of `AtomicU64` to use the type
provided by the `portable-atomic` crate, which uses the native version
of `AtomicU64` on all architectures except ones where that type isn't
supported.

I went ahead and updated sync nats with the latest version of rustls
which inflated the size and scope of this PR beyond just swapping out
the AtomicU64 implementation.
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Jarema Jarema merged commit 86b5bef into nats-io:main Mar 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants