Tweaks to reduce time holding locks #128
Conversation
The order stream ids are used should not matter
This avoids scanning through the 128 promises array for a free slot while holding the lock.
This way, locks are only required around access to the instance variable and not when modifying the list of connections. Given that the list of connections is expected to stay relatively stable, this should optimize the common case. This was already encapsulated by the snapshot method, so the remaining methods were rewritten to use that.
First, use the response passed in, to not rely on instance state when not really needed. Secondly, the early return doesn't seem very useful, given that each will not do anything for an empty list anyhow.
If we receive multiple responses in one batch, then this will flush the request queue only once per batch instead of once per response. In order to achieve this, the promise fulfillment was moved outside complete request.
|
What's the worst case scenario visibility-wise with this change? Can we be sure that all threads will eventually see new connections, and that no threads will use closed connections? (and if they do see closed connections that that will not be a problem) |
Assuming you are talking specifically about the connection-manager change, I think the main difference lies in the each-method, where you previously could not call any methods from the connection manager from the callback, as that would result in a thread-error, whereas the new code allows retrieving a possibly inconsistent state from this callback. Obviously, connected, snapshot and random connection can return stale results now, but nothing prevented the results from becoming stale once the method returned before, so I'm not sure that distinction matters. Given that we still protect the instance variable access with the lock, I think the updated list should be visible to all other threads within reasonable time, and thus the only thing preventing new connections from appearing ought to be holding on to the lock forever, which was certainly possible before (with each), but shouldn't be possible now. |
See individual commits for details.