feat: Network Update Loop (RFC #8) by 0xFA11 · Pull Request #483 · Unity-Technologies/com.unity.netcode.gameobjects

0xFA11 · 2021-02-16T07:29:51Z

Implements RFC #8: Network Update Loop (tracked by Track RFC #8: Network Update Loop #479)
Replaces existing NetworkUpdateManager and NetworkUpdateLoopSystem with RFC-backed NetworkUpdateLoop and INetworkUpdateSystem
Migrates NetworkingManager, NetworkedBehaviour, RpcQueueProcessor, RpcQueueContainer and other types to the new infrastructure
Implements UpdateStagesPlain and UpdateStagesMixed runtime tests in NetworkUpdateLoopTests

0xFA11 · 2021-02-16T07:33:30Z

+    /// <summary>
+    /// Represents the network update loop injected into low-level player loop in Unity.
+    /// </summary>
+    public static class NetworkUpdateLoop


This is the main class that implements the network update loop which needs to be reviewed thoroughly.

Should we have a data structure passed into the NetworkUpdate method? This could be useful in the event we want to include other useful information (i.e. network tick).

0xFA11 · 2021-02-16T07:35:57Z

+        /// </summary>
+        public static NetworkUpdateStage UpdateStage;
+
+        private static readonly List<INetworkUpdateSystem> m_Initialization_List = new List<INetworkUpdateSystem>();


aaand I forgot to attach List vs HashSet performance comparison to the PR description — I will do that soon :)

HashSet turned out to be the better choice — HashSet's Add and Remove are faster than List's. @mattwalsh-unity FYI :)

also 2D array (INetworkUpdateSystem[][] m_UpdateSystem_Arrays aka NetworkUpdateStage → INetworkUpdateSystem[]) performance is on par with multiple linear arrays approach (INetworkUpdateSystem[] m_Initialization_Array, INetworkUpdateSystem[] m_EarlyUpdate_Array, ... etc.) — @jeffreyrainy FYI

I guess it's because once you get hit by pointer indirection, it probably doesn't get much worse if you jump between 2 pointers (2D) vs one pointer only (linear) ¯_(ツ)_/¯

jeffreyrainy · 2021-02-16T20:55:00Z

+                {
+                    var subsystems = playerLoopSystem.subSystemList.ToList();
+                    {
+                        int indexOfScriptRunBehaviourFixedUpdate = subsystems.FindIndex(s => s.type == typeof(FixedUpdate.ScriptRunBehaviourFixedUpdate));


Lines 72 to 84 mostly duplicates lines56 to 68. This is an anti-pattern. Could we have a single block that uses variables/parameters?
Same for lines 88 to 100, 104 to 116, 120 to 132, and 136 to 148.

repeating this pattern for different PlayerLoopSystems is necessary:

var subsystems = playerLoopSystem.subSystemList.ToList(); ... playerLoopSystem.subSystemList = subsystems.ToArray();

we can't reuse the same List instance and/or set it to subSystemsList as a static array.
we copy out what's in there, modify the list and set it back — we don't orchestrate from scratch.

jeffreyrainy · 2021-02-16T21:08:37Z

+            PlayerLoop.SetPlayerLoop(customPlayerLoop);
+        }
+
+        private struct NetworkInitialization


I find the use of multiple classes (NetworkInitialization, NetworkEarlyUpdate, NetworkFixedUpdate, ...) to be very class-heavy. Since the class don't have different functions and members, it seems a single class could do with a parameter, maybe of type NetworkUpdateStage.

Seems like this would be less error prone / more extensible / easier to read...but @MFatihMAR I thought you had a rationale for going this way?

they are not exposed and they don't implement anything other than CreateLoopSystem.

2 main reasons:

it's Unity-like: EarlyUpdate.ScriptRunDelayedStartupFrame

it allows us to differentiate between PlayerLoopSystems by type = typeof(XXX) (that's also how we find specific places to inject our stuff)

so, I'd say they serve a pretty valid purpose here.

Ok, but why not (similar to my other comment) declare a dictionary with key of PostLateUpdate, etc. and they the value being the function?

wackoisgod · 2021-02-17T19:18:19Z

@@ -0,0 +1,525 @@
+using System.Linq;


I don't understand why we need this file at all... seems overly complicated for just adding something to the loop, like are we just wanting any sort of object be able to register itself in the loop ?

From RFC's motivation section:

Even though it is possible to use low-level Player Loop API directly to insert custom PlayerLoopSystems into the existing player loop, it also requires a non-trivial amount of boilerplate code and could cause fundamental issues in the engine runtime if not done very carefully. We would like to have less boilerplate code and also be less error-prone at runtime.

Beyond that, the proposed design standardizes NetworkUpdateStages which are going to be executed at specific points in the player loop. This allows other framework systems tied into NetworkUpdateLoop to be aligned, such as RPCs executing at specific stages. We would like to standardize the execution of network stages as an infrastructure in the framework.

mattwalsh-unity · 2021-02-18T00:50:40Z

+                        int indexOfScriptRunDelayedStartupFrame = subsystems.FindIndex(s => s.type == typeof(EarlyUpdate.ScriptRunDelayedStartupFrame));
+                        if (indexOfScriptRunDelayedStartupFrame < 0)
+                        {
+                            Debug.LogError($"{nameof(NetworkUpdateLoop)}.{nameof(Initialize)}: Cannot find index of `{nameof(EarlyUpdate.ScriptRunDelayedStartupFrame)}` loop system in `{nameof(EarlyUpdate)}`'s subsystem list!");


Is this an error log or should it be a hard assert / crash?

To Matt point if we return what happens? I assume all other things are just messed up and its not even clear what the implication of the return would be here for code relying on in the rest or other loops being setup.

mattwalsh-unity · 2021-02-18T00:55:06Z

+                }
+                else if (playerLoopSystem.type == typeof(Update))
+                {
+                    var subsystems = playerLoopSystem.subSystemList.ToList();


Could move this repeated logic to a helper function?

Suggested change

var subsystems = playerLoopSystem.subSystemList.ToList();

void helper(var subSystemList, var someType)

{

var subsystems = subSystemList.ToList();

{

int indexOfScriptRunBehaviourUpdate = subsystems.FindIndex(s => s.type == typeof(someType));

if (indexOfScriptRunBehaviourUpdate < 0)

{

// debug error or probably assert

return;

}

// insert before `Update.ScriptRunBehaviourUpdate`

subsystems.Insert(indexOfScriptRunBehaviourUpdate, NetworkUpdate.CreateLoopSystem());

}

}

PostLateUpdate adds after not before, also Initialization adds to the bottom. we would still keep those two around and can't use helper to work with them :(

mattwalsh-unity · 2021-02-18T00:59:47Z

+            }
+        }
+
+        private static readonly HashSet<INetworkUpdateSystem>[] m_UpdateSystem_Sets =


Seems like you could declare this as a dictionary with key of type NetworkUpdateStage and be more maintainable (vs. remembering to maintain the comments, having a potentially missing index)

I was mainly concerned about Dictionary lookup on every update but apparently, one lookup per update stage group is not looking too bad. I verified my performance concerns with a local benchmark, both index-to-array and dictHash-to-array approaches are looking almost identical in terms of runtime performance (tested with 128, 1k, 4k and 8k objects).

I'll update shortly.

mattwalsh-unity

Commented

wackoisgod · 2021-02-19T07:42:53Z

+            if (!m_UpdateSystem_Sets[updateStage].Contains(updateSystem))
+            {
+                m_UpdateSystem_Sets[updateStage].Add(updateSystem);
+                m_UpdateSystem_Arrays[updateStage] = m_UpdateSystem_Sets[updateStage].ToArray();


This is going to generate garbage with the ToArray() aka an allocation every time a thing is registered.

wackoisgod · 2021-02-19T07:43:17Z

+            if (m_UpdateSystem_Sets[updateStage].Contains(updateSystem))
+            {
+                m_UpdateSystem_Sets[updateStage].Remove(updateSystem);
+                m_UpdateSystem_Arrays[updateStage] = m_UpdateSystem_Sets[updateStage].ToArray();


Same as above

* implement NetworkUpdateLoop * remove AdvanceFrame() and FrameCount * update player loop injection points and INetworkUpdateSystem API * add xmldoc and notes * add NetworkUpdateLoopTests * implement NetworkUpdateLoop plain and mixed tests * update xmldoc * small meta fix * replace old network update loop with new RFC-backed network update loop * replace WaitForEndOfFrame with WaitUntil in tests * comment 'default' for NetworkUpdateStage.Update enum value * optimization & refactoring pass * dictionary lookups instead of index access to arrays * optimize array allocation

0xFA11 added 12 commits February 5, 2021 04:20

implement NetworkUpdateLoop

f40a7d3

merge branch 'develop' into 'experimental/network-update-loop'

20ae513

merge branch 'develop' into 'experimental/network-update-loop'

c8f0a77

merge branch 'develop' into 'experimental/network-update-loop'

4568136

remove AdvanceFrame() and FrameCount

6da2c42

update player loop injection points and INetworkUpdateSystem API

7fe5c3b

add xmldoc and notes

379102b

add NetworkUpdateLoopTests

2c08e60

implement NetworkUpdateLoop plain and mixed tests

c891ddf

update xmldoc

558599f

small meta fix

7837599

replace old network update loop with new RFC-backed network update loop

db058d7

0xFA11 requested review from NoelStephensUnity, fraserhutch2020, jeffreyrainy and mattwalsh-unity February 16, 2021 07:29

0xFA11 commented Feb 16, 2021

View reviewed changes

0xFA11 force-pushed the feature/network-update-loop branch from 3b55465 to db058d7 Compare February 16, 2021 14:10

replace WaitForEndOfFrame with WaitUntil in tests

d4d6b3f

jeffreyrainy reviewed Feb 16, 2021

View reviewed changes

Comment thread com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs Outdated

jeffreyrainy reviewed Feb 16, 2021

View reviewed changes

Comment thread com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs Outdated

jeffreyrainy reviewed Feb 16, 2021

View reviewed changes

Comment thread com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs Outdated

comment 'default' for NetworkUpdateStage.Update enum value

f82542c

wackoisgod reviewed Feb 17, 2021

View reviewed changes

0xFA11 added 2 commits February 17, 2021 20:12

optimization & refactoring pass

b50b31a

Merge branch 'develop' into feature/network-update-loop

104de45

mattwalsh-unity reviewed Feb 18, 2021

View reviewed changes

dictionary lookups instead of index access to arrays

74e6a12

wackoisgod reviewed Feb 19, 2021

View reviewed changes

0xFA11 added 2 commits February 20, 2021 11:40

optimize array allocation

92d9732

Merge branch 'develop' into feature/network-update-loop

69a2c61

0xFA11 requested review from mattwalsh-unity and wackoisgod February 21, 2021 08:33

NoelStephensUnity approved these changes Feb 23, 2021

View reviewed changes

Merge branch 'develop' into feature/network-update-loop

d924684

0xFA11 merged commit bae18df into develop Feb 24, 2021

0xFA11 deleted the feature/network-update-loop branch March 5, 2021 17:44

-                    var subsystems = playerLoopSystem.subSystemList.ToList();
+void helper(var subSystemList, var someType)
+{
+    var subsystems = subSystemList.ToList();
+    {
+        int indexOfScriptRunBehaviourUpdate = subsystems.FindIndex(s => s.type == typeof(someType));
+                        if (indexOfScriptRunBehaviourUpdate < 0)
+                        {
+     // debug error or probably assert
+                            return;
+                        }
+                        // insert before `Update.ScriptRunBehaviourUpdate`
+                        subsystems.Insert(indexOfScriptRunBehaviourUpdate, NetworkUpdate.CreateLoopSystem());
+       }
+ }

Conversation

0xFA11 commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

0xFA11 Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

mattwalsh-unity left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NoelStephensUnity Feb 23, 2021 •

edited

Loading

0xFA11 Feb 17, 2021 •

edited

Loading

0xFA11 Feb 17, 2021 •

edited

Loading