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 alternate implementations of synchronization primitives #27

Merged
merged 8 commits into from Oct 7, 2022
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Sep 11, 2022

This PR adds two alternate implementations of synchronization primitives:

  • One based on loom. This can be enabled using cfg(loom). Unfortunately there is no real way to test this, since loom tests require some kind of synchronization primitive to make progress.
  • One based on portable-atomic, enabled using the portable-atomic feature. This allows concurrent-queue to be used on platforms without available atomics.

Obsoletes #5

@taiki-e
Copy link
Collaborator

taiki-e commented Sep 12, 2022

Thanks! Could you add tests for cfg(loom)? When we did something similar before in crossbeam, unbounded queue panicked with odd errors like tokio-rs/loom#283.

@notgull
Copy link
Member Author

notgull commented Sep 12, 2022

I added a basic spsc test for Loom, but I keep bumping my head on this panic:

thread 'spsc' panicked at 'Model exceeded maximum number of branches. This is often caused by an algorithm requiring the processor to make progress, e.g. spin locks.', /home/jtnunley/.cargo/registry/src/github.com-1ecc6299db9ec823/loom-0.5.6/src/rt/path.rs:136:9

I'm not that familiar with loom, but is there a way around this? If I have the receiver thread wait on a Condvar, it complains about the thread being deadlocked and panics.

@notgull
Copy link
Member Author

notgull commented Sep 18, 2022

With some tinkering, I built a channel implementation with loom primitives based on concurrent_queue, specifically to use in this test. It looks like it works well enough to be tested.

Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • Could you add docs about portable-atomic feature? like atomic-waker's docs
  • I want to treat loom as an unstable cfg so docs in lib.rs is probably not needed, but could you document that it is unstable in Cargo.toml? like crossbeam-utils's Cargo.toml

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Sep 19, 2022

Thank you for taking the time to review, @taiki-e! I implemented the suggestions you made.

The only real issue here is that running the CI now takes significantly longer. Even with LOOM_MAX_PREEMPTIONS=2, it looks like it still takes a few minutes to run. It's not really that big of a deal, and we could probably even get away with using LOOM_MAX_PREEMPTIONS=3 if we really wanted to.

@notgull notgull merged commit 8c42b8d into master Oct 7, 2022
@notgull notgull deleted the loom branch October 7, 2022 13:48
@notgull notgull mentioned this pull request Nov 6, 2022
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.

None yet

2 participants