Improve TreeCutter#findTree Performance #9327
Merged
+11
−9
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes - #9284
Is probably sufficient to resolve - #5785
So as pointed out in the more recent issue, we were performing a very expensive List#contains call, which dates back to fca3b74. I don't exactly see how that line ever helped with preventing infinite looping, it just seems to bypass the contains check for logs that are in the visited set (simpler solution to that though: don't add them to the visited set then) so that we can still search their neighbours.
I've resolved this by just always going to forNeighbours if something is in the frontier (and passes the relevant checks at that stage, ofc). The visited contains check (add) still needs to be done though, which brings us to improvement number 2:
The more recent issue also pointed out very high numbers of leaves and logs in the respective lists, and if we actually looked at the number of unique logs and leaves in them, they are way too high, this is just from a single oak tree, #unique after the comma:
And mangroves are excessive:
Create/src/main/java/com/simibubi/create/content/kinetics/saw/TreeCutter.java
Lines 147 to 159 in 880ec94
Because some roots are already in the list of logs, they fail the !contains check, and get added to the list again. Also, they will likely have downward neighbours that are also already in the list. The leaves have a similar problem where the same blockpos can get added to the frontier (and leaves list) from multiple directions before it's ever added to the visited set, exacerbated by the fact that there will be duplicate roots that all do this. So solution to both cases, move the visited check/addition to directly before adding to the frontier. Do need to keep a copy of the initial logs list (as a set though, obviously) to ensure we can still add new roots while not duplicating any.
Tests were performed on this line of 8 mangroves grown into each other on a non-mob spawning superflat world, inside my dev env, which is already quite stuttery compared to a production environment.

Before:
https://spark.lucko.me/1Jtr6zYYdE, 97% of SawBlockEntity#onBlockbroken is taken up by TreeCutter#findTree
After:
https://spark.lucko.me/SpQK7Gsk5b, only 13% now.