Execute IO.blocking on WSTP without BlockContext indirection
#3903
Conversation
| @@ -797,7 +797,7 @@ private final class WorkerThread( | |||
| * There is no reason to enclose any code in a `try/catch` block because the only way this | |||
| * code path can be exercised is through `IO.delay`, which already handles exceptions. | |||
| */ | |||
| override def blockOn[T](thunk: => T)(implicit permission: CanAwait): T = { | |||
| def prepareBlocking(): Unit = { | |||
| val rnd = random | |||
|
|
|||
| pool.notifyParked(rnd) | |||
There was a problem hiding this comment.
Unrelated, but I wonder why we invoke notifyParked() unconditionally, even if this thread is already blocking = true.
There was a problem hiding this comment.
Good question. I'm guessing there's no reason and we can probably guard that notification as you say.
| if (worker.canExecuteBlockingCodeOn(this)) | ||
| worker.prepareBlocking() | ||
| true |
There was a problem hiding this comment.
Can we move this logic up into IOFiber instead? I don't think there's any particular reason for it to be here, and you're making something that looks like a boolean check into a side-effecting method.
There was a problem hiding this comment.
Well conceptually I liked that if canExecuteBlockingCode() returns true then you can run blocking code, immediately.
I do see your point. We can expose this as a separate method which IOFiber calls. This requires:
-
Doing the whole
Thread.currentThread()dance a second time. How should we handle the case where the current thread is not aWorkerThread, or does not belong to the current pool? Do we care about those cases? -
Also exposing whatever new API we invent on the JS
WorkStealingThreadPoolshim. Not a big deal, just annoying.
Worth it?
There was a problem hiding this comment.
Oh lol, also I just realized this is bugged as-written 😅 fixed in c2ef8bc.
| @@ -797,7 +797,7 @@ private final class WorkerThread( | |||
| * There is no reason to enclose any code in a `try/catch` block because the only way this | |||
| * code path can be exercised is through `IO.delay`, which already handles exceptions. | |||
| */ | |||
| override def blockOn[T](thunk: => T)(implicit permission: CanAwait): T = { | |||
| def prepareBlocking(): Unit = { | |||
| val rnd = random | |||
|
|
|||
| pool.notifyParked(rnd) | |||
There was a problem hiding this comment.
Good question. I'm guessing there's no reason and we can probably guard that notification as you say.
We already have all the fantastic machinery to execute blocking code on a
WorkerThreadand it is pointless to indirect through theBlockContextinterface when we can just invoke this mechanism directly.