ARROW-17350: [C++] Create a scheduler for asynchronous work #13912
Conversation
|
This is a draft, there is still a flaky test and a deadlock in the exec plans but those should be pretty straightforward to address. I was able to replace the dataset writer's scheduling with the new AsyncTaskScheduler and all dataset writer tests are passing so I think the scheduler itself is getting pretty solid. |
|
|
| /// If the scheduler is in an ended state then this call will cause an abort. This | ||
| /// represents a logic error in the program and should be avoidable. |
There was a problem hiding this comment.
I wonder if we should error instead just so that a bug doesn't, say, tear down a notebook kernel or crash the R interpreter?
There was a problem hiding this comment.
I can change it to a warning. It is "pretty safe" to just ignore the task. However, once a scheduler has ended the scheduler, and any resources tied to it, are effectively available for deletion. So even if we don't abort here it is likely that there will be some dangling references/pointers that will explode later on.
There was a problem hiding this comment.
Hmm, in that case it might be better to crash in a controlled fashion. Maybe adding a message to the DCHECK to give some context would help?
There was a problem hiding this comment.
I added a message to the dcheck and added a lengthier explanation in a comment before the dcheck which will hopefully assist future developers.
There is potentially one scenario where adding a task after the scheduler has ended is ok and this is where the code doing the task-adding is a task on the scheduler itself. This scenario popped up in the tpc-h generator but I was able to work around it. For the moment I'd like to keep things as they are but I'm open to possibly softening this requirement down the road.
It's may be possible to get of End entirely by requiring that all tasks are added by existing tasks. Creating the initial scheduler would then require supplying an "initial_task". In that case we would know that a scheduler is finished when it runs out of tasks and would never need an End call. I'll leave that for a follow-up (ARROW-17509)
westonpace
force-pushed
the
experiment/task-group
branch
from
ba573ab to
317368d
Compare
|
The ASAN error seems legitimate but I believe I've addressed review comments and fixed most of the bugs. This is probably ready for another review while I work through this issue. |
westonpace
force-pushed
the
experiment/task-group
branch
from
65efaff to
ee646b0
Compare
|
Sorry, let this linger a bit, I'm going to rebase and then merge if still green. |
… of existing utilities for managing async task lifetime.
…ch ensures that it is called at the end, after all the write batch calls have been made
…n ended scheduler. Fixed bug in tee node pointed out in code review
… scheduler has been ended.
westonpace
force-pushed
the
experiment/task-group
branch
from
ee646b0 to
f6b3491
Compare
|
Benchmark runs are scheduled for baseline = a8bb7f4 and contender = 0527197. 0527197 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…3912) Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>
…3912) Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>
No description provided.