Change TaskRunner to limit context switches. #5532
Conversation
| @@ -56,8 +56,6 @@ abstract class Task( | |||
| /** Undefined unless this is in [TaskQueue.futureTasks]. */ | |||
| internal var nextExecuteNanoTime = -1L | |||
|
|
|||
| internal var runRunnable: Runnable? = null | |||
|
|
|||
| /** Returns the delay in nanoseconds until the next execution, or -1L to not reschedule. */ | |||
There was a problem hiding this comment.
nit: Negative values < -1 seem to be effectively a priority, is that ok? Or should be an error?
Not a big concern given this is internal.
There was a problem hiding this comment.
Intended contract is for implementations to never return such values! If ever we wanted to make this more broadly usable we’d need to enforce that requirement.
| while (true) { | ||
| val task = synchronized(this@TaskRunner) { | ||
| awaitTaskToRun() | ||
| } ?: return |
There was a problem hiding this comment.
I think my biggest concern is that the delay of the time to execute a solitary task becomes the limiting time to execute a later (but soon) task on another queue.
Or am I missing something?
Do you schedule a kick for next next task time somewhere?
There was a problem hiding this comment.
Yeah, awaitTaskToRun might be misnamed. It’ll always schedule a thread if there’s something to do now (run or sleep) that competes with something to do later (run or sleep).
| var readyTask: Task? = null | ||
| var multipleReadyTasks = false | ||
|
|
||
| // Decide what to run. This loop's goal wants to: |
There was a problem hiding this comment.
Crazy Idea: Instead of this loop goal, could you store the next two scheduled times, and loop to find the third at this point?
There was a problem hiding this comment.
Maybe the question I should answer is, why two items?
The first one is what we’re going to execute on the current thread.
The second one justifies starting a new thread that’ll itself look for up to two items.
We stop after two because once we know we’re going to fork a thread, it becomes that thread’s problem to fork a thread for the third thing.
|
Overall, I'd love to see real world thread usage for this. Any thoughts of how it looks in a profiler? Or whether we could output something like the Google Chrome Trace Event format? |
|
Before & after effects of fewer context switches. These are measurements for the cost of 100 sequential empty tasks on the same queue. BEFOREAFTER |
|
Figuring out how to mature this up is the next challenge. The best way to do that is some real world exercise. I’m going to run our crawler a bunch, and maybe write some torture tests. |
swankjesse
force-pushed
the
jwilson.1004.new_task_runner
branch
from
b785000 to
264e932
Compare
Now we don't have to alternate between the coordinator thread and the task thread between task runs if the task returns 0. Instead the task thread can stay resident. This implementation works by having task runnables that can switch from the coordinator role (sleeping until the next task starts) and the executor role. #5512
swankjesse
force-pushed
the
jwilson.1004.new_task_runner
branch
from
264e932 to
ef4b5ec
Compare
| @@ -87,7 +81,7 @@ class TaskQueue internal constructor( | |||
| fun awaitIdle(delayNanos: Long): Boolean { | |||
| val latch = CountDownLatch(1) | |||
|
|
|||
| val task = object : Task("awaitIdle") { | |||
| val task = object : Task("awaitIdle", cancelable = false) { | |||
There was a problem hiding this comment.
this was the problem with flaky tests!
| @@ -134,6 +134,9 @@ class CallKotlinTest { | |||
| recordedRequest = server.takeRequest() | |||
| assertEquals("HEAD", recordedRequest.method) | |||
|
|
|||
| recordedRequest = server.takeRequest() | |||
| assertThat(recordedRequest.failure).isNotNull() | |||
There was a problem hiding this comment.
this fixes a green/green collision with two earlier pull requests
Now we don't have to alternate between the coordinator thread and the task
thread between task runs if the task returns 0. Instead the task thread can
stay resident.
This implementation works by having task runnables that can switch from
the coordinator role (sleeping until the next task starts) and the executor
role.
#5512