feat(driver,executor,runtime)!: remove notify-always#902
feat(driver,executor,runtime)!: remove notify-always#902Berrysoft wants to merge 14 commits intocompio-rs:masterfrom
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the notify-always feature flag and reworks wake/notify behavior so drivers are always eligible to be notified, while attempting to de-duplicate expensive driver notifications inside each driver implementation.
Changes:
- Removed the
notify-alwaysfeature wiring acrosscompio,compio-runtime, andcompio-executor. - Removed the runtime-side
OptWakeroptimization and switchedRuntime::block_onback to using the driver waker directly. - Added driver-side de-duplication (
AtomicBool awake) for wake notifications in polling / io-uring / IOCP drivers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| compio/Cargo.toml | Removes top-level notify-always feature exposure. |
| compio-runtime/src/waker/opt.rs | Deletes OptWaker implementation (optimized same-thread wake avoidance). |
| compio-runtime/src/waker/mod.rs | Drops the opt module + public re-export. |
| compio-runtime/src/lib.rs | Removes Runtime::opt_waker and uses the driver waker directly in block_on. |
| compio-runtime/Cargo.toml | Removes notify-always feature. |
| compio-executor/src/task/local.rs | Always wakes the configured waker when locally scheduling a task. |
| compio-executor/src/lib.rs | Updates ExecutorConfig docs by removing notify-always references (but doc intent now needs adjustment). |
| compio-executor/Cargo.toml | Removes notify-always feature. |
| compio-driver/src/sys/driver/poll/mod.rs | Adds awake flag to de-duplicate Poller::notify() calls while driver is active. |
| compio-driver/src/sys/driver/iour/notify.rs | Adds awake flag to de-duplicate eventfd writes. |
| compio-driver/src/sys/driver/iour/mod.rs | Sets/clears notifier awake state around CQ processing. |
| compio-driver/src/sys/driver/iocp/mod.rs | Adds awake flag to de-duplicate IOCP wake posting. |
Comments suppressed due to low confidence (2)
compio-runtime/src/waker/mod.rs:6
- Removing the
optwaker module and its public re-exports (OptWaker) is a breaking change forcompio-runtime's public API. If this is intentional, it should be called out explicitly (e.g., changelog/release notes) or replaced with an alternative API so downstream crates aren’t silently broken.
//! Waker related utilities.
mod ext;
pub(crate) use ext::*;
compio-runtime/src/lib.rs:204
Runtime::block_onno longer usesOptWaker/reset()to track same-thread wakes. Given the public API removals, please ensure the new driver-side wake de-duplication preserves the previous wake/liveness guarantees (especially around transitions intopoll()when no tasks remain), and consider adding a regression test for the scenario that originally motivatedOptWaker.
/// Block on the future till it completes.
pub fn block_on<F: Future>(&self, future: F) -> F::Output {
self.enter(|| {
let waker = self.waker();
let mut context = Context::from_waker(&waker);
let mut future = std::pin::pin!(future);
loop {
if let Poll::Ready(result) = future.as_mut().poll(&mut context) {
self.run();
return result;
}
// We always want to reset the waker here.
let remaining_tasks = self.run();
if remaining_tasks {
self.poll_with(Some(Duration::ZERO));
} else {
self.poll();
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @Berrysoft ! I think |
|
Thanks! @abh1nav10 |
b9a0686 to
00fbd71
Compare
00fbd71 to
204bd0a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
It finally works:) |
Closes #895
Previously, we don't wake the driver if the waker is on the same thread, because we assume that if
Waker::wake_by_refis called, the thread is not waiting for the driver. However, this is not always true:pollingcrate handlesEINTRand loops internally.winio.So I added the "notify-always" flag, making things even more complicated.
Actually we don't have to optimize it on the runtime side. We want to optimize it because it is heavy to notify the driver, so we only need to insert a bool flag beside the driver, and check it when woken.
As the bool flag is tightly coupled with the control flow, I have to repeat the logic 3 times...
UPDATE: I also removed the
OptWaker, because actually it is useless. If the waker is woken, the driver will be notified, and we don't have to deal with it separately.