-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implementation of task_group dynamic dependencies - part 4 - support for task cancellation #1788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…/kboyarinov/poc-dynamic-dependencies-part3
…/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part3' into dev/kboyarinov/poc-dynamic-dependencies-cancellation-support
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part2.5' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part3' into dev/kboyarinov/poc-dynamic-dependencies-cancellation-support
// TODO: complete_and_try_get_successor returns one ready successor task, others are spawned and cancelled by the scheduler | ||
// Should cancel() be called directly instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tradeoff is not obvious. Spawning carries some overhead but allows finalization to be done by multiple threads. And we cannot predict how many tasks are there for finalization. Perhaps some experimentation is needed to see how many (in the order of magnitude) tasks are needed for the break-even point between the two approaches.
#if __TBB_PREVIEW_TASK_GROUP_EXTENSIONS | ||
// TODO: complete_and_try_get_successor returns one ready successor task, others are spawned and cancelled by the scheduler | ||
// Should cancel() be called directly instead? | ||
task_ptr = this->complete_and_try_get_successor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of cancellation, do we even need to preserve the dependencies? Can the dependency counting to be fully skipped, and instead some marker in task_dynamic_state
put in place of the task pointer to indicate that the task has been taken for cancellation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is not that straightforward to implement. Consider submitting the predecessor task_handle and not submitting the successor handle with the cancelled execution.
Since there are not dependencies for the predecessor task, it would be cancelled by the scheduler. Yes, we can mark the successor list of the predecessor as completed (or using the special flag), but what should we do with the successor list.
I don't think that cancelling all of the tasks without dependency counting is a good idea, since the successor would be cancelled before being submitted for execution in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the "correct" support for cancellation needs to be reconsidered in a similar way with the partial graph destruction. Imagine cancelling some of the tasks and then resetting the task_group_context
.
The idea of this patch is to avoid hangs on waiting if the execution is cancelled as a "preliminary" support for cancellation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we have more open questions to add to the design RFC it seems.
…dencies-part1' into dev/kboyarinov/poc-dynamic-dependencies-part3
…dencies-part3' into dev/kboyarinov/poc-dynamic-dependencies-cancellation-support
src/tbb/task_dispatcher.h
Outdated
@@ -338,6 +338,7 @@ d1::task* task_dispatcher::local_wait_for_all(d1::task* t, Waiter& waiter ) { | |||
|
|||
ITT_CALLEE_ENTER(ITTPossible, t, itt_caller); | |||
|
|||
m_innermost_running_task = t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same double setting of m_innermost_running_task issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased to part1 after merging the transferring support. Resolved.
…dencies-part1' into dev/kboyarinov/poc-dynamic-dependencies-cancellation-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest (at some point) adding a test that runs at least some part of the task graph before the task_group is canceled. So probably, cancel from within a task at an intermediate level.
cde849b
into
dev/kboyarinov/poc-dynamic-dependencies-part1
Description
Improve
function_task::cancel
to propagate the cancellation signalFixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information