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

Add Oneshot implementation #21

Closed
zzau13 opened this issue Nov 12, 2022 · 17 comments
Closed

Add Oneshot implementation #21

zzau13 opened this issue Nov 12, 2022 · 17 comments
Labels
discussion enhancement New feature or request

Comments

@zzau13
Copy link
Contributor

zzau13 commented Nov 12, 2022

From what you have, a async oneshot without a clone that I can use to synchronize pairs of threads would not be complicated.
And also features to be able to reduce the compilation time.
And also I can help!

@fereidani fereidani added enhancement New feature or request discussion labels Nov 12, 2022
@fereidani
Copy link
Owner

Great idea, Let me think about the API and the possibility of implementation for a few days. Other community members can discuss and think about it too.
I want to be sure that we can implement it in the same crate as Kanal. I don't like to add unnecessary complexity to the crate.

@fereidani
Copy link
Owner

I want to add these points to the issue, buffered one-shot and blocking one-shot channels are completely different in design.
To implement correctly, they should be implemented separately, and I'm unsure that Kanal is the right place to implement those.

to implement those I think we need:
4 public functions to generate channel's sides.
8 public structures for communication
AsyncOneShotSender AsyncOneShotReceiver
AsyncOneShotBufferedSender AsyncOneShotBufferedReceiver
SyncOneShotSender SyncOneShotReceiver
SyncOneShotBufferedSender SyncOneShotBufferedReceiver

@fereidani
Copy link
Owner

fereidani commented Nov 12, 2022

We can use an enum to merge these public structures into 4 types. but there will be an unknown performance impact.

@zzau13
Copy link
Contributor Author

zzau13 commented Nov 12, 2022

Its usage is a:

  • Sender with send -> Result<(), DroppedRecvError>
  • Receiver impl Future<Output = Result<(), DroppedSenderErrorAndNotSent>>

Exactly what I want for this. https://github.com/botika/unblock/blob/master/src/lib.rs#L132-L137
Send Sender to worker thread and return Receiver.

@fereidani
Copy link
Owner

I have an idea for a completely lock-free design for an unbuffered Oneshot channel.
I'm in the process of implementing it. In my calculations in the case of the one-shot channel application, it heavily outperforms Kanal's MPMC channel.

@fereidani
Copy link
Owner

@botika implementation added to the library but I will not release it to crates.io until we run some tests on it and complete the documentation.
Implementation is completely lock-free and will use only 8 bytes heap allocation on 64-bit systems to move data in between.
I added only the unbuffered one-shot so send is blocking/suspending operation too if the sender reaches the send function first which is not common when using one-shot channels.
Oneshot implementation benefits from the stack-to-stack copy that the MPMC channel is using too and there is no need to copy data again from the heap of the one-shot channel like other implementations.

@zzau13
Copy link
Contributor Author

zzau13 commented Jan 13, 2023

Fails

@fereidani
Copy link
Owner

fereidani commented Jan 14, 2023

OneshotReceiverFuture<T> in Kanal is not Unpin, which means it relies on it to be pinned to a memory location as it is doing stack-to-stack transfer to its field.

here you have two options:

using something like:

        // Safety: OneShotReceiverFuture will not be moved out of Join in any condition
        let fut = unsafe { self.as_mut().map_unchecked_mut(|this| &mut this.0) };
        Poll::Ready(
            match fut.poll(cx) {
                Poll::Ready(t) => t,
                Poll::Pending => return Poll::Pending,
            }
            .map_err(|_| Error),
        )

or doing the job with pin-project or pin-project-lite without any unsafe, which I recommend the second one.

@fereidani
Copy link
Owner

you also need to convert the sender and use the sync API: tx.to_sync().send($f());

@zzau13
Copy link
Contributor Author

zzau13 commented Jan 15, 2023

Looks good. But it has Miri error when test multiple thread:

MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-ignore-leaks" cargo miri test --all-features
...
 test test_thread ... error: Undefined Behavior: trying to retag from <wildcard> for SharedReadWrite permission at alloc1885061[0x8], but no exposed tags have suitable permission in the borrow stack for this location
   --> /home/lucky/.cargo/git/checkouts/kanal-35bf5e2ecaf10304/d3702c0/src/signal.rs:224:9
    |
224 |         (*this).ptr.copy(d);
    |         ^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <wildcard> for SharedReadWrite permission at alloc1885061[0x8], but no exposed tags have suitable permission in the borrow stack for this location
    |         this error occurs as part of retag at alloc1885061[0x8..0x10]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
    = note: BACKTRACE:
    = note: inside `kanal::signal::Signal::<()>::send_copy` at /home/lucky/.cargo/git/checkouts/kanal-35bf5e2ecaf10304/d3702c0/src/signal.rs:224:9
    = note: inside `kanal::signal::SignalTerminator::<()>::send_copy` at /home/lucky/.cargo/git/checkouts/kanal-35bf5e2ecaf10304/d3702c0/src/signal.rs:268:9
    = note: inside `kanal::oneshot::OneshotSender::<()>::send` at /home/lucky/.cargo/git/checkouts/kanal-35bf5e2ecaf10304/d3702c0/src/oneshot.rs:244:33
note: inside closure at /home/lucky/workspace/unblock/src/lib.rs:149:21
   --> /home/lucky/workspace/unblock/src/lib.rs:185:9
    |
185 |         run!(f in self)
    |         ^^^^^^^^^^^^^^^
    = note: inside `<[closure@unblock::Executor::spawn<(), fn() {sleep}>::{closure#0}] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/lucky/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:510:5
    = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /home/lucky/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2000:9
note: inside `unblock::Executor::main_loop` at /home/lucky/workspace/unblock/src/lib.rs:198:17
   --> /home/lucky/workspace/unblock/src/lib.rs:198:17
    |
198 |                 runnable();
    |                 ^^^^^^^^^^
note: inside closure at /home/lucky/workspace/unblock/src/lib.rs:242:36
   --> /home/lucky/workspace/unblock/src/lib.rs:242:36
    |
242 |                     .spawn(move || self.main_loop())
    |                                    ^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `run` (in Nightly builds, run with -Z macro-backtrace for more info)

@zzau13
Copy link
Contributor Author

zzau13 commented Jan 16, 2023

@fereidani
Copy link
Owner

fereidani commented Jan 16, 2023

I'm receiving error[E0425]: cannot find value 'EXECUTOR' in this scope error with Miri on your library, but it's passing the test for me. could you please retest with the latest commit?

@zzau13
Copy link
Contributor Author

zzau13 commented Jan 17, 2023

Need lazy feature for use in miri. Deadlock sometimes in slow systems https://github.com/botika/unblock/actions/runs/3934312687/jobs/6728941431

MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-ignore-leaks" cargo miri test --no-default-features --features mt,kanal,lazy

In my machine (ryzen 9) is all ok

@fereidani
Copy link
Owner

unfortunately, I can't reproduce either Miri error or deadlock, I'm going to write a fuzzer to see if it's possible to reproduce the problem that you are encountering, I'm stopping the pre8 release until this problem is solved.
If you discover any new information about the bug please share.

@fereidani
Copy link
Owner

seems fixed with the latest commit: https://github.com/fereidani/unblock/actions/runs/3941226089/jobs/6743356805

@zzau13
Copy link
Contributor Author

zzau13 commented Jan 18, 2023

Looks good in my test. Can you bump version?

@fereidani
Copy link
Owner

Thanks for finding the bug, it was a mistype by me.
I need to review the code once more and finish the documentation, then I run the benchmarks and release. I think the release will be in 2 or 3 days.
Feel free to close the issue, if you think it's solved.

@zzau13 zzau13 closed this as completed Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants