Private/caolan/async perf #9069
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
caolanm
force-pushed
the
private/caolan/async_perf
branch
4 times, most recently
from
2e407cb
to
3130e7c
Compare
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: Ie211fe596629af50eec01dd8512d2a12929545ea
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: I577af4280ac5a0a4389bb11ac6c531d846a196d1
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: I2d8ab8f3c7e42d6b2f80d68861e54dca9fc4adf2
as a separate stage getCapabilitiesJson turns out to not be called in the MOBILEAPP case so put inside !MOBILEAPP ifdef and remove unused branch Signed-off-by: Caolán McNamara <[email protected]> Change-Id: Ie144e4612aa36be88169e44d7eda7825b49a03c8
queue addresses that need resolution, use async dns to resolve them and when final result known dispatch the capabilities to be sent Signed-off-by: Caolán McNamara <[email protected]> Change-Id: I13b6d0c4d47e6e8ecd06f7a449c8f808a41e5e7a
caolanm
force-pushed
the
private/caolan/async_perf
branch
from
3130e7c
to
1794e4c
Compare
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: I1e2d818c0f5b678f3de179b6fb7ce747f9e76ac3
|
|
||
| Lookup lookup = _lookups.front(); | ||
| _lookups.pop(); | ||
| std::string hostToCheck, exception; |
There was a problem hiding this comment.
Seems we hold the lock while we do the slow synchronous lookup (?) =)
|
|
||
| void AsyncDNS::addLookup(const std::string& lookup, const DNSThreadFn& cb) | ||
| { | ||
| std::unique_lock<std::mutex> guard(_lock); |
There was a problem hiding this comment.
Given the above - superficially it looks like this will still block the main thread while trying to push a lookup if the other thread is doing a slow sync. lookup (?) - which might defeat the parallelism surely ?
| { | ||
| net::AsyncDNS::DNSThreadFn pushHostnameResolvedToPoll = [this](const std::string& hostname, | ||
| const std::string& exception) { | ||
| COOLWSD::getWebServerPoll()->addCallback([this, hostname, exception]() { |
There was a problem hiding this comment.
Great - glad we push into the right thread at the end =) I wonder if API-wise we might want to take a SocketPoll & - as well as a function - so it's clear in the API that we will always want a callback to happen in the right thread and it's not possible to re-use the async-DNS stuff outside that safety-net ;-) but - fine for now I guess.
|
|
||
| void startThread(); | ||
| void joinThread(); | ||
| }; |
There was a problem hiding this comment.
Would like to have a dumpState() mechanism that chains to the classes waiting on things too - so we can see all the pending work in the queue, and where it goes to etc. on the console as/when we're head-scratching over some odd behavior here :-)
| @@ -1284,7 +1377,7 @@ void ClientRequestDispatcher::handlePostRequest(const RequestDetails& requestDet | |||
| requestDetails.equals(1, "get-thumbnail")) | |||
| { | |||
| // Validate sender - FIXME: should do this even earlier. | |||
| if (!allowConvertTo(socket->clientAddress(), request)) | |||
| if (!allowConvertTo(socket->clientAddress(), request, nullptr)) | |||
There was a problem hiding this comment.
Surely this is still rather synchronous with a nullptr callback ?
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: If8ee89ef0e7436675c596461243d82a2e0412358
Signed-off-by: Caolán McNamara <[email protected]> Change-Id: Ib40a6d1e3d6e983da674c5a7051ac5e7a565d0d1
caolanm
force-pushed
the
private/caolan/async_perf
branch
from
35b14ee
to
badb33a
Compare
Summary
TODO
Checklist
make checkmake runand manually verified that everything looks okay