Lag in RunicAltar
LemADEC opened this issue · 6 comments
Version Information
Forge version: 14.23.4.2759
Botania version: r1.10-355
Further Information
=> could we freeze/hold recipes lookup until inventory is updated to save from constant recomputations?
Issue is also reproduced without CustomNPCs and AstralSorcery:
Steps to reproduce:
- Place a Runic altar (without a mana source)
- Add a stick to the altar
- profile!
What I expected to happen:
Block using less than 20 µs per tick.
What happened instead:
Lag goggles reports more than 1000 µs tick time.
this lag is mostly from item copying overhead, so I'll patch that out.
The other "lookup recipe many times" thing I'm going to wait until 1.13 to tackle for real.
ok, just pushed some optimizations. Let me know if it helps! (you can build it from master or try to find a jenkins someone might have up for botania)
This seems to be a similiar issue to the portal and the pure daisy - every tick, the tile checks the entire recipe registry to see if anything matches its contents.
I think there needs to be some kind of caching - why does the pure daisy check every tick if a blockstate matches if it isn't different than in the previous tick? Why does the altar check if its inventory didn't change?
The portal is somewhat better now that it doesn't store items it can't use (the recipe check logic should totally be moved to the recipe itself by the way, I forgot about that completely when making that PR...), but you can still store a single manasteel ingot + block in it, and an another mod or the pack developer can add a recipe that uses multiple items enabling storing just one ingredient 500 times.
Also, recipe comparisons are using the weird "copy the stack, set the damage to wildcard, then compare stacks" pattern which causes unnecessary event overhead.
Might look into patching this, possibly tweaking the portal logic and the daisy as well. Looked into the altar logic already (why is there a cached currentRecipe if it is always null?), I think it might be easily tweaked to make it check only on content changes.
From what we've seen, it seems to lag when there's no valid recipe. If feels like the cache is null when it's not initialized or no recipe was found, while in fact, those are 2 different states...
Jared has a Jenkins, the builds are here but it doesn't seem like it triggers on every commit so it doesn't have the latest build.
I did some profiling with a WarmRoast-based profiler, Spark (which seems to be what the OP used too, now that I look at it closer) to compare build 354 and 80aea6f.
The altar definitely is way lighter than before, thanks to the removal of stack copying - you definitely were right on that front and I jumped to an incorrect conclusion there.
The pure daisy has negligible impact when doing nothing.
The portal gained a bit, now most of the overhead seems to be checking for the pylon presence every tick multiple times, unless filled with a lot of items.
354: https://sparkprofiler.github.io/?caKZK8RieC
80aea6f: https://sparkprofiler.github.io/?u2Ui7FxMAH
(Test conditions: default superflat world with villages disabled, around 15 altars with 3 items each, 4 elven portals with about 3 stacks of items stuck inside, 8 pure daisies fully surrounded by inconvertible blocks; most gamerules toggled off; only mods other than Botania + Baubles were Astral Sorcery (for capability event usage) and JEI)