Spark performance improvements #4459
Conversation
(everything should work now)
Having done some profiling, caching doesn't seem to be worth the effort, with some of my tests without caching performing better than with
|
Very nice, could you go through and add inline comments explaining what's being done at a high level? I think that would help review. Like for example, I see the state/BE params being removed from findManaReceiver, which I'm assuming is to boost perf, but would be nice to have that called out instead of me assuming. |
|
Actually I think that one was a leftover from me implementing caching. I can change it, although that pattern might be common enough that its better to just keep it... |
anonymous123-code
force-pushed
the
spark-improvements
branch
from
0327dc8 to
a6a21fd
Compare
|
Stuff should be better now, I left the findManaReceiver refactorings in as that method already existed before the PR. |
Xplat/src/main/java/vazkii/botania/common/entity/ManaSparkEntity.java
Outdated
Show resolved
Hide resolved
| transfersTowardsSelfToRegister.add(spark); | ||
| } | ||
| } | ||
| Collections.shuffle(transfersTowardsSelfToRegister); |
There was a problem hiding this comment.
reasoning for the shuffles that were added here?
There was a problem hiding this comment.
Otherwise mana pools would be loaded in a relatively consistent order, making only some fill up. This makes it so if they're reloaded, that order is shuffled at least sometimes. A similar thing was in here before, I'm only guessing the reason why it has been implemented: https://github.com/VazkiiMods/Botania/pull/4459/files#diff-6ab7cbbb33c3d206fc8ca670958f924a1353804a7fb8f234f3308622d23723f7L172
I can also add the transfers directly, but that would be slightly breaking with older behavior (Previosly, and in this implementation dominant sparks would only add one transfer each tick, taking some time to initialize). This would make transfersTowardsSelfToRegister unnecessary.
Xplat/src/main/java/vazkii/botania/common/entity/ManaSparkEntity.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public boolean areIncomingTransfersDone() { | ||
| if (getAttachedManaReceiver() instanceof ManaPool) { | ||
| return removeTransferants > 0; |
There was a problem hiding this comment.
tbh I'm not really sure what this old code did lol, do you know and can you explain why it should be removed?
There was a problem hiding this comment.
RemoveTransferants was the old reset logic when stuff like the upgrade or the network changed. This is now handled differently. And the only reason why it might be removed is if it is full, which is checked seperately, so I changed it to be a constant false
Better names Remove at end, to save copying performance Avoid potential memory leak when dominant is switched in and out quickly
|
let's get it tested in the next beta |
Fixes #4389
Sparks already had a lot of the infrastructure needed, but I basically changed spark from recalculating everything every tick to being notified when stuff changes. Note that this approach is more prone to bugs, especially when more funcitonality gets added. The issue also goes a lot more into detail. I also tested using Fabric's block cache API but it didn't turn out to be worth it.
Setup profiled:
(Screenshot TBD)
10 Minutes superflat, no mob spawning, no daylight cycle (In dev)
Runtime of:
vazkii.botania.common.entity.ManaSparkEntity.tick()With mana running:
When empty:
I already tested a lot of stuff before rebasing (But that one was like 1.19.2), so I'll need to retest.
Tested: