Fix memory leak for static_partitioner #1404
Conversation
Signed-off-by: pavelkumbrasev <[email protected]>
Signed-off-by: pavelkumbrasev <[email protected]>
pavelkumbrasev
requested review from
aleksei-fedotov,
dnmokhov,
kboyarinov and
isaevil
|
Potentially, idle workers can drain tasks from free slots. |
| unsigned max_threads_in_arena = unsigned(std::min(static_cast<std::size_t>(max_concurrency()), | ||
| tbb::global_control::active_value(tbb::global_control::max_allowed_parallelism))); |
There was a problem hiding this comment.
Looks a little bit clumsy, don't you think? Does it make sense to move calculation of maximum_arena_concurrency into some dedicated function that could be reused in get_initial_auto_partitioner_divisor as well?
There was a problem hiding this comment.
I'm not sure. if need to introduce new method for these 2 lines. (we check not arena concurrency min(arena_concurrency, allowed_concurrency) but rather available amount of workers).
There was a problem hiding this comment.
I agree to move it into something like get_num_possible_workers() to highlight that the solution is based on the immediate value of workers as std::min(...) does not mean much to reader.
There was a problem hiding this comment.
Remove from affinity
This will require additional synchronization on mailbox due to concurrent access to it. So while this solution will solve all potential problems it also brings potential performance problems |
| tbb::global_control gbl_ctrl{ tbb::global_control::max_allowed_parallelism, std::size_t(tbb::this_task_arena::max_concurrency() / 2) }; | ||
|
|
||
| size_t current_memory_usage = 0, previous_memory_usage = 0, stability_counter = 0; | ||
| bool no_memory_leak = false; | ||
| std::size_t num_iterations = 100; | ||
| for (std::size_t i = 0; i < num_iterations; ++i) { | ||
| for (std::size_t j = 0; j < 100; ++j) { | ||
| tbb::parallel_for(0, 1000, [] (int) {}, tbb::static_partitioner{}); | ||
| } | ||
|
|
||
| current_memory_usage = utils::GetMemoryUsage(); | ||
| stability_counter = current_memory_usage==previous_memory_usage ? stability_counter + 1 : 0; | ||
| // If the amount of used memory has not changed during 5% of executions, | ||
| // then we can assume that the check was successful | ||
| if (stability_counter > num_iterations / 20) { | ||
| no_memory_leak = true; | ||
| break; | ||
| } | ||
| previous_memory_usage = current_memory_usage; | ||
| } | ||
| REQUIRE_MESSAGE(no_memory_leak, "Seems we get memory leak here."); |
There was a problem hiding this comment.
Wrap it into the loop with gradual decrease of the global_control's limit? E.g.,
std::size_t current_limit = std::size_t(tbb::this_task_arena::max_concurrency());
while (current_limit /= 2) {
tbb::global_control gc{ tbb::global_control::max_allowed_parallelism, current_limit };
// iterations loop goes here {
// repetitions loop goes here {
// }
// }
}| unsigned max_threads_in_arena = unsigned(std::min(static_cast<std::size_t>(max_concurrency()), | ||
| tbb::global_control::active_value(tbb::global_control::max_allowed_parallelism))); |
There was a problem hiding this comment.
I agree to move it into something like get_num_possible_workers() to highlight that the solution is based on the immediate value of workers as std::min(...) does not mean much to reader.
Description
global_controlwe limit concurrency that all the internal arenas can share between each other. Therefore, each particular arena doesn't not know its actual concurrency limit but only one you explicitly set during construction or fordefault arena(one that will be used for simpleparallel_forcall for example) the concurrency will be a whole machine.When you start
parallel_forwithstatic_partitionerit will create as many internalproxy tasksasnormal tasks(proxy tasksare used to assign tasks to specific threads).Proxy taskshave a property they should be executed twice. When execute is called for theproxy taskfor the first time it will return actual task that it propagated. When execute is called for the second timeproxy taskcan be deleted.In test case each time we call
parallel_forwithstatic_partitionerit will createhardware_concurrencyproxy tasks but becauseglobal_controlis present the concurrency of the default arena will not be fully satisfied and some of the proxy tasks will be called only once so they never be destroyed.This fix will not help if
global_controlis set concurrently with parallel algorithm execution. Perhaps, this scenario is less probable.Fixes # - issue number(s) if exists
#1403
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
@lennoxho
Other information