ADOPTED: Datum softdels (#1920)

Co-authored-by: moonheart08 <[email protected]> Co-authored-by: Moony <[email protected]> Co-authored-by: wixoa <[email protected]> Co-authored-by: amylizzle <[email protected]>
OpenDreamProject · Oct 4, 2024 · 644ebb1 · 644ebb1
1 parent 202fe0f
commit 644ebb1
Show file tree

Hide file tree

Showing 17 changed files with 204 additions and 50 deletions.
diff --git a/Content.Tests/DMTests.cs b/Content.Tests/DMTests.cs
@@ -100,6 +100,9 @@ public void TestFiles(string sourceFile, DMTestFlags testFlags, int errorCode) {
                 Assert.That(returned?.IsTruthy(), Is.True, "Test was expected to return TRUE");
             }
 
+            // Due to how softdels work we need to make sure we clean up /now/.
+            GC.Collect();
+            _dreamMan.Update();
             Cleanup(compiledFile);
             TestContext.WriteLine($"--- PASS {sourceFile}");
         } finally {

diff --git a/DMCompiler/DMStandard/Types/Datum.dm b/DMCompiler/DMStandard/Types/Datum.dm
@@ -7,7 +7,8 @@
 	var/tag
 
 	proc/New()
-
+	//SAFETY: If you redefine this to anything except empty, please revisit how DreamObject handles Del() or it will
+	//        attempt to run DM on a GC thread, potentially causing problems.
 	proc/Del()
 
 	proc/Topic(href, href_list)

diff --git a/OpenDreamRuntime/DreamManager.cs b/OpenDreamRuntime/DreamManager.cs
@@ -1,6 +1,8 @@
-using System.IO;
+using System.Collections.Concurrent;
+using System.IO;
 using System.Linq;
 using System.Text.Json;
+using System.Threading;
 using DMCompiler.Bytecode;
 using DMCompiler.Json;
 using OpenDreamRuntime.Objects;
@@ -9,6 +11,7 @@
 using OpenDreamRuntime.Procs.Native;
 using OpenDreamRuntime.Rendering;
 using OpenDreamRuntime.Resources;
+using OpenDreamRuntime.Util;
 using OpenDreamShared;
 using OpenDreamShared.Dream;
 using Robust.Server;
@@ -44,10 +47,12 @@ public sealed partial class DreamManager {
         // Global state that may not really (really really) belong here
         public DreamValue[] Globals { get; set; } = Array.Empty<DreamValue>();
         public List<string> GlobalNames { get; private set; } = new();
-        public Dictionary<DreamObject, int> ReferenceIDs { get; } = new();
-        public Dictionary<int, DreamObject> ReferenceIDsToDreamObject { get; } = new();
+        public ConcurrentDictionary<int, WeakDreamRef> ReferenceIDsToDreamObject { get; } = new();
         public HashSet<DreamObject> Clients { get; set; } = new();
-        public HashSet<DreamObject> Datums { get; set; } = new();
+
+        // I solemnly swear this benefits from being a linked list (constant remove times without relying on object hash) --kaylie
+        public LinkedList<WeakDreamRef> Datums { get; set; } = new();
+        public ConcurrentBag<DreamObject> DelQueue = new();
         public Random Random { get; set; } = new();
         public Dictionary<string, List<DreamObject>> Tags { get; set; } = new();
         public DreamProc ImageConstructor, ImageFactoryProc;
@@ -103,6 +108,13 @@ public void Update() {
             _dreamMapManager.UpdateTiles();
             DreamObjectSavefile.FlushAllUpdates();
             WorldInstance.SetVariableValue("cpu", WorldInstance.GetVariable("tick_usage"));
+            ProcessDelQueue();
+        }
+
+        public void ProcessDelQueue() {
+            while (DelQueue.TryTake(out var obj)) {
+                obj.Delete();
+            }
         }
 
         public bool LoadJson(string? jsonPath) {
@@ -206,10 +218,14 @@ public string CreateRef(DreamValue value) {
                         }
                     }
 
-                    if (!ReferenceIDs.TryGetValue(refObject, out idx)) {
-                        idx = _dreamObjectRefIdCounter++;
-                        ReferenceIDs.Add(refObject, idx);
-                        ReferenceIDsToDreamObject.Add(idx, refObject);
+                    if (refObject.RefId is not {} id) {
+                        idx = Interlocked.Increment(ref _dreamObjectRefIdCounter);
+                        refObject.RefId = idx;
+
+                        // SAFETY: Infallible! idx is always unique and add can only fail if this is not the case.
+                        ReferenceIDsToDreamObject.TryAdd(idx, new WeakDreamRef(refObject));
+                    } else {
+                        idx = id;
                     }
                 }
             } else if (value.TryGetValueAsString(out var refStr)) {
@@ -241,6 +257,26 @@ public string CreateRef(DreamValue value) {
             return $"[0x{((int) refType+idx):x}]";
         }
 
+        /// <summary>
+        /// Iterates the list of datums
+        /// </summary>
+        /// <returns>Datum enumerater</returns>
+        /// <remarks>As it's a convenient time, this will collect any dead datum refs as it finds them.</remarks>
+        public IEnumerable<DreamObject> IterateDatums() {
+            // This isn't a common operation so we'll use this time to also do some pruning.
+            var node = Datums.First;
+
+            while (node is not null) {
+                var next = node.Next;
+                var val = node.Value.Target;
+                if (val is null)
+                    Datums.Remove(node);
+                else
+                    yield return val;
+                node = next;
+            }
+        }
+
         public DreamValue LocateRef(string refString) {
             bool canBePointer = false;
 
@@ -269,7 +305,7 @@ public DreamValue LocateRef(string refString) {
                     case RefType.DreamObjectMob:
                     case RefType.DreamObjectTurf:
                     case RefType.DreamObject:
-                        if (ReferenceIDsToDreamObject.TryGetValue(refId, out var dreamObject))
+                        if (ReferenceIDsToDreamObject.TryGetValue(refId, out var weakRef) && weakRef.Target is {} dreamObject)
                             return new(dreamObject);
 
                         return DreamValue.Null;

diff --git a/OpenDreamRuntime/Objects/DreamObject.cs b/OpenDreamRuntime/Objects/DreamObject.cs
@@ -6,6 +6,7 @@
 using OpenDreamRuntime.Objects.Types;
 using OpenDreamRuntime.Rendering;
 using OpenDreamRuntime.Resources;
+using OpenDreamRuntime.Util;
 using Robust.Server.GameObjects;
 using Robust.Server.GameStates;
 using Robust.Server.Player;
@@ -21,6 +22,9 @@ public class DreamObject {
         [Access(typeof(DreamObject))]
         public bool Deleted;
 
+        [Access(typeof(DreamManager), typeof(DreamObject))]
+        public int? RefId;
+
         public virtual bool ShouldCallNew => true;
 
         // Shortcuts to IoC dependencies & entity systems
@@ -81,22 +85,20 @@ public DreamObject(DreamObjectDefinition objectDefinition) {
 
             // Atoms are in world.contents
             if (this is not DreamObjectAtom && IsSubtypeOf(ObjectTree.Datum)) {
-                ObjectDefinition.DreamManager.Datums.Add(this);
+                ObjectDefinition.DreamManager.Datums.AddLast(new WeakDreamRef(this));
             }
         }
 
         public virtual void Initialize(DreamProcArguments args) {
             // For subtypes to implement
         }
 
-        protected virtual void HandleDeletion() {
+        protected virtual void HandleDeletion(bool possiblyThreaded) {
             // Atoms are in world.contents
-            if (this is not DreamObjectAtom && IsSubtypeOf(ObjectTree.Datum)) {
-                DreamManager.Datums.Remove(this);
-            }
+            // Datum removal used to live here, but datums are now tracked weakly.
 
-            if (DreamManager.ReferenceIDs.Remove(this, out var refId))
-                DreamManager.ReferenceIDsToDreamObject.Remove(refId);
+            if (RefId is not null)
+                DreamManager.ReferenceIDsToDreamObject.Remove(RefId.Value, out _);
 
             Tag = null;
             Deleted = true;
@@ -106,14 +108,40 @@ protected virtual void HandleDeletion() {
             ObjectDefinition = null!;
         }
 
-        public void Delete() {
+        /// <summary>
+        ///     Enters the current dream object into a global del queue that is guaranteed to run on the DM thread.
+        ///     Use if your deletion handler must be on the DM thread.
+        /// </summary>
+        protected void EnterIntoDelQueue() {
+            DreamManager.DelQueue.Add(this);
+        }
+
+        /// <summary>
+        ///     Del() the object, cleaning up its variables and refs to minimize size until the .NET GC collects it.
+        /// </summary>
+        /// <param name="possiblyThreaded">If true, Delete() will be defensive and assume it may have been called from another thread by .NET</param>
+        public void Delete(bool possiblyThreaded = false) {
             if (Deleted)
                 return;
 
-            if (TryGetProc("Del", out var delProc))
-                DreamThread.Run(delProc, this, null);
+            if (TryGetProc("Del", out var delProc)) {
+                // SAFETY: See associated comment in Datum.dm. This relies on the invariant that this proc is in a
+                //         thread-safe subset of DM (if such a thing exists) or empty. Currently, it is empty.
+                var datumBaseProc = delProc is DMProc {Bytecode.Length: 0};
+                if (possiblyThreaded && !datumBaseProc) {
+                    EnterIntoDelQueue();
+                    return; //Whoops, cannot thread.
+                } else if (!datumBaseProc) {
+                    DreamThread.Run(delProc, this, null);
+                }
+            }
+
+            HandleDeletion(possiblyThreaded);
+        }
 
-            HandleDeletion();
+        ~DreamObject() {
+            // Softdel, possibly.
+            Delete(true);
         }
 
         public bool IsSubtypeOf(TreeEntry ancestor) {

diff --git a/OpenDreamRuntime/Objects/DreamObjectTree.cs b/OpenDreamRuntime/Objects/DreamObjectTree.cs
@@ -129,6 +129,8 @@ public IEnumerable<TreeEntry> GetAllDescendants(TreeEntry treeEntry) {
             IEnumerator<TreeEntry> typeChildren = GetAllDescendants(type).GetEnumerator();
 
             while (typeChildren.MoveNext()) yield return typeChildren.Current;
+
+            typeChildren.Dispose();
         }
     }
 

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs b/OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs
@@ -27,10 +27,16 @@ public override void Initialize(DreamProcArguments args) {
         ObjectDefinition.Variables["desc"].TryGetValueAsString(out Desc);
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: RemoveAtom is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         AtomManager.RemoveAtom(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs b/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs
@@ -25,11 +25,17 @@ public DreamObjectClient(DreamObjectDefinition objectDefinition, DreamConnection
         View = DreamManager.WorldInstance.DefaultView;
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Client hashset is not threadsafe, this is not a hot path so no reason to change this.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         Connection.Session?.Channel.Disconnect("Your client object was deleted");
         DreamManager.Clients.Remove(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs b/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs
@@ -23,9 +23,14 @@ public override void Initialize(DreamProcArguments args) {
         throw new DMCrashRuntime("Unable to open database.");
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         Close();
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     /// <summary>

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs b/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs
@@ -21,10 +21,15 @@ public override void Initialize(DreamProcArguments args) {
         SetupCommand(command, args.Values[1..]);
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         ClearCommand();
         CloseReader();
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     /// <summary>

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs b/OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs
@@ -11,8 +11,14 @@ public sealed class DreamObjectFilter(DreamObjectDefinition objectDefinition) :
 
     public DreamFilter Filter;
 
-    protected override void HandleDeletion() {
-        base.HandleDeletion();
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Attachment dictionary is not threadsafe, no reason to change this.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
+        base.HandleDeletion(possiblyThreaded);
 
         FilterAttachedTo.Remove(Filter);
     }

diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs b/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs
@@ -239,10 +239,17 @@ public EntityUid GetEntity() {
         return _entity;
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Deleting entities is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         if(_entity != EntityUid.Invalid) {
             EntityManager.DeleteEntity(_entity);
         }
-        base.HandleDeletion();
+
+        base.HandleDeletion(possiblyThreaded);
     }
 }
diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs
@@ -56,11 +56,17 @@ public override void Initialize(DreamProcArguments args) {
         SetLoc(loc); //loc is set before /New() is ever called
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Deleting entities is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         WalkManager.StopWalks(this);
         AtomManager.DeleteMovableEntity(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {
-Original file line number
+Diff line change
@@ Expand Up @@
                 IEnumerator<TreeEntry> typeChildren = GetAllDescendants(type).GetEnumerator();
                 while (typeChildren.MoveNext()) yield return typeChildren.Current;
+                typeChildren.Dispose();
             }
         }
@@ Expand Down @@