-
Notifications
You must be signed in to change notification settings - Fork 461
feat: Dynamically size the UnityTransport send queues [MTT-2816] #2212
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
Changes from 4 commits
1913c91
2fa18a9
056dc8e
669eedb
258f87f
1cf4d94
7e9ecb7
819ed97
52147d4
4248189
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could be worth it to add a link to the prog side way of setting this, for users that still want to use this |
||
| public const int InitialMaxSendQueueSize = 16 * InitialMaxPayloadSize; | ||
|
|
||
| private static ConnectionAddressData s_DefaultConnectionAddressData = new ConnectionAddressData { Address = "127.0.0.1", Port = 7777, ServerListenAddress = string.Empty }; | ||
|
|
@@ -186,15 +187,17 @@ public int MaxPayloadSize | |
| set => m_MaxPayloadSize = value; | ||
| } | ||
|
|
||
| [Tooltip("The maximum size in bytes of the transport send queue. The send queue accumulates messages for batching and stores messages when other internal send queues are full. If you routinely observe an error about too many in-flight packets, try increasing this.")] | ||
| [SerializeField] | ||
| private int m_MaxSendQueueSize = InitialMaxSendQueueSize; | ||
| private int m_MaxSendQueueSize = 0; | ||
|
|
||
| /// <summary>The maximum size in bytes of the transport send queue.</summary> | ||
| /// <remarks> | ||
| /// The send queue accumulates messages for batching and stores messages when other internal | ||
| /// send queues are full. If you routinely observe an error about too many in-flight packets, | ||
| /// try increasing this. | ||
| /// send queues are full. Note that there should not be any need to set this value manually | ||
| /// since the send queue size is dynamically sized based on need. | ||
| /// | ||
| /// This value should only be set if you have particular requirements (e.g. if you want to | ||
| /// limit the memory usage of the send queues). Note however that setting this value too low | ||
| /// can easily lead to disconnections under heavy traffic. | ||
| /// </remarks> | ||
| public int MaxSendQueueSize | ||
| { | ||
|
|
@@ -533,11 +536,6 @@ private static RelayConnectionData ConvertConnectionData(byte[] connectionData) | |
| } | ||
| } | ||
|
|
||
| internal void SetMaxPayloadSize(int maxPayloadSize) | ||
| { | ||
| m_MaxPayloadSize = maxPayloadSize; | ||
| } | ||
|
|
||
| private void SetProtocol(ProtocolType inProtocol) | ||
| { | ||
| m_ProtocolType = inProtocol; | ||
|
|
@@ -1191,7 +1189,23 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel | |
| var sendTarget = new SendTarget(clientId, pipeline); | ||
| if (!m_SendQueue.TryGetValue(sendTarget, out var queue)) | ||
| { | ||
| queue = new BatchedSendQueue(Math.Max(m_MaxSendQueueSize, m_MaxPayloadSize)); | ||
| // The maximum size of a send queue is determined according to the disconnection | ||
| // timeout. The idea being that if the send queue contains enough reliable data that | ||
| // sending it all out would take longer than the disconnection timeout, then there's | ||
| // no point storing even more in the queue (it would be like having a ping higher | ||
| // than the disconnection timeout, which is far into the realm of unplayability). | ||
| // | ||
| // The throughput used to determine what consists the maximum send queue size is | ||
| // the maximum theoritical throughput of the reliable pipeline assuming we only send | ||
| // on each update at 60 FPS, which turns out to be around 2.688 MB/s. | ||
| // | ||
| // Note that we only care about reliable throughput for send queues because that's | ||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:magic number 2688 |
||
|
|
||
| queue = new BatchedSendQueue(Math.Max(maxCapacity, m_MaxPayloadSize)); | ||
| m_SendQueue.Add(sendTarget, queue); | ||
| } | ||
|
|
||
|
|
@@ -1205,8 +1219,7 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel | |
|
|
||
| var ngoClientId = NetworkManager?.TransportIdToClientId(clientId) ?? clientId; | ||
| Debug.LogError($"Couldn't add payload of size {payload.Count} to reliable send queue. " + | ||
| $"Closing connection {ngoClientId} as reliability guarantees can't be maintained. " + | ||
| $"Perhaps 'Max Send Queue Size' ({m_MaxSendQueueSize}) is too small for workload."); | ||
| $"Closing connection {ngoClientId} as reliability guarantees can't be maintained."); | ||
|
|
||
| if (clientId == m_ServerClientId) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ namespace Unity.Netcode.EditorTests | |
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| public class BatchedSendQueueTests | ||
| { | ||
| private const int k_TestQueueCapacity = 1024; | ||
| private const int k_TestQueueCapacity = 16 * 1024; | ||
| private const int k_TestMessageSize = 42; | ||
|
|
||
| private ArraySegment<byte> m_TestMessage; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big issue, but when moving from
NativeArraytoNativeListwe start having some redundancy here:capacity,lengthandtail.The array doesn't have the concept of capacity, so that's why a
TailIndexis required, but once we have a list, thencapacitybecomes the reserved size, whilelengthbecomes the tail.For this to work as expected, I'd assume then that
m_Data.Lengthis always matching them_Data.Capacity?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
TailIndexand use the list'sLengthinstead, but that turned out to be a bit more complicated in the end. Although now that I look at the documentation forNativeList, theLengthproperty 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having tried it, we can't rely on the list's capacity as the capacity of our queue here, because
NativeListmay elect to set the capacity higher than what we tell it to be. For example, if callingSetCapacity(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.