Skip to content

*: Add support for MIPS and PowerPC, which lack support for Atomic{U,I}64#37

Closed
chitoku-k wants to merge 2 commits intoprometheus:masterfrom
chitoku-k:support-mips-and-powerpc
Closed

*: Add support for MIPS and PowerPC, which lack support for Atomic{U,I}64#37
chitoku-k wants to merge 2 commits intoprometheus:masterfrom
chitoku-k:support-mips-and-powerpc

Conversation

@chitoku-k
Copy link
Contributor

Signed-off-by: Chitoku odango@chitoku.jp


According to the official documentation, Rust does not provide std::sync::atomic::AtomicU64 and std::sync::atomic::AtomicI64 for MIPS and PowerPC with 32-bit pointers.

PowerPC and MIPS platforms with 32-bit pointers do not have AtomicU64 or AtomicI64 types.

See: https://doc.rust-lang.org/std/sync/atomic/

This means that prometheus-client even doesn't compile for those platforms:

[lychee  client_rust | master  $] cross build --release --target=mipsel-unknown-linux-gnu
   Compiling prometheus-client v0.15.0 (/project)
error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
 --> src/metrics/counter.rs:7:36
  |
7 | use std::sync::atomic::{AtomicU32, AtomicU64, Ordering};
  |                                    ^^^^^^^^^
  |                                    |
  |                                    no `AtomicU64` in `sync::atomic`
  |                                    help: a similar name exists in the module: `AtomicU8`

error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
 --> src/metrics/exemplar.rs:9:5
  |
9 | use std::sync::atomic::AtomicU64;
  |     ^^^^^^^^^^^^^^^^^^^---------
  |     |                  |
  |     |                  help: a similar name exists in the module: `AtomicU8`
  |     no `AtomicU64` in `sync::atomic`

error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
 --> src/metrics/gauge.rs:7:36
  |
7 | use std::sync::atomic::{AtomicU32, AtomicU64, Ordering};
  |                                    ^^^^^^^^^
  |                                    |
  |                                    no `AtomicU64` in `sync::atomic`
  |                                    help: a similar name exists in the module: `AtomicU8`

For more information about this error, try `rustc --explain E0432`.
error: could not compile `prometheus-client` due to 3 previous errors

The atomic-shim crate provides alternative implementation for those platforms if the build target is MIPS or PowerPC, otherwise just re-exports std::sync::atomic::AtomicU64 and std::sync::atomic::AtomicI64. With this patch it successfully compiles.

[lychee  client_rust | support-mips-and-powerpc  $] cross build --release --target=mipsel-unknown-linux-gnu                                                   
    Updating crates.io index                                           
  Downloaded syn v1.0.86                                               
  Downloaded crossbeam-utils v0.8.6                                                                                                            
  Downloaded atomic-shim v0.2.0                                        
  Downloaded 3 crates (284.2 KB) in 0.90s                                                                                                      
   Compiling proc-macro2 v1.0.36                                                                                                               
   Compiling unicode-xid v0.2.2                                                                                                                
   Compiling crossbeam-utils v0.8.6                                    
   Compiling syn v1.0.86                                                                                                                       
   Compiling cfg-if v1.0.0                                             
   Compiling lazy_static v1.4.0                                        
   Compiling stable_deref_trait v1.2.0                                                                                                         
   Compiling itoa v1.0.1                                                                                                                       
   Compiling dtoa v1.0.2                                                                                                                       
   Compiling owning_ref v0.4.1                                                                                                                 
   Compiling atomic-shim v0.2.0                                                                                                                
   Compiling quote v1.0.14                                             
   Compiling prometheus-client-derive-text-encode v0.2.0 (/project/derive-text-encode)                                                         
   Compiling prometheus-client v0.15.0 (/project)                                                                                              
    Finished release [optimized] target(s) in 7.88s

@mxinden
Copy link
Member

mxinden commented Jan 21, 2022

Hi @chitoku-k,

Thanks for the patch.


First of all, would you mind adding the targets that you would like to see supported to the cross-compile matrix in our CI workflow?

- armv7-unknown-linux-gnueabihf
- powerpc64-unknown-linux-gnu

That way we ensure to never break compatibility.


Now to the patch itself:

I wonder whether we should support AtomicU64 through atomic-shim on PowerPC and MIPS platforms with 32-bit pointers. Instead, how about using AtomicU32 on PowerPC and MIPS platforms with 32-bit pointers by default and not offer AtomicU64 at all.

Benefits:

  • No performance surprises for users on PowerPC and MIPS platforms with 32-bit pointers.
  • No additional dependency and sub-dependencies through atomic-shim.
  • Users that do need 64 bit counters can still use Counter with the AtomicU64 provided through atomic-shim.

Drawbacks:

  • Potential counter overflows given the restricted 32 bits only.

I would argue, that with enough documentation around the drawback, the benefits outweigh it.

What do you think @chitoku-k?

@chitoku-k chitoku-k force-pushed the support-mips-and-powerpc branch from 688fb16 to 6120834 Compare January 21, 2022 14:07
@chitoku-k
Copy link
Contributor Author

Thanks @mxinden for your effort made on the cross compilation set up, and your prompt review! I've added mipsel-unknown-linux-gnu that I'm currently dealing with to the target matrix.


For what you've pointed out about pros and cons over atomic-shim, it could really depend on what kind of metrics a user tries to expose in their implementation; however, well-known metrics related to UNIX timestamp (such as certification expiries) often want 64-bit integers so it is the best if a user can selectively choose whether to use 32-bits only (reasonable default) or atomic-shim for this use case by the use of Cargo features.

@mxinden
Copy link
Member

mxinden commented Jan 31, 2022

so it is the best if a user can selectively choose whether to use 32-bits only (reasonable default) or atomic-shim for this use case by the use of Cargo features.

I don't think one should use Cargo features to toggle this behavior. Cargo features are additive across your entire dependency tree. Thus, if some upstream dependency of yours enables the prometheus-client XXX feature, it is enabled across the entire binary.

I would still suggest to use AtomicU32 on architectures that don't support AtomicU64.

In cases where one would still need an AtomicU64 one can provide it themselves, e.g. via atomic-shim.

use std::sync::atomic::Ordering;

struct AtomicU64(atomic_shim::AtomicU64);

impl prometheus_client::metrics::counter::Atomic<u64> for AtomicU64 {
    fn inc(&self) -> u64 {
        self.0.inc_by(1)
    }

    fn inc_by(&self, v: u64) -> u64 {
        self.0.fetch_add(v, Ordering::Relaxed)
    }

    fn get(&self) -> u64 {
        self.0.load(Ordering::Relaxed)
    }
}

fn main() {
    use prometheus_client::metrics::counter::Counter;
    let _counter: Counter<AtomicU64> = Counter::default();
}

@chitoku-k what do you think?

@chitoku-k
Copy link
Contributor Author

Thank you for responding. As it was difficult to reach a consensus over this approach, I am closing this PR and will create a different PR that simply drops support for Atomic64 on those platforms which don't have its native equivalent type.

@chitoku-k chitoku-k closed this Jan 31, 2022
@chitoku-k chitoku-k deleted the support-mips-and-powerpc branch January 31, 2022 15:19
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.

2 participants