Armourer's Workshop

Armourer's Workshop

2M Downloads

[BUG] Memory allocation lag when creating new entities

LemADEC opened this issue ยท 1 comments

commented

Describe the bug
Whenever new entities are created a lot of small objects are created by the mod, causing unnecessary GC load.

To Reproduce
Botania setup with lots of active Mana spreaders.
Found while profiling another issue, confirmed through code review.

Expected behavior
Avoid short term memory allocation, especially in frequently called methods.

Screenshots
n/a

Debug crash log
n/a

Additional context
Every time an entity is created, all capabilities are attached through the AttachCapabilitiesEvent, here: https://github.com/RiskyKen/Armourers-Workshop/blob/91f1ee066a0ad262463461180a16430250dc7de3/src/main/java/moe/plushie/armourers_workshop/common/capability/ModCapabilityManager.java#L81
For every entity, this will call getSkinnableEntity() which is defined here:
https://github.com/RiskyKen/Armourers-Workshop/blob/421937cbcbdb0eb24ec8758d184bf0eb4971fbe5/src/main/java/moe/plushie/armourers_workshop/common/skin/entity/SkinnableEntityRegisty.java#L61
For every non-explicitly configured entity, it'll do a for loop on the configured entities and, for every entry, it'll create a new array of all the keys in the registry (calling toArray() is the bad boy here, while the keySet() method is fine):
https://github.com/RiskyKen/Armourers-Workshop/blob/421937cbcbdb0eb24ec8758d184bf0eb4971fbe5/src/main/java/moe/plushie/armourers_workshop/common/skin/entity/SkinnableEntityRegisty.java#L68

On a side note, entityMap is declared with key of type <? extends Living> but the for loop is casting it to <? extend EntityLivingBase>, possibly rising a class cast exception.
=> for what I understood, the rest of the code expect a generic Entity type, so the cast shouldn't be there.

Here's a possible fix for this:

		for (Entry<Class<? extends EntityLivingBase>, String> entry : entityMap.entrySet()) {
			if (entry.getKey().isAssignableFrom(entity.getClass())) {
				ISkinnableEntity skinnableEntity = entry.getValue();
				entityMap.put(entity.getClass(), skinnableEntity);
				return skinnableEntity;
			}
		}

Also, to further optimize the code, before returning null, consider adding the new class to the entityMap with a null value, so the loop is much less frequently used. This imply changing a couple other method in SkinnableEntityRegistry class.

commented

Nice catch. Not sure what I was thinking when I coded that. ๐Ÿ™„

The EntityLivingBase was a left over from 1.7 IExtendedEntityProperties, everything should be Entity now.

You can grab the dev build from the Jenkins or the Discord server, the public build with the fix should be out in a week.