Integrated timers #3219
Integrated timers #3219
Conversation
Btw, I have an alternative |
|
Published as |
|
Ran a quick test on Ember. With this change, peak RPS on a basic GET request improved by a little over 25%. |
|
Um ... holy smokes! |
|
Did a little more testing. A slightly less trivial test involving a POST body and some JSON parsing (using Circe) shows peak RPS improvements around 14%, which makes intuitive sense since that test is going to be a little more bounded by the body processing than by the pure connection overhead. Still, 25% improvements in trivial GET peak RPS is pretty damn great. P99 latencies in both cases were improved by 13.5%. I think this number is probably a bit more trustworthy, and also still very impressive IMO. |
|
Did a quick run through the We can do a lot better than the implementation as it stands, but that can be incrementally layered onto this once it lands. |
|
TODO
|
| @@ -514,6 +522,52 @@ private[effect] final class WorkStealingThreadPool( | |||
| */ | |||
| override def reportFailure(cause: Throwable): Unit = reportFailure0(cause) | |||
|
|
|||
| override def monotonicNanos(): Long = System.nanoTime() | |||
|
|
|||
| override def nowMillis(): Long = System.currentTimeMillis() | |||
There was a problem hiding this comment.
Can this be plugble ,as what if I want to make use of https://github.com/OpenHFT/Chronicle-Ticker
There was a problem hiding this comment.
@He-Pin yes, you can always replace the Scheduler in the IORuntime with your own implementation. See also the Cats Effect test runtime, which supports time mocking.
https://typelevel.org/cats-effect/docs/core/test-runtime#mocking-time
|
Released as snapshot |
|
A few simple canaries were promising. At least, nothing exploded. We should look more closely. In the meantime, I think this is ready for more serious review. Note that there are definitely follow-ups we can explore which will absolutely micro-optimize this further. We can do that once this lands in |
I think one scenario affected by this is when (1) a compute thread is blocked/spinwaiting on a condition (without using In any case, this is a possible (if contrived) scenario, from which the current system can recover eventually (the timers are independent), but with this PR it can deadlock. (With timer stealing, I think it would also recover eventually, because the timer in question would get stolen.) I'm not sure how important supporting badly behaved code like this is, I just wanted to mention it. |
|
@durban So you're thinking about something like this? val flag = new AtomicBoolean(false)
IO(flag.set(true)).delayBy(2.seconds) &> IO(while (!flag.get()) {})I agree that, without timer stealing, the above can hang forever if the timer happens to be on the same thread as the |
|
@djspiewak Yeah, something like that. The |
| } | ||
|
|
||
| implicit val sleepCallbackReverseOrdering: Ordering[SleepCallback] = | ||
| Ordering.fromLessThan(_.triggerTime > _.triggerTime) |
There was a problem hiding this comment.
| Ordering.fromLessThan(_.triggerTime > _.triggerTime) | |
| Ordering.fromLessThan(_.triggerTime - _.triggerTime > 0) |
| while (cont) { | ||
| val head = sleepers.head() | ||
|
|
||
| if (head.triggerTime <= now) { |
There was a problem hiding this comment.
| if (head.triggerTime <= now) { | |
| if (head.triggerTime - now <= 0) { |
|
Btw, I had to make a small patch on my JVM polling branch in e44a802. |
| if (!isInterrupted()) { | ||
| val now = System.nanoTime() | ||
| val head = sleepersQueue.head() | ||
| val nanos = head.triggerTime - now |
There was a problem hiding this comment.
cherry-pick e44a802
| val nanos = head.triggerTime - now | |
| val nanos = Math.max(head.triggerTime - now, 0) |
| if (scheduler.isInstanceOf[WorkStealingThreadPool]) | ||
| scheduler.asInstanceOf[WorkStealingThreadPool].sleepInternal(delay, cb) | ||
| else | ||
| scheduler.sleep(delay, () => cb(RightUnit)) |
There was a problem hiding this comment.
Actually match is often slower because of how it gets compiled. The JIT fast-paths the sequential combination of a conditional jump branching on isInstanceOf, followed immediately by a dynamic cast, and it turns that into a single operation on most architectures. match is more declarative but can easily generate bytecode which messes up this JIT optimization, so in very hot-path code we tend to be more explicit about it.
|
|
||
| import java.util.concurrent.atomic.AtomicBoolean | ||
|
|
||
| private final class SleepCallback private ( |
There was a problem hiding this comment.
ScheduledCallback might be a better name here.
Dusted off @vasilmkd's old branch (first PRed in #2252) and merged it with the latest head. Still needs some work:
sleepimplementation inWorkStealingThreadPoolreferencesIORuntime.globaljust to get things to compile. This doesn't strike me as strictly necessary given the way thatsleepInternalworks, but trivially removing it got me into trouble.lazySet(false)onSleepCallback, which is concerning because it leaks memory. I believe this is why the fiber dump specs are failing.The runtime has changed meaningfully since Vasil's original implementation, so this definitely needs a bit of careful thought to ensure that we aren't doing anything weird or conflicty. Notably, this implementation simply doesn't implement timer stealing at all, and instead all timers are held local to their scheduling thread. We're going to need to work to validate the hypothesis that we can get away with this. If we need to implement theft, things become a lot more complex.
This is the first step in moving us toward a fully integrated runtime.