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

Introduce crossbeam-deque #21

Merged
3 commits merged into from Nov 17, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 165 additions & 0 deletions text/2017-11-05-deque.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
# Summary

Introduce crate `crossbeam-deque` with an implementation of the Chase-Lev deque.

# Motivation

Work-stealing deque is a common building block in schedulers, for example in
[Rayon](https://github.com/nikomatsakis/rayon/) and
[futures-pool](https://github.com/carllerche/futures-pool).
Currently, these crates are using the deque from [Coco](https://github.com/stjepang/coco),
which is an experimental crate that will soon be deprecated in favor of Crossbeam.

`crossbeam-deque` will use the new Crossbeam's epoch-based garbage collector.

# Detailed design

The basis for implementation is the deque from Coco that already had a good run in
Rayon and consequently in Stylo. However, there will be a few differences:

1. Memory management is done using `crossbeam-epoch`.
2. The `Worker`/`Stealer` interface is redesigned as `Deque`/`Stealer`.
3. Several new convenience methods are introduced.

Other than that, the crucial (and the most tricky to implement) methods like
`push`, `pop`, and `steal` do not depart from the original implementation
(except for the changes associated with porting it to Crossbeam's new epoch-based GC).

## The interface

```rust
/// The "worker" side of the deque.
pub struct Deque<T>;

unsafe impl<T: Send> Send for Deque<T> {}

impl<T> Deque<T> {
/// Creates a new deque.
pub fn new() -> Deque<T>;

/// Creates a new deque with specified minimum capacity.
pub fn with_min_capacity(min_cap: usize) -> Deque<T>;

/// Returns `true` if the deque is empty.
pub fn is_empty(&self) -> bool;

/// Returns the number of elements in the deque.
pub fn len(&self) -> usize;

/// Pushes an element onto the bottom of the deque.
pub fn push(&self, value: T);

/// Pops an element from the bottom of the deque.
pub fn pop(&self) -> Option<T>;

/// Steals an element from the top of the deque.
pub fn steal(&self) -> Steal<T>;

/// Creates a new stealer for the deque.
pub fn stealer(&self) -> Stealer<T>;
}

/// The "stealer" side of the deque.
pub struct Stealer<T>;

unsafe impl<T: Send> Send for Stealer<T> {}
unsafe impl<T: Send> Sync for Stealer<T> {}

impl<T> Stealer<T> {
/// Returns `true` if the deque is empty.
pub fn is_empty(&self) -> bool;

/// Returns the number of elements in the deque.
pub fn len(&self) -> usize;

/// Steals an element from the top of the deque.
pub fn steal(&self) -> Steal<T>;
}

impl<T> Clone for Stealer<T> { ... }

/// Possible outcomes of a steal operation.
pub enum Steal<T> {
/// The deque was empty.
Empty,
/// Some data was stolen.
Data(T),
/// Lost the race for stealing data to another concurrent operation. Try again.
Retry,
}
```

An interesting difference from Coco's API is that a deque is now
constructed using `Deque::new()` rather than using a global function returning
a `(Worker<T>, Stealer<T>)` pair.
Also, there are two ways of creating multiple stealers: you can either create each of them
with `Deque::stealer` or by clone an existing one - whatever works best for you.

Another addition is the `with_min_capacity` constructor. Deques dynamically grow and
shrink as elements are inserted and removed. By specifying a large minimum capacity
it is possible to reduce the frequency of reallocations.

The `steal` method returns a `Steal<T>`. Instead of retrying on a failed CAS operation, the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly inclined to like the following interface better than Steal:

enum StealError {
    LostRace,
}

fn Stealer<T>::steal(&self) -> Result<Option<T>, StealError>;

Here, Ok(Some(val)) means it returns the value val and Ok(None) means the deque is empty; Err(LostRace) means the thread couldn't get any useful information from the deque since it lost a race over the deque. I think this application of Result and Option types aligns with the intended use cases of these types.

A drawback is it's a little bit verbose.

What do you think?

Copy link
Author

@ghost ghost Nov 6, 2017

Choose a reason for hiding this comment

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

Hmm, I don't know - both versions look fine to me. Let's see what others think.

However, one aspect of your method signature feels a bit unusual. Typically, the empty case is considered to be an error, not a special case of the success case. For example, in the mpsc channel, try_recv method has the following signature:

pub fn try_recv(&self) -> Result<T, TryRecvError>;

pub enum TryRecvError {
    Empty,
    Disconnected,
}

So the Ok case happens when an element is successfully received, and the Err case happens when the channel is either empty or disconnected. In your signature, steal results with Ok even if the deque is empty.

Choose a reason for hiding this comment

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

Typically, the empty case is considered to be an error

@stjepang I've always considered this to be a mistake in the case of try_recv. The method itself suggests that receiving both Some and None are expected behaviours and could be considered successful cases - after all, the point of calling try_recv instead of recv is because we know that there may not be value waiting.

I've found that this tends to make error handling frustrating. I often find myself wanting to write code like this:

while let Some(msg) = rx.try_recv()? {
    // handle msg
}

However the closest I can think of with the current API is significantly more verbose:

loop {
    match rx.try_recv() {
        Ok(msg) => {
            // handle msg
        },
        Err(mpsc::TryRecvError::Empty) => break,
        Err(err) => return Err(MyError::Disconnected),
    }
}

Also, notice how I use a custom error type in the second example too - this is because I rarely find myself wanting to wrap TryRecvError in my own error type, as the Empty variant is never really an "error" that I want to propagate up through my results in practise as it is almost always expected behaviour.

Anyway, this is just my two cents following personal experience - I could very well be using this wrong! Also I just want to quickly add I really appreciate all the work you're putting into crossbeam at the moment :)

Copy link
Author

Choose a reason for hiding this comment

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

@mitchmindtree Thanks for chiming in - you're presenting a very good argument!

While I agree that returning Ok(None) instead of Err(Empty) would make implementing some patterns easier, it's still slightly odd to return an Ok when a receive operation fails.

I mean, try_recv didn't recv a message, but it still returned an Ok? Hmmm... :)

Also, what if you want to receive a message from a channel, while asserting that it's not disconnected nor empty? You'd have to write:

let msg = rx.try_recv().unwrap().unwrap();

Aren't there too many layers of wrapping?

Perhaps this is a sign that we should keep the Steal enum instead of returning a Result? The steal operation is pretty unusual, so enumerating the entire list of cases on each call would make the intent of the code more obvious.

Furthermore, concurrent deques and, consequently, calls to steal are not very common in Rust programs (unlike channels and try_recv), so I believe there isn't a strong incentive to make the method as ergonomic as possible.

Choose a reason for hiding this comment

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

I mean, try_recv didn't recv a message, but it still returned an Ok? Hmmm... :)

Hmmm I find it interesting that you bring this up as a point against, as this seems like the intuitive behaviour to me 😄

In my view, try_recv exists in order to avoid blocking when no values are present, so receiving None is often the expected, "successful" (Ok) result - I haven't personally come across a case yet where I'd describe the channel being empty as an "error" in the flow of my code or unexpected behaviour, as it seems to me this is the point of using try_recv instead of recv in the first place.

let msg = rx.try_recv().unwrap().unwrap();

Aren't there too many layers of wrapping?

In this case I'd argue that perhaps the user should be using recv if they are expecting a value to exist and panic!ing otherwise, e.g. rx.recv().unwrap()?

On the other hand, if a user really did come across a use-case for this, I'd imagine this would be more common in practise rx.try_recv()?.unwrap().

Perhaps this is a sign that we should keep the Steal enum instead of returning a Result?

Admittedly I'm not very familiar with this proposal - I just happened to come across your reference to the try_recv API and it sparked some frustrating memories :) Perhaps I'd be better off raising this as an alternative ergonomic approach in #22?

method returns immediately with `Steal::Inconsistent`. This gives schedulers
fine control over what to do in case of contention.

There is an open [pull request](https://github.com/crossbeam-rs/crossbeam-deque/pull/1)
implemented according to this RFC.

# Drawbacks

None.

# Alternatives

### Hazard pointers

Ultimately, hazard pointers would probably be a better fit for the deque than
epoch-based garbage collection.

In theory, epoch-based GC may leak memory indefinitely if a pinned thread is preempted,
while hazard pointers give stricter guarantees. HP-based GC guarantees that there
is always a bounded number of still-unreclaimed garbage objects.
On the other hand, the advantage of epoch-based GC is that it allows fast
traversal through linked data structures. The Chase-Lev deque isn't a linked data
structure, so here we don't gain anything by choosing epoch-based GC over HP-based GC.

Moreover, HP-based GC would allow us to force perfectly eager destruction of
buffers: just like `Arc`, but without the overhead of contended reference counting.
The idea is that when a thread wants to destroy a buffer, it would check
all hazard pointers to see if it is still used. If there is a thread using it,
we'd CAS the hazard pointer using the buffer and set it to null. When that thread
notices that someone has set its hazard pointer to null, it would take over the
responsibility of destroying the buffer. Then it would check all hazard pointers to see if
anyone is still using it and continue in the same vein.

At some point in the future, we should definitely experiment with HP-based
garbage collection.

### Signature of `Stealer::steal`

Instead of returning an explicit enum `Steal<T>`, the `steal` function could also
return a `Result`, in one of the following two forms:

1. `Result<Option<T>, StealError>`, where `StealError` is equivalent to `Steal::Retry`.
2. `Result<T, StealError>`, where `StealError` is an enumeration of `Empty` and `Retry`.

Returning a `Result` would allow one to use it with the `?` operator and all
the other commonly used combinators. However, since stealing is a rather unusual
and rarely used operation, ergonomics are not of high priority in this situation.

# Unresolved questions

When building `futures-pool`, **[@carllerche](https://github.com/carllerche)**
wanted a few additional methods:

1. [A method that steals more than one value at a time.](https://github.com/stjepang/coco/issues/11#issuecomment-339785208)

Choose a reason for hiding this comment

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

Definitely interesting, but I agree that this isn't a requirement for moving forward and could be explored later.

2. [A `steal_when_greater` method.](https://github.com/stjepang/coco/issues/10#issuecomment-339785563)

Choose a reason for hiding this comment

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

This is pretty low in priority IMO. As you pointed you can approximate this with the len API.


While the lack of those is not a deal-breaker, it would still be nice to have them.

At the moment, I'd probably prefer to push forward with the current minimal design,
and then later on discuss how exactly these methods would work, possibly after (if?)
we switch to HP-based GC.

All that said, suggestions are always welcome.