Optimize AngleMask #3072
Optimize AngleMask #3072
Conversation
...t-core/src/main/java/com/fastasyncworldedit/core/queue/implementation/blocks/CharBlocks.java
Outdated
Show resolved
Hide resolved
worldedit-core/src/main/java/com/fastasyncworldedit/core/function/mask/AngleMask.java
Show resolved
Hide resolved
| return true; | ||
| } | ||
| if (!mask.test(extent, mutable.setComponents(x, y, z + 1))) { | ||
| // other positions might be in different chunks, go a slower, cached path there |
There was a problem hiding this comment.
Potentially worth coming back to this after the merge of this and #3051 - if the cache performance is still poor then probably worth testing if x/z are actually on the edges of the chunk. Maybe this could be implemented into the cache mask constructor - we can potentially look at ways of compressing the data further.
| } | ||
|
|
||
| @Override | ||
| public boolean test(BlockVector3 vector) { | ||
|
|
||
| if (!mask.test(vector)) { | ||
| if (!fastMask.test(vector)) { |
There was a problem hiding this comment.
I think we should cache this at least if on the edge of the chunk, as there's little point using the cache mask on x/z +/- 1 if there's unlikely to be a value already in the cache. We know we're going to be testing every block here
There was a problem hiding this comment.
Not sure if it's worth the complexity to have different code paths for those cases. I tried again to revert this change to use the cached mask and it makes the whole mask slower even in the overlay scenario.
I also replaced the usages of the CachedMask in adjacentAir, bringing time spent in adjacentAir down from ~25s (when using the cached mask everywhere) to ~10s in my test... so I guess we really need to look into CachedMask after your PR.
There was a problem hiding this comment.
Okay that's definitely concerning then. I suppose the caching makes it slower if the chunks are already available, and we will already have those GET chunks loaded in FAWE
There was a problem hiding this comment.
Yes that code path doesn't seem to run into chunk loading, so any access is more or less direct (going through STQE)
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
SirYwell
force-pushed
the
perf/AngleMask-improvements
branch
from
72e49fc to
2ea3626
Compare
|
After #3051 got merged, it still seems to be a performance win avoiding the CachedMask usage completely: |
|
I suppose it's probably worth removing the CachedMask in this case entirely? |
|
I think so, yeah. Maybe it also comes down to how chunks are spread across threads, that might make caching values of surrounding chunks less useful too. The CachedMask field is protected, do you think it's still fine to remove it? |
Overview
Description
AngleMaskcaused unnecessarily slow block lookups due to its access patterns. That can be improved. I'll add some inline comments about specific changes.Submitter Checklist