mitsuba3 icon indicating copy to clipboard operation
mitsuba3 copied to clipboard

Fixes for running dedicated built-in mi.Threads

Open tszirr opened this issue 2 years ago • 1 comments

Bugfix

Description

Two of the three commits are simple Thread class fixes (no undefined default priorities, give back GIL while waiting for Thread methods to resume).

The third commit addresses a rather ugly conflict between mi.Thread-derived and ThreadNotifier WorkerThread instances. I am sure you are well aware of the somewhat unfortunate layering of threading functionality at this point. The provided fix addresses too possible implementations of C++ thread_local storage:

  • The least surprising (and likely originally expected) behavior of ThreadNotifier initialization on thread start (not actually happening on my system), in which case the fix now replaces the WorkerThread instance by the instance of the dispatched mi.Thread-derived object with proper reference counting and destruction.
  • The more surprising (but correct and observed on my system) initialization-before-use behavior of ThreadNotifier, in which case the fix prevents correct mi.Thread-derived objects to accidentally be replaced with anonymous WorkerThread instances that easily lead to crashes when lacking the right ThreadEnvironment settings.

Testing

Minimal repro (Ubuntu 20.04 LTS, Python 3.8):

import mitsuba as mi
import drjit as dr

import matplotlib.pyplot as plt

mi.set_variant('scalar_rgb')

class RenderThread(mi.Thread):
    def __init__(self, scene):
        super().__init__(name='RenderThread')
        self.scene = scene

    def run(self):
        mi.set_variant('scalar_rgb')
        self.img = mi.render(self.scene)

scene_desc = mi.cornell_box()
scene = mi.load_dict(mi.cornell_box())
render_thread = RenderThread(scene)
render_thread.start()

render_thread.join()
plt.imshow(render_thread.img)
plt.show()

Checklist:

Please make sure to complete this checklist before requesting a review.

  • [x] My code follows the style guidelines of this project
  • [x] My changes generate no new warnings
  • [x] My code also compiles for cuda_* and llvm_* variants. If you can't test this, please leave below
  • [x] I have commented my code
  • ~[ ] I have made corresponding changes to the documentation~
  • ~[ ] I have added tests that prove my fix is effective or that my feature works~
  • [x] I cleaned the commit history and removed any "Merge" commits
  • [x] I give permission that the Mitsuba 3 project may redistribute my contributions under the terms of its license

tszirr avatar Jul 26 '22 13:07 tszirr

Hi Tobias, Thanks for the PR. I will hold off with merging this until the rest of the team is back from vacation. I not super familiar with the nitty gritty details of threading in Python/Pybind11 and can't fully assess if we should merge this as is or not. Delio

dvicini avatar Jul 27 '22 13:07 dvicini