feat: Dynamically size the UnityTransport send queues [MTT-2816] #2212
Conversation
| // the only case where a full send queue causes a connection loss. Full unreliable | ||
| // send queues are dealt with by flushing it out to the network or simply dropping | ||
| // new messages if that fails. | ||
| var maxCapacity = m_MaxSendQueueSize > 0 ? m_MaxSendQueueSize : m_DisconnectTimeoutMS * 2688; |
There was a problem hiding this comment.
nit:magic number 2688
| @@ -130,6 +130,7 @@ private enum State | |||
| /// <summary> | |||
| /// The default maximum send queue size | |||
| /// </summary> | |||
| [Obsolete("MaxSendQueueSize is now determined dynamically. This initial value is not used anymore.", false)] | |||
There was a problem hiding this comment.
could be worth it to add a link to the prog side way of setting this, for users that still want to use this
| @@ -8,7 +8,7 @@ namespace Unity.Netcode.EditorTests | |||
| { | |||
There was a problem hiding this comment.
I'm not 100% familiar with those tests, maybe they cover this case already, but with a new feature, shouldn't there be new tests as well?
There was a problem hiding this comment.
My memory of writing these tests is that they were pretty thorough and would cover all the new code paths. After looking at the tests, it seems my memory was wrong. I'll add some more tests.
com.unity.netcode.gameobjects/Runtime/Transports/UTP/BatchedSendQueue.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Transports/UTP/BatchedSendQueue.cs
Outdated
Show resolved
Hide resolved
| // If there's enough space left at the end for the message, now is a good time to trim | ||
| // the capacity of m_Data if it got very large. We define "very large" here as having | ||
| // more than 75% of m_Data unused after adding the new message. | ||
| if (m_Data.Length - TailIndex >= sizeof(int) + message.Count) |
There was a problem hiding this comment.
Not a big issue, but when moving from NativeArray to NativeList we start having some redundancy here: capacity, length and tail.
The array doesn't have the concept of capacity, so that's why a TailIndex is required, but once we have a list, then capacity becomes the reserved size, while length becomes the tail.
For this to work as expected, I'd assume then that m_Data.Length is always matching the m_Data.Capacity?
There was a problem hiding this comment.
Yes, you're correct, the list's length always matches its capacity. I'll add a comment about that since that's a non-obvious but important part of how the code is meant to work.
Originally I had planned on getting rid of TailIndex and use the list's Length instead, but that turned out to be a bit more complicated in the end. Although now that I look at the documentation for NativeList, the Length property can be set, so maybe I'll modify the code to do that instead of tracking the tail index.
There was a problem hiding this comment.
After having tried it, we can't rely on the list's capacity as the capacity of our queue here, because NativeList may elect to set the capacity higher than what we tell it to be. For example, if calling SetCapacity(2044) the capacity will actually be set to 2048. This could be worked around, but I find it simpler to rely on the length of the list instead.
…ty-Technologies#2212) * Make BatchedSendQueue dynamically-sized * Remove MaxSendQueueSize as a serialized field * Add CHANGELOG entries * Add PR number to CHANGELOG entries * Add extra tests for BatchedSendQueue * Address some PR comments * Keep the standards checks happy * Address a review comment + some cleanup
This PR (finally!) makes the send queues in
UnityTransportdynamically-sized.This means that there should be no need to tweak the 'Max Send Queue Size' property anymore. Send queues will be grown automatically to accommodate new messages. This should also reduce the overall memory footprint since with this change we don't allocate large amounts of memory for "nothing" (e.g. for the unreliable send queues which never grow that large).
There is still a maximum size to these send queues. They won't be grown to some extreme ridiculous sizes. A long comment in
UnityTransport.csexplains how this value is picked and the rationale for this choice. There is still a way for users to specify their own value if they so desire through the existingMaxSendQueueSizeproperty (although it is not recommended).Because tuning that parameter shouldn't be required anymore, this PR removes the 'Max Send Queue Size' field from the inspector. This will be one less obscure setting users will have to care about. This PR also removes the serialization of that parameter to avoid having a "phantom" serialized field possibly limiting the size of the send queues without having a clear indication of what's going on. Users that desire a custom value should explicitly set the
MaxSendQueueSizeproperty.This PR also obsoletes the
InitialMaxSendQueueSizeproperty ofUnityTransport. Despite what the name indicates, it was actually just the default value. And since send queues don't really have a default size anymore, having this property around doesn't make much sense.Changelog
UnityTransportare now dynamically-sized. This means that there shouldn't be any need anymore to tweak the 'Max Send Queue Size' value. In fact, this field is now removed from the inspector and will not be serialized anymore. It is still possible to set it manually using theMaxSendQueueSizeproperty, but it is not recommended to do so aside from some specific needs (e.g. limiting the amount of memory used by the send queues in very constrained environments).UnityTransport.InitialMaxSendQueueSizefield is now deprecated. There is no default value anymore since send queues are dynamically-sized.Testing and Documentation