Suspend thread pool #3080

msimberg · 2017-12-15T16:22:04Z

This adds a suspend method to thread_pool_base/scheduled_thread_pool. It waits for all work to be done on the pool before suspending.

Cover the check for if the OS thread is joinable or not.

hkaiser · 2017-12-16T15:05:57Z

hpx/runtime/threads/detail/scheduled_thread_pool_impl.hpp

+        {
+            while (sched_->Scheduler::get_thread_count() >
+                get_background_thread_count())
+            {}


Should this at least call sleep(0) instead of unconditionally spinning? Alternatively, we could re-spawn the resume function on an HPX thread instead of spinning, not sure...

sleep(0) (or yield) is a good idea. HPX thread I'm not sure about. It would require returning a future (if we want to know when it's done), and so far I've assumed this can be called from outside the runtime.

See here for how this could be handled: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/util/detail/yield_k.hpp. In fact, you could use yield_k() instead of having your own loop.

Updated to use yield_k - made most waits nice and short. Thanks for pointing that out.

hkaiser · 2017-12-16T15:08:12Z

hpx/runtime/threads/detail/thread_pool_base.hpp

@@ -80,7 +80,8 @@ namespace hpx { namespace threads { namespace detail
        virtual void stop(
            std::unique_lock<compat::mutex>& l, bool blocking = true) = 0;

-        virtual void resume(error_code& ec = throws) = 0;
+        virtual void suspend(error_code& ec = throws) {};
+        virtual void resume(error_code& ec = throws) {};


Should the default implementations at least assert, if not throw an exception instead of silently ignoring the request?

Yes, you're right. I went back and forth on this, but I think now it should be required (I will need to implement suspend/resume for the io_service_pool to suspend the runtime). If it should be ignored it can be empty in the inheriting class.

Thanks for this fix. Looks fine now.

hkaiser · 2017-12-16T15:13:01Z

tests/unit/resource/suspend_pool.cpp

+    }
+    catch (std::runtime_error const&)
+    {
+    }


Where is the std::runtime_error thrown? If this is thrown by HPX it should be changed to using an hpx::exception instead.

Either way, it looks like the exception is expected in this test case, if so, please add a bool checking that it was indeed thrown:

bool exception_thrown = false; try { ... } catch(hpx::exception const& e) { exception_thrown = true; } HPX_TEST(exception_thrown);

Hmm, yes, it is thrown by HPX. I did not realize it will be an hpx::exception then... Will update it the way you've suggested.

Thanks, LGTM.

hkaiser · 2017-12-16T15:14:39Z

tests/unit/resource/suspend_pool.cpp

+            // Can only suspend once all work is done
+            auto f = hpx::when_all(std::move(fs));
+            HPX_TEST(f.is_ready());
+


Is there a way for the suspend() API to return a future<void> allowing to synchronize with the fact that the pool actually has been suspended?

I suppose it could. However, I've again assumed that this can be called from outside the runtime in which case waiting for the future will not work. I don't have a strong case for calling it from outside the runtime but thought it's unnecessary to restrict it unless needed. This may be a reason to restrict it though.

Alternatively you could add a callback that is to be called once the operation is finished executing. That also would allow for splitting the API into two functions, one taking the callback (to be used when called from outside HPX), and one (internally) passing a hpx::lcos::local::packaged_task to the first API and returning a future extracted from the packaged_task (to be called from HPX threads). That should decouple the waiting from the actual context the suspend is invoked from.

Would you then spawn an OS thread or HPX thread depending on from where it's called? The downside is that there will be two functions, and the user has to know (find out) that only one of them can be called from outside the runtime (asserts help though, and returning a future is already a good sign it can't be called from the outside). I'll give it a shot in any case as it's a nice idea.

I think you can always spawn an HPX thread. Invoking the callback from an HPX thread should be a viable option, always.

Ah, you're right. The problem is that the suspending thread will then run on the pool it's supposed to suspend. So either it gets stuck or it has to call the callback before it has fully suspended the pool. The latter feels wrong but could be good enough (it does first wait for all work to be done before really suspending). Maybe there's another way.

Two corrections to my previous comments: I have to launch an OS thread in the case of suspending from non-HPX threads, and suspending from non-HPX threads is required for suspending the whole runtime...

I implemented a futurized and a callback version of suspend the way you suggested and the principle works well but I still need to tidy it up. The real question is: do you think it's useful that all of suspend/resume/suspend_processing_unit/resume_processing_unit have a futurized and a callback version? I think the answer is clearly yes if it's useful for suspend.

@hkaiser Besides technical details, do you have an opinion on the last question? (In case this slipped past you...)

hkaiser · 2017-12-16T15:17:25Z

tests/unit/resource/suspend_pool.cpp

+
+            return pool;
+        });
+


I still think that the API for creating a new pool is too complicated to be exposed to mere mortal users. This needs to be streamlined somehow - but I understand that this is orthogonal to this PR and should be handled by a separate ticket.

100% agreed on both points.

Opened an issue: #3082

…d_pool test

…plementations

…tion

hkaiser · 2018-01-08T12:46:27Z

hpx/runtime/threads/detail/io_service_thread_pool.hpp

@@ -65,6 +65,13 @@ namespace hpx { namespace threads { namespace detail
        ///////////////////////////////////////////////////////////////////////
        void stop (std::unique_lock<compat::mutex>& l, bool blocking = true);

+        ///////////////////////////////////////////////////////////////////////
+        hpx::future<void> suspend(error_code& ec = throws);


The API returning a future does not need to take an error_code as all possible exceptions will be reported through the future mechanism. Do I miss something?

I don't think you're missing anything. This part of what I meant when I said it needs tidying up (just wanted to have the basics working first to try it out).

Fair enough. Things start looking very good now. Thanks for your work on this!

hkaiser · 2018-01-08T12:48:59Z

hpx/runtime/threads/detail/scheduled_thread_pool_impl.hpp

            return;
        }

+        auto suspend_func_wrapper =
+            [this, callback{std::move(callback)}, ec]() mutable


This is C++14 only. We try to stay compatible with C++11. Please use the (new) HPX_CAPTURE_FORWARD macro instead of the direct initialization of the callback capture variable (see https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/config/lambda_capture.hpp).

Yep, I'm aware. Again needs tidying up. Sorry, should've been more explicit about what's still to do. Thanks for pointing to the macro, I wasn't quite sure how to do this in a C++11 way.

In C++11 we just copy into the capture :/ The only alternative is to use bind/deferred_call instead of the lambda.

I had problems with the packaged_task as it's non-copyable (I need the move). But I think I can do without the packaged_task and just do an hpx::async for that case (IMO neither more complicated nor simpler). Otherwise it's like you say, bind/deferred_call.

hkaiser · 2018-01-08T12:51:08Z

hpx/runtime/threads/detail/scheduled_thread_pool_impl.hpp

            {
-                hpx::this_thread::suspend();
+                util::detail::yield_k(k,
+                    "scheduled_thread_pool::remove_processing_unit_internal");
            }
        }


This loop looks like to be a good candidate for being extracted into a utility function (similar construct are used during shutdown as well, iirc).

You're right. The loops at shutdown are at least similar, I'll see what common functionality I can get out of there.

I've pulled out a yield_while function into yield_k.hpp, which takes a predicate for terminating the loop and some optional arguments (same as in yield_k). It doesn't really make the code shorter but I think the intention is clearer, and k is now hidden which I think is good.

Don't wait for suspended threads. Not compatible with futures and suspending hpx::get_worker_thread_num(). As future.get() suspends the thread and does not wake so that it can be stolen, waiting for suspended threads leads to lockup.

hkaiser

Very nice, thanks! LGTM.

msimberg added 13 commits December 15, 2017 11:34

Add suspend function to scheduled thread pool

9c8e2d2

Fix checks for suspending pool

9fef6b4

Add suspend function to thread_pool_base

9bde104

Fix missing else when suspending pool

8946ed6

Fix waiting for suspension with multiple pools

1009777

Add test for suspending pool

d808cda

Make asserts after scheduling loop exits more strict

a884c5a

Add test for suspending own pool

966a96a

Make thread_pool_base suspend/resume empty by default

3f67f0c

Stop background thread only when no more work is left

58b7366

Move locks for remove/add/suspend/resume_processing_unit

d1e0fcd

Cover the check for if the OS thread is joinable or not.

Allow suspending a pu which is already suspended

dcbb24f

Add test for suspending a pool with some threads already suspended

85d8041

msimberg added this to the 1.1.0 milestone Dec 15, 2017

hkaiser added category: resource_partitioner category: threadmanager type: enhancement labels Dec 16, 2017

hkaiser reviewed Dec 16, 2017

View reviewed changes

msimberg mentioned this pull request Dec 18, 2017

Suspension of runtime #3003

Closed

4 tasks

msimberg added 7 commits December 18, 2017 12:53

Yield OS thread when waiting for suspension outside runtime

dc3d5ea

Catch hpx::exception and explicitly test that it was thrown in suspen…

9c90a2f

…d_pool test

Require suspend for thread pools, assert in io_service_thread_pool im…

62eee0a

…plementations

Add missing include

3ff81cb

Add future-returning and callback version of thread pool suspend func…

f7bab55

…tion

Add test for suspending pool from outside runtime

cd4e4d8

Use yield_k for waiting in scheduled_thread_pool

305c769

Merge branch 'master' into suspend-pool

027d260

hkaiser reviewed Jan 8, 2018

View reviewed changes

msimberg added 3 commits January 9, 2018 16:02

Futurize all pool/thread suspension functions

cb71087

Use futurized suspension functions in resource tests

e1704ff

Change scheduling exit criteria for suspension

25a1ee5

Don't wait for suspended threads. Not compatible with futures and suspending hpx::get_worker_thread_num(). As future.get() suspends the thread and does not wake so that it can be stolen, waiting for suspended threads leads to lockup.

msimberg self-assigned this Jan 10, 2018

msimberg added 2 commits January 15, 2018 12:36

Factor out loops using yield_k

4687e72

Remove unnecessary yield loop in scheduled_thread_pool_impl

ec97d9e

hkaiser approved these changes Jan 15, 2018

View reviewed changes

Shorten long comment line in libfabric receiver.cpp

0273adc

hkaiser merged commit 479520e into STEllAR-GROUP:master Jan 17, 2018

This was referenced Jan 18, 2018

Revert "Suspend thread pool" #3108

Merged

Suspend thread pool #3110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suspend thread pool #3080

Suspend thread pool #3080

msimberg commented Dec 15, 2017

hkaiser Dec 16, 2017

msimberg Dec 18, 2017

hkaiser Dec 18, 2017

msimberg Jan 8, 2018

hkaiser Dec 16, 2017

msimberg Dec 18, 2017

hkaiser Dec 18, 2017

hkaiser Dec 16, 2017

msimberg Dec 18, 2017

hkaiser Dec 18, 2017

hkaiser Dec 16, 2017

msimberg Dec 18, 2017

hkaiser Dec 18, 2017

msimberg Dec 20, 2017 •

edited

hkaiser Dec 20, 2017

msimberg Dec 20, 2017

msimberg Jan 8, 2018

msimberg Jan 8, 2018

hkaiser Dec 16, 2017

msimberg Dec 18, 2017

msimberg Dec 18, 2017

hkaiser Jan 8, 2018

msimberg Jan 8, 2018

hkaiser Jan 8, 2018

hkaiser Jan 8, 2018

msimberg Jan 8, 2018 •

edited

hkaiser Jan 8, 2018 •

edited

msimberg Jan 8, 2018

hkaiser Jan 8, 2018

msimberg Jan 8, 2018

msimberg Jan 15, 2018

hkaiser left a comment

Suspend thread pool #3080

Suspend thread pool #3080

Conversation

msimberg commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimberg Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimberg Jan 8, 2018 • edited

Choose a reason for hiding this comment

hkaiser Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser left a comment

Choose a reason for hiding this comment

msimberg Dec 20, 2017 •

edited

msimberg Jan 8, 2018 •

edited

hkaiser Jan 8, 2018 •

edited