[ANGRY PIXEL] The Betweenlands

[ANGRY PIXEL] The Betweenlands

24M Downloads

This mod is too CPU-heavy

alfuken opened this issue ยท 8 comments

commented

In a modpack with over 200 mods Betweenlands event handlers still consumes almost 30% of tick time. That's a huge number.

I'd suggest following optimisations to be applied:

  1. In https://github.com/Angry-Pixel/The-Betweenlands/blob/1.12-dev/src/main/java/thebetweenlands/common/herblore/elixir/PotionRootBound.java#L64
    Instead of iterating 20 times per second, you could check for the effect every 5 ticks, and change the status effect on/off. Also, since it's only 1/4 of a second, does it even makes sense to have this effect cool-off at all?... Maybe it makes more sense to just remove the cool-off? Or at least change it to 1/2s so you could perform the check 2 times per second, not 20.

  2. in https://github.com/Angry-Pixel/The-Betweenlands/blob/1.12-dev/src/main/java/thebetweenlands/common/block/plant/BlockWeedwoodBush.java#L218
    that's a LOT of calculation just for a simple step effect. Maybe it'll be better to implement this with onEntityCollidedWithBlock? Honestly, I'd think twice if it's really necessary to have this feature at all.

  3. https://github.com/Angry-Pixel/The-Betweenlands/blob/1.12-dev/src/main/java/thebetweenlands/common/handler/ItemEquipmentHandler.java
    Uhm... try Baubles, maybe?..
    Also, can safely be run once per second or even two, not every single tick.

  4. https://github.com/Angry-Pixel/The-Betweenlands/blob/1.12-dev/src/main/java/thebetweenlands/common/handler/PlayerPortalHandler.java
    Another good candidate for onEntityCollidedWithBlock. If that's not an option, this check can also be throttled down to 2 times per second, not 20.

  5. https://github.com/Angry-Pixel/The-Betweenlands/blob/1.12-dev/src/main/java/thebetweenlands/common/handler/PuppetHandler.java#L182
    That's a huge method. Can it be split into few smaller ones, just for the sake of readability? Like, AI assignments, for example. Anyway, is every tick is necessary here?

All in all, just try to refrain from running callbacks EVERY tick. That's it.

commented

Screenshot 2020-01-09 at 12 09 03

Checked with LagGoggles mod...

  1. When you hit else condition in updateClientEntity, for 5 ticks the only thing you do is decrease counter, and after 5 ticks have passed, you remove the tag. Now, this is what I've had in mind:
int ticker = 0;
@SideOnly(Side.CLIENT)
private static void updateClientEntity(EntityLivingBase entity) {
  ticker++;
  if (ticker < 5) return; else ticker = 0;
  
  if(entity instanceof AbstractClientPlayer) {
    if(entity.getActivePotionEffect(ElixirEffectRegistry.ROOT_BOUND) != null) {
      entity.addTag("thebetweenlands.rootBound")
      // now, for the next 5 ticks entity will be tagged with "thebetweenlands.rootBound", untill ticker counts to 5
    } else {
      if (entity.getTags().contains("thebetweenlands.rootBound")) {
        entity.removeTag("thebetweenlands.rootBound");
      }
    }
  }
}

and since tags is just a HashSet, it's pretty fast.

  1. My guess is that's because you want to render the effect while player is moving through the bushes. But you can use onEntityCollision to add a tag "inBushes", and only perform the full check if tag is set, and then at some point just remove it when player is no longer in the bushes.

  2. Yeah, that'd be good idea. Still, do all ticking items has to be updated every tick? Or some of them can be checked less often? Also, when player them picks up, you can add them to some watch list, and tick them from the list instead of going through the whole inventory every tick. Alternatively, you can rebuild this list from inventory every 20 ticks, and then again tick the items from the direct list to skip the inventory scan.

  3. See net.minecraft.block.BlockPortal#onEntityCollidedWithBlock how it's implemented in vanilla, i think it makes sense to go the same way. Or not?

commented

Where's that 30% number from? 30% of total tick time seems quite unrealistic to me.

Anyways, thanks for the suggestions...

  1. Yes, that needs to run every tick because setting motion to 0 in the potion's performEffect doesn't do much. I guess I can move updateClientEntity to ClientTickEvent though.
  2. IIRC there was a reason I didn't put it in onEntityCollision, but I'll check again.
  3. Needs to run every tick to update equipped items, just like when you have an item in your inventory. But I can add a boolean to the entity NBT when an entity has any equipment, so it can skip getting the capability and inventories for entities without any equipment.
  4. Will change it to PlayerTickEvent.
  5. Irrelevant to this topic. Yeah, could do that.

Uhm... try Baubles, maybe?..

Uhm.. no?.. Because Baubles doesn't have certain things we need.

commented

Well, they're easy changes so why not ๐Ÿ‘

commented

I've profiled Crackpack 3 (~200 mods) and this is the result:

May not be 100% accurate due to sampling rate, but I get a similar result with LagGoggles.
That's nowhere near 30%. If your result really is correct then there's probably some bug.

  1. When you hit else condition in updateCl...

That would make practically no difference because the entity instanceof AbstractClientPlayer is only true for the one single client player, so using ClientTickEvent makes more sense. Also what you suggested would not work because the timer needs to stay for 5 ticks after the effect has worn off, but by doing it like that it may only stay for anywhere between 1-5 ticks after effect has worn off :P

  1. My guess is that's because you want to re

It works fine in onEntityCollision with some slight tweaks.

  1. Yeah, that'd be good idea. Still, do all ticking items has ...

Further optimizations than what I suggested seem pointless to me since a vast majority of mobs will not have any equipment and the equipment capability and ticking code (apart from the boolean check) will only be called for a small fraction of mobs.

  1. See net.minecraft.block.BlockPortal#onEntityCollidedWithBlock how it's impl...

It needs to reset the portal timer somehow when the entity is no longer colliding. That can't be done in onEntityCollision and needs to be done on entity tick, since there's no onEntityLeaveCollision or something like that. Using PlayerTickEvent instead will reduce the overhead of this to pretty much none already.

commented

Oh, what's that screenshot is made with? But yeah, those a drastically different results. Curious...

  1. you could add a tag on collision, just like in a suggestion โ„–2 ;) collide with portal - start checks every tick - eventually during tick check it shows you-re out of portal - remove tag - no longer checks every tick

Thanks for the comments, tho. Was nice chatting with you. :)

commented

VisualVM

commented

Why did you close this? Do you not want the optimizations anymore? ;P

commented

Ah, sorry, you can keep it open if needed. Purpose of this ticket was a discussion, but to implement something or not is completely up to you, of course. :)