Conversation
softins
force-pushed
the
faster-channel-lookup
branch
from
d0a589d to
98d4d6c
Compare
| emit ClientDisconnected ( iCurChanID ); // TODO do this outside the mutex lock? | ||
| } | ||
|
|
||
| FreeChannel ( iCurChanID ); // note that the channel is now not in use |
There was a problem hiding this comment.
Should this be before telling the JamController the client left? (JamController has its own structures to worry about IIRC.)
There was a problem hiding this comment.
I didn't think so. I thought it would be better if the channel still existed at the point the jam controller was notified. But in fact the channel object continues to exist - FreeChannel() just removes it from the channel order list and the binary search.
But I admit I haven't yet tested this code with recording enabled. It would be good to do so.
There was a problem hiding this comment.
Well I've run a test server with recording enabled, and some test clients. All appears to work ok. I've also looked at the code for CJamRecorder::OnDisconnected(), and there doesn't appear to be anything that would interact with the server.cpp code.
There was a problem hiding this comment.
I'm okay if it makes no difference.
|
I can't imagine it causes any measurable performance degradation on small servers (only 22 out of 145 servers on Any Genre 1 are 25 or above, all the rest lower, the vast majority on the default of 10). The memory overhead is also trivial, even for 150 channels as it's just ints. All looks pretty good. Shame the Windows auto-build is currently broken for some reason. |
|
Needs a rebase once #1981 is merged. Please ping me if I‘m needed here (approval). As I said I won’t comment much on the code. |
This is preparatory for improving the performance of FindChannel() with high numbers of channels. Also call IsConnected() separately from GetAddress().
The old algorithm used sequential search, which was very inefficient for large numbers of channels. This is now changed to use a binary search, by maintaining an array of channel IDs ordered by IP+port. The actual ordering is not important, so long as it is deterministic.
softins
force-pushed
the
faster-channel-lookup
branch
from
98d4d6c to
be326c0
Compare
|
OK, spent time with the client - it's had no unexpected adverse effects there (given it shouldn't touch it at all). I've not had a chance to try the server and I don't run with sufficient load to notice, I would expect. I'll get it installed now. Oh, command line ... to the extent I'm happy it's sane and sound.
|
There was a problem hiding this comment.
Tried locally on my intel core m5 (skylake) laptop and sound is still ok/so-so with 15 clients (however, the server is probably coming close to its limits and my device is not powerful at all).
Scanned the code
Installed it locally
... and I trust Peter that everything else is ok ;-).
So happy to approve
|
BTW: maybe there are also other places where binary search might be useful. Didn't check the code though. |
Yes, I plan to have a look when I get time. |
…faster-channel-lookup"" This reverts commit b69c3b3.
3 participants
Improve the performance of
FindChannel(), using binary search instead of linear.Context: Fixes an issue?
Every received audio or connected protocol packet needs to call
FindChannel()to find the channel ID fromthe incoming IP address and port number. Currently,
FindChannel()uses a simple linear search, which becomesvery inefficient when there is a large number of active channels. Later connections will always need to search
most of the
vecChannels[]array to find the matching channel.This PR introduces a channel order array
vecChannelOrder[], which lists the active channel IDs in sorted order.FindChannel()can then use a binary search to locate the channel. For 100 active channels, this would then need on averageabout 6 or 7 comparisons instead of an average of 50 or more for a linear search.
A function
FreeChannel()is also introduced to return a disconnected channel to the free list. Also a separate Mutex to protect the channel order array.Does this change need documentation? What needs to be documented and how?
No documentation - just a performance enhancement.
Status of this Pull Request
Working and tested with up to 25 channels, showing a small reduction in CPU usage. The performance improvement would likely be more significant with a larger number of channels. But in any case, the improved algorithm is worth having.
What is missing until this pull request can be merged?
More testing is always good, but I am confident in the algorithm.
Checklist