Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent iteration over inactive peers #266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dimage1
Copy link

@dimage1 dimage1 commented Sep 23, 2024

Improves enet_protocol_send_outgoing_commands by iterating over active peers only.

@bjorn
Copy link

bjorn commented Sep 25, 2024

Seems like a good idea. But I'm just wondering, what kind of performance improvement are you seeing with this patch? It seems to me that the maximum number of peers would have to be rather high for this to matter.

@mman
Copy link

mman commented Sep 25, 2024

@bjorn @dimage1 sorry to hijack the discussion, I did some extensive profiling in the past months and send_outgoing_commands is indeed super expensive operation as it walks all peers, and all outgoing commands for each peer to see what is confirmed and what needs to be resent so it is essentially O(N*M). (N being number of peers and M average number of reliable outgoing commands in a queue for peer).

My proposal to fix this would be to introduce a proper C implementation of a hash table to track active peers and to track outgoing commands for each peer for faster lookup, and solve the problem once and for all.

This fix is a partial way forward in that it walks the list of all peers to derive list of active peers on any peer state change. So it reduces number N. But it still walks the list of sent outgoing commands in a linear fashion (number M) to see what to confirm and what to resend. Depending on what type of app you have it may or may not actually help. I for example have an app where peers come and go rather frequently, so in my case this fix will not actually help, only will it shift load elsewhere.

@dimage1
Copy link
Author

dimage1 commented Sep 25, 2024

@bjorn I considered this for 256 - 4096 peers per 1 instance, e.g. on a relay / proxy / dedicated server with several simultaneous sessions and a short frame time (1 - 20ms, which equals to 1000 - 50 FPS).
@mman the change indeed more helps on rather long lasting connections (e.g. 1 - 60 minutes), where a server himself is underpopulated, leaving quite some empty peer slots. Your M number concern looks reasonable and the hash table idea could be a good fix / improvement! I'm not sure when I will have time to try it though.

@mman
Copy link

mman commented Sep 25, 2024

@dimage1 The hash table thingy is something I have in mind for a long time, but have not got around to implement. It's is trickier than it looks on the first thought. I just wanted to confirm your observation that...

  1. Storage of the peers / tracking of active peers
  2. Storage of outgoing reliable commands, and their retransmissions

are indeed the main source of performance problems in enet and are worth working on improving...

@mman
Copy link

mman commented Sep 25, 2024

@dimage1 @bjorn Dropping this here for when I get around to implementing it: https://benhoyt.com/writings/hash-table-in-c/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants