Inline Sender into Address
#141
Conversation
thomaseizinger
requested review from
Restioson
and removed request for
Restioson
thomaseizinger
force-pushed
the
inline-sender
branch
from
a794e59
to
91e48f5
Compare
| @@ -47,11 +43,13 @@ where | |||
| pub fn priority(self, priority: u32) -> Self { | |||
| match self.inner { | |||
| Inner::Initial { | |||
| message, sender, .. | |||
| message, | |||
| chan: sender, | |||
There was a problem hiding this comment.
Note to self: Remove this extra binding.
I think that this would cause unintuitive behaviour. For instance, I'd expect the following to always send: let addr: Address<MyActor, Strong> = addr; // just asserting this for test purposes
tokio::task::spawn(addr.send(MyMessage));
drop(addr); |
Fair enough, so we need to keep a strong reference around in the |
| New { | ||
| msg: SentMessage<A>, | ||
| tx: Sender<A, Rc>, | ||
| chan: Arc<Chan<A>>, |
There was a problem hiding this comment.
I guess we should just replace this with tx again?
There was a problem hiding this comment.
You mean the variable name?
There was a problem hiding this comment.
I mean instead of holding the inner channel, we can just clone the sender, which is just chan + rc, right?
There was a problem hiding this comment.
There is no more Sender with this PR :)
There was a problem hiding this comment.
Ah, right! It would be Address then, I guess!
There was a problem hiding this comment.
I guess we could embed the Address yeah. Although that would be a cyclic dependency. It is not a problem but conceptually weird.
I am doing some other work on SendFuture atm so I'll go think about this once that is done.
There was a problem hiding this comment.
How exactly is it a circular dependency? Do you mean in terms of logic?
I am doing some other work on SendFuture atm so I'll go think about this once that is done.
Sure, no problem, and thanks! :)
| ord => ord, | ||
| }) | ||
| Some( | ||
| match Arc::as_ptr(&self.inner).cmp(&Arc::as_ptr(&other.inner)) { |
There was a problem hiding this comment.
Just to check, does this properly compare data ptrs and not vtables?
There was a problem hiding this comment.
According to the docs, yes: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.ptr_eq
There was a problem hiding this comment.
ptr::eq has this same issue, as far as I know. as_ptr might explicitly return the data ptr, but it might also return *const dyn _, which would not be enough to ensure data equality is being tested. See rust-lang/rust#80505 for more
There was a problem hiding this comment.
Interesting, I guess this also needs to be changed then.
|
I am going to close this for now, want to get some of the internal channel refactorings over the line first. |
Another step towards #126.
What is interesting about this patch-set is that it removes the reference count from
SendFutureandBroadcastFuture. I think this is okay?