Proactive asset cleaning #2528
Proactive asset cleaning #2528
Conversation
ajrb
left a comment
There was a problem hiding this comment.
Couple of comments more from curiosity than criticism... I'm far to out of the loop for that these days but I'd like to understand why I didn't see these issues at the time. Thanks.
|
This seems to be in a good state now. I'm not noticing any textures left behind by the UI or meshes by the world streaming. I was having issues getting dungeons to clean up all their meshes when the player leaves them so I opted to force a full asset clean when that occurs. Your mileage may vary on whether that's too aggressive but I suppose it depends on the performance hit. I think that, since there's a bit of a hitch when leaving a dungeon regardless, players won't notice it. I want to do a bit more testing with the periodic collection turned off. Still need to try starting a new character with D.R.E.A.M installed so I can be sure custom assets don't throw a wrench in anything. I think that we should eventually turn the periodic collection back on (albeit at a slower pace) since we can't guarantee mods will collect their orphaned assets. |
If an asset is loaded from an asset bundle then CleanAsset does not call Destroy on it.
|
I've found that loading assets from asset bundles mitigates memory leak issues. Assets loaded in this way are cleaned from memory without having to call Destroy. Since most assets in DFU can be swapped out with mod asset bundles I wrapped the Destroy method into my own method that ensures the asset is not in any bundles before actually calling it. Not noticing any problems currently with the periodic memory sweep disabled. There is a full memory sweep when leaving dungeons but nowhere else. I'm open to reinstating the periodic sweep but we shouldn't have to do it as often now. |
|
Now that we're in post-1.0, we can consider this change. It's a bit big, but I think the only obstacles now are some of the public API changes. If we can remove or workaround these, I'd like to give this a try. Unlike some of the other PRs, I don't think we'll be able to extensively test it by just one maintainer, we'll need to deploy this to playtesters. |
|
Sounds like a good plan Kab. I would advise only making the experimental build available to people on discord/forums so we actually hear feedback. |
ajrb
left a comment
There was a problem hiding this comment.
Looks okay to me from reviewing code changes.
I wonder if there's any noticable performance impact for players who have a lot of mods that all the asset cleanup calls have to iterate though all of their asset bundles?
(I still can't believe Unity requires devs to manually handle this kind of thing when they're using a garbage collected language)
This is definitely something worth considering. I haven't yet looked into ways to search the library without enumerating the entire thing but I'll see if that's possible. |
Probably isn't, but worth a look. it's something that we should get someone running 100+ mods to test out. |
Assets/Scripts/Game/Automap.cs
Outdated
| { | ||
| if (gameObjectUserNoteMarkers != null) | ||
| { | ||
| var meshFilters = gameObjectUserNoteMarkers.GetComponentsInChildren<MeshFilter>(); |
There was a problem hiding this comment.
Why do you get meshFilters here? It's not used in this scope.
Did you mean to Destroy those too?
Same for line 1700
There was a problem hiding this comment.
Those were leftovers from some debugging I was doing. They're removed now.
| uiManager.PopWindow(); | ||
| RaiseOnCloseHandler(); | ||
| DaggerfallGC.ThrottledUnloadUnusedAssets(); | ||
| //DaggerfallGC.ThrottledUnloadUnusedAssets(); |
There was a problem hiding this comment.
Did you mean to keep this comment? Should we remove the line instead?
There was a problem hiding this comment.
Line is now removed.
|
Oh yeah, I should probably run a test build of this, while we have no new changes compared to the latest release |
|
Test build can be found here: https://github.com/Interkarma/daggerfall-unity/releases/tag/v1.1.1-pac. I've posted it on the Lysandus' Tomb Discord test channel |
|
I can't rule out setup issues, but it seems like multiple people get missing doors inside. Maybe this is while using Transparent Windows?
|
|
I'm going to take a different approach using finalizers. That should hopefully require a lot less code and be less error-prone. |
The purpose of this PR is to prevent any allocated Unity assets from becoming orphaned so that we can be free of asset leaks in the DFU code base.
This is currently a work-in-progress. As of the creation of this PR I've eliminated most (if not all) asset leaks in the windowing system. I will make another pass at it after I work on the world streaming and anything else that might leak memory.
This will not prevent mods from leaking memory. Therefore, we will probably still need to periodically call a global asset cleanup. However, I've noticed in my tests that the memory footprint is greatly decreased between cleanup calls. Whereas the editor would show memory usage exceeding 3 gb after opening and closing shelves in a shop in under a minute, it now never exceeds 2.45 gb as long as I don't travel around the world map and stream in new areas. I haven't tested this with high-definition texture mods but I can imagine the effect is even more dramatic.