Task: Separate Finalize into Reduce And Free
ClosedPublic
Actions

Authored by Jeroen Bakker (jbakker) on Apr 10 2020, 4:17 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Bastien Montagne (mont29)

Maniphest Tasks

T70709: Switch Blender task scheduler to use TBB

Commits

rBd923fb784f4f: Task: Separate Finalize into Reduce And Free

Summary

In preparation of TBB we need to split the finalize function into reduce
and free. Reduce is used to combine results and free for freeing any
allocated memory.

The reduce function is called to join user data chunk into another, to reduce the
result to the original userdata_chunk memory. These functions should have no side
effects so that they can be run on any thread.
The free functions should free data created during execution (TaskParallelRangeFunc).

Original patch by Brecht van Lommel
rB61f49db843cf: Task: change func_finalize to func_reduce and func_free.

Diff Detail

Repository: rB Blender

Event Timeline

Jeroen Bakker (jbakker) created this revision.Apr 10 2020, 4:17 PM

Harbormaster completed remote builds in B7535: Diff 23628.Apr 10 2020, 4:17 PM

Jeroen Bakker (jbakker) requested review of this revision.Apr 10 2020, 4:17 PM

migrate task range pool or remove it.
test the test cases.

Fixed Tasks: TaskRangePool

Harbormaster completed remote builds in B7537: Diff 23633.Apr 10 2020, 6:32 PM

Jeroen Bakker (jbakker) retitled this revision from [WIP] Task: Separate Finalize into Reduce And Free to Task: Separate Finalize into Reduce And Free.Apr 10 2020, 6:32 PM

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Apr 16 2020, 9:38 AM

Jeroen Bakker (jbakker) added reviewers: Brecht Van Lommel (brecht), Bastien Montagne (mont29).

Jeroen Bakker (jbakker) added a project: BF Blender (2.90).

This is the final step of preparation. The rest of the development will happen in a branch.

There was a mention about the usage of TaskPoolRange with TBB. This patch assumes that TaskPoolRange can be migrated and is useful when switching over to TBB. In a later stage we have to see if this is the case.

Brecht Van Lommel (brecht) accepted this revision.Apr 16 2020, 1:14 PM

Main (huge) issue for me with that patch are the differences of treatment of TLS data between threaded and non-threaded codepathes in that patch. Unless there is a very good reason to do that, in which case it has to be explained in details in comments in the code.

source/blender/blenlib/intern/task_iterator.c
263–276	I don’t think that that set of changes is valid? If I follow the code properly with this you: Directly use given TLS data/memory into worker callback, which is never done in threaded case; Do not call reduce callback at all, when that function may actually perform more work than only reducing TLS data into a single set of those. This might or might not 'work' with our current usages of this code, but imho this is wrong anyway. Call free callback on the user-given TLS data, which never happens either with the threaded codepath.
369–370	This one is missing the `&& (settings->func_reduce != NULL \|\| settings->func_free != NULL)` extra check ;)
383	That one should be protected by the `if (use_tls_data)`!
388–389	'reduce function', not 'finalize function'
447	would keep `finalize` name here, this one also handles freeing, not only reducing.
692–695	Same issues as above in `parallel_range_single_thread()`
tests/gtests/blenlib/BLI_task_test.cc
42	You can remove the `+ 1` then...
73	You can remove the `+ 1` then...

This revision now requires changes to proceed.Apr 16 2020, 2:33 PM

Note that I mostly reviewed the changes to task code itself (and its tests), assuming changes to user code are essentially monkey work...

Brecht Van Lommel (brecht) added inline comments.Apr 16 2020, 3:07 PM

source/blender/blenlib/intern/task_iterator.c
263–276	Directly use given TLS data/memory into worker callback, which is never done in threaded case; The new mechanism is to reduce everything into the user-given data. For the single-threaded case, there is nothing to reduce and it's fastest to not make a copy of it. Do not call reduce callback at all, when that function may actually perform more work than only reducing TLS data into a single set of those. This might or might not 'work' with our current usages of this code, but imho this is wrong anyway. The reduce functions should have no side effects, so that they can be run on any thread by TBB and not be bottlenecked by the main thread. Users of these callbacks were changed to work like this. Call free callback on the user-given TLS data, which never happens either with the threaded codepath. The assumption here is that `func_free` is used to free temporary working memory allocated by `func`. Any memory allocated to store the result of computations is expected to be freed by `func_reduce` and the caller. This is indeed somewhat weak, though not sure what the better way to do it would be besides better naming and comments.

Bastien Montagne (mont29) added inline comments.Apr 17 2020, 9:56 AM

source/blender/blenlib/intern/task_iterator.c
263–276	Ok… Not really thrilled by getting so specific and restrictive here, but I guess this is required by future switch to TBB. But at the very least, those specifics should be very clearly documented in the API! Right now there is no mention about any of those restrictions.

CodeReview

Added comments to explain the expected usage for func_free
Revert renaming parallel_range_func_finalize
Small changes to the control flow

Harbormaster completed remote builds in B7614: Diff 23842.Apr 17 2020, 10:05 AM

Jeroen Bakker (jbakker) added inline comments.Apr 17 2020, 10:10 AM

source/blender/blenlib/intern/task_iterator.c
263–276	Comments have been added to the structs and where func_free is invoked.
692–695	Comments have been added in the structs and where func_free is invoked