chore: supporting parallel libwaku requests #3273
Conversation
gabrielmer
force-pushed
the
chore-supporting-parallel-libwaku-requests
branch
from
cd30276 to
57951b2
Compare
gabrielmer
requested review from
Ivansete-status and
richard-ramos
and removed request for
Ivansete-status
|
You can find the image built from this PR at Built from 28ed4e8 |
| @@ -96,6 +99,7 @@ proc sendRequestToWakuThread*( | |||
| ): Result[void, string] = | |||
| let req = WakuThreadRequest.createShared(reqType, reqContent, callback, userData) | |||
| ## Sending the request | |||
| ctx.lock.acquire() | |||
There was a problem hiding this comment.
what happens with this lock during the return err... below?
There was a problem hiding this comment.
why is the lock even here? channels are mpmc, you should not need a lock..
There was a problem hiding this comment.
This is a ChannelSPSCSingle channel from taskpools. According to comments in its code, it can only buffer a single element. We observed the following behavior: When you try to call trySend more than once without having received any pending value on the other end of the channel, trySend will return false. With this lock we wait until the other end receives the value.
There was a problem hiding this comment.
I do wonder if we should instead use nim's builtin Channel instead of ChannelSPSCSingle. I just saw that it has a send that is blocking, as well as allowing to specify a max number of items in its buffer... 🤔
There was a problem hiding this comment.
Yes! So I first tried to move it to a Channel and had race conditions, not because of the Channel itself but because of how we designed the mechanism of sending signals between threads. It is designed in such a way that assumes that a signal won't be sent if we have a pending request.
I first wanted to try if we could have it working without redesigning too much. Given that the design already assumed there won't be concurrent requests to the channel and the only way for it to work was adding a lock, then ChannelSPSCSingle worked perfectly too so I switched back to it.
After attempting to add a lock to the existing code, it resolved all the race conditions and performed really well - the only period it is locked is between the time a request is received to the moment it starts getting processed, but multiple requests can be processed in parallel.
I actually commented @Ivansete-status that we could refactor and redesign the mechanism of sending signals between threads to work without assuming that requests come one at a time but the current version performed so well that it seems to be an optimization that we can take care of if we see that this becomes a bottleneck.
Not sure if it makes sense, lmk if that's not the case :)
There was a problem hiding this comment.
starts getting processed, but multiple requests can be processed in parallel.
"starts getting processed" in the loose sense of the term here, ie if the processing thread is running a synchronous computation that takes a while, it will not be removing things from the queue so all the other requests will form a line - the only "parallel" processing you get is what async gives you and these requests might end up getting blocked. This is probably worth documenting at least and it is what an MP queue would solve.
There was a problem hiding this comment.
"starts getting processed" in the loose sense of the term here, ie if the processing thread is running a synchronous computation that takes a while, it will not be removing things from the queue so all the other requests will form a line - the only "parallel" processing you get is what
asyncgives you and these requests might end up getting blocked
True! But isn't that the case also with a MP queue? in the sense that if there's a sync operation in the processing thread that takes lots of computation, the queue will start getting filled with requests too.
The MP queue helps us add elements to the queue concurrently, but the actual execution of the requests should be the same in both cases right? or maybe I'm missing something?
There was a problem hiding this comment.
I agree that we should document this! It's clear that a MP queue is a better design and we should not forget about it, this PR is only based on the fact that this solution works really well without the need of rearchitecting the logic of signaling between threads - so we can continue focusing on developing/fixing things critical for the integration of nwaku into Status Desktop and then optimize if needed
There was a problem hiding this comment.
the queue will start getting filled with requests too.
the queue will hold the work but the thread that places the work on the queue can move on - with the lock, it will instead be blocked until the receiver has removed the item.
the "usual" way to do mp*c queues is to bound them to a fixed number of items and have a policy (block/drop) for what to do when the consumer is too slow - but when there's only one spot in the queue the blocking unnecessarily happens during short bursts even if the average production is lower than the capacity of the consumer.
There was a problem hiding this comment.
Oh makes sense! Didn't think about that!
So 100% let's take care of it now - it can have a greater performance impact than what I envisioned.
Will close this PR and work on a MP*C solution.
Thanks so much for the help!
Description
Adding a lock so multiple requests sent by different threads can be handled in parallel.
The lock applies only from the moment a task enters the queue to the moment it starts getting handled. Many requests can be handled in parallel, however, the requests are communicated to the Waku Thread one at a time.
Changes
Issue
#3076