Specialize get_task for the case of no isolation#1805
Specialize get_task for the case of no isolation#1805
Conversation
akukanov
force-pushed
the
dev/get-task-no-isolation-akukanov
branch
from
2965223 to
c6870af
Compare
akukanov
force-pushed
the
dev/get-task-no-isolation-akukanov
branch
from
c6870af to
632adb6
Compare
|
|
||
| //! Checks if a given task is a proxy, then either extracts the real task or frees the proxy. | ||
| template<bool B = false> | ||
| static task* try_extract_task_from ( task* t, execution_data_ext& ed, std::integral_constant<bool, B> stolen = {} ) { |
There was a problem hiding this comment.
The line is too wide. Let's try keeping them not longer than ~100 characters.
| static task* try_extract_task_from ( task* t, execution_data_ext& ed, std::integral_constant<bool, B> stolen = {} ) { | |
| static task* try_extract_task_from ( task* t, execution_data_ext& ed, | |
| std::integral_constant<bool, B> stolen = {} ) { |
There was a problem hiding this comment.
Let me just remove a couple of extra spaces, so that it fits into 120 symbols. Given that in this very file there are lines with more than 140 symbols, and no consistent style for spaces around parentheses, I think that should be enough.
| //! Checks if a given task is a proxy, then either extracts the real task or frees the proxy. | ||
| template<bool B = false> | ||
| static task* try_extract_task_from ( task* t, execution_data_ext& ed, std::integral_constant<bool, B> stolen = {} ) { | ||
| __TBB_ASSERT(t && !is_poisoned(t), "Not a valid task pointer"); |
There was a problem hiding this comment.
try_extract_task_from seems to be always called from the places where t != nullptr. Perhaps, this function could accept a reference on the task instead. Thus, the dereferences would not be needed anywhere in this function, and the nullptr check would be naturally integrated into the function contract.
There was a problem hiding this comment.
It does not feel right to me. It seems like trading two pointer dereferences for getting a task address just as many times.
In general, these parts of the scheduler mostly deal with task pointers as data, not with tasks as objects. One can argue that operations with task_proxy need to access the object state and so to dereference the pointer - and that's true but this logic is encapsulated in task_accessor, and dereferencing is done when its functions are called. For this function, I prefer the semantics of "take what appears to be a valid task pointer, and return the pointer to the actual task. unwrapped from proxy if necessary".
| } | ||
| // Retrieve the task from local task pool | ||
| if (t || (slot.is_task_pool_published() && (t = slot.get_task(ed, isolation)))) { | ||
| if (t || (slot.is_task_pool_published() && (t = slot.get_task(ed, isolation, use_isolation)))) { |
There was a problem hiding this comment.
Probably, make this part a bit more readable?
| if (t || (slot.is_task_pool_published() && (t = slot.get_task(ed, isolation, use_isolation)))) { | |
| if ( !t && slot.is_task_pool_published() ) { | |
| t = slot.get_task(ed, isolation, use_isolation); | |
| } | |
| if ( t ) { |
There was a problem hiding this comment.
I instead moved the is_task_pool_published check into get_task.
| //------------------------------------------------------------------------ | ||
|
|
||
| d1::task* arena_slot::get_task(execution_data_ext& ed, isolation_type isolation) { | ||
| d1::task* arena_slot::get_task(execution_data_ext& ed, isolation_type isolation, std::false_type) { |
There was a problem hiding this comment.
Consider adding a comment describing what the last parameter mean for function definitions as well.
akukanov
force-pushed
the
dev/get-task-no-isolation-akukanov
branch
from
632adb6 to
2951d54
Compare
…y handling by owners and thieves
akukanov
force-pushed
the
dev/get-task-no-isolation-akukanov
branch
from
2951d54 to
3a7d027
Compare
3 participants
Description
After introducing in #1779 a couple of additional operations in
get_task(which is on the hot path in the task scheduler) that are only useful when the scheduler runs in the task isolation mode, I thought that maybe this function deserves two separate implementations, with and without isolation. This PR elaborates on that idea.Additionally, it unifies the handling of proxy tasks in
get_taskandsteal_taskinto a single routinetask_proxy::try_extract_task_from.Type of change
Tests
Documentation
Breaks backward compatibility?
Other information
The PR is developed on top of #1779, and so should either be merged afterwards or significantly changed if the base PR is rejected.