From 41ee24e731e59d9f2e246fa13217c683ec5f0f00 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Wed, 6 Mar 2024 15:48:30 -0500 Subject: [PATCH 01/15] Clean up leaks from MapView enter/exit events --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 100 ++++++++++++------- 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 4151a3d..cce569f 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -228,12 +228,6 @@ private void OnSceneUnloaded(Scene scene) // Implementation is relying on reflection because of generic GameEvents types, as well as the fact that the same // EvtDelegate class is reimplemented as a nested class in every GE type, despite each class using the same exact layout... - // Note : we only remove objects if they are declared by the stock assembly, or a PartModule, or a VesselModule. - // Some mods are relying on a singleton pattern by instantiating a KSPAddon once, registering GameEvents there and - // relying on those being called on the dead instance to manipulate static data... - // Questionable at best, but since it is functionally valid and seems relatively common, we can't take the risk to remove them. - // Those cases will still be logged when the LogDestroyedUnityObjectGameEventsLeaks flag is set in settings. - foreach (BaseGameEvent gameEvent in BaseGameEvent.eventsByName.Values) { // "fast" path for EventVoid @@ -245,17 +239,15 @@ private void OnSceneUnloaded(Scene scene) if (eventVoid.events[i].originator is UnityEngine.Object unityObject && unityObject.IsDestroyed()) { Type originType = unityObject.GetType(); - if (originType.Assembly == assemblyCSharp || unityObject is PartModule || unityObject is VesselModule) + bool remove = ShouldCleanType(originType); + LogUnityObjectGameEventLeak(gameEvent.EventName, originType, remove); + + if (remove) { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, true); eventVoid.events.RemoveAt(i); leakCount++; gameEventsCallbacksCount--; } - else - { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, false); - } } } @@ -273,17 +265,14 @@ private void OnSceneUnloaded(Scene scene) if (onLevelWasLoaded.events[i].originator is UnityEngine.Object unityObject && unityObject.IsDestroyed() && !(unityObject is ScenarioUpgradeableFacilities)) { Type originType = unityObject.GetType(); - if (originType.Assembly == assemblyCSharp || unityObject is PartModule || unityObject is VesselModule) + bool remove = ShouldCleanType(originType); + LogUnityObjectGameEventLeak(gameEvent.EventName, originType, remove); + if (remove) { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, true); onLevelWasLoaded.events.RemoveAt(i); leakCount++; gameEventsCallbacksCount--; } - else - { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, false); - } } } @@ -305,17 +294,14 @@ private void OnSceneUnloaded(Scene scene) if (originatorField.GetValue(list[count]) is UnityEngine.Object unityObject && unityObject.IsDestroyed()) { Type originType = unityObject.GetType(); - if (originType.Assembly == assemblyCSharp || unityObject is PartModule || unityObject is VesselModule) + bool remove = ShouldCleanType(originType); + LogUnityObjectGameEventLeak(gameEvent.EventName, originType, remove); + if (remove) { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, true); list.RemoveAt(count); leakCount++; gameEventsCallbacksCount--; } - else - { - LogUnityObjectGameEventLeak(gameEvent.EventName, originType, false); - } } } } @@ -325,18 +311,17 @@ private void OnSceneUnloaded(Scene scene) } } + // remove destroyed MapView event handlers + leakCount += CleanDelegateHandlers("MapView.OnEnterMapView", ref MapView.OnEnterMapView); + leakCount += CleanDelegateHandlers("MapView.OnExitMapView", ref MapView.OnExitMapView); + // MapView is instantiated on scene loads and is registering an anonymous delegate in the TimingManager, // but never removes it. Since MapView hold indirect references to every vessel, this is a major leak. - if (TimingManager.Instance.IsNotNullOrDestroyed() && TimingManager.Instance.timing5.onLateUpdate != null) + if (TimingManager.Instance.IsNotNullOrDestroyed()) { - foreach (Delegate del in TimingManager.Instance.timing5.onLateUpdate.GetInvocationList()) - { - if (del.Target is MapView mapview && mapview.IsDestroyed()) - { - TimingManager.Instance.timing5.onLateUpdate -= (TimingManager.UpdateAction)del; - leakCount++; - } - } + var del = TimingManager.Instance.timing5.onLateUpdate; + leakCount += CleanDelegateHandlers("TimingManager.Instance.timing5.onLateUpdate", ref del); + TimingManager.Instance.timing5.onLateUpdate = del; } // This is part of the resource flow graph mess, and will keep around tons of references @@ -377,21 +362,60 @@ private void OnSceneUnloaded(Scene scene) Debug.Log($"[KSPCF:MemoryLeaks] Leaving scene \"{currentScene}\", cleaned {leakCount} memory leaks in {watch.Elapsed.TotalSeconds:F3}s. GameEvents callbacks : {gameEventsCallbacksCount}. Allocated memory : {heapAlloc} (managed heap), {unityAlloc} (unmanaged)"); } - static void LogUnityObjectGameEventLeak(string eventName, Type origin, bool removed) + // Note : we only remove objects if they are declared by the stock assembly, or a PartModule, or a VesselModule. + // Some mods are relying on a singleton pattern by instantiating a KSPAddon once, registering GameEvents there and + // relying on those being called on the dead instance to manipulate static data... + // Questionable at best, but since it is functionally valid and seems relatively common, we can't take the risk to remove them. + // Those cases will still be logged when the LogDestroyedUnityObjectGameEventsLeaks flag is set in settings. + static bool ShouldCleanType(Type type) { - if (!logDestroyedUnityObjectGameEventsLeaks) - return; + return type.Assembly == assemblyCSharp || typeof(PartModule).IsAssignableFrom(type) || typeof(VesselModule).IsAssignableFrom(type); + } + + static int CleanDelegateHandlers(string eventName, ref T del) where T : Delegate + { + int leakCount = 0; + if (del == null) return 0; + foreach (Delegate handler in del.GetInvocationList()) + { + if (handler.Target is UnityEngine.Object obj && obj.IsDestroyed()) + { + bool remove = ShouldCleanType(obj.GetType()); + + LogUnityObjectGameEventLeak(eventName, obj.GetType(), remove); + + if (remove) + { + del = (T)Delegate.Remove(del, handler); + leakCount++; + } + } + } + return leakCount; + } + static string TypeNameForLog(Type origin) + { string typeName = origin.Assembly.GetName().Name + ":"; if (origin.IsNested) typeName += origin.DeclaringType.Name + "." + origin.Name; else typeName += origin.Name; + return typeName; + } + + static void LogUnityObjectGameEventLeak(string eventName, Type origin, bool removed) + { + if (!logDestroyedUnityObjectGameEventsLeaks) + return; + + string typeName = TypeNameForLog(origin); + if (removed) - Debug.Log($"[KSPCF:MemoryLeaks] Removed a {eventName} GameEvents callback owned by a destroyed {typeName} instance"); + Debug.Log($"[KSPCF:MemoryLeaks] Removed a {eventName} callback owned by a destroyed {typeName} instance"); else - Debug.Log($"[KSPCF:MemoryLeaks] A destroyed {typeName} instance is owning a {eventName} GameEvents callback. No action has been taken, but unless this mod is relying on this pattern, this is likely a memory leak."); + Debug.Log($"[KSPCF:MemoryLeaks] A destroyed {typeName} instance is owning a {eventName} callback. No action has been taken, but unless this mod is relying on this pattern, this is likely a memory leak."); } static void LogGameEventsSuscribers() From f23032b8d02d8ca09249d9dc6adf7edf51f6efa0 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Wed, 6 Mar 2024 23:11:43 -0500 Subject: [PATCH 02/15] clean up a lingering toolbar button --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index cce569f..c19c650 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -121,6 +121,17 @@ protected override void ApplyPatches(List patches) AccessTools.Method(typeof(FuelFlowOverlay), nameof(FuelFlowOverlay.OnDestroy)), this)); + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(ApplicationLauncher), nameof(ApplicationLauncher.RemoveApplication)), + this)); + + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(ApplicationLauncher), nameof(ApplicationLauncher.RemoveModApplication)), + this, + "ApplicationLauncher_RemoveApplication_Postfix")); + // general cleanup on scene switches assemblyCSharp = Assembly.GetAssembly(typeof(Part)); @@ -654,6 +665,14 @@ static void FuelFlowOverlay_OnDestroy_Postfix() FuelFlowOverlay.instance = null; } + static void ApplicationLauncher_RemoveApplication_Postfix(ApplicationLauncherButton button) + { + if (ApplicationLauncher.Instance.IsNotNullRef() && object.ReferenceEquals(ApplicationLauncher.Instance.lastPinnedButton, button.toggleButton)) + { + ApplicationLauncher.Instance.lastPinnedButton = null; + } + } + // patch previously silently failing code that now "properly" throw exceptions when attempting to access dead instances static bool UIPartActionControllerInventory_UpdateCursorOverPAWs_Prefix(UIPartActionControllerInventory __instance) { From 46363084245d12b48185b937877739a96a9df4a3 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Fri, 8 Mar 2024 03:05:13 -0500 Subject: [PATCH 03/15] Fix a leak in AudioMultiPooledFX.PooledAudioSource --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index c19c650..d27d308 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -85,6 +85,13 @@ protected override void ApplyPatches(List patches) AccessTools.Method(typeof(EffectList), nameof(EffectList.OnSave)), this)); + // Pooled Audio onRelease delegate leak + + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(AudioMultiPooledFX.PooledAudioSource), nameof(AudioMultiPooledFX.PooledAudioSource.Reset)), + this)); + // various static fields keeping deep reference stacks around... patches.Add(new PatchInfo( @@ -626,6 +633,12 @@ static void EffectList_OnSave_Postfix() EffectList.fxEnumerator = default; } + // PooledAudioSource leaks references to parts through its onRelease delegate + static void PooledAudioSource_Reset_Postfix(AudioMultiPooledFX.PooledAudioSource pooledAudioSource) + { + pooledAudioSource.onRelease = null; + } + // CraftSearch is keeping around an indirect reference to the last EditorLogic instance. static void CraftSearch_OnDestroy_Postfix() { From 0e9997482c22dad55522ed85cdc327ff3f20a94c Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Tue, 12 Mar 2024 03:52:43 -0400 Subject: [PATCH 04/15] Fix leaking landed and splashed vessels on kerbin via KSCVesselMarkers --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index d27d308..54f49f2 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -1,5 +1,6 @@ using CommNet; using HarmonyLib; +using KSP.UI; using KSP.UI.Screens; using RUI.Algorithms; using System; @@ -139,6 +140,12 @@ protected override void ApplyPatches(List patches) this, "ApplicationLauncher_RemoveApplication_Postfix")); + // KSC Vessel Markers inherit from this class. they don't get cleaned up properly + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(AnchoredDialog), nameof(AnchoredDialog.Terminate)), + this)); + // general cleanup on scene switches assemblyCSharp = Assembly.GetAssembly(typeof(Part)); @@ -686,6 +693,12 @@ static void ApplicationLauncher_RemoveApplication_Postfix(ApplicationLauncherBut } } + // KSC Vessel Marker - it seems like the logic in this has an `else` where it shouldn't. + static void AnchoredDialog_Terminate_Postfix(AnchoredDialog __instance) + { + __instance.gameObject.DestroyGameObject(); + } + // patch previously silently failing code that now "properly" throw exceptions when attempting to access dead instances static bool UIPartActionControllerInventory_UpdateCursorOverPAWs_Prefix(UIPartActionControllerInventory __instance) { From e3d7d2688d20c0c8c0546159015a301dbadcbd39 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Tue, 12 Mar 2024 21:27:46 -0400 Subject: [PATCH 05/15] Fix #203: clean up leaked clone of root part prefab --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 54f49f2..886d68b 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -74,6 +74,15 @@ protected override void ApplyPatches(List patches) AccessTools.Method(typeof(ModuleScienceExperiment), nameof(ModuleScienceExperiment.OnAwake)), this)); + // protopartsnapshot leaks a reference to a copy of the root part because of how it's created + + patches.Add(new PatchInfo(PatchMethodType.Postfix, + AccessTools.Method(typeof(ProtoPartSnapshot), nameof(ProtoPartSnapshot.Load)), + this)); + patches.Add(new PatchInfo(PatchMethodType.Postfix, + AccessTools.Method(typeof(ProtoPartSnapshot), nameof(ProtoPartSnapshot.ConfigurePart)), + this)); + // EffectList dictionary enumerator leaks patches.Add(new PatchInfo( @@ -629,6 +638,28 @@ static bool ModuleScienceExperiment_OnAwake_Prefix() return HighLogic.LoadedSceneIsFlight; } + // Because root parts are added directly to the Vessel gameobject, they're not just a clone of a part prefab + // instead, ProtoPartSnapshot.Load will clone the prefab, then copy the components and properties over to the Vessel GameObject + // Then it deletes the clone it made. + // This leaves a few references pointing to the now-deleted clone, because unlike regular serialization copying the properties won't fix the references to point to the real live part + + static void ProtoPartSnapshot_Load_Postfix(Part __result, bool loadAsRootPart) + { + if (loadAsRootPart) + { + foreach (var attachNode in __result.attachNodes) + { + attachNode.owner = __result; + } + } + } + + static void ProtoPartSnapshot_ConfigurePart_Postfix(ProtoPartSnapshot __instance) + { + // ConfigurePart destroys this gameobject but doesn't clear the reference + __instance.rootPartPrefab = null; + } + // EffectList is leaking Part references by keeping around this static enumerator static void EffectList_Initialize_Postfix() { From 9f0b9118a5509e65262df2d29a3ecab22eab9675 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Tue, 12 Mar 2024 21:29:39 -0400 Subject: [PATCH 06/15] clear the FlightGlobals part caches on scene unload --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 886d68b..700f74e 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -211,6 +211,10 @@ private void OnSceneUnloaded(Scene scene) int leakCount = 0; int gameEventsCallbacksCount = 0; + // these are big static dictionaries that track a lot of parts and gameobjects. Reset them here since all of them must have been destroyed + FlightGlobals.ResetObjectPartPointerUpwardsCache(); + FlightGlobals.ResetObjectPartUpwardsCache(); + // PartSet doesn't derive from UnityEngine.Object and suscribes to the onPartResourceFlowStateChange and // onPartResourceFlowModeChange GameEvents. Instances can be owned by a bunch of classes, including Vessel, // Part, ShipConstruct... Whoever implemented this though that he could avoid having to manage the GameEvents From e7325b1a9f3a38d942a9c2cb03b7ac1d8c512eac Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Tue, 12 Mar 2024 21:33:06 -0400 Subject: [PATCH 07/15] when parts are destroyed, unhook references from the ProtoPartSnapshot --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 700f74e..fcb7e3d 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -83,6 +83,13 @@ protected override void ApplyPatches(List patches) AccessTools.Method(typeof(ProtoPartSnapshot), nameof(ProtoPartSnapshot.ConfigurePart)), this)); + // Parts and vessels don't unhook themselves from things when they're destroyed or unloaded + + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(Part), nameof(Part.OnDestroy)), + this)); + // EffectList dictionary enumerator leaks patches.Add(new PatchInfo( @@ -664,6 +671,38 @@ static void ProtoPartSnapshot_ConfigurePart_Postfix(ProtoPartSnapshot __instance __instance.rootPartPrefab = null; } + // When a part is destroyed, we need to clean up all the references that are pointing back to this part, mostly from the ProtoPartSnapshot because that will live longer than this part + static void Part_OnDestroy_Postfix(Part __instance) + { + // if our protopartsnapshot is still pointing at this part, we need to clear all of its references + if (__instance.protoPartSnapshot != null && ReferenceEquals(__instance.protoPartSnapshot.partRef, __instance)) + { + foreach (var moduleSnapshot in __instance.protoPartSnapshot.modules) + { + moduleSnapshot.moduleRef = null; + } + + foreach (var partResourceSnapshot in __instance.protoPartSnapshot.resources) + { + partResourceSnapshot.resourceRef = null; + } + + __instance.protoPartSnapshot.partRef = null; + } + + // clear some additional containers to limit the impact of leaks and make reports less noisy + __instance.modules?.modules?.Clear(); + __instance.children?.Clear(); + __instance.parent = __instance.potentialParent = null; + +#if DEBUG + if (FlightGlobals.objectToPartUpwardsCache.Values.Contains(__instance)) + { + Debug.LogError($"Destroying part {__instance.GetInstanceID()} while it's still in the objectToPartUpwardsCache"); + } +#endif + } + // EffectList is leaking Part references by keeping around this static enumerator static void EffectList_Initialize_Postfix() { From 43c456a1d03e9995d1675503ee814f6b8807aa2d Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Tue, 12 Mar 2024 21:33:40 -0400 Subject: [PATCH 08/15] when vessels are unloaded, clean up their PartSets and some part references --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 43 ++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index fcb7e3d..caca075 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -89,6 +89,10 @@ protected override void ApplyPatches(List patches) PatchMethodType.Postfix, AccessTools.Method(typeof(Part), nameof(Part.OnDestroy)), this)); + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(Vessel), nameof(Vessel.Unload)), + this)); // EffectList dictionary enumerator leaks @@ -703,6 +707,45 @@ static void Part_OnDestroy_Postfix(Part __instance) #endif } + // PartSets are awful as noted in OnSceneUnloaded. But vessel unloading is also a good time to clean these up instead of waiting for scene switch + + static void CleanUpPartSet(PartSet partSet) + { + GameEvents.onPartResourceFlowStateChange.Remove(partSet.OnFlowStateChange); + GameEvents.onPartResourceFlowModeChange.Remove(partSet.OnFlowModeChange); + + // these aren't *necessary* but help to reduce noise in reports + partSet.targetParts.Clear(); + partSet.ship = null; + partSet.vessel = null; + } + + // general unhooking of stuff when unloading a vessel. Note that vessel gameobjects are not usually destroyed until the vessel itself is + static void Vessel_Unload_Postfix(Vessel __instance) + { + __instance.rootPart = null; + FlightGlobals.ResetObjectPartUpwardsCache(); + FlightGlobals.ResetObjectPartPointerUpwardsCache(); + + CleanUpPartSet(__instance.resourcePartSet); + CleanUpPartSet(__instance.simulationResourcePartSet); + + foreach (var partSet in __instance.crossfeedSets) + { + CleanUpPartSet(partSet); + } + + foreach (var partSet in __instance.simulationCrossfeedSets) + { + CleanUpPartSet(partSet); + } + + __instance.resourcePartSet = null; + __instance.simulationResourcePartSet = null; + __instance.crossfeedSets.Clear(); + __instance.simulationCrossfeedSets.Clear(); + } + // EffectList is leaking Part references by keeping around this static enumerator static void EffectList_Initialize_Postfix() { From 6e3f1f73c4a58ec6439ced5eb8c4445d0f969299 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Wed, 13 Mar 2024 11:01:30 -0400 Subject: [PATCH 09/15] nullcheck for partset --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index caca075..19ea8b2 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -711,6 +711,8 @@ static void Part_OnDestroy_Postfix(Part __instance) static void CleanUpPartSet(PartSet partSet) { + if (partSet == null) return; + GameEvents.onPartResourceFlowStateChange.Remove(partSet.OnFlowStateChange); GameEvents.onPartResourceFlowModeChange.Remove(partSet.OnFlowModeChange); From 93fc9e6930b19ae17885d14d35531e9f874d78cd Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Wed, 13 Mar 2024 11:49:12 -0400 Subject: [PATCH 10/15] more nullchecks for partSet --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 19ea8b2..a97d8c3 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -717,7 +717,7 @@ static void CleanUpPartSet(PartSet partSet) GameEvents.onPartResourceFlowModeChange.Remove(partSet.OnFlowModeChange); // these aren't *necessary* but help to reduce noise in reports - partSet.targetParts.Clear(); + partSet.targetParts?.Clear(); partSet.ship = null; partSet.vessel = null; } From 82030df93f7d7c31c41d8f1c4ab7361cecf5d128 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Thu, 14 Mar 2024 17:32:29 -0400 Subject: [PATCH 11/15] Free up references when disconnecting autostruts --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index a97d8c3..ddc6ac6 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -89,6 +89,10 @@ protected override void ApplyPatches(List patches) PatchMethodType.Postfix, AccessTools.Method(typeof(Part), nameof(Part.OnDestroy)), this)); + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(Part), nameof(Part.ReleaseAutoStruts)), + this)); patches.Add(new PatchInfo( PatchMethodType.Postfix, AccessTools.Method(typeof(Vessel), nameof(Vessel.Unload)), @@ -707,6 +711,12 @@ static void Part_OnDestroy_Postfix(Part __instance) #endif } + static void Part_ReleaseAutoStruts_Postfix(Part __instance) + { + __instance.autoStrutCacheVessel = null; + __instance.autoStrutVessel = null; + } + // PartSets are awful as noted in OnSceneUnloaded. But vessel unloading is also a good time to clean these up instead of waiting for scene switch static void CleanUpPartSet(PartSet partSet) From 1e0a05fb1d0a65d0f24f1fdd3a046fab67a35746 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Thu, 14 Mar 2024 17:33:17 -0400 Subject: [PATCH 12/15] clear Part.allparts on scene unload --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index ddc6ac6..a2aa967 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -229,6 +229,7 @@ private void OnSceneUnloaded(Scene scene) // these are big static dictionaries that track a lot of parts and gameobjects. Reset them here since all of them must have been destroyed FlightGlobals.ResetObjectPartPointerUpwardsCache(); FlightGlobals.ResetObjectPartUpwardsCache(); + Part.allParts.Clear(); // PartSet doesn't derive from UnityEngine.Object and suscribes to the onPartResourceFlowStateChange and // onPartResourceFlowModeChange GameEvents. Instances can be owned by a bunch of classes, including Vessel, From d7863c19ebb2026c3171693294a4e76b33c644fe Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Thu, 14 Mar 2024 17:34:43 -0400 Subject: [PATCH 13/15] Fix some issues that could leak parts and vessels via ProtoCrewMember --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index a2aa967..ccda24b 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -98,6 +98,18 @@ protected override void ApplyPatches(List patches) AccessTools.Method(typeof(Vessel), nameof(Vessel.Unload)), this)); + // Kerbals + + patches.Add(new PatchInfo( + PatchMethodType.Postfix, + AccessTools.Method(typeof(Kerbal), nameof(Kerbal.OnDestroy)), + this)); + + patches.Add(new PatchInfo( + PatchMethodType.Prefix, + AccessTools.Method(typeof(InternalSeat), nameof(InternalSeat.DespawnCrew)), + this)); + // EffectList dictionary enumerator leaks patches.Add(new PatchInfo( @@ -703,6 +715,7 @@ static void Part_OnDestroy_Postfix(Part __instance) __instance.modules?.modules?.Clear(); __instance.children?.Clear(); __instance.parent = __instance.potentialParent = null; + __instance.internalModel = null; #if DEBUG if (FlightGlobals.objectToPartUpwardsCache.Values.Contains(__instance)) @@ -759,6 +772,24 @@ static void Vessel_Unload_Postfix(Vessel __instance) __instance.simulationCrossfeedSets.Clear(); } + static void Kerbal_OnDestroy_Postfix(Kerbal __instance) + { + if (ReferenceEquals(__instance.protoCrewMember.KerbalRef, __instance)) + { + __instance.protoCrewMember.KerbalRef = null; + } + } + + static void InternalSeat_DespawnCrew_Prefix(InternalSeat __instance) + { + // DespawnCrew merely destroys the kerbal, but it doesn't unhook the protocrewmember from this seat + // ProtoCrewMembers will live until the next save game is loaded, so this can leak parts and vessels + if (__instance.kerbalRef.IsNotNullRef() && __instance.kerbalRef.protoCrewMember.seat.RefEquals(__instance)) + { + __instance.kerbalRef.protoCrewMember.seat = null; + } + } + // EffectList is leaking Part references by keeping around this static enumerator static void EffectList_Initialize_Postfix() { From 4ca88feb957c46f7154648c8ee1c6e62708fe014 Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Thu, 14 Mar 2024 17:35:56 -0400 Subject: [PATCH 14/15] make the TimingManager cleanup more generalized so we can report leaks from mods (there was one in KOS) --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 30 ++++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index ccda24b..8d1a568 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -385,9 +385,16 @@ private void OnSceneUnloaded(Scene scene) // but never removes it. Since MapView hold indirect references to every vessel, this is a major leak. if (TimingManager.Instance.IsNotNullOrDestroyed()) { - var del = TimingManager.Instance.timing5.onLateUpdate; - leakCount += CleanDelegateHandlers("TimingManager.Instance.timing5.onLateUpdate", ref del); - TimingManager.Instance.timing5.onLateUpdate = del; + // since the timing manager's delegates are properties, we can't directly modify them except through the set method... + leakCount += TimingManagerCleanObject("TimingManager.Instance", TimingManager.Instance); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing0", TimingManager.Instance.timing0); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing1", TimingManager.Instance.timing1); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing2", TimingManager.Instance.timing2); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing3", TimingManager.Instance.timing3); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing4", TimingManager.Instance.timing4); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timing5", TimingManager.Instance.timing5); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timingPre", TimingManager.Instance.timingPre); + leakCount += TimingManagerCleanObject("TimingManager.Instance.timingFI", TimingManager.Instance.timingFI); } // This is part of the resource flow graph mess, and will keep around tons of references @@ -438,6 +445,23 @@ static bool ShouldCleanType(Type type) return type.Assembly == assemblyCSharp || typeof(PartModule).IsAssignableFrom(type) || typeof(VesselModule).IsAssignableFrom(type); } + static int TimingManagerCleanObject(string name, object obj) + { + int leakCount = 0; + var delegateProperties = obj.GetType().GetProperties(); + foreach (var property in delegateProperties) + { + if (property.PropertyType == typeof(TimingManager.UpdateAction)) + { + var del = (TimingManager.UpdateAction)property.GetValue(obj); + leakCount += CleanDelegateHandlers(name + "." + property.Name, ref del); + property.SetValue(obj, del); + } + } + + return leakCount; + } + static int CleanDelegateHandlers(string eventName, ref T del) where T : Delegate { int leakCount = 0; From 411fca18808fb28871dcc9d894e961de7ffab14b Mon Sep 17 00:00:00 2001 From: JonnyOThan Date: Thu, 14 Mar 2024 17:36:38 -0400 Subject: [PATCH 15/15] More cleanup when a vessel is unloaded --- KSPCommunityFixes/Performance/MemoryLeaks.cs | 69 ++++++++++++++------ 1 file changed, 49 insertions(+), 20 deletions(-) diff --git a/KSPCommunityFixes/Performance/MemoryLeaks.cs b/KSPCommunityFixes/Performance/MemoryLeaks.cs index 8d1a568..cae58d7 100644 --- a/KSPCommunityFixes/Performance/MemoryLeaks.cs +++ b/KSPCommunityFixes/Performance/MemoryLeaks.cs @@ -757,37 +757,66 @@ static void Part_ReleaseAutoStruts_Postfix(Part __instance) // PartSets are awful as noted in OnSceneUnloaded. But vessel unloading is also a good time to clean these up instead of waiting for scene switch - static void CleanUpPartSet(PartSet partSet) - { - if (partSet == null) return; - - GameEvents.onPartResourceFlowStateChange.Remove(partSet.OnFlowStateChange); - GameEvents.onPartResourceFlowModeChange.Remove(partSet.OnFlowModeChange); - - // these aren't *necessary* but help to reduce noise in reports - partSet.targetParts?.Clear(); - partSet.ship = null; - partSet.vessel = null; - } - // general unhooking of stuff when unloading a vessel. Note that vessel gameobjects are not usually destroyed until the vessel itself is static void Vessel_Unload_Postfix(Vessel __instance) { __instance.rootPart = null; + __instance.referenceTransformPart = null; + __instance.referenceTransformPartRecall = null; + __instance.GroundContacts?.Clear(); + __instance._suspensionLoadBalancer?.suspensionModules?.Clear(); + __instance.dockingPorts?.Clear(); + + // the flight integrator holds onto a lot of references, which will be repopulated when the vessel is loaded again + // clear them all now + + if (__instance._fi != null) + { + __instance._fi.partRef = null; + __instance._fi.partThermalDataList?.Clear(); + __instance._fi.partThermalDataListSkin?.Clear(); + __instance._fi.partThermalDataCount = 0; + __instance._fi.partCount = 0; + __instance._fi.compoundParts?.Clear(); + __instance._fi.occlusionConv?.Clear(); + __instance._fi.occlusionSun?.Clear(); + __instance._fi.occlusionBody?.Clear(); + __instance._fi.overheatModules?.Clear(); + __instance._fi.previewModules?.Clear(); + __instance._fi.occludersConvection = null; + __instance._fi.occludersSun = null; + __instance._fi.occludersBody = null; + __instance._fi.occludersBodyCount = 0; + __instance._fi.occludersConvectionCount = 0; + __instance._fi.occludersSunCount = 0; + } + + // not sure about + // objectUnderVessel + FlightGlobals.ResetObjectPartUpwardsCache(); FlightGlobals.ResetObjectPartPointerUpwardsCache(); - CleanUpPartSet(__instance.resourcePartSet); - CleanUpPartSet(__instance.simulationResourcePartSet); - - foreach (var partSet in __instance.crossfeedSets) + for (int i = GameEvents.onPartResourceFlowStateChange.events.Count; i-- > 0;) { - CleanUpPartSet(partSet); + if (GameEvents.onPartResourceFlowStateChange.events[i].originator is PartSet partSet) + { + if (partSet.vessel == __instance) + { + GameEvents.onPartResourceFlowStateChange.events.RemoveAt(i); + } + } } - foreach (var partSet in __instance.simulationCrossfeedSets) + for (int i = GameEvents.onPartResourceFlowModeChange.events.Count; i-- > 0;) { - CleanUpPartSet(partSet); + if (GameEvents.onPartResourceFlowModeChange.events[i].originator is PartSet partSet) + { + if (partSet.vessel == __instance) + { + GameEvents.onPartResourceFlowModeChange.events.RemoveAt(i); + } + } } __instance.resourcePartSet = null;