Blood Magic

Blood Magic

90M Downloads

AlchemicalWizardryTickHandler cpu leak

Slind14 opened this issue · 18 comments

commented

Hey,

your AlchemicalWizardryTickHandler leaks massive cpu usage:
http://i.imgur.com/EwGutOu.png

What would happen if we comment it out ?

ObfuscationReflectionHelper.setPrivateValue(PlayerCapabilities.class, entityPlayer.capabilities, Float.valueOf(0.1f), new String[]{"walkSpeed", "g", "field_75097_g"});

Would it break something major ?

commented

mhh, that would mean that the effect would last for ever, right ?

I think it would be enough if you set it to 0.1 every 20th tick and only on players with the effect. If the effect lasts 1 second longer who cares. As the effect is permanent wouldn´t it be possible to only cast it when the effect gets removed and on playerJoin ?

commented

Hi, if that would be irreversible, couldnt you just reset players with a sigil of haste? Or only reset in the tick the players loses the 'sigil of haste'-effect?

commented

You'd still have to look if the player has that sigil, which might be worse. And what if they drop the sigil? What if they get the buff from another source?

commented

If that line is commented out, the Boost effect from the Sigil of Haste would... be irreversible and blow up. Thaumcraft does something similar, however Azanor may have a way that is a lot better.

commented

Would it be possible to save a list of players with active sigils? If a player is tested, you check if the Sigil is still there and if not (=dropped) you remove the buff. As only a few have the sigil, the total work would be less (I guess).

commented

yeah + every 20th tick. This would lower down the cpu usage by about +20000% for us.

100 * 20 * 10 (if every 10th player uses a sigil)

commented

Unfortunately that wouldn't exactly solve it (the one every 20th tick just can't work out). I think the issue is that the way it currently works also affects FakePlayers, which includes a bunch of blocks in the game (this is just a guess, but I don't know if it's actually the case). I'll think on it a bit and hope that I can find a solution that works out.

commented

After browsing the code a bit, would it work to add something along the following to SigilOfHaste.java:

public ItemStack onItemRightClick(ItemStack par1ItemStack, World par2World, EntityPlayer par3EntityPlayer)
{
  //...
  tag.setBoolean("isActive", !(tag.getBoolean("isActive")));
  //here comes my codeproposal
  if(tag.getBoolean("isActive"))
  {
    activePlayers.add(par3EntityPlayer);    //activePlayers would be a list which is checked in AlchemicalWizardryTickHandler
  }
  else
  {
   activePlayers.remove(par3EntityPlayer); 
  }
  //here does it end
  //...
}

On a server with 70 people with only 5 having the Sigil of Haste active, the extra work created by checking a list 65 times would be minimal compared to 65 unnecessary setPrivateAttribute-calls. Would of course extrawork if it would be reversed (eg. 65 having the sigil active)...
Maybe a Set would be better?
Would this be threadsafe? Please tell me, what do you think of this.

commented

Unfortunately that wouldn't work, although it may be close to what I need. I'll have a hashmap save the names of the people using my mod's speed-altering items/buffs, and on the tick end I'll instead iterate over that list instead of all of the players. It should also remove the player from the list at each tick end, and depending on how well it works through testing, should constantly reset and re-add the player to the list.

Will it be more efficient per player? Probably not. Overall, though? I hope so! I'll do a bit of testing on this later on, and will close & comment this issue if it works.

commented

@WayofTime come on IRC plox, I have a solution for this.

commented

whats the solution now ? We just removed the check and it works fine. I don´t see any need for it on a big server.

commented

@WayofTime Was this fixed?

commented

It will be fixed in the 1.7.2 version for sure, and if I get it working there I may be able to port the fix to 1.6.4.

commented

Please do, this is a major issue for us and would be a great fix to avoid disabling the item.

commented

@WayofTime Status report? :P

commented

@tterrag1098 Should be all fixed already!

commented

Nice! We have 1.0, I assume that's the version with the fix?

commented

That it does.