perf!: reduce linq, cleanup connectedclients #1196

LukeStampfli · 2021-09-16T19:52:27Z

MTT-574 addressing linq perf problems.

This is really two separate things:

refactoring how the NetworkManager,ConnectedClients stuff works
reducing linq / gc allocs in the hotpath
The reason why they are in the same PR is because it would have been really hard to make one change without the other.

Refactoring ConnectedClients

Before whenever ConnectedClientIds from NetworkManager was called we would allocate a new list and use a linq .where call to fill it :this_is_fine: (this was really problematic for instance sending a ClientRpc uses this property causing 2 allocs 🔥 )
The new code keeps an additinal id list cached.

In addition on the client side ConnectedClients is no longer partially broken by just containing the local client. The new system works like the following:

NetworkManager.ConnectedClients / ConnectedClientsList / ConnectedClientsIds can only be accessed by the server. If a non-server tries to access them an exception is thrown.
These lists are now exposed as properties and they are IReadOnlyLists because they really should not be modifiable.
There is a new NetworkManager.LocalClient property which can be used to access the local client's NetworkClient. This works on clients/host.

There are still some heavy allocs left around profiling which will be addressed in a separate PR. But this PR addresses all hot path usages of linq.

… some xmldoc comments though)

…(aside from INetworkSerializable, which isn't ready to be tested yet due to lack of BufferSerializer)

- Streamlined the implementations of bit-packing uints and ulongs.

…lacing with byte* allocated directly through UnsafeUtility.Malloc() (same function used under the hood by NativeArray). This is required in order to be able to store pointers to FastBufferReader and FastBufferWriter in order to be able to wrap them with BufferSerializer - NativeArray contains a managed variable in it that disallows taking a pointer to it. And since FBW and FBR are structs, pointers are the only way to wrap them "by reference" in another struct - ref fields aren't allowed even inside ref structs.

…g values in other ref structs in a capture-by-reference style, updated BitReader and BitWriter to use that. Also aggressively inlining properties in FBW and FBR.

…. it's a little slower, but apparently some platforms won't support unaligned memory access (e.g., WEBGL, ARM processors) and there's no compile time way to detect ARM processors since the bytecode is not processor-dependent... the cost of the runtime detection would be more expensive than the cost of just doing the memcpy.

…afeUtility.MemCpy for small values, while still supporting unaligned access.

…g it in.

… restructuring of some of the other tests.

…ork correctly (requesting beyond MaxCapacity and requesting more than double current capacity) -Added support for FastBufferReader to be used in a mode that doesn't copy the input buffer

- Fixed incorrect text in a warning in FastBufferReader

# Conflicts: # com.unity.netcode.gameobjects/Runtime/com.unity.netcode.runtime.asmdef # com.unity.netcode.gameobjects/Tests/Editor/com.unity.netcode.editortests.asmdef

…ConnectionApprovedMessage. A couple of tests are known to be failing... this is to let the work start being divided up.

Several tests are failing... fixing them will be the next checkin.

LukeStampfli · 2021-09-16T19:54:15Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -1,8 +1,8 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;


This is still here because of profiler stuff (which is really broken and needs a separate pass). It's now at the right position at least 🙃

LukeStampfli · 2021-09-16T19:54:45Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -112,7 +112,7 @@ public unsafe void SendClientRpc(ref FastBufferWriter writer, uint rpcMethodId,
            if (sendParams.Send.TargetClientIds != null)
            {
                // Copy into a localArray because SendMessage doesn't take IEnumerable, only IReadOnlyList
-                ulong* localArray = stackalloc ulong[sendParams.Send.TargetClientIds.Count()];


RPC params did allocate by using LINQ .Count on IEnumerable. Using IReadonlyList now. This was problematic in the first point because of multiple enumeration.

ShadauxCat · 2021-09-16T20:05:44Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

+        {
+            get
+            {
+                Assert.IsTrue(IsServer, $"{nameof(ConnectedClients)} should only be accessed on server.");


I don't think Assert should be used here - IIRC @JesseOlmer mentioned that Assert gets compiled out in production builds.

Yeah my goal was to get them compiled out for perf. But now thinking about it I don't think we should care in this case. Will change to NotServerException.

To elaborate, the problem with using exceptions inside properties is that it kills inlining for them.

ShadauxCat · 2021-09-16T20:06:57Z

com.unity.netcode.gameobjects/Runtime/Messaging/RpcParams.cs

@@ -25,7 +25,7 @@ public struct ClientRpcSendParams
        /// Note: Even if you provide a value such as NativeArray, enumerating it will cause boxing.
        /// If you want to avoid boxing, use TargetClientIdsNativeArray
        /// </summary>
-        public IEnumerable<ulong> TargetClientIds;
+        public IReadOnlyList<ulong> TargetClientIds;


If this becomes IReadOnlyList, then NetworkBehaviour.SendClientRpc() can be adjusted to not have to make a copy of it and just pass it directly to SendMessage() :)

Yes you are right, simplified the code to use the SendMessage overload with IReadOnlyList.

…factor/remove-linq # Conflicts: # com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

LukeStampfli · 2021-09-17T08:29:40Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -463,12 +461,21 @@ public override void DisconnectRemoteClient(ulong clientId)
                }

                // we need to cleanup any SendQueues for this connectionID;
-                var keys = m_SendQueue.Keys.Where(k => k.ClientId == clientId).ToList();
+                var keys = new NativeList<SendTarget>(16, Allocator.Temp); // use nativelist and manual foreach to avoid allocations


@Cosmin-B what do you think should we do this or keep linq here?

Yes. Also, is worth noting that this call would not happen frequently at all... so the performance benefit is super tiny.

JesseOlmer

lgtm

Would sure love to see some test cases around this (lists & properties being cleared at appropriate times, NotServerException being thrown when expected & not thrown when not expected, etc.)

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

JesseOlmer · 2021-09-17T15:06:50Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

+        /// <summary>
+        /// Gets the local <see cref="NetworkClient"/> for this client.
+        /// </summary>
+        public NetworkClient LocalClient { get; internal set; }


I see this getting set in connection approval, but I don't ever see it unset?

Yes this was already an issue before where ConnectedClients was not cleared until the next initialize. I added a line which clears LocalClient in initialize. So this will behave differently as before which is not ideal behavior but I think we decided to touch the initialization/shutdown flow it should be in a separate PR.

# Conflicts: # com.unity.netcode.adapter.utp/Tests/Runtime/Helpers/RuntimeTestsHelpers.cs # com.unity.netcode.gameobjects/Components/Interpolator.meta # com.unity.netcode.gameobjects/Components/Interpolator/BufferedLinearInterpolator.cs.meta # com.unity.netcode.gameobjects/Components/NetworkAnimator.cs # com.unity.netcode.gameobjects/Components/NetworkRigidbody.cs.meta # com.unity.netcode.gameobjects/Components/NetworkRigidbody2D.cs.meta # com.unity.netcode.gameobjects/Components/NetworkTransform.cs # com.unity.netcode.gameobjects/Editor/CodeGen/NetworkBehaviourILPP.cs # com.unity.netcode.gameobjects/Editor/CodeGen/RuntimeAccessModifiersILPP.cs # com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs # com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs # com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs # com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs # com.unity.netcode.gameobjects/Runtime/Messaging/BatchHeader.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs # com.unity.netcode.gameobjects/Runtime/Messaging/IMessageSender.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/INetworkHooks.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/INetworkMessage.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/InternalCommandContext.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/MessageBatcher.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/MessageQueue/MessageFrameItem.cs.meta # com.unity.netcode.gameobjects/Runtime/Messaging/Messages.meta # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/CreateObjectMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/DestroyObjectMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ServerLogMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/SnapshotDataMessage.cs # com.unity.netcode.gameobjects/Runtime/Messaging/MessagingSystem.cs # com.unity.netcode.gameobjects/Runtime/Messaging/RpcParams.cs # com.unity.netcode.gameobjects/Runtime/Metrics/MetricHooks.cs # com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs # com.unity.netcode.gameobjects/Runtime/Serialization/INetworkSerializable.cs # com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs # com.unity.netcode.gameobjects/Tests/Editor/Profiling.meta # com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs # com.unity.netcode.gameobjects/Tests/Runtime/Messaging/UnnamedMessageTests.cs # com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs # testproject/Assets/Tests/Manual/Scripts/StatsInfoContainer.cs # testproject/Assets/Tests/Runtime/RpcUserSerializableTypesTest.cs

Cosmin-B · 2021-09-23T11:05:56Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -463,12 +461,21 @@ public override void DisconnectRemoteClient(ulong clientId)
                }

                // we need to cleanup any SendQueues for this connectionID;
-                var keys = m_SendQueue.Keys.Where(k => k.ClientId == clientId).ToList();
+                var keys = new NativeList<SendTarget>(16, Allocator.Temp); // use nativelist and manual foreach to avoid allocations


Yes. Also, is worth noting that this call would not happen frequently at all... so the performance benefit is super tiny.

This is really two separate things: refactoring how the NetworkManager,ConnectedClients stuff works reducing linq / gc allocs in the hotpath The reason why they are in the same PR is because it would have been really hard to make one change without the other. Refactoring ConnectedClients Before whenever ConnectedClientIds from NetworkManager was called we would allocate a new list and use a linq .where call to fill it :this_is_fine: (this was really problematic for instance sending a ClientRpc uses this property causing 2 allocs 🔥 ) The new code keeps an additinal id list cached. In addition on the client side ConnectedClients is no longer partially broken by just containing the local client. The new system works like the following: NetworkManager.ConnectedClients / ConnectedClientsList / ConnectedClientsIds can only be accessed by the server. If a non-server tries to access them an exception is thrown. These lists are now exposed as properties and they are IReadOnlyLists because they really should not be modifiable. There is a new NetworkManager.LocalClient property which can be used to access the local client's NetworkClient. This works on clients/host.

ShadauxCat added 30 commits August 16, 2021 19:03

FastBufferWriter implemented and tested (still need to add and update…

2fa0f6d

… some xmldoc comments though)

A few additional tests to cover the last missed cases I can think of …

c36b3af

…(aside from INetworkSerializable, which isn't ready to be tested yet due to lack of BufferSerializer)

FastBufferReader + tests

5ac5ef9

- More tests

4776737

- Streamlined the implementations of bit-packing uints and ulongs.

Added utility ref struct "Ref<T>" to more generically support wrappin…

fc944c4

…g values in other ref structs in a capture-by-reference style, updated BitReader and BitWriter to use that. Also aggressively inlining properties in FBW and FBR.

BufferSerializer and tests.

cb95862

Removed unnecessary comment.

a3e0abc

XMLDocs + cleanup for PR

ea31f10

Merge branch 'develop' into feature/fast_buffer_reader_writer

057c1fc

Resurrected BytewiseUtil.FastCopyBytes as a faster alternative to Uns…

e9b9305

…afeUtility.MemCpy for small values, while still supporting unaligned access.

Reverting an accidental change.

aa56440

Removed files that got accidentally duplicated from before the rename.

fabb3b8

Standards fixes

3a2a50b

Removed accidentally added files.

58db1bd

Added BuildInfo.json to the .gitignore so I stop accidentally checkin…

c827339

…g it in.

Addressed most of the review feedback. Still need to do a little more…

f8bfb2f

… restructuring of some of the other tests.

standards.py --fix

9816b50

standards.py --fix

a576552

Fixed incorrect namespaces.

5193e39

-Fixed a couple of issues where growing a FastBufferWriter wouldn't w…

119ee2f

…ork correctly (requesting beyond MaxCapacity and requesting more than double current capacity) -Added support for FastBufferReader to be used in a mode that doesn't copy the input buffer

Fix a test failure and better implementation of large growths

7fb2d53

- Removed RefArray

8ee4610

- Fixed incorrect text in a warning in FastBufferReader

First INetworkMessage WIP - not hooked up to anything yet.

808f28a

Killed DelayUtil.

81c3dc3

Merge branch 'develop' into wip/inetworkmessage

8dfa959

# Conflicts: # com.unity.netcode.gameobjects/Runtime/com.unity.netcode.runtime.asmdef # com.unity.netcode.gameobjects/Tests/Editor/com.unity.netcode.editortests.asmdef

Hooked up MessagingSystem and converted ConnectionRequestMessage and …

b24d1a7

…ConnectionApprovedMessage. A couple of tests are known to be failing... this is to let the work start being divided up.

More converted messages

441d933

Finished converting all messages over and removed all the now-dead code.

1436cf8

Several tests are failing... fixing them will be the next checkin.

LukeStampfli changed the title ~~perf: reduce linq, cleanup connectedclients~~ perf!: reduce linq, cleanup connectedclients Sep 16, 2021

LukeStampfli commented Sep 16, 2021

View reviewed changes

Applied much review feedback.

e8b59c9

ShadauxCat reviewed Sep 16, 2021

View reviewed changes

LukeStampfli added 5 commits September 16, 2021 21:24

xml docs

bdfc6e6

Merge remote-tracking branch 'origin/feature/inetworkmessage' into re…

57b56c0

…factor/remove-linq # Conflicts: # com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

simplify readonlylist path

18a1d39

use not server exception instead of asserts

ad5d90a

remove unused usings

b82e130

LukeStampfli marked this pull request as ready for review September 17, 2021 08:27

LukeStampfli commented Sep 17, 2021

View reviewed changes

LukeStampfli requested review from TwoTenPvP and Cosmin-B September 17, 2021 08:30

JesseOlmer approved these changes Sep 17, 2021

View reviewed changes

make connectedlist fields private instead of internal.

fcafdb3

ShadauxCat approved these changes Sep 17, 2021

View reviewed changes

Base automatically changed from feature/inetworkmessage to develop September 17, 2021 19:01

Cosmin-B approved these changes Sep 23, 2021

View reviewed changes

LukeStampfli added 7 commits September 28, 2021 14:18

Merge remote-tracking branch 'origin/develop' into refactor/remove-linq

1276dd6

merge conflicts

34e0e6d

fix merge

4ce41eb

standards

20c91bc

clear LocalClient on initialize

e5ec6aa

add tools usings back

4ec10a2

fix metrics integration

50cc9eb

JesseOlmer merged commit c215d15 into develop Sep 28, 2021

JesseOlmer deleted the refactor/remove-linq branch September 28, 2021 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf!: reduce linq, cleanup connectedclients #1196

perf!: reduce linq, cleanup connectedclients #1196

Uh oh!

LukeStampfli commented Sep 16, 2021 •

edited

Loading

Uh oh!

LukeStampfli Sep 16, 2021

Uh oh!

LukeStampfli Sep 16, 2021

Uh oh!

ShadauxCat Sep 16, 2021

Uh oh!

LukeStampfli Sep 17, 2021

Uh oh!

LukeStampfli Sep 17, 2021

Uh oh!

ShadauxCat Sep 16, 2021

Uh oh!

LukeStampfli Sep 17, 2021

Uh oh!

LukeStampfli Sep 17, 2021

Uh oh!

Cosmin-B Sep 23, 2021

Uh oh!

JesseOlmer left a comment

Uh oh!

Uh oh!

JesseOlmer Sep 17, 2021

Uh oh!

LukeStampfli Sep 28, 2021

Uh oh!

Cosmin-B Sep 23, 2021

Uh oh!

Uh oh!

perf!: reduce linq, cleanup connectedclients #1196

perf!: reduce linq, cleanup connectedclients #1196

Uh oh!

Conversation

LukeStampfli commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactoring ConnectedClients

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseOlmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukeStampfli commented Sep 16, 2021 •

edited

Loading