Foam​Fix

Foam​Fix

97M Downloads

Fix Forge Duping Class Cache

NullSector76 opened this issue · 23 comments

commented

You say you want to free up the memory. This is the main issue with mods and it's due to the launch wrapper storing the byte array and class object one only the class one is needed past initial dev.
MinecraftForge/MinecraftForge#5532

What can you do. Every time a class is loaded on your new FMLCorePlugin with the sorting index of Integer.max_value clear the class resourceCache via reflection and input back the world class and bytes if available

commented

You mean this?

commented

no I mean LaunchClassLoader#resourceCache it contains a huge cache between ob class and byte array forge only uses the private array for one thing to check the world class bytes so empty the array all except for the world class.

So empty it when possible all except that one object and forge should be happy but, your ram to. There is no need to keep an unused obfuscated hashmap byte arrays loaded into memory

commented

Did you read the code?

commented

yeah I don't think it's good enough you clear the maps once in init it looks like? here is the proper way to keep a constant clean ob map

"What can you do. Every time a class is loaded on your new FMLCorePlugin with the sorting index of Integer.max_value clear the class resourceCache via reflection and input back the world class and bytes if available"

Also make sure your function to get not in use keys actually works for what you think it does and not look for is this a weak key instanceof of a weakhashmap? no ok do nothing.

commented

Okay, so.

I don't think it's good enough you clear the maps once in init it looks like?

Most classes are loaded after then.

on your new FMLCorePlugin with the sorting index of Integer.max_value

Last I checked, you can only have one coremod per JAR? And this one can't just sit on Integer.MAX_VALUE; I wholly expect other mods to mess with its output.

clear the class resourceCache via reflection

That's slow. A better way would be to create a dummy Map<> which doesn't allow putting new values in (blindly consuming them), if you wanted to do it.

the world class and bytes if available

Why only the World class? How do you know there is no mod in existence relying on resourceCache in some manner?

Also make sure your function to get not in use keys actually works for what you think it does and not look for is this a weak key instanceof of a weakhashmap? no ok do nothing.

Admittedly, the resourceCache patch is a bit stupid - could be done better, but it gets the job done IIRC.

commented

"Why only the World class? How do you know there is no mod in existence relying on resourceCache in some manner?" Open Call hierarchy many coremods, fml itself cache their isObf from LaunchClassLoader.getBytes(net.minecraft.world.World) So yeah you definitely need the world entry or mods hard coding it such as apple core and other coremods might crash from a wrong boolean. It would also screw up my coremods as they do the same thing because I copied a class from asm helper

a dummy map which would get then get populated where? pre-init before pre init on initialization of your class plugin/transformer? I like that idea alot better then letting it happen then overriding it.

commented

When I work on my fix with a dummy map I will give you the code without needing to modify your transformer much at all

commented

I'm not interested in this type of PR, sorry. FoamFix is largely done for 1.12.2 at this point, and I would rather not touch it for what is comparatively a miniscule gain - and if I do, I'd rather do it myself.

commented

https://pastebin.com/BrSP2r5j very simple to fix your stuff do the 10 line of patches. Notice the Class Initializer that's when you should use your hooks for LaunchClassLoader. This worked on my end

After getting some sleep that's all you have to do nothing with hard coding an entry it will just fetch it when getting class bytes each time from the disk.

You shouldn't need a config for this as it will just fetch it from the disk each time a method manually calls getClassBytes() or when a new class not in memory get's transformed

commented

hey do classloaders even need to store an array of cached classes. I just tested with both maps being the dummy map and my transformers only ran once I kept the game open for 20 minuets and I edit alot of classes.

If not you can clear the LaunchClassLoader#cachedClasses as well for an even bigger memory optimization.
https://pastebin.com/t5JHWatY

commented

Ok so use reflection to disable your config option manually I guess that could work? I still recommend removing the code for future versions or use the code I sent you thanks for your time

commented

I might. Who knows? It's not a thing in 1.13+ anyway...

commented

If not you can clear the LaunchClassLoader#cachedClasses as well for an even bigger memory optimization.

It's not like the Class objects will disappear. The VM still has them.

commented

Right so can you fix your fix? Make it clear both mappings in LaunchClassLoader to dummy maps that can't store anything I already gave you the code for both please apply the fix for 1.12.2 as it's 2 seconds for you and will save ram for modders.

With my coremod test your foamfix actually replaces resource cache then adds back memory objects that should have been clear. So fix your fix and make it use a dummy map

Code again. Put this in your transformer class doesn't matter which use your reflection util and call it a day.
https://pastebin.com/zd3MFmUv

commented

2 seconds for you

At least ten minutes to start the game, stop the game, add the necessary try...catches, push the code and push the release to two mod distribution locations. And this doesn't even cover the (slim, but real) chance of something going wrong. (For instance, I had to ensure the code doesn't run with FastCraft 2 - as its LaunchClassLoader replacement does not provide a resourceCache field)

commented

Why does the code not run with fastcraft? Launch.classLoader should always be the instance set.

Edit: O I see your point the field won't get set because they replaced it. Well my reflection util catches the exception and prints stacktrace that's all that happens for me you would have to do the same

commented

If your implementation is so good, why not release it as a separate mod? :V

I'll get around to it when I have some time and aren't busy working on Fabric. The memory difference caused by it is very miniscule compared to the things FoamFix targets most, anyway.

commented

because it's already in a mod Evil Notch Lib next beta at least. It's a massive library that fixes alot of bugs.

commented

That's great! That means I don't have to do it, just like when FastWorkbench came along. Less work for me \o/

commented

But, it's for your sake not everyone wants a massive library. Multiple overrides for one field will work across multiple mods and don't need to depend upon each other for this fix.

commented

Also your code still breaks my code because your fix is improper replaces the mapping for resource cache and then adds objects back into the mappings rather then just keeping empty maps.

So if you don't want to do this fix properly please just remove the code as it breaks my fix because your fix doesn't clear everything and replaces the map with a different one and fires after mine

commented

People can disable it with a config option, for now .I've planned for this.

commented

and again it's not just the resource cache it's both hashmaps are not needed. ClassLoaders do not require an additional hashmap to store the class for you so clear both maps there