Fix #104915: Race condition writing subsurf optimal display edges #105156

Hans Goudey · 2023-02-23T22:51:48+01:00

Hans Goudey commented

2023-02-23 22:51:48 +01:00

Writing to a bitmap from multiple threads causes races when writing to
bits within the same integer. Instead, write to a separate boolean array
while subdividing, and move that to the final mesh bit vector after.

Notes:

The final copy to the bit vector could be replaced by a generic
copy_from(Span<bool>) call in the future.
Theoretically we could entirely replace the BitVector with an
Array<bool>, but 1/8 the memory use for edges is likely worth it.

Writing to a bitmap from multiple threads causes races when writing to bits within the same integer. Instead, write to a separate boolean array while subdividing, and move that to the final mesh bit vector after. Notes: - The final copy to the bit vector could be replaced by a generic `copy_from(Span<bool>)` call in the future. - Theoretically we could entirely replace the `BitVector` with an `Array<bool>`, but 1/8 the memory use for edges is likely worth it.

Hans Goudey added this to the 3.5 milestone 2023-02-23 22:51:48 +01:00

Hans Goudey added the

Module

Modeling

label 2023-02-23 22:51:48 +01:00

Hans Goudey added 1 commit 2023-02-23 22:52:01 +01:00

de78a588af Fix #104915 : Race condition writing subsurf optimal display edges

Writing to a bitmap from multiple threads causes races when writing to
bits within the same integer. Instead, write to a separate boolean array
while subdividing, and move that to the final mesh bit vector after.

Notes:
 - The final copy to the bit vector could be replaced by a generic
   `copy_from(Span<bool>)` call in the future.
 - Theoretically we could entirely replace the `BitVector` with an
   `Array<bool>`, but 1/8 the memory use for edges is likely worth it.

Hans Goudey referenced this pull request

2023-02-23 22:52:43 +01:00

Regression: Multires wireframes are displayed incomplete #104915

Hans Goudey added this to the Modeling project 2023-02-23 22:52:50 +01:00

Hans Goudey requested review from Jeroen Bakker 2023-02-23 22:53:08 +01:00

Hans Goudey requested review from Sergey Sharybin 2023-02-23 22:53:08 +01:00

Hans Goudey requested review from Jacques Lucke 2023-02-23 22:53:21 +01:00

Hans Goudey commented

2023-02-23 22:54:40 +01:00

A few reviewers here since I'm using the new Bit map classes and want Jacques to see this, and Sergey because subdiv is his area, and Jeroen because he investigated the bug already. I don't think you all need to look into this, but I want to make it an option at least. Thanks.

Jeroen Bakker commented

2023-02-23 23:34:06 +01:00

Just quickly looked over it. Some thoughts:
Normally an blender::Array<bool> stores a a bool in an uint32_t. Stdlib does some trickery here, but not sure that we also added that. If this is the case this solution just over-allocates memory. It just feels like it makes the race condition less likely to happen.

Considerations:

L2 cache isn't shared between the CPU cores (fe. NUMA). Write back to L3/Ram will happen with cache lines.
Group data that all bits of a cache line is handled by the same core.
Use work stealing to ensure less faults. (not sure how though)
Using subdiv only a known fraction of the edges will have this state set. Store the indexes in a per thread list and combine later.

Just quickly looked over it. Some thoughts: Normally an `blender::Array<bool>` stores a a bool in an uint32_t. Stdlib does some trickery here, but not sure that we also added that. If this is the case this solution just over-allocates memory. It just feels like it makes the race condition less likely to happen. Considerations: - L2 cache isn't shared between the CPU cores (fe. NUMA). Write back to L3/Ram will happen with cache lines. - Group data that all bits of a cache line is handled by the same core. - Use work stealing to ensure less faults. (not sure how though) - Using subdiv only a known fraction of the edges will have this state set. Store the indexes in a per thread list and combine later.

Iliya Katushenock commented

2023-02-23 23:43:33 +01:00

@Jeroen-Bakker Isn't it important for the race condition what type of data is read/written, and not how the size is rounded during allocation (uint8_t or uint32_t)?

@Jeroen-Bakker Isn't it important for the race condition what type of data is read/written, and not how the size is rounded during allocation (`uint8_t` or `uint32_t`)?

Brecht Van Lommel commented

2023-02-24 00:53:55 +01:00

sizeof(bool) is 1 on any platform/compiler we support, and blender::Array stores it as such. Multiple threads writing to different individual bytes in an array is safe, as they are at separate memory locations and the C++ standard guarantees that works.

Performance may not be great, but there should be no race condition.

`sizeof(bool)` is 1 on any platform/compiler we support, and `blender::Array` stores it as such. Multiple threads writing to different individual bytes in an array is safe, as they are at separate memory locations and the C++ standard guarantees that works. Performance may not be great, but there should be no race condition.

👍 2

Sergey Sharybin approved these changes 2023-02-24 09:25:41 +01:00

Jeroen Bakker approved these changes 2023-02-24 10:06:53 +01:00

Jacques Lucke approved these changes 2023-02-24 14:31:51 +01:00

Hans Goudey referenced this issue from a commit

2023-02-26 23:59:13 +01:00

Fix #104915: Race condition writing subsurf optimal display edges

Hans Goudey merged commit 3db246a3ce into blender-v3.5-release

2023-02-26 23:59:13 +01:00

Hans Goudey deleted branch fix-subdiv-optimal-edges-threadsafety

2023-02-26 23:59:16 +01: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

Fix #104915: Race condition writing subsurf optimal display edges #105156