blender/blender
154
489
Fork 727
Code Issues 6.7k Pull Requests 885 Projects 18 Wiki Activity

Fix #109583: Avoid allocation new custom loop normals for write #109671

Closed
Iliya Katushenock wants to merge 3 commits from mod_moder:tmp_fix_data_race_to_compute_normals into blender-v3.6-release
pull from: mod_moder:tmp_fix_data_race_to_compute_normals
merge into: blender:blender-v3.6-release

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 1 Commits 3 Files Changed 1 +4 -3
1 changed files with 4 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
source/blender/blenkernel/intern/mesh.cc
View File

@ -1884,9 +1884,10 @@ void BKE_mesh_calc_normals_split_ex(Mesh *mesh,
((mesh->flag & ME_AUTOSMOOTH) != 0);
const float split_angle = (mesh->flag & ME_AUTOSMOOTH) != 0 ? mesh->smoothresh : float(M_PI);
/* may be nullptr */
blender::short2 *clnors = (blender::short2 *)CustomData_get_layer_for_write(
&mesh->ldata, CD_CUSTOMLOOPNORMAL, mesh->totloop);
/* May be nullptr. Const_cast to avoid data race while normals computing from different threads
* for the same mesh. */
blender::short2 *clnors = const_cast<blender::short2 *>(static_cast<const blender::short2 *>(
CustomData_get_layer(&mesh->ldata, CD_CUSTOMLOOPNORMAL)));
Jacques Lucke commented 2023-07-04 10:13:22 +02:00
Review
Copy Link

That does not seem quite right to me. How can BKE_mesh_calc_normals_split_ex know that it is allowed to use const_cast and ignore implicit sharing?

That does not seem quite right to me. How can `BKE_mesh_calc_normals_split_ex` know that it is allowed to use `const_cast` and ignore implicit sharing?
Iliya Katushenock commented 2023-07-04 10:56:43 +02:00
Review
Copy Link

The problem was exactly that 10 nodes were taking it for the writing. Since there is only one real owner (only the object itself), different threads create new owners (that is, they simply add a reference count) and copy the data. But the data itself must continue to belong to the object. That is, now, the object must own all copies of this data for different threads. I really don't know how to fix this...
This doesn't look right.
For this reason, I'd rather just have a different version of the function (which would ignore this data). But Hans' suggestion was that it doesn't work just because of const correctness. If the streams will only read, everything should be fine. But they can still make a changes...
And a future normal refactoring might fix that..

The problem was exactly that 10 nodes were taking it for the writing. Since there is only one real owner (only the object itself), different threads create new owners (that is, they simply add a reference count) and copy the data. But the data itself must continue to belong to the object. That is, now, the object must own all copies of this data for different threads. I really don't know how to fix this... This doesn't look right. For this reason, I'd rather just have a different version of the function (which would ignore this data). But Hans' suggestion was that it doesn't work just because of const correctness. If the streams will only read, everything should be fine. But they can still make a changes... And a future normal refactoring might fix that..
Jacques Lucke commented 2023-07-04 10:59:54 +02:00
Review
Copy Link

If this is just a cache, then it should be protected by a mutex. I probably don't fully understand what's going on here right now.

If this is just a cache, then it should be protected by a mutex. I probably don't fully understand what's going on here right now.
Iliya Katushenock commented 2023-07-04 11:01:26 +02:00
Review
Copy Link

10 curve deform nodes are asked to compute normals on a single object. Each of the nodes, through this function, requires the creation of local data for writing. And either a leak occurs, or now the reference counter is causing a crash ...

10 curve deform nodes are asked to compute normals on a single object. Each of the nodes, through this function, requires the creation of local data for writing. And either a leak occurs, or now the reference counter is causing a crash ...
Iliya Katushenock commented 2023-07-04 12:14:54 +02:00
Review
Copy Link

Although, now it seems to me that if each node creates its own copy of the mesh, implicit sharing will work correctly here.

Although, now it seems to me that if each node creates its own copy of the mesh, implicit sharing will work correctly here.
const bool *sharp_edges = static_cast<const bool *>(
CustomData_get_layer_named(&mesh->edata, CD_PROP_BOOL, "sharp_edge"));
const bool *sharp_faces = static_cast<const bool *>(