smol-rs / async-broadcast

Async broadcast channels
Apache License 2.0
151 stars 26 forks source link

Use `AtomicU64` instead of u64 for the position field in `Receiver`. #66

Open hozan23 opened 1 week ago

hozan23 commented 1 week ago

Hello,

I noticed that the only reason recv(), recv_direct(), and try_recv() need to take &mut self to modify the value of self.pos. If AtomicU64 is used instead, there will be no need to use &mut self there.

zeenix commented 1 week ago

Thanks. Atomics always have a cost so best to leave it to the user if they're ok with paying that cost.

hozan23 commented 1 week ago

@zeenix I agree about the cost of using AtomicU64, but async-broadcastis generally used for managing many receivers that are shared across many tasks. As a user, I would end up wrapping all the receivers with a Mutex, which would be more expensive than just using AtomicU64 for pos field, no?

zeenix commented 1 week ago

async-broadcastis generally used for managing many receivers that are shared across many tasks.

I'd believe so (even though I don't have sufficient data to back it up), yes. However, each task can own a clone of the receiver and don't necessarily need to use interior mutability.

As a user, I would end up wrapping all the receivers with a Mutex,

If you need interior mutability in your design, yes.

which would be more expensive than just using AtomicU64 for pos field, no?

Perhaps but I don't know how they both compare in terms of performance. If you've some data on this, I'd be very curious to know.

hozan23 commented 5 days ago

@zeenix Sorry for the late response here.

I'd believe so (even though I don't have sufficient data to back it up), yes. However, each task can own a clone of the receiver and don't necessarily need to use interior mutability.

That's indeed true, but most of the time the task will be spawned inside a method of a struct, and usually the struct will own the receiver (that's how I use it :)). So, I think using interior mutability is important most of the time

Perhaps but I don't know how they both compare in terms of performance. If you've some data on this, I'd be very curious to know.

I will open a draft pull request soon with some benchmarks.

zeenix commented 5 days ago

I'd believe so (even though I don't have sufficient data to back it up), yes. However, each task can own a clone of the receiver and don't necessarily need to use interior mutability.

That's indeed true, but most of the time the task will be spawned inside a method of a struct, and usually the struct will own the receiver (that's how I use it :)).

But why does that necessitate interior mutability? You need mutability on the struct level then.

zeenix commented 5 days ago

I will open a draft pull request soon with some benchmarks.

👍

hozan23 commented 5 days ago

But why does that necessitate interior mutability? You need mutability on the struct level then.

Here is an example:

use std::sync::Arc;

struct Task {
    stop_signal: async_channel::Receiver<String>,
    b_stop_signal: async_broadcast::Receiver<String>,
}

impl Task {
    async fn run(self: Arc<Self>) {
        smol::spawn({
            async move {
                loop {
                    let msg = self.stop_signal.recv().await;
                }
            }
        })
        .detach();
    }

    async fn run2(self: Arc<Self>) {
        smol::spawn({
            async move {
                loop {
                    // XXX this will not work.
                    let msg = self.b_stop_signal.recv().await;
                }
            }
        })
        .detach();
    }
}
zeenix commented 5 days ago

Here is an example:

Thanks but I don't need examples of cases where you need interior mutability. In my own project, I need interior mutability. My point was that it all depends on the design. You don't always have to put things in an Arc.

The question is how much faster are atomic locks compared to (async) mutexes/rwlocks to justify:

hozan23 commented 5 days ago

Thanks but I don't need examples of cases where you need interior mutability. In my own project, I need interior mutability. My point was that it all depends on the design. You don't always have to put things in an Arc.

Yes indeed, I was just showing an example of the need for interior mutability.

The question is how much faster are atomic locks compared to (async) mutexes/rwlocks to justify:

I am aware that there might be breaking changes in the API. I need to research more about that and conduct some benchmarks.

Keep in mind, we want to go 1.0 soon (https://github.com/smol-rs/smol/issues/310) of API break would be more an issue after that.

That's amazing news!

hozan23 commented 5 days ago

So if we decide in the future that we actually need mutability, we'll have a problem.

To be honest, I can't imagine any case where mutability is needed. async-channel doesn't need mutability and is used more widely than async-broadcast.