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

Change task counter AtomicU64 to AtomicU32 #141

Closed
wants to merge 1 commit into from
Closed

Change task counter AtomicU64 to AtomicU32 #141

wants to merge 1 commit into from

Conversation

kolapapa
Copy link

@kolapapa kolapapa commented Sep 4, 2019

cross compile with mipsel-unknown-linux-uclibc(32bit) error:

root@8bc5bd79cb06:/opt/liukai/rust/async-t# xargo build --target mipsel-unknown-linux-uclibc --release
   Compiling async-std v0.99.4
error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
 --> /root/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/async-std-0.99.4/src/task/task.rs:6:25
  |
6 | use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
  |                         ^^^^^^^^^
  |                         |
  |                         no `AtomicU64` in `sync::atomic`
  |                         help: a similar name exists in the module: `AtomicU16`

error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
 --> /root/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/async-std-0.99.4/src/task/blocking.rs:5:25
  |
5 | use std::sync::atomic::{AtomicU64, Ordering};
  |                         ^^^^^^^^^
  |                         |
  |                         no `AtomicU64` in `sync::atomic`
  |                         help: a similar name exists in the module: `AtomicU16`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0432`.
error: Could not compile `async-std`.

To learn more, run the command again with --verbose.

@ghost
Copy link

ghost commented Sep 4, 2019

I worry that 32 bits might not be enough and that one could easily spawn tasks in a loop and overflow the counter. Perhaps we could "simulate" 64-bit atomics on platforms that don't support them with a mutex or some kind of trick with two 32-bit atomic numbers?

@kolapapa
Copy link
Author

kolapapa commented Sep 4, 2019

I worry that 32 bits might not be enough and that one could easily spawn tasks in a loop and overflow the counter. Perhaps we could "simulate" 64-bit atomics on platforms that don't support them with a mutex or some kind of trick with two 32-bit atomic numbers?

Can we set it to AtomicU64 on 64-bit and AtomicU32 on 32-bit?

@ghost
Copy link

ghost commented Sep 4, 2019

I still worry that the IDs could be overflowed on 32-bit platforms.

@taiki-e
Copy link
Contributor

taiki-e commented Sep 6, 2019

Perhaps we could "simulate" 64-bit atomics on platforms that don't support them with a mutex or some kind of trick with two 32-bit atomic numbers?

Perhaps like this:

cfg_if::cfg_if! {
    if #[cfg(target_pointer_width = "64")] {
        pub(crate) use std::sync::atomic::AtomicU64;
    } else {
        #[derive(Debug)]
        pub(crate) struct AtomicU64(std::sync::Mutex<u64>);
        
        impl AtomicU64 {
            pub(crate) fn load(&self, _: Ordering) -> u64 {
                *self.0.lock().unwrap()
            }
            // .. some methods
        }
    }
}

@ghost
Copy link

ghost commented Sep 8, 2019

@taiki-e Yes, although I suggest using Mutex on mips platforms only rather than on all 32-bit platforms.

@taiki-e
Copy link
Contributor

taiki-e commented Sep 8, 2019

@stjepang Ah, Indeed. I missed that AtomicU64 is #[cfg(target_has_atomic = "64")], not #[cfg(target_pointer_width = "64")].

@nbdd0121
Copy link
Contributor

IMO DYNAMIC_THREAD_COUNT can be replaced with AtomicUsize.

@kolapapa
Copy link
Author

IMO DYNAMIC_THREAD_COUNT can be replaced with AtomicUsize.

I thought so before, but it seems that they want to find a better solution.

std::sync::atomic

@ghost
Copy link

ghost commented Nov 1, 2019

It's totally fine for DYNAMIC_THREAD_COUNT to be an AtomicUsize :)

@kolapapa
Copy link
Author

kolapapa commented Nov 4, 2019

The new PR is here: #451

@kolapapa kolapapa closed this Nov 4, 2019
dignifiedquire pushed a commit that referenced this pull request May 7, 2020
dignifiedquire pushed a commit that referenced this pull request May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants