Add makePool method on ThreadMap#283
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
rustaceanrob
changed the title
makePool method on ThreadMapmakePool method on ThreadMap
rustaceanrob
force-pushed
the
26-5-29-pooled-threads
branch
from
d56e637 to
158f30d
Compare
rustaceanrob
changed the title
makePool method on ThreadMapmakePool method on ThreadMap
| kj::Promise<void> ProxyServer<ThreadMap>::makePool(MakePoolContext context) | ||
| { | ||
| EventLoop& loop{*m_connection.m_loop}; | ||
| const uint32_t count = context.getParams().getCount(); | ||
| for (uint32_t i = 0; i < count; ++i) { | ||
| const std::string name = "pool/" + std::to_string(i); | ||
| std::promise<ThreadContext*> thread_context; | ||
| std::thread thread([&loop, &thread_context, name]() { | ||
| g_thread_context.thread_name = ThreadName(loop.m_exe_name) + " (" + name + ")"; | ||
| g_thread_context.waiter = std::make_unique<Waiter>(); | ||
| Lock lock(g_thread_context.waiter->m_mutex); | ||
| thread_context.set_value(&g_thread_context); | ||
| g_thread_context.waiter->wait(lock, [] { return !g_thread_context.waiter; }); | ||
| }); | ||
| auto thread_server = kj::heap<ProxyServer<Thread>>(m_connection, *thread_context.get_future().get(), std::move(thread)); | ||
| m_connection.m_thread_pool.push_back(m_connection.m_threads.add(kj::mv(thread_server))); | ||
| } | ||
| return kj::READY_NOW; | ||
| } |
There was a problem hiding this comment.
Repeated calls silently grow the pool beyond the intended size, and since naming is based on the loop index starting from 0 each time, you'd end up with duplicate names like two threads both called pool/0 making logs ambiguous.
I'm not sure what would be the correct approach but I thought about:
- Return an error if m_connection.m_thread_pool is not empty
- Replace the old pool by cleaning the m_connection.m_thread_pool and create a fresh one with the new count. This makes sense if the client needs to resize its dedicated pool.
There was a problem hiding this comment.
Added a guard statement:
if (!m_connection.m_thread_pool.empty()) {
throw std::runtime_error("makePool called on connection with existing pool");
}I think throwing is appropriate here as I would like to get user feedback first. If they anticipate pool resizing during runtime will be valuable we can add resizing APIs
rustaceanrob
force-pushed
the
26-5-29-pooled-threads
branch
3 times, most recently
from
d5bc804 to
8050527
Compare
This patch introduces a pool of threads to the `Connection` class, and allows this pool to be populated with the thread map via `makePool`. When a client thread is not set in a request context, it is delegated to the pool. This is unable to handle the guarentees with server-invoked callbacks that the current API offers, but these callbacks are not yet present in the interface. The pool is implemented as round-robin as it is simplest, but perhaps the pool could be a queue of requests with work-stealing for threads that are available. This was raised to me by Rust users, as they did not particularly care where work is executed on the server-side, but they have to set the thread regardless. ref: https://github.com/2140-dev/bitcoin-capnp-types/blob/master/tests/util/bitcoin_core.rs#L149
rustaceanrob
force-pushed
the
26-5-29-pooled-threads
branch
from
8050527 to
0ac739b
Compare
|
Concept ACK. This is a good idea that should make development of rust & python clients easier, while making the behavior opt-in and not affecting c++ clients or existing clients using current behavior. With this feature, we may also want to add a way to mark certain methods that can take long time before returning to not use the thread pool, so they do not block other work. These methods could create their own threads to run on, or require threads to be specified.
Actually I think this is not an issue. If a server method is called with a callbackThread value it can still pass the same value back to the client when making the callback, no matter where the server thread is running.
Yes I need to look more closely at current implementation, but would seem better to have a single queue of waiting tasks instead of giving each thread its own queue. Otherwise execution order could be very unpredictable and if a single request is slow, it could create a backlog of requests assigned to the same thread. It seems like it should be possible to have a shared queue without too much complexity, so probably worth looking into. |
|
Concept ACK. This feature significantly reduces integration burden for clients written in other programming languages. I just glanced at it, planning to review it deeply soon. One thing to note: This PR will definitely need release notes. |
5 participants
This patch introduces a pool of threads to the
Connectionclass, and allows this pool to be populated with the thread map viamakePool. When a client thread is not set in a request context, it is delegated to the pool. This is unable to handle the guarentees with server-invoked callbacks that the current API offers, but these callbacks are not yet present in the interface.The pool is implemented as round-robin as it is simplest, but perhaps the pool could be a queue of requests with work-stealing for threads that are available.
This was raised to me by Rust users, as they did not particularly care where work is executed on the server-side, but they have to set the thread regardless.
Tests in Rust can be seen here: 2140-dev/bitcoin-capnp-types#24
ref: https://github.com/2140-dev/bitcoin-capnp-types/blob/master/tests/util/bitcoin_core.rs#L149
ref: #281