perf: Reduce the amount of redundant 'Change' data serialized by SyncDictionary

Assets/Mirror/Core/SyncDictionary.cs

@@ -28,12 +28,82 @@ struct Change
             internal TValue item;
         }
 
-        // list of changes.
+        abstract class ChangesTracker
+        {
+            protected readonly List<Change> changes = new List<Change>();
+            public uint Count => (uint)changes.Count;
+            public List<Change> CurrentChanges => changes;
+            public virtual void ClearChanges() => changes.Clear();
+            public virtual void AddChange(Change changeToAdd) => changes.Add(changeToAdd);
+            public virtual void ResetTrackedFields() { }
+        }
+
+        // Traditional list of changes
         // -> insert/delete/clear is only ONE change
-        // -> changing the same slot 10x caues 10 changes.
+        // -> changing the same slot 10x causes 10 changes.
         // -> note that this grows until next sync(!)
-        // TODO Dictionary<key, change> to avoid ever growing changes / redundant changes!
-        readonly List<Change> changes = new List<Change>();
+        // -> may still be useful for some code that relies on knowing about every change that happens, including redundant changes
+        class AllChangesTracker : ChangesTracker { /* Nothing to override here */ }
+
+        // Map of changes
+        // -> insert/delete/set of any field results in AT MOST one change
+        // -> changing the same slot 10x causes 1 change
+        // -> clear operation deletes all tracked changes and replaces them with just 1 single tracked change
+        // -> Things only get a little trickier with tracking "changes ahead", aka "which changes were previously sent/received via SerializeAll/DeserializeAll when Deltas are being set/received."
+        // -> ResetTrackedFields is important for "changes head" as it means the next time any new slot gets changed, it results in at most 1 more change being added.
+        class SingleChangePerKeyTracker : ChangesTracker
+        {
+            readonly Dictionary<TKey, int> changeMap = new Dictionary<TKey, int>();
+            int changeCountDuringLastChangeMapReset = 0;
+
+            public override void ClearChanges()
+            {
+                changeMap.Clear();
+                changeCountDuringLastChangeMapReset = 0;
+                base.ClearChanges();
+            }
+
+            public override void AddChange(Change changeToAdd)
+            {
+                if(changeToAdd.operation == Operation.OP_CLEAR)
+                {
+                    changeMap.Clear();
+                    if(changeCountDuringLastChangeMapReset == 0)
+                    {
+                        // ResetTrackedFields was never called so just clear the whole changes list
+                        changes.Clear();
+                    }
+                    if(changes.Count > changeCountDuringLastChangeMapReset)
+                    {
+                        // Remove everything from the changes list that was added after the last call to ResetTrackedFields
+                        int removeCount = changes.Count - (changeCountDuringLastChangeMapReset + 1);
+                        changes.RemoveRange(changeCountDuringLastChangeMapReset, removeCount);
+                    }
+                    base.AddChange(changeToAdd);
+                }
+                else if(changeMap.TryGetValue(changeToAdd.key, out int changeIndex))
+                {
+                    // changeMap should never contain an index that does not exist in the changes list
+                    // changeMap should always provide the index pointing to the last recorded change in the changes list for the given key
+                    changes[changeIndex] = changeToAdd;
+                }
+                else
+                {
+                    base.AddChange(changeToAdd);
+                    changeMap[changeToAdd.key] = changes.Count - 1;
+                }
+            }
+
+            public override void ResetTrackedFields()
+            {
+                // This has to do with the 'changes ahead' system. By clearning changeMap, it means that up to 1 new change per slot
+                // this point will get added to the changes list even if the changes list itself already contains a change for said slot.
+                changeMap.Clear();
+                changeCountDuringLastChangeMapReset = changes.Count;
+            }
+        }
+
+        readonly ChangesTracker changeTracker;
 
         // how many changes we need to ignore
         // this is needed because when we initialize the list,
@@ -43,7 +113,7 @@ struct Change
 
         public override void Reset()
         {
-            changes.Clear();
+            changeTracker.ClearChanges();
             changesAhead = 0;
             objects.Clear();
         }
@@ -58,11 +128,22 @@ public override void Reset()
 
         // throw away all the changes
         // this should be called after a successful sync
-        public override void ClearChanges() => changes.Clear();
+        public override void ClearChanges() => changeTracker.ClearChanges();
 
-        public SyncIDictionary(IDictionary<TKey, TValue> objects)
+        public SyncIDictionary(IDictionary<TKey, TValue> objects, bool efficientChangeTrackingAndSync = false)
         {
             this.objects = objects;
+
+            if (efficientChangeTrackingAndSync)
+            {
+                // Nearly all of the time, we should just sync 1 change per key to save bandwidth by not sending redundant changes
+                this.changeTracker = new SingleChangePerKeyTracker();
+            }
+            else
+            {
+                // Some applications may need to sync all changes in great detail, even if those changes are redundant
+                this.changeTracker = new AllChangesTracker();
+            }
         }
 
         void AddOperation(Operation op, TKey key, TValue item, bool checkAccess)
@@ -72,16 +153,14 @@ void AddOperation(Operation op, TKey key, TValue item, bool checkAccess)
                 throw new System.InvalidOperationException("SyncDictionaries can only be modified by the owner.");
             }
 
-            Change change = new Change
-            {
-                operation = op,
-                key = key,
-                item = item
-            };
-
             if (IsRecording())
             {
-                changes.Add(change);
+                changeTracker.AddChange(new Change()
+                {
+                    operation = op,
+                    key = key,
+                    item = item
+                });
                 OnDirty?.Invoke();
             }
 
@@ -103,19 +182,19 @@ public override void OnSerializeAll(NetworkWriter writer)
             // thus the client will need to skip all the pending changes
             // or they would be applied again.
             // So we write how many changes are pending
-            writer.WriteUInt((uint)changes.Count);
+            changeTracker.ResetTrackedFields();
+            writer.WriteUInt(changeTracker.Count);
         }
 
         public override void OnSerializeDelta(NetworkWriter writer)
         {
             // write all the queued up changes
-            writer.WriteUInt((uint)changes.Count);
+            writer.WriteUInt(changeTracker.Count);
 
-            for (int i = 0; i < changes.Count; i++)
+            List<Change> changes = changeTracker.CurrentChanges;
+            foreach(Change change in changes)
             {
-                Change change = changes[i];
                 writer.WriteByte((byte)change.operation);
-
                 switch (change.operation)
                 {
                     case Operation.OP_ADD:
@@ -134,11 +213,11 @@ public override void OnSerializeDelta(NetworkWriter writer)
 
         public override void OnDeserializeAll(NetworkReader reader)
         {
-            // if init,  write the full list content
+            // if init, write the full list content
             int count = (int)reader.ReadUInt();
 
             objects.Clear();
-            changes.Clear();
+            changeTracker.ClearChanges();
 
             for (int i = 0; i < count; i++)
             {
@@ -159,14 +238,13 @@ public override void OnDeserializeDelta(NetworkReader reader)
 
             for (int i = 0; i < changesCount; i++)
             {
-                Operation operation = (Operation)reader.ReadByte();
-
                 // apply the operation only if it is a new change
                 // that we have not applied yet
                 bool apply = changesAhead == 0;
                 TKey key = default;
                 TValue item = default;
 
+                Operation operation = (Operation)reader.ReadByte();
                 switch (operation)
                 {
                     case Operation.OP_ADD:
@@ -316,9 +394,9 @@ public bool Remove(KeyValuePair<TKey, TValue> item)
 
     public class SyncDictionary<TKey, TValue> : SyncIDictionary<TKey, TValue>
     {
-        public SyncDictionary() : base(new Dictionary<TKey, TValue>()) {}
-        public SyncDictionary(IEqualityComparer<TKey> eq) : base(new Dictionary<TKey, TValue>(eq)) {}
-        public SyncDictionary(IDictionary<TKey, TValue> d) : base(new Dictionary<TKey, TValue>(d)) {}
+        public SyncDictionary(bool efficientChangeTrackingAndSync = false) : base(new Dictionary<TKey, TValue>(), efficientChangeTrackingAndSync) {}
+        public SyncDictionary(IEqualityComparer<TKey> eq, bool efficientChangeTrackingAndSync = false) : base(new Dictionary<TKey, TValue>(eq), efficientChangeTrackingAndSync) {}
+        public SyncDictionary(IDictionary<TKey, TValue> d, bool efficientChangeTrackingAndSync = false) : base(new Dictionary<TKey, TValue>(d), efficientChangeTrackingAndSync) {}
         public new Dictionary<TKey, TValue>.ValueCollection Values => ((Dictionary<TKey, TValue>)objects).Values;
         public new Dictionary<TKey, TValue>.KeyCollection Keys => ((Dictionary<TKey, TValue>)objects).Keys;
         public new Dictionary<TKey, TValue>.Enumerator GetEnumerator() => ((Dictionary<TKey, TValue>)objects).GetEnumerator();

Assets/Mirror/Tests/Editor/SyncCollections/SyncDictionaryTest.cs

@@ -4,13 +4,15 @@
 
 namespace Mirror.Tests.SyncCollections
 {
-    [TestFixture]
+    [TestFixture(true)]
+    [TestFixture(false)]
     public class SyncDictionaryTest
     {
         SyncDictionary<int, string> serverSyncDictionary;
         SyncDictionary<int, string> clientSyncDictionary;
         int serverSyncDictionaryDirtyCalled;
         int clientSyncDictionaryDirtyCalled;
+        bool optimized;
 
         void SerializeAllTo<T>(T fromList, T toList) where T : SyncObject
         {
@@ -29,11 +31,16 @@ void SerializeDeltaTo<T>(T fromList, T toList) where T : SyncObject
             fromList.ClearChanges();
         }
 
+        public SyncDictionaryTest(bool optimized)
+        {
+            this.optimized = optimized;
+        }
+
         [SetUp]
         public void SetUp()
         {
-            serverSyncDictionary = new SyncDictionary<int, string>();
-            clientSyncDictionary = new SyncDictionary<int, string>();
+            serverSyncDictionary = new SyncDictionary<int, string>(optimized);
+            clientSyncDictionary = new SyncDictionary<int, string>(optimized);
 
             // set writable
             serverSyncDictionary.IsWritable = () => true;