3TUSK / RailwayDepotWorker

They fix rusty locomotives
6 stars 0 forks source link

Optimize firestone tick handler #5

Open jchung01 opened 1 day ago

jchung01 commented 1 day ago

FirestoneTickHandler is supposed to only run on players, but the expensive operations end up being run on all entities. Non-player entities are partially handled by passing null into the inventory checker, but it can still cause a needless impact on tps due to the stream operations (see example spark report). An early return for non-player entities should fix this. Here's the simplest possible patch:

public void tick(LivingEvent.LivingUpdateEvent event) {
        EntityLivingBase entity = event.getEntityLiving();
        ...
+       if (!(entity instanceof EntityPlayer)) return;
        EntityPlayer holder = entity instanceof EntityPlayer ? (EntityPlayer) entity : null;
        InventoryComposite.of(entity).streamStacks().forEach(stack -> FirestoneTools.trySpawnFire(entity.world, entity.getPosition(), stack, holder));
    }
3TUSK commented 1 day ago

I will take a look this weekend.

Or next weekend. Hopefully I am not overloaded this weekend.


I took a look at the spark report. The overall time cost was not that significant, though? Just 0.07% of the entire recording time. Is this truly an issue?

jchung01 commented 1 day ago

Certainly not a priority, but it does scale on the number of entities in the world. It does seem to consistently appear as one of the higher TPS users in LivingUpdateEvent which is why I mention it. Here's a different spark report, which shows its usage relative to other event handlers, albeit it not being the main issue.