remove frequent global is_shutdown flag check on timer operations #5301
Conversation
Make timer driver's is_shutdown flag nonatomic and move it into the driver lock. On shtudown, the driver wake the all pending timer with shutdown error. Simillar to the tokio-rs#5300, but for timer driver not io driver. Refs: tokio-rs#5227, tokio-rs#5300
Darksonn
added
A-tokio
| @@ -126,7 +126,7 @@ impl StateCell { | |||
| fn when(&self) -> Option<u64> { | |||
| let cur_state = self.state.load(Ordering::Relaxed); | |||
|
|
|||
| if cur_state == u64::MAX { | |||
| if cur_state >= STATE_MIN_VALUE { | |||
There was a problem hiding this comment.
It seems like you've also included a fix for #5082 here?
There was a problem hiding this comment.
It seems so, though I discovered the issue just now. During working on this issue I messed up more in this module and cleared up most of them but this line survived. If you think it's better to not mix #5082 here I'll revert it.
| /// True if the driver is being shutdown. | ||
| pub(super) is_shutdown: AtomicBool, |
There was a problem hiding this comment.
I don't remember this being an atomic, and I assumed it wasn't when I made the comment that the time driver has a similar issue.
There was a problem hiding this comment.
That makes sense. It's questionable that this change improves performance at all. TBH I have no idea how to benchmark timers only.
|
I'll close this because it seems like it wasn't necessary after all. |
Make timer driver's is_shutdown flag nonatomic and move it into the driver lock. On shtudown, the driver wake the all pending timer with shutdown error.
Simillar to the #5300, but for timer driver not io driver.
Refs: #5227, #5300
Motivation
The idea is based on @carllerche's suggestion, and @Darksonn mentioned that the timer driver has a similar issue.
Solution
It's virtually same change with #5300 but for this case the flag was already an atomic flag so I'm not sure how much perf improvement it actually brings. I think it's worth on its own having same structure between timer and io drivers though. I tried to reuse benchmark I've used for #5300 by wrapping each tasks with
select!{biased; _ = sleep(1h)but it doesn't show any noticeable difference, which is not surprising as it's comparing atomic loads with (nonblocking) IO.Edit: "I believe it's worth" -> "I think it's worth" since "believe" sounds too strong. At this point This PR is more like refactoring than optimization.