Content Patcher

Content Patcher

378k Downloads

[Content Patcher] investigate OutOfMemoryException errors

Pathoschild opened this issue ยท 5 comments

commented

Some players report OutOfMemoryException errors since the Stardew Valley 1.4.2 release.

Reports

report game SMAPI CP log
2019-12-05 hiabara on Nexus 1.4.2 3.0.1 1.10.1 log A and B
2019-12-12 Alvaror on Nexus
(says on Discord it started around the 8th)
1.4.2 3.0.1 1.10.1 log
2019-12-12 Spartan69 on Nexus 1.4.2 3.0.1 1.10.1 log

Timeline

date event
2019-11-26 Stardew Valley 1.4
SMAPI 3.0
Content Patcher 1.10
2019-12-02 Stardew Valley 1.4.1
SMAPI 3.0.1
Content Patcher 1.10.1
2019-12-04 Stardew Valley 1.4.2
2019-12-05 first report
commented

Content Patcher and SMAPI can't safely safely dispose the old textures when reloading an asset, because the game or other mods often have a direct reference to it (so the game would crash with ObjectDisposedException). We rely on garbage collection to clean up textures when they're no longer referenced, but unmanaged memory accumulates even with the GC finalizing Texture2D instances.

For example, here's memory usage where I forced the game to repeatedly reload some textures. Note that GC is correctly cleaning up managed instances (like Texture2D), but unmanaged memory accumulates regardless:

I proposed changes to the game code which would let SMAPI properly dispose textures, but it's a large change so it'd only be in Stardew Valley 1.5 if they do it at all. In the meantime I reworked tokenizable strings to significantly reduce the number of asset reloads when dealing with location-dependent patches.

The memory leak itself isn't fixed, but this should avoid hitting the limit with current features. Unless the proposed change in the game code is done, we shouldn't implement more frequent context updates in Content Patcher (e.g. time conditions).

commented

I'm tinkering with this in SpriteMaster:

// This is necessary to prevent recursion.
private static readonly ThreadLocal<Texture2D> CurrentFinalizer = new ThreadLocal<Texture2D>();

[HarmonyPatch("~Texture2D", HarmonyPatch.Fixation.Postfix, PriorityLevel.Last)]
[System.Diagnostics.CodeAnalysis.SuppressMessage("Code Quality", "IDE0051:Remove unused private members", Justification = "Harmony")]
private static void Finalize (Texture2D __instance) {
	if (!__instance.IsDisposed && !(__instance is ManagedTexture2D)) {
		if (CurrentFinalizer.Value != __instance) {
			Contract.AssertNull(CurrentFinalizer.Value);
			CurrentFinalizer.Value = __instance;
			try {
				__instance.Dispose();
			}
			finally {
				CurrentFinalizer.Value = null;
			}
		}
	}
}

It's a little harmony hack to Dispose of a Texture2D when it is Finalized. I see this getting hit a LOT.

commented

The garbage collector does not clean up Texture2D native resources (the textures themselves) as they are unmanaged. They must be disposed, as far as I recall.

You might be able to just add destructors to your texture class (~ContentPatcherTexture() or whatever) and Dispose() from there, but I'm unsure if textures actually get garbage collected in the first place, or if they hold a strong reference in native code in XNA.

Worst-case scenario, I can add intrusive texture sniffing to SpriteMaster which will aggressively prune textures, caching their data somewhere on the filesystem cache, and 'reloading' them if a mod/user tries to access them, bypassing the Disposed exception.

The reason I specify that I am unsure if references are being held is that I am unsure if Texture2Ds can ever be collected by the garbage collector prior to being disposed. It may make more sense to attach some kind of weird sentinel object to the Texture2D class it is wrapped in and rely on that as a signal, having it trip when only one reference to it exists (which indicates that the texture no longer is referenced)?

commented

Are you sure about that? Looking at the decompiled code, it seems like Dispose() and the finalizer both call the same internal Dispose(bool) method.

commented