Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for running dedicated built-in mi.Threads #138

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tszirr
Copy link
Contributor

@tszirr tszirr commented Jul 26, 2022

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.

  • My code follows the style guidelines of this project
  • My changes generate no new warnings
  • My code also compiles for cuda_* and llvm_* variants. If you can't test this, please leave below
  • 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
  • I cleaned the commit history and removed any "Merge" commits
  • I give permission that the Mitsuba 3 project may redistribute my contributions under the terms of its license

@dvicini
Copy link
Member

dvicini commented Jul 27, 2022

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

@Speierers Speierers requested a review from wjakob August 15, 2022 04:11
Copy link
Member

@Speierers Speierers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two first commits look good to me.

Regarding the third commit, I will let Wenzel take a lot a this patch, he is more familiar with this part of the codebase.

Comment on lines +410 to +411
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
else {
} else {

@wjakob wjakob force-pushed the master branch 4 times, most recently from 4af72bd to 199b607 Compare November 30, 2022 20:51
@wjakob wjakob force-pushed the master branch 2 times, most recently from df4cbe0 to 64fedcd Compare January 17, 2023 23:05
@wjakob wjakob force-pushed the master branch 3 times, most recently from 3f3b8d0 to 1bdea6e Compare October 12, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants