Page MenuHome

Multithreading Object Sync in Cycles Render (Simpler)
ClosedPublic

Authored by Jagannadhan Ravi (easythrees) on Oct 19 2020, 1:21 AM.

Details

Diff Detail

Repository
rB Blender

Event Timeline

Jagannadhan Ravi (easythrees) requested review of this revision.Oct 19 2020, 1:21 AM
Jagannadhan Ravi (easythrees) created this revision.

@Brecht Van Lommel (brecht) regarding your comment in the other review:

I think there is still some non-thread safe code in BlenderSync::sync_geometry(...). It's accessing geom->transform_applied and geom->used_shaders in the main thread while a task pool thread may also be modifying them. Moving the geometry_synced test and insert earlier in the function or duplicating it (haven't checked which is correct) would avoid the issue.

Do you think it makes sense to move that test into sync_object(...) instead?

intern/cycles/blender/blender_geometry.cpp
18

I recommend keeping this around, good way to quickly disable the feature if needed.

intern/cycles/blender/blender_object.cpp
383

I'll remove the timing code in a future push.

I've added 2 inline comments

intern/cycles/blender/blender_object.cpp
186–196

For memory, brecht indicated in previous review :

b_permanent_ob is also the same as b_ob_instance as far as I can tell (maybe it needs to be renamed for clarity).

From what I saw, this is indeed the case, this whole block could go away (except perhaps the comment), as it duplicates the logic that obtains b_ob_instance.

intern/cycles/blender/blender_sync.h
268

I think this mutex is not used anywhere, it can be removed.

Do you think it makes sense to move that test into sync_object(...) instead?

I don't see how that would help, the test depends on the geometry pointer which you only get in sync_geometry.

intern/cycles/blender/blender_geometry.cpp
18

Don't keep it around, there's no need to disable this feature specifically. When we want to debug, it's still possible to disable multithreading at the task scheduler level.

intern/cycles/blender/blender_object.cpp
186–196

I haven't had my coffee yet, so bear with me. So replace b_permanent_ob with b_ob_instance instead?

intern/cycles/blender/blender_object.cpp
186–196

Yes :)

Do you think it makes sense to move that test into sync_object(...) instead?

I don't see how that would help, the test depends on the geometry pointer which you only get in sync_geometry.

Right, so you’d have to move/copy its creation over as well, which is what your comment sounded like. If not, then I’m unclear what you mean. Was it to move the existence test to earlier in this function?

Right, so you’d have to move/copy its creation over as well, which is what your comment sounded like. If not, then I’m unclear what you mean. Was it to move the existence test to earlier in this function?

Here's how I think it's thread safe, moving the geometry_synced test earlier:

diff --git a/intern/cycles/blender/blender_geometry.cpp b/intern/cycles/blender/blender_geometry.cpp
index 002f5e0..80bac09 100644
--- a/intern/cycles/blender/blender_geometry.cpp
+++ b/intern/cycles/blender/blender_geometry.cpp
@@ -77,8 +77,15 @@ Geometry *BlenderSync::sync_geometry(BL::Depsgraph &b_depsgraph,
       used_shaders.push_back(default_shader);
   }
 
-  /* Test if we need to sync. */
+  /* Ensure we only sync instanced geometry once. */
   Geometry *geom = geometry_map.find(key);
+  if (geom) {
+    if (geometry_synced.find(geom) != geometry_synced.end()) {
+      return geom;
+    }
+  }
+
+  /* Test if we need to sync. */
   bool sync = true;
   if (geom == NULL) {
     /* Add new geometry if it did not exist yet. */
@@ -125,15 +132,10 @@ Geometry *BlenderSync::sync_geometry(BL::Depsgraph &b_depsgraph,
     }
   }
 
-  /* Ensure we only sync instanced geometry once. */
-  if (geometry_synced.find(geom) != geometry_synced.end()) {
-    return geom;
-  }
+  geometry_synced.insert(geom);
 
   progress.set_sync_status("Synchronizing object", b_ob.name());
 
-  geometry_synced.insert(geom);
-
   geom->name = ustring(b_ob_data.name().c_str());
 
   if (geom_type == Geometry::HAIR) {
Jagannadhan Ravi (easythrees) marked 4 inline comments as done.

Updated review with comments. Forgot to remove the timing code, so I'll do that in a minute. There is one "fail" in the regression tests (attached).

I'm not entirely sure where the issue could be here, should we just update the test for this result?

EDIT: @Brecht Van Lommel (brecht) do you think we could ignore this and update the test (which would need to be done by whoever checks it in I believe)?

intern/cycles/blender/blender_object.cpp
377

Forgot to remove, will do so now.

There is one "fail" in the regression tests (attached).

Well this is new :( Same issue here, with patch applied on current master. The issue goes away when pulling the sync_hair call from the task_pool.

intern/cycles/blender/blender_object.cpp
345

You could also remove this last bit of timing code.

intern/cycles/blender/blender_object.cpp
345

D'Oh! Doing so now ...

removed one last straggler of timing code

There is one "fail" in the regression tests (attached).

Well this is new :( Same issue here, with patch applied on current master. The issue goes away when pulling the sync_hair call from the task_pool.

I'll do that, seems like a good solution.

There is one "fail" in the regression tests (attached).

Well this is new :( Same issue here, with patch applied on current master. The issue goes away when pulling the sync_hair call from the task_pool.

How did you remove it? Simply moving it out of the task pool's scope makes it crash on the barbershop scene.

We can't update the test, it's randomly giving different results here.

What is happening is that sync_dupli_particle accesses object->geometry, which is not fully initialized since that happens in another thread.

In particular object->geometry->need_attribute(scene, ATTR_STD_PARTICLE) requires used_shaders to be initialized.

The easiest solution for now would be to disable using the task pool for the case where sync_dupli_particle is called. I plan to change how we export such particle data after D2057 lands, and once that happens it should no longer be a problem.

We can't update the test, it's randomly giving different results here.

What is happening is that sync_dupli_particle accesses object->geometry, which is not fully initialized since that happens in another thread.

In particular object->geometry->need_attribute(scene, ATTR_STD_PARTICLE) requires used_shaders to be initialized.

The easiest solution for now would be to disable using the task pool for the case where sync_dupli_particle is called. I plan to change how we export such particle data after D2057 lands, and once that happens it should no longer be a problem.

How do I disable the TaskPool? I don't see a method on the class.

The easiest solution for now would be to disable using the task pool for the case where sync_dupli_particle is called. (...)

How do I disable the TaskPool? I don't see a method on the class.

I believe the idea would be to have a test before pushing to the task_pool, preventing to process geometry that will be used later by sync_dupli_particle.

Maybe sync_object could change to be something more like (pseudocode) ?

bool will_need_sync_dupli_particle = (is_instance && b_instance. particle_system());
TaskPool *optional_task_pool = will_need_sync_dupli_particle ? NULL : &task_pool;
sync_geometry(..., optional_task_pool);

The call to sync_geometry with a NULL TaskPool* would instruct to run the sync in the main thread

The easiest solution for now would be to disable using the task pool for the case where sync_dupli_particle is called. (...)

How do I disable the TaskPool? I don't see a method on the class.

I believe the idea would be to have a test before pushing to the task_pool, preventing to process geometry that will be used later by sync_dupli_particle.

Maybe sync_object could change to be something more like (pseudocode) ?

bool will_need_sync_dupli_particle = (is_instance && b_instance. particle_system());
TaskPool *optional_task_pool = will_need_sync_dupli_particle ? NULL : &task_pool;
sync_geometry(..., optional_task_pool);

The call to sync_geometry with a NULL TaskPool* would instruct to run the sync in the main thread

You're gonna laugh at me, but this is what I'm coding up right now. I was also toying with the idea of forcing the underlying TBB task group to execute the functions immediately, but that doesn't seem to work.

This passes regression tests and looks like it preserves the speed gains, certainly in the barbershop and BMW scenes. Also synced to latest master. @Brecht Van Lommel (brecht) please let me know if this is good for you.

I'll commit this with some minor tweaks, thanks for all the work!

This revision is now accepted and ready to land.Oct 21 2020, 6:55 PM

I'll commit this with some minor tweaks, thanks for all the work!

Hi @Brecht Van Lommel (brecht) thank you, please let me know when this gets checked in so I can grab it to do some benchmarks (since you mentioned doing some tweaks, I'd rather have something up to date for data gathering).

It's in the Blender master branch now. All I did was skip the task pool for instances also for motion blur, and tweaking some comments.