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

Add preliminary support for loom #487

Merged
merged 18 commits into from
Dec 30, 2020
Merged

Add preliminary support for loom #487

merged 18 commits into from
Dec 30, 2020

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 8, 2020

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
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

/cc @cynecx since it seemed you were interested

@cynecx
Copy link
Contributor

cynecx commented Apr 8, 2020

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

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
Copy link
Contributor Author

jonhoo commented Apr 9, 2020

I added one! And interestingly enough, it fails with

Causality violation: Concurrent load and mut accesses.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 9, 2020

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 9, 2020

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
Copy link
Contributor Author

jonhoo commented Apr 10, 2020

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
Copy link
Contributor Author

jonhoo commented Apr 12, 2020

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 12, 2020

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 14, 2020

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
Copy link
Member

taiki-e commented Apr 14, 2020

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 14, 2020

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
Copy link
Contributor Author

jonhoo commented Apr 14, 2020

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

crossbeam-epoch/src/lib.rs Show resolved Hide resolved
crossbeam-utils/Cargo.toml Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented Apr 18, 2020

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.

@jonhoo jonhoo mentioned this pull request May 3, 2020
@cynecx
Copy link
Contributor

cynecx commented May 9, 2020

Ping.

@jonhoo
Copy link
Contributor Author

jonhoo commented May 19, 2020

@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
Copy link
Contributor Author

jonhoo commented May 19, 2020

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
Copy link
Contributor

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

@jonhoo
Copy link
Contributor Author

jonhoo commented May 23, 2020

@jeehoonkang rebased!

@jonhoo
Copy link
Contributor Author

jonhoo commented May 23, 2020

Hey, it works again! 🎉

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 22, 2020

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
Copy link
Member

taiki-e commented Dec 22, 2020

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

@taiki-e
Copy link
Member

taiki-e commented Dec 22, 2020

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

@taiki-e taiki-e merged commit e4c6650 into crossbeam-rs:master Dec 30, 2020
@jonhoo jonhoo deleted the loom branch December 30, 2020 16:23
@taiki-e
Copy link
Member

taiki-e commented Jan 4, 2021

  • 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
Copy link
Contributor Author

jonhoo commented Jan 5, 2021

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
Copy link
Member

taiki-e commented Feb 13, 2021

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

bors bot added a commit that referenced this pull request Feb 14, 2021
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>
bors bot added a commit that referenced this pull request Feb 20, 2021
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
Copy link
Contributor

matklad commented Feb 22, 2021

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
Copy link
Member

taiki-e commented Feb 22, 2021

@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
Copy link
Contributor

matklad commented Feb 22, 2021

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
Copy link
Member

taiki-e commented Feb 22, 2021

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
Copy link
Member

taiki-e commented Feb 23, 2021

cfg-ed deps issue will be fixed by #666

bors bot added a commit that referenced this pull request Feb 23, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add loom support to crossbeam-epoch
5 participants