Use AtomicCell<u64> on targets with target_has_atomic less than 64 by taiki-e · Pull Request #286 · async-rs/async-std

taiki-e · 2019-10-07T09:25:31Z

Closes #141

taiki-e · 2019-10-07T09:34:34Z

+// `#[cfg(target_has_atomic = "64")]`.
+// Refs: https://github.com/rust-lang/rust/tree/master/src/librustc_target
+cfg_if::cfg_if! {
+    if #[cfg(not(any(target_arch = "arm", target_arch = "mips", target_arch = "powerpc")))] {


Many target_arch = "arm" targets are target_has_atomic = "64", but only some targets seems different.

arm-linux-androideabi

armv5te-unknown-linux-musleabi

cc #48 Should we support these targets?

I would definitely support arm-linux-androideabi, I'm hard pressed to find a deployment target with armv5 that is relevant for async-std.

taiki-e · 2019-10-07T09:37:55Z

CI failure seems due to the fact that I enabled the test.

ghost · 2019-10-17T19:15:12Z

This PR needs another rebase.

taiki-e · 2019-10-18T09:04:55Z

This PR should not have changed the behavior on macOS, but this test failed on macOS on CI... But I couldn't reproduce this locally.

nbdd0121 · 2019-10-18T09:26:55Z

Would it be better to name it as something else rather than AtomicU64? Rust atomics are guaranteed to be layout-wise identical to their corresponding integer type, so naming it as AtomicU64 may incorrectly represent that.

Also: std::sync::Mutex is too heavy to protect just an integer. It'd be better to have a spinlock instead.

taiki-e · 2019-10-20T09:52:42Z

Would it be better to name it as something else rather than AtomicU64? Rust atomics are guaranteed to be layout-wise identical to their corresponding integer type, so naming it as AtomicU64 may incorrectly represent that.

I don't intend to make these implementations public, so I don't think it's a problem. However, if #355 is merged, we may need to consider this.

Also: std::sync::Mutex is too heavy to protect just an integer. It'd be better to have a spinlock instead.

I agree. And another advantage of this is AtomicU64::new can be const function.

nbdd0121 · 2019-10-20T14:59:53Z

Another thought, would it make sense to do similar thing as crossbeam's AtomicCell which uses a global Mutex instead? This can guarantee layout compatibility with u64, as well as a const new fn.

ghost

oops, wanted to click "request changes"

taiki-e · 2019-11-04T18:44:10Z

I like the idea of using AtomicCell<u64> from crossbeam-channel as a cross-platform version of AtomicU64 :)

@taiki-e let's do that?

I think it needs a new release of crossbeam-utils including crossbeam-rs/crossbeam#437. So filed crossbeam-rs/crossbeam#444.

ghost · 2019-11-06T23:47:44Z

@taiki-e New versions of crossbeam have been published now.

taiki-e · 2019-11-07T22:10:21Z

looks like ci failure is #286 (comment).

ghost · 2019-11-07T22:27:15Z

@taiki-e Can you try rebasing/merging with the master branch to see if that helps?

taiki-e · 2019-11-08T04:53:47Z

Updated. It seems to succeed on the existing targets (linux,macos,windows), but fails on the newly added targets.

faulesocke · 2020-01-09T23:02:11Z

Rebasing this is almost trivial (just remove the Cargo.toml changes). It then applies cleanly and makes async-std work on MIPS and other platforms.

…4 targets

…on x86-64 targets" This reverts commit b3a65e1.

taiki-e · 2020-01-29T10:30:47Z

Hmm, still a test mentioned in #286 (comment) is failed on the newly added targets.

faulesocke · 2020-03-23T13:28:13Z

Why was this closed? It still does not compile on MIPS.

flxo · 2020-03-26T15:26:08Z

I found this PR today while trying to cross compile async-std for arm-linux-androideabi (32bit). There's no u64 atomic support on 32bit arm v7 and thus also the here proposed usage of crossbeam::atomic doesn't solve the problem. The proposal to use AtomicUsize from #451 sounds reasonable to me but it got closed with a reference to here. Maybe someone can elaborate on the state? I'm happy to support on this issue (I would have to brew my own solution/hack anyway...)

kolapapa · 2020-04-01T11:46:28Z

I found this PR today while trying to cross compile async-std for arm-linux-androideabi (32bit). There's no u64 atomic support on 32bit arm v7 and thus also the here proposed usage of crossbeam::atomic doesn't solve the problem. The proposal to use AtomicUsize from #451 sounds reasonable to me but it got closed with a reference to here. Maybe someone can elaborate on the state? I'm happy to support on this issue (I would have to brew my own solution/hack anyway...)

I thought this PR was going to pass, so I closed it first……

taiki-e · 2020-04-26T16:26:48Z

Why was this closed?

Closed because inactive

taiki-e commented Oct 7, 2019

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

taiki-e commented Oct 7, 2019

View reviewed changes

taiki-e force-pushed the atomic branch from 4630ec0 to dcce474 Compare October 7, 2019 09:39

yoshuawuyts added the enhancement New feature or request label Oct 8, 2019

taiki-e force-pushed the atomic branch 2 times, most recently from 2b036b2 to 72167f0 Compare October 16, 2019 12:43

taiki-e mentioned this pull request Oct 16, 2019

Enable tests on CI #357

Merged

taiki-e force-pushed the atomic branch 2 times, most recently from 2352bff to bade3b7 Compare October 17, 2019 14:36

taiki-e force-pushed the atomic branch 2 times, most recently from bbd761b to 83487da Compare October 18, 2019 05:00

taiki-e mentioned this pull request Oct 18, 2019

expose std::sync::atomic #355

Closed

taiki-e force-pushed the atomic branch 2 times, most recently from 83487da to d6c5377 Compare October 18, 2019 08:55

taiki-e force-pushed the atomic branch 3 times, most recently from 45484e9 to cb197e8 Compare October 23, 2019 12:08

nbdd0121 mentioned this pull request Oct 25, 2019

Add utility type WakerSet to the sync module #390

Merged

taiki-e force-pushed the atomic branch from cb197e8 to a3e618b Compare October 26, 2019 17:14

taiki-e requested a review from a user October 26, 2019 17:31

taiki-e mentioned this pull request Oct 26, 2019

Remove usage of actions-rs/clippy-check #392

Merged

skade added the nominated for 1.0 Nominated for 1.0 release label Oct 28, 2019

taiki-e force-pushed the atomic branch 2 times, most recently from 1d7d149 to 4dbd49b Compare October 30, 2019 16:18

ghost suggested changes Nov 4, 2019

View reviewed changes

taiki-e force-pushed the atomic branch from d84d7cc to 876b7be Compare November 7, 2019 05:35

taiki-e changed the title ~~Use our own AtomicU64 on targets with target_has_atomic less than 64~~ Use AtomicCell<u64> on targets with target_has_atomic less than 64 Nov 7, 2019

taiki-e force-pushed the atomic branch 2 times, most recently from e9abb0c to 745cf87 Compare November 7, 2019 05:51

taiki-e force-pushed the atomic branch 2 times, most recently from b48511f to a80d6ae Compare November 8, 2019 04:44

kolapapa mentioned this pull request Nov 8, 2019

change DYNAMIC_THREAD_COUNT to be an AtomicUsize #451

Closed

yoshuawuyts mentioned this pull request Jan 9, 2020

Build error on MIPS 32bit #664

Open

taiki-e force-pushed the atomic branch from ae976e0 to a2f2671 Compare January 10, 2020 04:20

taiki-e force-pushed the atomic branch from a2f2671 to edc90ab Compare January 20, 2020 02:59

taiki-e added 3 commits January 29, 2020 19:11

Use AtomicCell<u64> on targets with target_has_atomic less than 64

e4f7017

Temporarily ignore io_timeout:: io_timeout_timedout test on non x86-6…

17a41fe

…4 targets

Revert "Temporarily ignore io_timeout:: io_timeout_timedout test on n…

f99d8aa

…on x86-64 targets" This reverts commit b3a65e1.

taiki-e force-pushed the atomic branch from edc90ab to f99d8aa Compare January 29, 2020 10:12

taiki-e closed this Mar 21, 2020

dignifiedquire mentioned this pull request Apr 26, 2020

Test and fix 32 bit targets #756

Merged

jaysonsantos mentioned this pull request May 19, 2020

async-std won't compile on armv5te because of AtomicU64 #789

Closed

taiki-e deleted the atomic branch September 9, 2020 07:54

Conversation

taiki-e commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

taiki-e Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

taiki-e Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

skade Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

taiki-e commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 17, 2019

Uh oh!

taiki-e commented Oct 18, 2019

Uh oh!

nbdd0121 commented Oct 18, 2019

Uh oh!

taiki-e commented Oct 20, 2019

Uh oh!

nbdd0121 commented Oct 20, 2019

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

taiki-e commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 6, 2019

Uh oh!

taiki-e commented Nov 7, 2019

Uh oh!

ghost commented Nov 7, 2019

Uh oh!

taiki-e commented Nov 8, 2019

Uh oh!

faulesocke commented Jan 9, 2020

Uh oh!

taiki-e commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faulesocke commented Mar 23, 2020

Uh oh!

flxo commented Mar 26, 2020

Uh oh!

kolapapa commented Apr 1, 2020

Uh oh!

taiki-e commented Apr 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

taiki-e commented Oct 7, 2019 •

edited

Loading

taiki-e commented Oct 7, 2019 •

edited

Loading

taiki-e commented Nov 4, 2019 •

edited

Loading

taiki-e commented Jan 29, 2020 •

edited

Loading