Skip to content

Conversation

@hrxi
Copy link
Contributor

@hrxi hrxi commented May 11, 2022

Replace atomic::Atomic<u64> by std::sync::atomic:AtomicU64.

The original motivation of using atomic::Atomic<u64> instead of
std's version in #1670 was the following:

I used the atomic crate and an Atomic because the AtomicU64 type
isn't available on all architectures. The atomic crate automatically
falls back to using a Mutex on platforms that don't support AtomicU64.

This argumentation is moot because the crate directly depends on
libp2p-core which also uses AtomicU64.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

  1. Can you write a few lines about this change? i.e. is the atomic crate fully superseded by the std lib?
  2. We don't have a lockfile checked into the repository because we are only developing a library here. Can you post the output of cargo tree -i -p atomic to show that we are actually dropping the dependency?
  3. Rustfmt doesn't seem to be happy with the changes, please fix that :)

@hrxi hrxi force-pushed the pr_drop_atomic branch from 0004c34 to fb60979 Compare May 11, 2022 23:25
Replace `atomic::Atomic<u64>` by `std::sync::atomic:AtomicU64`.

The original motivation of using `atomic::Atomic<u64>` instead of
`std`'s version in libp2p#1670 was the following:

> I used the atomic crate and an Atomic<u64> because the AtomicU64 type
> isn't available on all architectures. The atomic crate automatically
> falls back to using a Mutex on platforms that don't support AtomicU64.

This argumentation is moot because the crate directly depends on
`libp2p-core` which also uses `AtomicU64`.
@hrxi hrxi force-pushed the pr_drop_atomic branch from fb60979 to 2b1671a Compare May 11, 2022 23:28
@hrxi
Copy link
Contributor Author

hrxi commented May 11, 2022

  1. Can you write a few lines about this change? i.e. is the atomic crate fully superseded by the std lib?

Done.

  1. We don't have a lockfile checked into the repository because we are only developing a library here. Can you post the output of cargo tree -i -p atomic to show that we are actually dropping the dependency?
$ git checkout pr_drop_atomic
Previous HEAD position was f04f6bb4 identify/handler: Improve property name (#2639)
Switched to branch 'pr_drop_atomic'
$ cargo tree -i -p atomic
error: package ID specification `atomic` did not match any packages

	Did you mean `tokio`?
$ git checkout HEAD~
HEAD is now at f04f6bb4 identify/handler: Improve property name (#2639)
$ cargo tree -i -p atomic | head
atomic v0.5.1
├── libp2p v0.45.0 (/home/u/nimiq/rust-libp2p)
│   [dev-dependencies]
│   ├── libp2p-autonat v0.4.0 (/home/u/nimiq/rust-libp2p/protocols/autonat)
│   │   └── libp2p v0.45.0 (/home/u/nimiq/rust-libp2p) (*)
│   ├── libp2p-dcutr v0.3.0 (/home/u/nimiq/rust-libp2p/protocols/dcutr)
│   │   ├── libp2p v0.45.0 (/home/u/nimiq/rust-libp2p) (*)
│   │   └── libp2p-metrics v0.6.0 (/home/u/nimiq/rust-libp2p/misc/metrics)
│   │       └── libp2p v0.45.0 (/home/u/nimiq/rust-libp2p) (*)
│   ├── libp2p-mdns v0.37.0 (/home/u/nimiq/rust-libp2p/protocols/mdns)
  1. Rustfmt doesn't seem to be happy with the changes, please fix that :)

Done.

@thomaseizinger thomaseizinger changed the title Drop atomic dependency src/bandwidth: Drop atomic dependency May 12, 2022
@thomaseizinger thomaseizinger merged commit d69681b into libp2p:master May 12, 2022
@mxinden
Copy link
Member

mxinden commented May 12, 2022

Thanks @hrxi!

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.

3 participants