Improve falling tree performance and consistency on tree scan

[avavakin]

Improve falling tree performance and consistency on tree scan

1. Frequently used tree extremums cached:

• getTopMostLog
• getBottomMostLog
• getStart

they are updated only when new part added
retrieval is O(1) instead of O(n)

2. Tree builds only once

Created TreeHandlerState to build tree only once for one event without rebuling it

Reasons:

• performance improvements (both CPU and memory)
• consistency: in case of introduction multithreading in mod platforms or Minecraft itself we would have a risk of passing validation shouldCancelEvent and breaking tree breakTree could build different trees in rare circumstances

[Rakambda]

Wouldn't it be even more efficient to just cache the return values from the methods in the Tree class ?

These values won't be accessed before the tree is built, and instead of performing checks for every added part, it is done just one for each method call. I just don't know which is better, on one hand you perform "1" check for every part added, while caching would do n checks. Maybe the cache would have a little advantage as the computation would be performed only if the method is actually called, if an early return is performed, some of those cached values may not even have to be computed.

[avavakin]

@Rakambda Thank you for your comment and looking into PR!

That was my first idea to cache the getter response in lazy init way. As you mentioned, it would not create the overhead in case if those extremum points will never be called. And it's sounds really nice and provide the same total benefit in time and even better (in case when we never call extremum points' getters)

However, if we would cache result on getter call, then we will freeze this value and next parts which will be added via addPart method will be ignored which could produce bugs in future even while it is not a case for now

But we can avoid that if we will introduce caching of getters on invocation together with making parts field immutable list while building parts list in TreeBuilder and passing completed parts array to constructor of Tree right after this part:

try{  
    checkAdjacent(adjacentPredicate, level, originPos);  
      
    while(!toAnalyzePos.isEmpty()){  
       var analyzingPos = toAnalyzePos.remove();  
       if(analyzingPos.toTreePart().treePartType().isIncludeInTree()){  
          parts.addPart(analyzingPos.toTreePart());  
       }  
       analyzedPos.add(analyzingPos);  
         
       if(parts.getSize() > maxScanSize){  
          log.info("Tree at {} reached max scan size of {}", originPos, maxScanSize);  
          throw new TreeTooBigException();  
       }  
       if(analyzingPos.treePartType().isEdge() && analyzingPos.sequenceSinceLastLog() >= mod.getConfiguration().getTrees().getMaxLeafDistanceFromLog()){  
          continue;  
       }  
         
       var potentialPositions = analyzingPos.positionFetcher().getPositions(level, originPos, analyzingPos);  
       var nextPositions = filterPotentialPos(boundingBoxSearch, adjacentPredicate, level, originPos, originBlock, analyzingPos, potentialPositions, analyzedPos);  
         
       nextPositions.removeAll(analyzedPos);  
       nextPositions.removeAll(toAnalyzePos);  
       toAnalyzePos.addAll(nextPositions);  
    }  
      
    postProcess(parts);  
}  
catch(AbortSearchException e){  
    log.info("Didn't cut tree at {}, reason: {}", originPos, e.getMessage());  
    mod.notifyPlayer(player, mod.translate("chat.fallingtree.search_aborted").append(e.getComponent()));  
    return empty();  
}

final Tree tree = new Tree(level, originPos, parts);

WDYT? Will it work for you?

[Rakambda]

That works yeah. Makes more sense anyways for a builder to build the actual object at the end of the logic.

[avavakin]

@Rakambda thanks

I've applied change

I did it in a bit different way. Because postProcess use the extremum point in it's logic, I've decided to create object for building parts, but commited interface separation: now only immutable methods exposed outside, while with MutableTree (package private) works only TreeBuilder

Also getter will be uncached if corresponding part will be removed from list - should create not a big overhead though