BKE_reports: make the API thread-safe. #113561

Bastien Montagne · 2023-10-11T22:16:12+02:00

Bastien Montagne commented

2023-10-11 22:16:12 +02:00

This commit makes using (most of) BKE_report API safe in
multi-threaded situation.

This is achieved by adding a std::mutex lock to the ReportList
struct (in a slightly convoluted way unfortunately, due to this being a
DNA struct). This lock is then used to make most operations on
Reportlist data thread-safe.

Note that while working on this, a few other minor issues aroze in
existing usages of Reportlist by the WM code, mainly the fact that
wm_init_reports and wm_free_reports were both useless:

init was called in a context where there is not yet any WM, so it
was doing nothing.
free was called on a WM that would be later freed (as part of Main
freeing), which would also call cleanup code for its reports data.
Both have been removed.

Further more, wm_add_default (which is the only place where a WM ID is
created) did not initialize properly it reports data, this has been
fixed.

This change is related to the wmJob thread-safety tasks and PRs (#112537,
!113548).

This commit makes using (most of) `BKE_report` API safe in multi-threaded situation. This is achieved by adding a `std::mutex` lock to the `ReportList` struct (in a slightly convoluted way unfortunately, due to this being a DNA struct). This lock is then used to make most operations on `Reportlist` data thread-safe. Note that while working on this, a few other minor issues aroze in existing usages of Reportlist by the WM code, mainly the fact that `wm_init_reports` and `wm_free_reports` were both useless: - init was called in a context where there is not yet any WM, so it was doing nothing. - free was called on a WM that would be later freed (as part of Main freeing), which would also call cleanup code for its `reports` data. Both have been removed. Further more, `wm_add_default` (which is the only place where a WM ID is created) did not initialize properly it reports data, this has been fixed. This change is related to the wmJob thread-safety tasks and PRs (#112537, !113548).

Bastien Montagne added the

Module

Core

label 2023-10-11 22:16:34 +02:00

Bastien Montagne requested review from Brecht Van Lommel 2023-10-11 22:16:57 +02:00

Bastien Montagne requested review from Campbell Barton 2023-10-11 22:16:57 +02:00

Bastien Montagne requested review from Sergey Sharybin 2023-10-11 22:16:58 +02:00

Bastien Montagne referenced this pull request

2023-10-11 22:17:28 +02:00

Make wmJob worker thread use their own reports list instead of WM one. #113548

Bastien Montagne commented

2023-10-12 10:01:57 +02:00

@blender-bot build

Brecht Van Lommel requested changes 2023-10-12 14:08:53 +02:00

source/blender/blenkernel/BKE_report.h Outdated

						
				@ -31,0 +41,4 @@

				 * \note: Not thread-safe, should only be called from the same 'owner' thread of the report list as

				 * the one who created/initialized it, once no other concurrent access is possible.

				 */

				void BKE_reports_finish(ReportList *reports);

Brecht Van Lommel commented

2023-10-12 14:05:01 +02:00

Can this be called BKE_reports_free? I understand it does not free the report list itself, but that's not uncommon.

Finish to me implies it will do something more than freeing.

Can this be called `BKE_reports_free`? I understand it does not free the report list itself, but that's not uncommon. Finish to me implies it will do something more than freeing.

Bastien Montagne commented

2023-10-12 15:10:23 +02:00

Fair point, will do.

mont29 marked this conversation as resolved

source/blender/blenkernel/intern/report.cc Outdated

						
				@ -63,0 +74,4 @@

				  BKE_reports_clear(reports);

				  MEM_delete(static_cast<std::mutex *>(reports->lock));

Brecht Van Lommel commented

2023-10-12 14:05:38 +02:00

Is this cast needed?

Bastien Montagne commented

2023-10-12 15:10:39 +02:00

Indeed, not anymore!

Bastien Montagne force-pushed tmp-bke-reports-threadsafe from 753bf1f646 to 0fdd55e43d

2023-10-12 15:22:36 +02:00

Compare

Bastien Montagne requested review from Brecht Van Lommel 2023-10-12 15:22:57 +02:00

Bastien Montagne commented

2023-10-12 15:23:13 +02:00

@blender-bot build

Brecht Van Lommel reviewed 2023-10-12 15:28:52 +02:00

source/blender/blenloader/intern/readblenentry.cc Outdated

						
				@ -411,12 +411,14 @@ BlendFileData *BLO_read_from_file(const char *filepath,

				  BlendFileData *bfd = nullptr;

				  FileData *fd;

				  printf("A\n");

Brecht Van Lommel commented

2023-10-12 15:28:52 +02:00

Remove this and other debug prints.

😕 1

mont29 marked this conversation as resolved

Brecht Van Lommel approved these changes 2023-10-12 16:59:20 +02:00

Sergey Sharybin requested changes 2023-10-13 10:05:27 +02:00

Sergey Sharybin left a comment

Overall looks fine. The comments in the public API seems to need some attention.

source/blender/blenkernel/BKE_report.h Outdated

						
				@ -31,0 +39,4 @@

				 * re-initialized first.

				 *

				 * \note: Not thread-safe, should only be called from the same 'owner' thread of the report list as

				 * the one who created/initialized it, once no other concurrent access is possible.

Sergey Sharybin commented

2023-10-13 10:03:22 +02:00

I don't think this is a valid restriction to call "free" from the same thread which created the reports. It is important that an owner frees it, after all other users of the reports are done using it, but nothing really prevents transferring ownership of the reports across threads.

mont29 marked this conversation as resolved

source/blender/blenkernel/BKE_report.h Outdated

						
				@ -31,0 +44,4 @@

				void BKE_reports_free(ReportList *reports);

				/**

				 * Only frees the list of reports in given \a reports. Use #BKE_reports_finish to fully cleanup all

Sergey Sharybin commented

2023-10-13 10:04:36 +02:00

BKE_reports_finish does not exist?

`BKE_reports_finish` does not exist?

mont29 marked this conversation as resolved

Bastien Montagne force-pushed tmp-bke-reports-threadsafe from 667a9e9bf0 to 3c84508d4a

2023-10-13 10:41:04 +02:00

Compare

Bastien Montagne requested review from Sergey Sharybin 2023-10-13 10:41:34 +02:00

Sergey Sharybin approved these changes 2023-10-13 11:01:12 +02:00

Bastien Montagne merged commit 9859622a66 into main

2023-10-13 11:30:07 +02:00

Bastien Montagne referenced this issue from a commit

2023-10-13 11:30:08 +02:00

BKE_reports: make the API thread-safe.

Bastien Montagne deleted branch tmp-bke-reports-threadsafe

2023-10-13 11:30:09 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

BKE_reports: make the API thread-safe. #113561