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

Background Threads Suspend / Threading Issues #45

Open
biscuitWizard opened this issue Oct 3, 2021 · 6 comments
Open

Background Threads Suspend / Threading Issues #45

biscuitWizard opened this issue Oct 3, 2021 · 6 comments

Comments

@biscuitWizard
Copy link
Contributor

Background threads suspending presents problems in suspends happening in forked code where a suspend is not desired. This falls into side-effect function problems.

Recommendation is to change things like sort:: into non-background thread functions and instead use C++17's parallel processing execution methods.

https://en.cppreference.com/w/cpp/algorithm/sort

and

std::execution::par_unseq

https://en.cppreference.com/w/cpp/algorithm/execution_policy_tag

This will still net a significant performance gain while removing the side effect code smell that can cause issues with sort.

@biscuitWizard
Copy link
Contributor Author

Specifically, in our case, our scheduler relied on using sort() and the scheduler would reschedule forked tasks while suspending those tasks causing overflow scenarios with the scheduler itself due to sort() suspends.

@biscuitWizard
Copy link
Contributor Author

More info: https://www.cppstories.com/2018/11/parallel-alg-perf/

I don't think we need to remove background_thread entirely, or that the solutions are mutually exclusive. People like me can disable background threads, and we can all benefit from parallel processing

@lisdude
Copy link
Owner

lisdude commented Oct 5, 2021

This is possibly a case of poor documentation / defaults. In order for threading to work, the MOO task has to be suspended so that the MOO can continue to process tasks while your original expensive function is doing work. This implicit suspend makes these functions oftentimes slower in loops and, as you discovered, can have other undesired effects directly related to these implicit suspends.

The easiest solution in your case would be to throw set_thread_mode(0); at the top of your verb to disable threading for that specific verb only. But this brings up something I've been thinking about for a long time without conclusion: Should the default be the opposite? That is to say, should threading be globally disabled by default and require an explicit set_thread_mode(1); in verbs that know they would benefit from it? I think so, along with more obvious documentation on the how and why of threads.

Parallel sort sounds good in general, I'll have to look into it. My only mild hesitation would be C++17 and compiler support for it. I'm not all that familiar with versions being shipped these days.

@biscuitWizard
Copy link
Contributor Author

biscuitWizard commented Oct 6, 2021

That's exactly what we ended up doing (using set_thread_mode(0)). I think this is a problem less for newer MOOs and more for us old fogbies shambling in from our 2000s-era LambdaMoo codebases. In our attempts to be hip and modern with these new built-ins, they sometimes have "gotchas" like what we see here.

I want to be clear that threading can refer to "asynchronous" and "parallel" which are two different methods of concurrency. Asynchronous concurrency requires a suspend and callback, and parallel concurrency does not. There is GNU support under gnu_parallel for the C++17 functions that could be imported and used as an alternative.

I think calling out the gotchas clearly in the documentation and readme for any method that has a background_thread functionality could solve my concern, something along the lines of "This builtin is threaded by default and will suspend during execution, which can potentially have hazardous side effects. If you are unsure, be sure to set_thread_mode(0) before calling."

It was not intuitive to me switching from an older codebase that these built-ins would suspend, I had to dive into the sourcecode to get that answer, and I find it pertinent. I hazarded a guess that they might be, but wasn't sure. I suspect given the wide range of experience of newcomers to Toast, there will be future instances of others having similar uncertainty.

@ethindp
Copy link
Contributor

ethindp commented Nov 15, 2021

In general, C++17 should be fully supported by all the mainstream compilers; its C++20 that isn't even close to being fully supported. It shouldn't be a problem.

@ethindp
Copy link
Contributor

ethindp commented Nov 15, 2021

As for avoiding multithreading problems like this, I think that, as an amendment to the comment made by @biscuitWizard above, it might also be pertinent to note ways of avoiding these gotchas.

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

No branches or pull requests

3 participants