Introduce crossbeam-deque

[ghost]

This is a proposal to introduce crate crossbeam-deque with a work-stealing Chase-Lev
deque implementation that uses Crossbeam's new epoch-based GC.

If you're using a similar deque implementation right now and have any thoughts, comments, or requests for new functionality, now is a great time to say so. :)

Rendered
Implementation

cc @carllerche @nikomatsakis @cuviper

[jeehoonkang]

I'm almost approving this RFC. I have only one comment on the function signature of Stealer::steal().

[jeehoonkang]

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?

[ghost]

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.

[mitchmindtree]

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 :)

[ghost]

@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.

[mitchmindtree]

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?

[cuviper]

I don't have strong opinions about the API as far as Rayon is concerned, as long as we can still do the same things and the performance is similar or improved.

The minimum capacity is interesting. We could expose that indirectly in our Configuration builder for the ThreadPool, but I'm not sure how we should choose that in general. Since the JobRef is small, only two pointers, we might do well enough with just about a page of memory.

[Vtec234]

Looks great! One thing from me, now that we have a generic Collector-based API, maybe it would be good to add a constructor for deque from a custom Collector or Handle?

[carllerche]

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

[carllerche]

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

[jeehoonkang]

I agree with @Vtec234 on the interface: I'd like to have a Collector-based API for crossbeam crates. @stjepang Would you please say some words on the interface in the unresolved questions section?

Except for that and the interface of steal(), I'm basically approving this RFC. Thanks!

[ghost]

@jeehoonkang

I still believe we should keep the Steal<T> enum. Here's why.

When you call try_recv() to send an element into a channel, there are three outcomes:

1. An element was received.
2. The channel was empty.
3. The channel is disconnected.

The first case is clearly a success (Ok), and the third one is clearly a failure (Err). We'll disagree on the second case, though. :) I see it as a failure because the receive operation failed to receive an element. You might say it's a success because the channel wasn't disconnected. That's also a valid position.

The situation is even more confusing with concurrent deques. When you call steal(), there are three possible outcomes:

1. An element was stolen.
2. The deque was empty.
3. Inconsistent state observed.

Again, the first case is a clear success. The second case is, again, a success or a failure depending on how you think about it. Regarding the third case, you might say it's a failure because essentially nothing happened. But, I would argue, the third case can also be viewed as a "neutral" outcome (nothing happened, you should just retry), while the second case is a failure (failed to steal an element because the deque is empty).

In the end, whatever we decide is a success (Ok) and a failure (Err), someone will find the choice unintuitive. And I think this impairs readability, so when you see a line like this one, it's not completely obvious what exactly happens:

let msg = stealer.steal()?;

But if you're matching on an enum, there's no confusion at all:

let msg = match stealer.steal() {
    Steal::Empty => None,
    Steal::Data(d) => Some(d),
    Steal::Inconsistent => return Err(whatever),
};

Of course, this is much more verbose.

I appreciate @mitchmindtree's concerns about ergonomics. Since channels are very popular, it makes sense to discuss how to make their use as ergonomic as possible.

But concurrent deques are different - they are used very infrequently, so in their case, I believe we should pursue clarity over ergonomics instead.

There is already some precedent for choosing an explicit enum instead of relying on Result: an ancient version of deque in Rust's runtime, the deque crate, current Crossbeam, and the mpsc implementation (see here) in the standard library. I think it worked out quite well.

[jeehoonkang]

In the end, whatever we decide is a success (Ok) and a failure (Err), someone will find the choice unintuitive.

I thought the empty case is definitely a success, but it turns out that someone (you, at least) disagrees with me. In that case, I agree with you in choosing clarity over ergonomics or doing someone's "right" thing. Let's leave the Stealer::steal()'s interface as-is.

Anyway, I'd like to ask if you could put @mitchmindtree's and my argument for the changes to the interface in the RFC. Here are two more cents from me. I think Result<Option<T>, StealError> more clearly reveals the relation between Deque::pop()/steal() and Stealer::steal(): the result type of the latter is similar to that of the former, but it may have lost in a race (StealError). That relation makes me to think that the empty case is actually not an error for both Deque::pop()/steal() and Stealer::steal().

[glaebhoerl]

This seems quite similar to the questions around Futures and the ? operator (I think this was discussed in the RFC for the Try trait?), which similarly had a 3-way split when awaiting: got a value, got an error, or got a "try again next time", with the debate likewise being around whether that last one should count as an error, or what.

(Going by memory; I should dig it up but I'm lazy.)

Anyway, I think we could use a similar solution here as what I remember the resolution there being: go with the 3-way enum, and also have convenience methods on it to translate it to the different possible Result types, based on which things are to be considered errors in the given situation. This retains most of the convenience of using a Result, and arguably helps readability as well. So you'd write something like (specific names only for the sake of demonstration!) stealer.steal().consistent()? (resulting in an Option<T>) where only inconsistency is an error, or stealer.steal().stole_something()? (resulting in a T) where emptiness is as well.

[ghost]

Anyway, I'd like to ask if you could put @mitchmindtree's and my argument for the changes to the interface in the RFC.

Will add to the text.

Anyway, I think we could use a similar solution here as what I remember the resolution there being: go with the 3-way enum, and also have convenience methods on it to translate it to the different possible Result types, based on which things are to be considered errors in the given situation.

Sounds like a great idea - best of both worlds!

I've managed to dig up a few relevant links to similar discussion around Poll in futures: [1] [2] [3] [4] [5] [6].

@alexcrichton Do you perhaps have an opinion here? Here's a quick explanation of the dilemma. Currently, we have the following API for the steal operation in a concurrent deque:

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

/// Possible outcomes of a steal operation.
pub enum Steal<T> {
    /// The deque was empty.
    Empty,
    /// Some data was stolen.
    Data(T),
    /// Inconsistent state was encountered. Try again.
    Inconsistent,
}`

We're wondering whether method steal should return Result<Option<T>, Inconsistent> instead. This is kinda similar to Poll in futures. Although in our case ergonomics around error handling are a low-priority concern, I'm still wondering what is the right thing to do here. What do you think?

[alexcrichton]

@stjepang the primary use case for Result in poll for futures was to enable ? syntax, and it sounds like that may not be a primary concern here perhaps? (no need to enable the ? syntax that is).

We actually started out with enum Poll<T> { ... } so I'd probably go for the bare enum route myself if you don't need to use ? that much

[ghost]

Let's go with the explicit enum then. I've added a remark on Steal vs Result to the Alternatives section.

Also, after chatting with @jeehoonkang, I've renamed Inconsistent to Retry.