Excessive lock contention about gamenetworkingsockets HOT 18 CLOSED

@nxrighthere
MERRY CHRISTMAS!

I've got a change to implement fine-grained locking. I'd be very interested, if you try it out, how performance behaves, or if you even hit any bugs.

Let me know if you have a chance to look at this!

https://github.com/ValveSoftware/GameNetworkingSockets/tree/fine-grained-locking

from gamenetworkingsockets.

When thinking about optimizations, I think it's important to have a specific use case in mind. We intend to be a general purpose networking library, so I don't want to have very bad perf for an important use case. But right now lock contention hasn't been an issue in practice. We have had at least one user of this lib use a process model with a high number of threads and connections all within the same process. They were initially concerned about the global lock, as well, but we never did find any actual perf problems. It was an intentional design to keep the multi-threading design as simple as possible, and I'm hesitant to deviate from that design without a specific customer with a real world use case that is experiencing perf problems. Then we can look at more granular locking strategies.

At Valve, our approach is to use multi-process when we want to host thousands of players on a single box, and use a relatively small number of threads per process. This is how the SDR relays work -- they are single-threaded, and this is also how most of the different types of steam servers are designed to scale to fully utilize the CPU power of a box. Because multiple threads in the same process is just much more complicated. I personally think multi-process is the better way to skin the cat, but I also know that it doesn't work everywhere.

Also, this is not visible in the opensource code, but the relayed connections involve a lot more sharing of global data structures. E.g. two different connections might share a connection to one relay, but each have a separate session with two other relays, and the information about ping times used for route selection is all global. More granular locking would be very complicated.

from gamenetworkingsockets.

Hey @nxrighthere just checking in to see if you think you might have any time to re-run your tests with the threading improvements. I am going to be testing these changes using Dota and CSGO, and then eventually the Steamworks SDK, and maybe one day Bungie will ship it, too in Destiny. But having your test case, which is very different, would be really great, especially since your use case really hammers the problem very directly and is a big stress test. We never really had issues with lock contention before in CSGO or Dota, but those apps only make networking calls from one (maybe 2) threads, so it isn't really a stress test.

Let me know if you think you might end up re-testing this sometime. It'll help me set my expectations.

Thanks again for your contributions!

from gamenetworkingsockets.

We've been using this on Dota internally for a while with no issues. I think it's stable enable to merge into master.

I'll keep this bug open for a while longer. If somebody has a chance to really stress test the new code, please comment here.

from gamenetworkingsockets.

One question, what is the maximum number of players/messages per second a single dedicated server can now host for Valve games (which was determined using simulations or real-world experience)?

from gamenetworkingsockets.

We don't really have any use cases that approach the limits for our own games. We had a partner use the lib with a multi-threaded server, with hundreds of connections, and that was the closest we got to having problems.

I think a use case of 100's of connections to a server is within the design scope of this library. 1000 is probably not. (Depending on the nature of the connections, of course.) For 100s of connections, my expectation is that lock contention will not be an issue unless the application code is designed to be accessing network functionality from many different threads at once. If the application only accesses the networking API for 1 or 2 threads, I don't think that there will be high contention, even if the number of connections is relatively high.

To be specific, I would expect a battle royale server that doesn't access networking from many different threads to not have issues with lock contention.

The APIs that receive multiple messages at once (and send -- if that API is added) could reduce the lock contention a lot, too, I suspect.

Obviously these expectations could be wrong.

from gamenetworkingsockets.

@fletcherdvalve Do I understand correctly that lock synchronizer in steamnetworkingsockets_lowlevel is globally shared among the whole process and its threads? So if a user will try to start multiple clients within a single process they will share the same synchronizer and try to acquire/release it?

from gamenetworkingsockets.

That's right.
There's one, global, network service thread, and only one, global, lock.

from gamenetworkingsockets.

Ah, so this explains why my simulation fails at 70 concurrent connections within the same process... 😿

from gamenetworkingsockets.

Is there any read-only functionality that potentially could benefit from SRW lock instead of the mutex in cases where read operations exceed write operations?

from gamenetworkingsockets.

Yeah, for sure.

from gamenetworkingsockets.

@fletcherdvalve Another thing that came to my mind is that, wouldn't the scoped lock in CSteamNetworkingSockets::RunCallbacks cause micro-stuttering if a user will dispatch callbacks to the main thread (game loop basically) since this lock will wait to acquire the synchronizer instead of trying to acquire it only once to not block the thread?

Actually, this applies to all functions with the scoped lock that frequently invoked.

I believe that some sort of Task/Phase-Fair RW should be used there at least to avoid starvation.

from gamenetworkingsockets.

I can't find this test on my machine, sadly. But I know guys who did something similar before, so I'll ask them if they can do some tests.

from gamenetworkingsockets.

Sorry, it looks like every time I rebase and force push the fine-grained locking branch, it adds more notes to this issue. I'll see if there's a better way to do this.

from gamenetworkingsockets.

@fletcherdvalve, are you happy with how things look here? Looks like this issue has fallen into limbo pending feedback.

from gamenetworkingsockets.

Let's keep it open. Dota is going to ship with the fine-grained-locking fix soon. If that doesn't show any problems, CSGO will be soon thereafter.

from gamenetworkingsockets.

OK, well I'd love it if @nxrighthere was able to run some of his tests again (since he reported this issue). But, we have been doing some work sending a bunch of messages on different threads in the dota server, and this change has made a big difference in perf.

So, it isn't crashing, and it's made Dota server perf quite a bit better. I think that's good enough to call this one closed.

from gamenetworkingsockets.

BTW, the latest steam client beta includes the new fine grained locking code

https://steamcommunity.com/groups/SteamClientBeta/announcements/detail/3032590367259099679

(It's not mentioned in the patch notes.)

from gamenetworkingsockets.