KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase.
https://github.com/KSPModdingLibs/KSPCommunityFixes/releases/latest
57 stars 19 forks source link

Performance: unloaded Vessels register for lots of events #106

Closed JonnyOThan closed 1 year ago

JonnyOThan commented 1 year ago

The Vessel class registers event listeners for lots of things that could only possibly apply to them if they are loaded or unpacked. This means that every asteroid, debris, flag, deployed science, etc. in the universe is running some code every time one of these happens:

        GameEvents.onCrewTransferred.Add(onCrewTransferred);
        GameEvents.onVesselPartCountChanged.Add(OnVesselPartCountChanged);
        GameEvents.onPartResourceListChange.Add(UpdateResourceSetsEventCheckPart);
        GameEvents.onPartCrossfeedStateChange.Add(UpdateResourceSetsEventCheckPart);
        GameEvents.onPartFuelLookupStateChange.Add(UpdateCrossfeedResourceSetFL);
        GameEvents.onPartPriorityChanged.Add(UpdateResourceSetsEventCheckPart);
        GameEvents.onPartVesselNamingChanged.Add(OnPartVesselNamingChanged);
        GameEvents.onVesselWasModified.Add(OnVesselNamingVesselWasModified);

Seems like it should be possible to move these registrations to some kind of "onloaded" event and unregister them when the vessel becomes unloaded.

This is obviously going to have a bigger impact when there are more vessels in the save file, and especially in modded saves like GU where there are hundreds of asteroids right off the bat.

In particular I would be interested in examining the performance impact of the onVesselWasModified and onVesselPartCountChanged events while a ship is exploding - that usually causes terrible performance hitches.

gotmachine commented 1 year ago

Gave a quick look at this.

Those events are more or less no-ops for all but the concerned vessel, so even if you have thousands of receivers it boils down to a few thousands reference comparisons, which shouldn't take more than a few hundred microseconds. Yes, it does contribute to stutter on specific events like decoupling, but those events are easily 100ms+ stutters, so fixing 0.1% of the problem in a corner case scenario isn't gonna make a perceivable difference.

Apart from that, it's not so easy to move those events to a better place safely. KSP has multiple entry points for vessel load, and seems to rely on some of those events being hooked while loading occurs. I don't say it can't be done, but a careful code path analysis needs to be done for each of those events to ensure that that they are actually hooked in all cases, aren't hooked too late, nor hooked twice, and are properly unhooked as well.

So, to conclude, not really worth the effort/risk in my opinion, unless I'm given profiling proof that those events are actually significant in terms of performance.