Add preliminary support for loom #487

jonhoo · 2020-04-08T15:43:47Z

This patch only adds support to parts of utils and to epoch. Some
parts of utils had to be left out, since they rely on
AtomicUsize::new being const (which it is not in loom). Other
parts had to be left out due to the lack of thread::Thread in loom.
All the parts needed for epoch were successfully moved to loom.

The one loom test currently passes! I've added a loom pass on
crossbeam-epoch to CI.

Also, note that the uses of UnsafeCell in utils have not been moved
to loom::cell::UnsafeCell, as loom's UnsafeCell does not support T: ?Sized, which AtomicCell depends on.

Fixes #486.

jonhoo · 2020-04-08T15:46:01Z

/cc @cynecx since it seemed you were interested

crossbeam-epoch/src/lib.rs

cynecx · 2020-04-08T18:41:43Z

I think it would be nice if we could run some variant of the examples (treiber-stack) through loom too?

jonhoo · 2020-04-08T19:02:14Z

Should be quite possible! The easiest way would probably be to factor out the core of treiber-stack into some kind of function that we could easily call from a test. Either than, or we copy-paste the treiber stack into a test and wrap it in loom::model.

jonhoo · 2020-04-09T14:45:38Z

I added one! And interestingly enough, it fails with

Causality violation: Concurrent load and mut accesses.

jonhoo · 2020-04-09T14:46:17Z

@carllerche When I get the error above, how do I then find out where the violation happened?

jonhoo · 2020-04-09T17:21:16Z

I chatted to Carl, and it looks like the cause for this is probably a bug in loom: tokio-rs/loom#124. Let's see when it is fixed whether it then passes!

jonhoo · 2020-04-10T20:45:25Z

Oooh, with tokio-rs/loom#125, loom actually runs for the treiber stack example! Letting it go through the iterations now to see if it finds any problems. None so far :D

jonhoo · 2020-04-12T15:42:47Z

And with tokio-rs/loom#128, it runs to completion without errors! 😮

jonhoo · 2020-04-12T18:01:16Z

CI doesn't like the patch in 08b37ab 😅 It will go away once loom gets another release.

jonhoo · 2020-04-14T00:10:11Z

Hmm, how can I see what the errors were @taiki-e? All of them just say "This check failed" as far as I can tell?

taiki-e · 2020-04-14T04:18:20Z

@jonhoo I restarted CI -- looks like it fixed the issue: https://github.com/crossbeam-rs/crossbeam/pull/487/checks

jonhoo · 2020-04-14T15:58:14Z

The minimal version bump to 1.30 is a little awkward, but is needed to allow us to use paths for macro imports. While digging into it, I also noticed that technically 1.28 isn't the msrv at all for these crates. For example, the alloc crate didn't stabilize until 1.36, yet we use it if someone does a no_std build with --feature alloc..

jonhoo · 2020-04-14T20:08:51Z

@jeehoonkang I believe this is now ready for review :)

crossbeam-epoch/src/lib.rs

crossbeam-utils/Cargo.toml

taiki-e · 2020-04-18T11:03:14Z

the alloc crate didn't stabilize until 1.36, yet we use it if someone does a no_std build with --feature alloc..

I think this is fine given that alloc feature is not enabled by default and its name is the same as the name of the crate on which it depends (other features should also be fine). but I agree with it is preferable to mention features that have a different msrv from the default feature.

cynecx · 2020-05-09T01:35:15Z

Ping.

jonhoo · 2020-05-19T20:54:24Z

@cynecx I believe this is ready to merge once reviewed. I've pushed a rebased version now to make sure there aren't any conflicts.

jonhoo · 2020-05-19T20:56:30Z

The two failures appear unrelated to this PR. I think CI is simply broken on master. Once that's fixed I'd be happy to rebase again.

jeehoonkang · 2020-05-23T05:38:11Z

Crossbeam has evolved quite a lot recently, so I wonder if the unrelated failures are gone.

jonhoo · 2020-05-23T13:57:44Z

@jeehoonkang rebased!

jonhoo · 2020-05-23T14:33:51Z

Hey, it works again! 🎉

jonhoo · 2020-12-22T04:23:49Z

Err, rebasing this will probably be pretty painful. Do you mean squash? I merged with master just now, and that went off without a hitch, so it should just be a matter of using GitHub's "squash and merge" button I think.

taiki-e · 2020-12-22T04:34:20Z

Oh, I didn't notice about "squash and merge" worked. I'll use that. Thanks for pointing that out.

taiki-e · 2020-12-22T04:38:23Z

@jeehoonkang: Do you want another chance at reviewing this before I merge it?

taiki-e · 2021-01-04T07:09:53Z

Rename cfg(loom) to something like cfg(loom_crossbeam) to avoid conflict with other crates that using cfg(loom). (I don't know what the loom's recommendation for this is. If there is a recommendation, it is better to use it.)

Oh, I noticed that loom has a PR that seems to try to recommend the use of cfg like ${project_name}_loom, not loom_${project_name}: tokio-rs/loom#159 tokio-rs/tokio#2463 (comment)
@jonhoo That PR hasn't been merged yet, but should we probably follow it?

jonhoo · 2021-01-05T01:52:23Z

I don't feel strongly whether the name goes first or last, but I agree that crossbeam should probably follow whatever convention ends up being documented.

taiki-e · 2021-02-13T17:46:16Z

I've filed #658 to rename cfg(loom_crossbeam) to cfg(crossbeam_loom). Also, I've approved tokio-rs/loom#159.

@jeehoonkang

658: Rename cfg(loom_crossbeam) to cfg(crossbeam_loom) r=jonhoo a=taiki-e It matches with the convention that *will* be documented in `loom`. See #487 (comment) for more. r? @jeehoonkang @jonhoo Co-authored-by: Taiki Endo <te316e89@gmail.com>

659: Prepare for the next release r=taiki-e a=taiki-e It's been over two months since the previous release. There are some improvements and deprecations in the master branch, and it would be nice to release them. Also, there is no breaking change that needs a major version bump. Changes: - crossbeam-epoch 0.9.1 -> 0.9.2 - Add `Atomic::compare_exchange` and `Atomic::compare_exchange_weak`. (#628) - Deprecate `Atomic::compare_and_set` and `Atomic::compare_and_set_weak`. (#628) - Make `const_fn` dependency optional. (#611) - Add unstable support for `loom`. (#487) - crossbeam-utils 0.8.1 -> 0.8.2 - Deprecate `AtomicCell::compare_and_swap`. (#619) - Add `Parker::park_deadline`. (#563) - Improve implementation of `CachePadded`. (#636) - Add unstable support for `loom`. (#487) Co-authored-by: Taiki Endo <te316e89@gmail.com>

matklad · 2021-02-22T13:14:27Z

Is [target.'cfg(crossbeam_loom)'.dependencies] in Cargo.toml the right pattern here? With this, loom (and its deps) go into rev-dep Cargo.locks unconditionally. If this were a Cargo feature (--private--loom), then it would be invisible to consumers.

Context: was wondering about

  Downloaded generator v0.6.24
  Downloaded loom v0.4.0
  Downloaded 2 crates (81.8 KB) in 1.08s

output when building rust-analyzer.

taiki-e · 2021-02-22T13:29:19Z

@matklad

Is [target.'cfg(crossbeam_loom)'.dependencies] in Cargo.toml the right pattern here?

Yes. see Alex's this comment.

With this, loom (and its deps) go into rev-dep Cargo.locks unconditionally.

As far as I know, it will be downloaded but not actually compiled. See also tokio-rs/bytes#400 and tokio-rs/bytes#411.
That said, I agree that the cargo messages and crates.io UI are confusing...

If this were a Cargo feature (--private--loom), then it would be invisible to consumers.

Unfortunately, as far as I know, (at least in the v1 resolver) the cargo feature doesn't provide enough functionality to support loom. See also #638

matklad · 2021-02-22T13:44:35Z

As far as I know, it will be downloaded but not actually compiled.

compilation is not the only cost: I read Cargo.lock from time to time, and I read .lock diffs regularly. I care a lot about keeping those as small as possible.

Unfortunately, as far as I know, (at least in the v1 resolver) the cargo feature doesn't provide enough functionality to support loom.

Can loom then be gated by both feature and cfg? The following seems to work:

[target.'cfg(loom)'.dependencies]
loom = {version="*", optional=true}

taiki-e · 2021-02-22T13:55:28Z

Can loom then be gated by both feature and cfg? The following seems to work:
[target.'cfg(loom)'.dependencies]
loom = {version="*", optional=true}

Oh, I didn't know it works. I would like to switch to it if it works.

taiki-e · 2021-02-23T10:03:10Z

cfg-ed deps issue will be fixed by #666

@matklad

666: Make loom dependency optional r=jonhoo a=taiki-e The loom dependency will never compile unless users enable cfg, but the way cargo and crates.io handle cfg-ed dependencies is not very good and confuses users. This issue seems to work around by the way proposed by @matklad in #487 (comment). So this PR uses that workaround. Closes #665 r? @jeehoonkang @jonhoo Co-authored-by: Taiki Endo <te316e89@gmail.com>

@jeehoonkang

658: Rename cfg(loom_crossbeam) to cfg(crossbeam_loom) r=jonhoo a=taiki-e It matches with the convention that *will* be documented in `loom`. See crossbeam-rs/crossbeam#487 (comment) for more. r? @jeehoonkang @jonhoo Co-authored-by: Taiki Endo <te316e89@gmail.com>

@matklad

666: Make loom dependency optional r=jonhoo a=taiki-e The loom dependency will never compile unless users enable cfg, but the way cargo and crates.io handle cfg-ed dependencies is not very good and confuses users. This issue seems to work around by the way proposed by @matklad in crossbeam-rs/crossbeam#487 (comment). So this PR uses that workaround. Closes #665 r? @jeehoonkang @jonhoo Co-authored-by: Taiki Endo <te316e89@gmail.com>

jonhoo mentioned this pull request Apr 8, 2020

Missing types from std that we should probably support tokio-rs/loom#117

Open

11 tasks

jonhoo mentioned this pull request Apr 8, 2020

Use loom in the tests jonhoo/flurry#34

Open

cynecx reviewed Apr 8, 2020

View reviewed changes

crossbeam-epoch/src/lib.rs Show resolved Hide resolved

jonhoo mentioned this pull request Apr 11, 2020

lazy_static: very rare (?) BorrowMut panic tokio-rs/loom#126

Closed

jonhoo force-pushed the loom branch from d9e4961 to 03e04ba Compare April 14, 2020 20:08

taiki-e reviewed Apr 18, 2020

View reviewed changes

crossbeam-epoch/src/lib.rs Show resolved Hide resolved

crossbeam-utils/Cargo.toml Outdated Show resolved Hide resolved

jonhoo mentioned this pull request May 3, 2020

Relax racy fairness test #495

Merged

jonhoo force-pushed the loom branch from 03e04ba to 9dc2226 Compare May 19, 2020 20:53

jonhoo force-pushed the loom branch from 9dc2226 to ecd63ff Compare May 23, 2020 13:57

jonhoo force-pushed the loom branch from ecd63ff to 0101676 Compare May 23, 2020 14:02

Merge branch 'master' into loom

bcf0c68

taiki-e mentioned this pull request Dec 30, 2020

Switch from lazy_static to once_cell #623

Closed

taiki-e added 3 commits December 30, 2020 23:27

newline at eol

660641c

remove needless pub(crate)

8638951

remove needless #![cfg(not(loom_crossbeam))]

5d8304d

taiki-e merged commit e4c6650 into crossbeam-rs:master Dec 30, 2020

jonhoo deleted the loom branch December 30, 2020 16:23

taiki-e mentioned this pull request Feb 13, 2021

Rename cfg(loom_crossbeam) to cfg(crossbeam_loom) #658

Merged

taiki-e mentioned this pull request Feb 13, 2021

Prepare for the next release #659

Merged

This was referenced Feb 23, 2021

Hide loom from dependency graph #665

Closed

Make loom dependency optional #666

Merged

taiki-e mentioned this pull request Oct 13, 2021

[Question] Integration with loom or shuttle? smol-rs/event-listener#23

Closed

taiki-e mentioned this pull request Jun 11, 2022

Loom support in crossbeam-deque #846

Open

taiki-e mentioned this pull request Sep 19, 2022

Add alternate implementations of synchronization primitives smol-rs/concurrent-queue#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preliminary support for loom #487

Add preliminary support for loom #487

jonhoo commented Apr 8, 2020

jonhoo commented Apr 8, 2020

cynecx commented Apr 8, 2020

jonhoo commented Apr 8, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 10, 2020

jonhoo commented Apr 12, 2020

jonhoo commented Apr 12, 2020

jonhoo commented Apr 14, 2020

taiki-e commented Apr 14, 2020

jonhoo commented Apr 14, 2020

jonhoo commented Apr 14, 2020

taiki-e commented Apr 18, 2020

cynecx commented May 9, 2020

jonhoo commented May 19, 2020

jonhoo commented May 19, 2020

jeehoonkang commented May 23, 2020

jonhoo commented May 23, 2020

jonhoo commented May 23, 2020

jonhoo commented Dec 22, 2020

taiki-e commented Dec 22, 2020

taiki-e commented Dec 22, 2020

taiki-e commented Jan 4, 2021 •

edited

jonhoo commented Jan 5, 2021

taiki-e commented Feb 13, 2021

matklad commented Feb 22, 2021

taiki-e commented Feb 22, 2021

matklad commented Feb 22, 2021

taiki-e commented Feb 22, 2021

taiki-e commented Feb 23, 2021 •

edited

Add preliminary support for loom #487

Add preliminary support for loom #487

Conversation

jonhoo commented Apr 8, 2020

jonhoo commented Apr 8, 2020

cynecx commented Apr 8, 2020

jonhoo commented Apr 8, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 9, 2020

jonhoo commented Apr 10, 2020

jonhoo commented Apr 12, 2020

jonhoo commented Apr 12, 2020

jonhoo commented Apr 14, 2020

taiki-e commented Apr 14, 2020

jonhoo commented Apr 14, 2020

jonhoo commented Apr 14, 2020

taiki-e commented Apr 18, 2020

cynecx commented May 9, 2020

jonhoo commented May 19, 2020

jonhoo commented May 19, 2020

jeehoonkang commented May 23, 2020

jonhoo commented May 23, 2020

jonhoo commented May 23, 2020

jonhoo commented Dec 22, 2020

taiki-e commented Dec 22, 2020

taiki-e commented Dec 22, 2020

taiki-e commented Jan 4, 2021 • edited

jonhoo commented Jan 5, 2021

taiki-e commented Feb 13, 2021

matklad commented Feb 22, 2021

taiki-e commented Feb 22, 2021

matklad commented Feb 22, 2021

taiki-e commented Feb 22, 2021

taiki-e commented Feb 23, 2021 • edited

taiki-e commented Jan 4, 2021 •

edited

taiki-e commented Feb 23, 2021 •

edited