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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Iliya Katushenock commented

2023-07-03 22:20:36 +02:00

Member

Curve Deform node has never worked correctly.
This used to be a leaked copy made by each node
to write its own version of the custom loop normals.
Const correctness and sharing increased the chances of a crash.

Curve Deform node has never worked correctly. This used to be a leaked copy made by each node to write its own version of the custom loop normals. Const correctness and sharing increased the chances of a crash.

Iliya Katushenock added the

labels 2023-07-03 22:20:37 +02:00

Iliya Katushenock requested review from Jacques Lucke 2023-07-03 22:20:55 +02:00

Iliya Katushenock requested review from Hans Goudey 2023-07-03 22:20:55 +02:00

Iliya Katushenock added this to the Nodes & Physics project 2023-07-03 22:20:59 +02:00

Iliya Katushenock force-pushed tmp_fix_data_race_to_compute_normals from e42064db77 to a61d126618

2023-07-03 22:29:35 +02:00

Compare

Iliya Katushenock changed title from ~~Fix #109583: Const version of mesh_calc_normals to only eval normals in geometry nodes~~ to Fix #109583: Const version of mesh_calc_normals to only eval normals in geometry nodes

2023-07-03 22:29:39 +02:00

mod_moder changed target branch from main to blender-v3.6-release

2023-07-03 22:29:50 +02:00

Iliya Katushenock added 1 commit 2023-07-03 22:30:56 +02:00

113a84aaff cleanup

Iliya Katushenock added 1 commit 2023-07-04 01:24:19 +02:00

b5b66cd6b6 progress

Iliya Katushenock changed title from ~~Fix #109583: Const version of mesh_calc_normals to only eval normals in geometry nodes~~ to Fix #109583: Avoid allocation new custom loop normals for write

2023-07-04 02:07:17 +02:00

Jacques Lucke reviewed 2023-07-04 10:13:24 +02:00

source/blender/blenkernel/intern/mesh.cc

						
				@ -1890,0 +1887,4 @@

				  /* 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

Member

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

Author

Member

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

Member

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

Author

Member

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

Author

Member

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.

Iliya Katushenock referenced this pull request

2023-07-04 13:49:47 +02:00

Fix #109583: The temporal mesh copies to calculate normals from multiple nodes #109695

Iliya Katushenock commented

2023-07-04 13:50:02 +02:00

Author

Member

https://projects.blender.org/blender/blender/pulls/109695

Iliya Katushenock closed this pull request

2023-07-04 13:50:03 +02:00

Iliya Katushenock changed title from ~~Fix #109583: Avoid allocation new custom loop normals for write~~ to Fix #109583: Avoid allocation new custom loop normals for write

2023-07-04 15:11:41 +02:00

Iliya Katushenock changed title from ~~Fix #109583: Avoid allocation new custom loop normals for write~~ to Fix #109583: Avoid allocation new custom loop normals for write

2023-07-04 15:15:57 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#109671

No description provided.