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

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

Closed
hozan23 opened this issue Jun 27, 2024 · 11 comments
Closed

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

hozan23 opened this issue Jun 27, 2024 · 11 comments

Comments

@hozan23
Copy link
Contributor

hozan23 commented Jun 27, 2024

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
Copy link
Member

zeenix commented Jun 27, 2024

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

@hozan23
Copy link
Contributor Author

hozan23 commented Jun 27, 2024

@zeenix I agree about the cost of using AtomicU64, but async-broadcast is 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
Copy link
Member

zeenix commented Jun 27, 2024

async-broadcast is 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
Copy link
Contributor Author

hozan23 commented Jul 3, 2024

@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
Copy link
Member

zeenix commented Jul 3, 2024

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
Copy link
Member

zeenix commented Jul 3, 2024

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

👍

@hozan23
Copy link
Contributor Author

hozan23 commented Jul 3, 2024

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
Copy link
Member

zeenix commented Jul 3, 2024

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:

  • Breaking API.
  • Locking us into taking non-mutable reference. Switching from &mut self to &self isn't a breakage but the other way around is. So if we decide in the future that we actually need mutability, we'll have a problem. Keep in mind, we want to go 1.0 soon (Expose async-broadcast in smol smol#310) of API break would be more an issue after that.

@hozan23
Copy link
Contributor Author

hozan23 commented Jul 3, 2024

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 (smol-rs/smol#310) of API break would be more an issue after that.

That's amazing news!

@hozan23
Copy link
Contributor Author

hozan23 commented Jul 3, 2024

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.

hozan23 added a commit to hozan23/async-broadcast that referenced this issue Jul 4, 2024
The `recv()`, `recv_direct()`, `recv_blocking()`, and `try_recv()`
methods currently require `&mut self` to modify the value of the
`position` field. By using AtomicU64 for the `position` field eliminates
the need for mutability.

Fixes issue smol-rs#66
hozan23 added a commit to hozan23/async-broadcast that referenced this issue Jul 16, 2024
The `recv()`, `recv_direct()`, `recv_blocking()`, and `try_recv()`
methods currently require `&mut self` to modify the value of the
`position` field. By using AtomicU64 for the `position` field eliminates
the need for mutability.

Fixes issue smol-rs#66
@hozan23
Copy link
Contributor Author

hozan23 commented Jul 18, 2024

I will close this issue due to the performance regression when using atomic operations. Check out the benchmarks in PR #70 for more details.

@hozan23 hozan23 closed this as completed Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants