Add a bypass scheduler to improve mpi continuations #1195

biddisco · 2024-06-27T14:10:05Z

I would like this to go through testing ASAP, so I'm pushing it, but it can be can be considered a draft PR for now
Relies on #1151 and #1180

Here, we add a stdexec style scheduler that executes a task/function - it behaves like the main pika scheduling loop - by takinf the function, wrapping it in a task, then switching context to it immediately, so that if the task later tries to suspend, then there are no problems and the task goes ontoo the queues as usual.

It is an error to call this on a pika thread and is only supported by certain modes of the mpi continuations

codacy-production · 2024-06-27T14:11:52Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.45% (target: -1.00%)	❌ 82.81% (target: 90.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`1e052bc`)	18376	13721	74.67%
Head commit (`7867f22`)	18360 (-16)	13791 (+70)	75.11% (+0.45%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1195)	64	53	82.81%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

msimberg

Just a few initial comments. This adds a lot of complexity and I'll need a bit more time to look at this.

msimberg · 2024-07-04T11:47:10Z

libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp

@@ -32,6 +32,8 @@
 #include <type_traits>
 #include <utility>

+#define PIKA_MPI_ENABLE_EARLY_POLL


Can we make this a proper CMake configuration option, or a runtime configuration option?

msimberg · 2024-07-04T11:48:36Z

libs/pika/async_mpi/include/pika/async_mpi/mpi_polling.hpp

+            handler_method h = get_handler_method(mode);
+            /// these task modes always trigger continuations on a pika task and can be safely inlined
+            return (h == handler_method::yield_while) ||
+                (h == handler_method::suspend_resume) | (h == handler_method::new_task);


Suggested change

(h == handler_method::suspend_resume) | (h == handler_method::new_task);

(h == handler_method::suspend_resume) || (h == handler_method::new_task);

msimberg · 2024-07-04T11:50:47Z

libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp

@@ -77,29 +79,47 @@ namespace pika::mpi::experimental {
                execution::thread_priority::boost :
                execution::thread_priority::normal;

+            auto dgb = []() {


Short for degub? Could you guard this with PIKA_DEBUG, otherwise it'll be an unused variable in release builds (with corresponding warnings)? And perhaps give it a slightly longer name?

msimberg · 2024-07-04T11:53:01Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+    static print_threshold<Level, bplevel> bps_deb("SBYPASS");
+}    // namespace pika::debug::detail
+
+namespace pika { namespace execution { namespace experimental {


Suggested change

namespace pika { namespace execution { namespace experimental {

namespace pika::execution::detail {

if we're not intending to make this a user-facing scheduler (I think I'd prefer that anyway) in the short term?

msimberg · 2024-07-04T11:59:27Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+        // TODO: Can we simply dispatch to the default implementation? This is
+        // disabled with the P2300 reference implementation because we don't
+        // want to use implementation details of it.
+#if !defined(PIKA_HAVE_P2300_REFERENCE_IMPLEMENTATION)


This was changed in March to be PIKA_HAVE_STDEXEC.

msimberg · 2024-07-24T07:26:40Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+                throw std::runtime_error("Bypass scheduler - already on task thread");
+                return;


Suggested change

throw std::runtime_error("Bypass scheduler - already on task thread");

return;

PIKA_THROW_EXCEPTION(pika::error::invalid_status, "thread_pool_scheduler_queue_bypass::execute", "Already on a pika task");

return;

(with appropriate line wrapping)

msimberg · 2024-07-24T07:30:35Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+            if (!threaddata->set_state_tagged(thread_schedule_state::active, task_state,
+                    active_state, std::memory_order_relaxed))
+            {
+                throw std::runtime_error("Thread state cannot fail here");


Suggested change

throw std::runtime_error("Thread state cannot fail here");

PIKA_THROW_EXCEPTION(pika::error::no_success, "thread_pool_scheduler_queue_bypass::execute", "Setting thread state failed");

msimberg · 2024-07-24T07:35:00Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+                thread_state(task_return.first, task_state.state_ex(), task_state.tag() + 1);
+
+            // if the threaddata->state still matches active_state update to new state
+            // (could be stolen/changed by another thread here?)


Could it? I don't think it could be stolen if it's not in any queue...

msimberg · 2024-07-24T07:35:45Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+                case thread_schedule_state::pending:
+                {
+                    data.scheduler_base->schedule_thread(id, thread_schedule_hint(thread_num),
+                        false /*allow_fallback*/, execution::thread_priority::high);


Why should this be high priority?

msimberg · 2024-07-24T07:36:17Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+                    // thread sits in thread map until resumed
+                    return;
+                }
+                default: throw std::runtime_error("fix this thread state type");


Can you update all of the plain throws to use PIKA_THROW_EXCEPTION with some appropriate error (no_success if nothing else).

msimberg · 2024-07-24T07:36:30Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+                break;
+                case thread_schedule_state::suspended:
+                {
+                    std::cout << "thread_schedule_state::suspended" << std::endl;    //


Please remove or replace with our logging.

msimberg · 2024-07-24T07:40:08Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+            thread_description desc(
+                f, (fallback_annotation != nullptr) ? fallback_annotation : "Bypass");


I think the fallback annotation should be non-null. Maybe replace with an assert and

Suggested change

thread_description desc(

f, (fallback_annotation != nullptr) ? fallback_annotation : "Bypass");

thread_description desc(f, fallback_annotation);

?

msimberg · 2024-07-24T07:40:37Z

libs/pika/threading_base/include/pika/threading_base/print.hpp

+namespace pika::threads::detail {
+    class thread_data;    // forward declaration only
+}
+


Why do we do a forward declaration here instead of including the header?

msimberg · 2024-07-24T07:41:03Z

libs/pika/schedulers/include/pika/schedulers/shared_priority_queue_scheduler.hpp

@@ -56,7 +56,7 @@
 namespace pika::debug::detail {
    // a debug level of zero disables messages with a level>0
    // a debug level of N shows messages with level 1..N
-    constexpr int debug_level = 0;
+    constexpr int debug_level = 9;


Should this be disabled?

msimberg · 2024-07-24T07:41:31Z

libs/pika/executors/include/pika/executors/thread_pool_scheduler_queue_bypass.hpp

+namespace pika::debug::detail {
+    // a debug level of zero disables messages with a level>0
+    // a debug level of N shows messages with level 1..N
+    constexpr int bplevel = 0;


Suggested change

constexpr int bplevel = 0;

inline constexpr int bplevel = 0;

If pika tries to use completion modes and the user did not setup a pool, the default pool must be returned to prevent accessing one that has not been created.

Ensure polling is started and handle MPI errors more generically Remove return type from mpi transform Changes to mpi internals broke the RAII polling setup.

To test continuations, we need to add 16 to the numeric mpi modes so detecting the presence at configure time is necessary

…tion

mpi transform tests using set_error are triggering stack corruption errors that might or might not be real, but disabling them (for now) will clean up the dashboard

Co-authored-by: Mikael Simberg <[email protected]>

… thread use

The existing mode uses suspend/resume which can be problematic if threads suspend and block, but in principle we should directly execute continuations as function calls which this new mode does

Callbacks to sender set_value/error should not be triggered directly on a polling thread as continuations will be called on the polling thread. Instead the execution should be tranferred to a pika thread/task. Adding a transfer adaptor costs task creation, plus enqueuing/dequeuing of the task, so the bypass scheduler supports inline execution, but with the creation of a task wrapper around the callback so that it is safe to suspend/resume etc and no raw scheduler/polling threads are running user code.

Bypass scheduler must never be run on top of an existing pika task, otherwise coroutine self pointer is clobberred (thread local). Rearrange transfers in transform_mpi to better route inline continuations through the bypass scheduler and skip it when not needed

We do not need the RAII state restorer object as our thread creation is always on the executing thread and certain race conditions are not valid Improve comments and remove unsupported API features

The bypass needs to be added to the cuda polling, but this termporary fix inserts and extra bypass before issuing mpi requests if the thread id is invalid. Also, if the bypass scheduler is invoked on a thread that is already a task, it skips task creation and becomes a simple pass through execution.

biddisco requested review from aurianer and msimberg as code owners June 27, 2024 14:10

biddisco force-pushed the mpi-bypass branch 2 times, most recently from 77f0eb5 to 36fedf3 Compare July 1, 2024 08:37

msimberg assigned biddisco Jul 2, 2024

biddisco force-pushed the mpi-bypass branch from c5febb9 to f5aa963 Compare July 4, 2024 11:19

msimberg reviewed Jul 4, 2024

View reviewed changes

msimberg requested changes Jul 24, 2024

View reviewed changes

biddisco and others added 15 commits July 25, 2024 11:13

Remove accidentally duplicated init of mpi

1092ab3

Fix bug fetching non existant mpi pool

c617fdd

If pika tries to use completion modes and the user did not setup a pool, the default pool must be returned to prevent accessing one that has not been created.

Ensure completion mode commmand line and ENV var defaults match

a4c4202

Fix mpi algorithm test

a8246e2

Ensure polling is started and handle MPI errors more generically Remove return type from mpi transform Changes to mpi internals broke the RAII polling setup.

Fix inshpect violation

7f86541

Fix a warning (that is not valid, but annoying)

e4aecd2

Remove debug message left in by mistake

fab14c9

Stop command line completion mode from clobbering $ENV var if set

7adc4f6

Fix review comments

c907efb

Get fallback MPI completion mode from runtime configuration

980b5b4

Fix review comments: remove incorrect doc/comments

27e5ba7

Fix (intentional) error msg detection in mpi algorithm test

f3a3b07

Fix pika::util::detail::function lookup fail

058e27d

Detect MPIx continuations at configure time to improve testing

eed85ed

To test continuations, we need to add 16 to the numeric mpi modes so detecting the presence at configure time is necessary

Fix copyright header on new material

2148ea8

biddisco force-pushed the mpi-bypass branch from f5aa963 to 2473eb1 Compare August 5, 2024 18:16

biddisco and others added 5 commits August 5, 2024 20:19

use try_compile (instead of try run) test for mpi continuations detec…

a58f844

…tion

Move cmake tests/check for openmpi to cmake folder

b7b61f4

Disable sanitizer checks for troublesome error handler tests

ac45f96

mpi transform tests using set_error are triggering stack corruption errors that might or might not be real, but disabling them (for now) will clean up the dashboard

add repeat-until-fail to tests to better check mpi testing branch

2128b42

Update cmake/tests/check_openmpi_continuations.cpp

f9b583c

Co-authored-by: Mikael Simberg <[email protected]>

biddisco added 18 commits August 5, 2024 20:19

clang-format cleanup

14024b0

Use MPIX_CONT_POLL_ONLY in mpi continuation polling to better control…

e8d51df

… thread use

Simplify the exception error msg generation (asan flagging problems)

bfc0a4e

make sure transfer is always after trigger to ensure correct behaviour

1740ade

Add new MPIX continuation mode that executes continuations directly

c398315

The existing mode uses suspend/resume which can be problematic if threads suspend and block, but in principle we should directly execute continuations as function calls which this new mode does

Remove old MPIx suspend mode and use continuation mode by default

6941f7a

Cleanup namespace "detail" usage (remove unnecessary uses)

6a01cbc

Fix a thread create error - do not schedule was being ignored

9f0d830

Clean up bypass scheduler, remove debug, extraneous code

fad487d

We do not need the RAII state restorer object as our thread creation is always on the executing thread and certain race conditions are not valid Improve comments and remove unsupported API features

Adding more (shared scheduler) debug info - especially threadinfo

548f2d5

Fix inshpect/spelling violations

2fa4aed

Disable debug messages

53b29da

Fix CMakeLists header check violation

1cb5a17

Hide dbg() lambda inside PIKA_DEBUG ifdef

5f581dc

Use thread id check to decide if bypass scheduler is needed

cc1941d

Fix a bad error code use

fcbf6bb

biddisco force-pushed the mpi-bypass branch from 2473eb1 to fcbf6bb Compare August 5, 2024 18:22

biddisco added 2 commits August 7, 2024 08:27

Remove obsolete cout message

7867f22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a bypass scheduler to improve mpi continuations #1195

Add a bypass scheduler to improve mpi continuations #1195

biddisco commented Jun 27, 2024

codacy-production bot commented Jun 27, 2024 •

edited

Loading

msimberg left a comment

msimberg Jul 4, 2024

msimberg Jul 4, 2024

msimberg Jul 4, 2024

msimberg Jul 4, 2024

msimberg Jul 4, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

msimberg Jul 24, 2024

	(h == handler_method::suspend_resume) \| (h == handler_method::new_task);
	(h == handler_method::suspend_resume) \|\| (h == handler_method::new_task);

	namespace pika { namespace execution { namespace experimental {
	namespace pika::execution::detail {

		throw std::runtime_error("Bypass scheduler - already on task thread");
		return;

	throw std::runtime_error("Thread state cannot fail here");
	PIKA_THROW_EXCEPTION(pika::error::no_success, "thread_pool_scheduler_queue_bypass::execute", "Setting thread state failed");

		thread_description desc(
		f, (fallback_annotation != nullptr) ? fallback_annotation : "Bypass");

	thread_description desc(
	f, (fallback_annotation != nullptr) ? fallback_annotation : "Bypass");
	thread_description desc(f, fallback_annotation);

Add a bypass scheduler to improve mpi continuations #1195

Are you sure you want to change the base?

Add a bypass scheduler to improve mpi continuations #1195

Conversation

biddisco commented Jun 27, 2024

codacy-production bot commented Jun 27, 2024 • edited Loading

Coverage summary from Codacy

msimberg left a 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

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

codacy-production bot commented Jun 27, 2024 •

edited

Loading