Add runtime setters for MULTI_POLL reactor count, dispatch, and request ceiling - #1049
Add runtime setters for MULTI_POLL reactor count, dispatch, and request ceiling#1049Matt711 wants to merge 3 commits into
Conversation
Matt711
added
feature request
madsbk
left a comment
madsbk
left a comment
There was a problem hiding this comment.
Overall looks good.
- update the stale sentence at
runtime_settings.rst:82 - add missing docs for three newly user-facing settings
| void defaults::set_remote_io_num_reactors(unsigned int num_reactors) | ||
| { | ||
| KVIKIO_EXPECT( | ||
| num_reactors > 0, "remote_io_num_reactors must be a positive integer", std::invalid_argument); | ||
| instance()->_remote_io_num_reactors = num_reactors; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it would be helpful if we could throw if the reactor pool has already been started?
Maybe add something like:
// multi_poll_reactor.hpp
static bool is_instantiated() noexcept;
// multi_poll_reactor.cpp
namespace { std::atomic<bool> _pool_instantiated{false}; }
bool MultiReactorPool::is_instantiated() noexcept
{
return _pool_instantiated.load(std::memory_order_acquire);
}
MultiReactorPool::MultiReactorPool() : _dispatch{defaults::remote_io_reactor_dispatch()}
{
...
_pool_instantiated.store(true, std::memory_order_release); // end of ctor
}
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
There was a problem hiding this comment.
nit: Like Mads, I think we should throw if this set call would not take effect.
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
|
|
||
| void defaults::set_remote_io_reactor_dispatch(RemoteReactorDispatch dispatch) | ||
| { | ||
| instance()->_remote_io_reactor_dispatch = dispatch; |
There was a problem hiding this comment.
Yeah, I think especially because the getters will return this value (even if it was not the one used for setting up the multi-poll instance), we should throw. Otherwise inspecting the defaults later to report behaviour will be misleading.
| * runtime, overriding `KVIKIO_REMOTE_IO_NUM_REACTORS`. | ||
| * | ||
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
There was a problem hiding this comment.
I agree with the suggestion. Just want to point out that for the BS thread pool used for local I/O and easy backend, we do support changing the number of threads midway at runtime, which would block the calling thread, drain the task queue, destroy the worker threads, and then recreate a new set of worker threads. So I think the phrasing "like the default thread pool" should be removed from the doc string here.
4 participants
Adds setters for
remote_io_num_reactors,remote_io_reactor_dispatch, andremote_io_max_concurrent_requestsMULTI_POLLbackend NVIDIA/cudf#23839