perf: NetworkServer.dirtySpawned via NetworkIdentity.OnBecameDirty callback

Assets/Mirror/Core/NetworkIdentity.cs

@@ -852,6 +852,8 @@ internal void OnBecameDirty(NetworkBehaviour component)
             // OnSerialize will decide how to use them.
             if (isServer)
             {
+                bool clean = (serverOwnerDirtyMask | serverObserversDirtyMask) == 0;
+
                 // owner needs to be considered for both SyncModes, because
                 // Observers mode always includes the Owner.
                 //
@@ -869,6 +871,14 @@ internal void OnBecameDirty(NetworkBehaviour component)
                 // observers which aren't the owner.
                 if (component.syncMode == SyncMode.Observers)
                     serverObserversDirtyMask |= nthBit;
+
+                // add to dirty spawned, but only when we get dirty the first time.
+                // don't do a dictionary lookup for every [SyncVar] change...
+
+                // only add if observed.
+                // otherwise no point in adding + iterating from broadcast.
+                if (clean && observers.Count > 0)
+                        NetworkServer.dirtySpawned[netId] = this;
             }
             if (isClient)
             {
@@ -885,6 +895,9 @@ internal void OnBecameDirty(NetworkBehaviour component)
                     // set the n-th bit if dirty
                     // shifting from small to large numbers is varint-efficient.
                     clientDirtyMask |= (1u << component.ComponentIndex);
+
+                    // note: we don't have NetworkClient.dirtySpawned.
+                    // checking the few owned objects is enough.
                 }
             }
         }

Assets/Mirror/Core/NetworkServer.cs

@@ -51,6 +51,10 @@ public static partial class NetworkServer
         public static readonly Dictionary<uint, NetworkIdentity> spawned =
             new Dictionary<uint, NetworkIdentity>();
 
+        // all spawned which are dirty (= need broadcasting).
+        internal static readonly Dictionary<uint, NetworkIdentity> dirtySpawned =
+            new Dictionary<uint, NetworkIdentity>();
+
         /// <summary>Single player mode can use dontListen to not accept incoming connections</summary>
         // see also: https://github.com/vis2k/Mirror/pull/2595
         public static bool dontListen;
@@ -91,6 +95,11 @@ public static partial class NetworkServer
         // capture full Unity update time from before Early- to after LateUpdate
         public static TimeSample fullUpdateDuration;
 
+        // cache broadcast writers.
+        // don't need to get/return from/to pool for every identity.
+        static NetworkWriter ownerWriter = new NetworkWriter();
+        static NetworkWriter observersWriter = new NetworkWriter();
+
         // initialization / shutdown ///////////////////////////////////////////
         static void Initialize()
         {
@@ -1709,41 +1718,14 @@ static NetworkWriter SerializeForConnection(NetworkConnectionToClient connection
         internal static readonly List<NetworkConnectionToClient> connectionsCopy =
             new List<NetworkConnectionToClient>();
 
-        static void Broadcast()
+        // if we have large worlds with 10k+ Npcs, Items, etc.
+        // then we can easily ignore all of those which didn't change.
+        // otherwise this loop would be very costly.
+        static void BroadcastDirty()
         {
-            // copy all connections into a helper collection so that
+            // if (dirtySpawned.Count > 0) Debug.Log($"NetworkServer: Broadcasting {dirtySpawned.Count} dirty out of {spawned.Count} spawned.");
-            // OnTransportDisconnected can be called while iterating.
-            // -> OnTransportDisconnected removes from the collection
-            // -> which would throw 'can't modify while iterating' errors
-            // => see also: https://github.com/vis2k/Mirror/issues/2739
-            // (copy nonalloc)
-            // TODO remove this when we move to 'lite' transports with only
-            //      socket send/recv later.
-            connectionsCopy.Clear();
-            connections.Values.CopyTo(connectionsCopy);
 
-            // PULL-Broadcasting vs. PUSH-Broadcasting:
+            foreach (NetworkIdentity identity in dirtySpawned.Values)
-            //
-            // - Pull: foreach connection: foreach observing: send
-            //   + easier to build LocalWorldState
-            //   + avoids iterating _all_ spawned. only iterates those w/ observers
-            //   - doesn't work with dirtyIdentities.
-            //     each connection would require .dirtyObserving
-            //   - requires .observing
-            //
-            // - Push: foreach spawned: foreach observer: send
-            //   + works with dirtyIdentities
-            //   + doesn't require .observing
-            //   - iterates all spawned. unless we use dirtyIdentities.
-            //   - LocalWorldState building is more complex, but still possible
-            //   - need to cache identity.Serialize() so it's only called once.
-            //     instead of once per observing.
-            //
-            using (NetworkWriterPooled ownerWriter = NetworkWriterPool.Get(),
-                                       observersWriter = NetworkWriterPool.Get())
-            {
-                // let's use push broadcasting to prepare for dirtyObjects.
-                foreach (NetworkIdentity identity in spawned.Values)
             {
                 // make sure it's not null or destroyed.
                 // (which can happen if someone uses
@@ -1751,9 +1733,21 @@ static void Broadcast()
                 //  NetworkServer.Destroy)
                 if (identity != null)
                 {
-                        // only serialize if it has any observers
+                    // only serialize if it has any observers.
-                        // TODO only set dirty if has observers? would be easiest.
+                    // this is checked in NetworkIdentity.OnBecameDirty too.
+                    // however, a connection may have disconnected since then.
                     if (identity.observers.Count > 0)
+                        BroadcastIdentity(identity);
+                }
+                // spawned list should have no null entries because we
+                // always call Remove in OnObjectDestroy everywhere.
+                // if it does have null then someone used
+                // GameObject.Destroy instead of NetworkServer.Destroy.
+                else Debug.LogWarning($"Found 'null' entry in spawned. Please call NetworkServer.Destroy to destroy networked objects. Don't use GameObject.Destroy.");
+            }
+        }
+
+        static void BroadcastIdentity(NetworkIdentity identity)
         {
             // serialize for owner & observers
             ownerWriter.Position     = 0;
@@ -1784,16 +1778,9 @@ static void Broadcast()
                 }
             }
         }
-                    }
-                    // spawned list should have no null entries because we
-                    // always call Remove in OnObjectDestroy everywhere.
-                    // if it does have null then someone used
-                    // GameObject.Destroy instead of NetworkServer.Destroy.
-                    else Debug.LogWarning($"Found 'null' entry in spawned. Please call NetworkServer.Destroy to destroy networked objects. Don't use GameObject.Destroy.");
-                }
-            }
 
-            // flush all connection's batched messages
+        static void FlushConnections()
+        {
             foreach (NetworkConnectionToClient connection in connectionsCopy)
             {
                 // has this connection joined the world yet?
@@ -1816,6 +1803,47 @@ static void Broadcast()
                 // update connection to flush out batched messages
                 connection.Update();
             }
+        }
+
+        static void Broadcast()
+        {
+            // copy all connections into a helper collection so that
+            // OnTransportDisconnected can be called while iterating.
+            // -> OnTransportDisconnected removes from the collection
+            // -> which would throw 'can't modify while iterating' errors
+            // => see also: https://github.com/vis2k/Mirror/issues/2739
+            // (copy nonalloc)
+            // TODO remove this when we move to 'lite' transports with only
+            //      socket send/recv later.
+            connectionsCopy.Clear();
+            connections.Values.CopyTo(connectionsCopy);
+
+            // PULL-Broadcasting vs. PUSH-Broadcasting:
+            //
+            // - Pull: foreach connection: foreach observing: send
+            //   + easier to build LocalWorldState
+            //   + avoids iterating _all_ spawned. only iterates those w/ observers
+            //   - doesn't work with dirtyIdentities.
+            //     each connection would require .dirtyObserving
+            //   - requires .observing
+            //
+            // - Push: foreach spawned: foreach observer: send
+            //   + works with dirtyIdentities
+            //   + doesn't require .observing
+            //   - iterates all spawned. unless we use dirtyIdentities.
+            //   - LocalWorldState building is more complex, but still possible
+            //   - need to cache identity.Serialize() so it's only called once.
+            //     instead of once per observing.
+            //
+
+            // only broadcast dirty entities.
+            BroadcastDirty();
+
+            // clear dirty spawned, now that all were broadcasted
+            dirtySpawned.Clear();
+
+            // flush all connection's batched messages
+            FlushConnections();
 
             // TODO this is way too slow because we iterate ALL spawned :/
             // TODO this is way too complicated :/

Assets/Mirror/Tests/Editor/NetworkServerTest.cs

@@ -1251,17 +1251,18 @@ public void HasExternalConnections_WithHostAndConnection()
         // should log a warning. someone probably used GameObject.Destroy
         // instead of NetworkServer.Destroy.
         [Test]
-        public void UpdateDetectsNullEntryInObserving()
+        public void UpdateDetectsNullEntryInDirtySpawned()
         {
             // start
             NetworkServer.Listen(1);
             ConnectHostClientBlockingAuthenticatedAndReady();
 
             CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni);
+            NetworkServer.dirtySpawned[ni.netId] = ni; // add manually
             Assert.That(NetworkServer.spawned.ContainsKey(ni.netId));
 
             // set null
-            NetworkServer.spawned[ni.netId] = null;
+            NetworkServer.dirtySpawned[ni.netId] = null;
 
             // update
             LogAssert.Expect(LogType.Warning, new Regex("Found 'null' entry in spawned.*"));
@@ -1274,14 +1275,15 @@ public void UpdateDetectsNullEntryInObserving()
         //
         // => need extra test because of Unity's custom null check
         [Test]
-        public void UpdateDetectsDestroyedSpawned()
+        public void UpdateDetectsDestroyedDirtySpawned()
         {
             // start
             NetworkServer.Listen(1);
             ConnectHostClientBlockingAuthenticatedAndReady();
 
             CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni);
-            Assert.That(NetworkServer.spawned.ContainsKey(ni.netId));
+            NetworkServer.dirtySpawned[ni.netId] = ni; // add manually
+            Assert.That(NetworkServer.dirtySpawned.ContainsKey(ni.netId));
 
             // destroy
             GameObject.DestroyImmediate(go);