⚙ D9258 Multithreading Object Sync in Cycles Render (Simpler)

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
377	I'll remove the timing code in a future push.

Nicolas Lelong (rotoglup) mentioned this in D8324: Multithreading Object Sync in Cycles Render.Oct 19 2020, 11:21 AM

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.

In D9258#228793, @Jagannadhan Ravi (easythrees) wrote:

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.

Jagannadhan Ravi (easythrees) added inline comments.Oct 19 2020, 4:02 PM

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?

Nicolas Lelong (rotoglup) added inline comments.Oct 19 2020, 4:08 PM

intern/cycles/blender/blender_object.cpp
186–196	Yes :)

In D9258#228990, @Brecht Van Lommel (brecht) wrote:

In D9258#228793, @Jagannadhan Ravi (easythrees) wrote:

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?

In D9258#229232, @Jagannadhan Ravi (easythrees) wrote:

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) {

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)?

Jagannadhan Ravi (easythrees) added inline comments.Oct 19 2020, 9:31 PM

intern/cycles/blender/blender_object.cpp
377	Forgot to remove, will do so now.

Removed timing code.

In D9258#229330, @Jagannadhan Ravi (easythrees) wrote:

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.

Nicolas Lelong (rotoglup) added inline comments.Oct 19 2020, 11:05 PM

intern/cycles/blender/blender_object.cpp
345	You could also remove this last bit of timing code.

Jagannadhan Ravi (easythrees) added inline comments.Oct 19 2020, 11:09 PM

intern/cycles/blender/blender_object.cpp
345	D'Oh! Doing so now ...

removed one last straggler of timing code

In D9258#229369, @Nicolas Lelong (rotoglup) wrote:

In D9258#229330, @Jagannadhan Ravi (easythrees) wrote:

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.

In D9258#229369, @Nicolas Lelong (rotoglup) wrote:

In D9258#229330, @Jagannadhan Ravi (easythrees) wrote:

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.

Brecht Van Lommel (brecht) mentioned this in T81892: Multithreading Object Sync in Cycles.Oct 20 2020, 5:09 PM

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.

In D9258#229762, @Brecht Van Lommel (brecht) wrote:

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.

In D9258#229815, @Jagannadhan Ravi (easythrees) wrote:

In D9258#229762, @Brecht Van Lommel (brecht) wrote:

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

In D9258#229857, @Nicolas Lelong (rotoglup) wrote:
In D9258#229815, @Jagannadhan Ravi (easythrees) wrote:

In D9258#229762, @Brecht Van Lommel (brecht) wrote:

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

Closed by commit rBbb49aa0d6978: Cycles: multithreaded export of geometry (authored by Jagannadhan Ravi (easythrees), committed by Brecht Van Lommel (brecht)). · Explain WhyOct 21 2020, 6:56 PM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) added a commit: rBbb49aa0d6978: Cycles: multithreaded export of geometry.

In D9258#230233, @Brecht Van Lommel (brecht) wrote:

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.

Olivier Maury (omaury) mentioned this in T81976: Crash at Render Startup Time in Object Sync code from "Multithreading Object Sync in Cycles Render (Simpler)" (D9258).Oct 22 2020, 8:21 PM

Stefan Werner (swerner) added a commit: rCe7c506b27bb6: Multithreaded export of geometry.Mar 1 2021, 2:19 AM

Multithreading Object Sync in Cycles Render (Simpler)
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 30134

intern/cycles/blender/blender_geometry.cpp

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_sync.h

Multithreading Object Sync in Cycles Render (Simpler)ClosedPublicActions