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.

Sign up for GitHub

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

Jump to bottom

Add loom support to crossbeam-epoch #486

Closed
jonhoo opened this issue Apr 8, 2020 · 4 comments · Fixed by #487
Closed

Add loom support to crossbeam-epoch #486

jonhoo opened this issue Apr 8, 2020 · 4 comments · Fixed by #487

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Apr 8, 2020

It'd be really cool if crossbeam-epoch had support for loom. That would allow us to write more reliable tests where every execution interleaving and atomic visibility case actually gets exercised, without resorting to some of the "spin up a bunch of threads" tests we have today. It would also allow downstream crates to write similar tests on top of crossbeam-epoch, such as in flurry where we need loom to exercise some tricky contention cases that are hard to hit reliably otherwise.

The loom README has a decent amount of info on how to go about adding loom support, but it basically boils down to using types from loom instead of from std (or core) when cfg(loom). Then, for tests, you just surround the test with loom::model(|| {}), and loom will automatically run the closure under different concurrent execution schedules.

/cc @jeehoonkang @carllerche @domenicquirl
@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

The sanitize feature is another example of something that could probably go away if we had loom support.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

One tricky thing we'd have to figure out is how to expose loom support to downstream crates. If we make it a cfg(loom) the way the loom docs suggest, then I think we'd have to make loom a non-optional dependency since you can't set per-cfg dependencies. Alternatively, we just make loom a feature (like sanitize) — not sure why that's not what the loom docs propose.

@jonhoo jonhoo mentioned this issue Apr 8, 2020
Open
14 tasks
@cynecx
Copy link
Contributor

cynecx commented Apr 8, 2020

Just wondering, does loom support atomic::fence?

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

Yup, though not compiler_fence.

jonhoo added a commit to jonhoo/crossbeam that referenced this issue Apr 8, 2020
@jonhoo
Add preliminary support for loom
64a643c 
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 crossbeam-rs#486.
@jonhoo jonhoo mentioned this issue Apr 8, 2020
Merged
jonhoo added a commit to jonhoo/crossbeam that referenced this issue Apr 14, 2020
@jonhoo
util/epoch: Add preliminary support for loom
03e04ba 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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](https://caniuse.rs/features/alloc), yet we use it if someone does
a `no_std` build with `--feature alloc`..

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 crossbeam-rs#486.
jonhoo added a commit to jonhoo/crossbeam that referenced this issue May 19, 2020
@jonhoo
util/epoch: Add preliminary support for loom
9dc2226 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

The minimal version bump to 1.30 is a little awkward, but is needed to
allow us to use paths for macro imports. Now, technically we already
rely on MSRV above 1.30 for a bunch of features (like `alloc`), but this
is a change that affects all of `crossbeam-epoch`.

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 crossbeam-rs#486.
jonhoo added a commit to jonhoo/crossbeam that referenced this issue May 23, 2020
@jonhoo
util/epoch: Add preliminary support for loom
ecd63ff 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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 crossbeam-rs#486.
jonhoo added a commit to jonhoo/crossbeam that referenced this issue May 23, 2020
@jonhoo
util/epoch: Add preliminary support for loom
0101676 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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 crossbeam-rs#486.
jonhoo added a commit to jonhoo/crossbeam that referenced this issue May 23, 2020
@jonhoo
util/epoch: Add preliminary support for loom
4756ee1 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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 crossbeam-rs#486.
tomtomjhj pushed a commit to tomtomjhj/crossbeam that referenced this issue Dec 4, 2020
@jonhoo @tomtomjhj
util/epoch: Add preliminary support for loom
409cddd 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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 crossbeam-rs#486.
taiki-e pushed a commit that referenced this issue Dec 30, 2020
@jonhoo
Add preliminary support for loom (#487)
e4c6650 
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.

For this initial patch, there are two loom tests, both in `epoch`. One
is a simple test of defer_destroy while a pin is held, and the other is
the Triber stack example. They both pass loom with
`LOOM_MAX_PREEMPTIONS=3` and `LOOM_MAX_PREEMPTIONS=2`. The latter tests
fewer possible interleavings, but completes in 13 minutes on my laptop
rather than ~2 hours. I have added loom testing of `epoch` to CI as
well.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add preliminary support for loom jonhoo/crossbeam
2 participants
@jonhoo @cynecx